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

[Jim Fehlig]

On 03/14/2018 07:19 AM, John Ferlan wrote:
> 
> 
> 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 at suse.com>
>> ---
>>   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.

I'll send a V2 of this patch that adds a ref after ListAdd so the pattern is 
consistent throughout the driver.

> 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?

Yes, it makes sense. Thanks again for the details!

Regards,
Jim