On 04/10/2018 04:47 AM, Marc Hartmayer wrote:
On Wed, Mar 28, 2018 at 11:19 PM +0200, John Ferlan <jferlan@redhat.com> 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.
Well - that was the point of what I started last year, but there hasn't been any general agreement or acceptance of patches for that. My changes made use of objects and more generic naming to unify things; however, they weren't acceptable with (IIRC and in my words) the preference being more specific naming using switch/case statements and shim API's. The last full posting (a v5) of what I had done is here: https://www.redhat.com/archives/libvir-list/2017-August/msg00659.html If you feel so inclined you can follow the history of comments through v4: https://www.redhat.com/archives/libvir-list/2017-August/msg00537.html and v3: https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html w/ review comments starting here: https://www.redhat.com/archives/libvir-list/2017-July/msg01032.html Maybe once the domain code is modified to be more common (in process now) and if nwfilter ever could gain acceptance, something could be done. Still I have my doubts it'll happen especially since nwfilter patches just cannot get any sort of agreement, last try here: https://www.redhat.com/archives/libvir-list/2018-February/msg00325.html John
[…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