Re: [libvirt] [PATCH v2 0/9] Be consistent with vir*Obj*Remove* APIs

On Wed, Mar 28, 2018 at 11:19 PM +0200, John Ferlan <jferlan(a)redhat.com&gt; wrote: > v1: https://www.redhat.com/archives/libvir-list/2018-March/msg01295.html > > NB: This can wait until 4.2.0 is release, but I figured I'd post this > now just to put it on the radar and of course in hopes that someone > will look during the idle moment or two before the release. > > Changes since v1: > > Short story: Rework the processing of the code > > Longer story: > > In his review Erik noted that there's a "fire dance" when processing > the vir*Obj*Remove APIs of requiring a locked object upon entry, then > adding a reference to that object, unlocking the object, locking the > list to which it is contained, and then relocking the object. > > So it took some time to think about it and during one lengthy meeting > today I had the aha moment that the *Remove API's could take the same > key (e.g., uuid or name) used to Add or Find the object and use it for > the Remove API. This allows the Remove API to not require a locked (and > reffed) object upon entry and perform the lock dance, remove the object, > and return just just a reffed object forcing the caller to know to Unref > object. > > Instead, let's simplify things. The *Remove API can take the key, Find > the object in the list, remove it from the hash tables, and dispose of > the object. In essence the antecedent to the Add or AssignDef API's > taking a def, creating an object, and adding it the object to the hash > table(s). If there are two *Remove threads competing, one will win and > perform the removal, while the other will call *Remove, but won't find > the object in the hash table, and just return none the wiser. > > And yes, I think this can also work for the Domain code, but it's going > to take a few patch series to get there as that code is not consistent > between consumers. > > John Ferlan (9): > secret: Rework LoadAllConfigs > secret: Alter virSecretObjListRemove processing > interface: Alter virInterfaceObjListRemove processing > nodedev: Alter virNodeDeviceObjListRemove processing > conf: Clean up virStoragePoolObjLoad error processing > storage: Clean up storagePoolCreateXML error processing > test: Clean up testStoragePoolCreateXML error processing > conf: Move virStoragePoolObjRemove closer to AssignDef > storagepool: Alter virStoragePoolObjRemove processing Side note: Wouldn’t is be useful to refactor all the vir*ObjList* things as they’re looking quite similar? Not sure if it’s easily feasible in all places.

[…snip] -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294