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

[John Ferlan]

On 04/10/2018 04:47 AM, Marc Hartmayer wrote:
> On Wed, Mar 28, 2018 at 11:19 PM +0200, John Ferlan <jferlan at 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
>