[libvirt] [PATCH 6/6] conf: Clean up object referencing for Add and Remove

[John Ferlan]

On 05/03/2018 12:00 PM, Erik Skultety wrote:
> On Tue, Apr 24, 2018 at 08:28:09AM -0400, John Ferlan wrote:
>> When adding a new object to the domain object list, there should
>> have been 2 virObjectRef calls made one for each list into which
>> the object was placed to match the 2 virObjectUnref calls that
>> would occur during Remove as part of virHashRemoveEntry when
>> virObjectFreeHashData is called when the element is removed from
>> the hash table as set up in virDomainObjListNew.
>>
>> Some drivers (libxl, lxc, qemu, and vz) handled this inconsistency
>> by calling virObjectRef upon successful return from virDomainObjListAdd
>> in order to use virDomainObjEndAPI when done with the returned @vm.
>> While others (bhyve, openvz, test, and vmware) handled this via only
>> calling virObjectUnlock upon successful return from virDomainObjListAdd.
>>
>> This patch will "unify" the approach to use virDomainObjEndAPI
>> for any @vm successfully returned from virDomainObjListAdd.
>>
>> Because list removal is so tightly coupled with list addition,
>> this patch fixes the list removal algorithm to return the object
>> as entered - "locked and reffed".  This way, the callers can then
>> decide how to uniformly handle add/remove success and failure.
>> This removes the onus on the caller to "specially handle" the
>> @vm during removal processing.
>>
>> The Add/Remove logic allows for some logic simplification such
>> as in libxl where we can Remove the @vm directly rather than
>> needing to set a @remove_dom boolean and removing after the
>> libxlDomainObjEndJob completes as the @vm is locked/reffed.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
> 
> ...
> 
>>
>>
>> @@ -386,8 +386,7 @@ virDomainObjListRemoveLocked(virDomainObjListPtr doms,
>>   * no one else is either waiting for 'dom' or still using it.
>>   *
>>   * When this function returns, @dom will be removed from the hash
>> - * tables, unlocked, and returned with the refcnt that was present
>> - * upon entry.
>> + * tables and returned with lock and refcnt that was present upon entry.
> 
> I know ^this is pre-existing, but I reads strange, when we get here, refcnt is
> 3 (after your patches), when we get out, refcnt is 1, I know what the idea is,
> but it can be confusing, it sounds like the refcnt isn't changing when in fact
> it is.

Yeah - I'm not sure of the best way to describe this...

BTW: This is the issue being fixed... If a driver didn't perform a
virObjectRef after the ListAdd, then when ListRemove is called, we
return w/ refcnt == 0 and those callers "knew" that and would be sure to
set @vm = NULL after ListRemove...

For those drivers that had the virObjectRef after ListAdd, we returned
with a refcnt >= 1, without the lock, and we'd (ahem) re-lock (in some
instances) or for others there was an Unref... Oh and I think along the
way there were a few that used EndAPI, which would Unlock something that
wasn't locked any more and Unref.

The reason I noted >= 1 is that @vm can be in other structures, but
those would (hopefully) have done the virObjectRef when placed there.
There was the libxl one in patch 4, plus there's some others dealing
with close callbacks and qemu_process event processing.

> I went through this twice and figured that even if we missed something
> somewhere, it's so early in the new release-cycle that we have plenty of time
> to catch it, the changes look very good and it's an improvement.
> 
> Reviewed-by: Erik Skultety <eskultet at redhat.com>
> 

Great - thanks. Kind of my sentiments at least with respect to why
waiting until 4.4.0 opened was perfectly reasonable for this change. I
believe I've covered everything, but those are famous last words when it
comes to bizarre corner cases.

Tks -

John