Re: [libvirt] [PATCH 3/4] libxl: MigratePrepare: properly cleanup after virDomainObjListAdd

On 03/13/2018 01:26 PM, Jim Fehlig wrote: > libxlDomainMigrationPrepare adds the incoming domain def to the list of > domains via virDomainObjListAdd, which returns a locked and ref counted > virDomainObj. On exit the object is unlocked but not unref'ed. The same > is true for libxlDomainMigrationPrepareTunnel3. Convert both to use the > virDomainObjEndAPI function for cleanup. > > Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; > --- > src/libxl/libxl_migration.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > These two leave me concerned - mainly because as I described in my review of 2/4 the ListAdd function will return 2 refs and 1 lock on the object unlike the libxlDomObjFromDomain where there are at least 3 refs and 1 lock. Since neither of these code paths adds a Ref(vm) after the ListAdd call (like CreateXML and RestoreFlags do) that means calling EndAPI will remove 1 ref and the lock leaving only 1 ref on the object.

Later on when ListRemove is called it would enter with 2 refs and 1 lock and leave with @vm being destroyed. Not having a clear picture of all the paths a @vm could have in libxl makes me concerned because there are a few places where ListRemove is called *and* @vm is referenced afterwards such as libxlDomainDestroyFlags I think once ListAdd returns with the right number of refs, then yes, this is the proper adjustment, but for now unless there's an extra Ref done after the ListAdd, then we should leave things as is. Thoughts? And does this make sense?