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

[John Ferlan]

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.

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?


John

> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> index fef1c998b..6b53f9587 100644
> --- a/src/libxl/libxl_migration.c
> +++ b/src/libxl/libxl_migration.c
> @@ -639,9 +639,7 @@ libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
>      }
>  
>   done:
> -    if (vm)
> -        virObjectUnlock(vm);
> -
> +    virDomainObjEndAPI(&vm);
>      return ret;
>  }
>  
> @@ -820,8 +818,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
>          VIR_FREE(hostname);
>      else
>          virURIFree(uri);
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>      virObjectUnref(cfg);
>      return ret;
>  }
>