On 01/10/2017 01:23 AM, Wang King wrote: I assume: s/was been/is (there's 3 occurrences). I'm not sure I'm following your description... So, restated in order to help me understand: 1. Host A domain migrates to Host B 2. Host B adds a closeCallback during qemuProcessLaunch 3. Host A's libvirtd is restarted causing Host B to destroy the VM 4. Host B calls virCloseCallbacksRun in order to run closeCallbacks for the domain from qemuConnectClose. 5. Host B creates the look-side list by removing the entry from closeCallbacks and placing it on the look-aside list for "later" processing. 6. Host B removes the closeCallbacks lock 7. Host B runs through the list->nentries, searches by UUID for the domain, returning with a locked domain 8. Host B calls the callback function qemuProcessAutoDestroy 9. During that processing the call to qemuProcessAutoDestroyRemove will fail because this UUID isn't on that list, but that's no big deal since the failure is ignored. 10. After returning from the callback there's a call to virObjectUnlock However, we still own a reference on the domain because virCloseCallbacksUnset that wasn't run would virObjectUnref(vm) BUT, if you move the list, I believe you run into the possibility of running the callback function twice (one may cause an error). So I'm not convinced yet that moving the list will do as you claim and resolve the object leak. Something's not right here. Has this been tested using the scenario described? You're taking out the closeCallbacks lock twice and attempting to call the closeCallback with the lock. So let's assume the Lock should be an Unlock... I don't see how moving the list to after resolves the leak and furthermore I think it causes more problems. In order to resolve the leak, it would seem that after the callback has been run we can run virObjectUnlock(); however, doing so on 'vm' is a no-op (at least for the path you're concerned about) since the end of qemuProcessAutoDestroy there is: virDomainObjEndAPI(&dom); return dom; The *EndAPI will set *dom = NULL;, which means we return NULL, which means that virObjectUnlock generally doesn't do what it claims I think. So wouldn't a virUnrefObject(vm); do what you expect? That is unref the vm just like virCloseCallbacksUnset would have done when it removed the callback from the list. So not only does this code remove/process the callback, but it reduces the refcount when it's done. John

On 01/10/2017 01:23 AM, Wang King wrote: I don't see how getting an extra Ref resolves the issue... Let's see how 2/3 falls out, but I think this one is dropped. John

On 01/18/2017 06:34 PM, John Ferlan wrote: I ended up pushing this change (a few days ago) and then realizing almost immediately that the !list option was to Lock not Unlock - so I fixed that... I guess the power of my original suggestion to just have the Unlock on the !list outweighed the reality of what was written the patch and I just assumed. In any case, that part is resolved. As for patches 2 & 3, I've proposed something slightly different with attribution to you, see: http://www.redhat.com/archives/libvir-list/2017-January/msg01025.html I decided that using your patch 3 concept of taking the extra ref would be wise, although perhaps unnecessary since the callers each would essentially return NULL when the vm object was removed from the list, so as long as we removed the reference before the call the result would be the same. Still for a better look and feel with respect to consistency, using EndAPI is the better solution IMO. John