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

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(a)redhat.com&gt; > --- ... > > > @@ -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.

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(a)redhat.com&gt;