[libvirt] [PATCH] qemu: fix domain unlock/unref in qemuMigrationSrcPerform

[Michal Privoznik]

On 4/1/19 12:03 PM, Nikolay Shirokovskiy wrote:
> qemuMigrationSrcPerform callers expect it to call virDomainObjEndAPI
> in any case.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
> ---
>   src/qemu/qemu_migration.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 419a729..ce4c443 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -4799,6 +4799,7 @@ qemuMigrationSrcPerform(virQEMUDriverPtr driver,
>           if (cookieinlen) {
>               virReportError(VIR_ERR_OPERATION_INVALID,
>                              "%s", _("received unexpected cookie with P2P migration"));
> +            virDomainObjEndAPI(&vm);
>               return -1;
>           }
>   
> @@ -4813,6 +4814,7 @@ qemuMigrationSrcPerform(virQEMUDriverPtr driver,
>           if (dconnuri) {
>               virReportError(VIR_ERR_INTERNAL_ERROR,
>                              "%s", _("Unexpected dconnuri parameter with non-peer2peer migration"));
> +            virDomainObjEndAPI(&vm);
>               return -1;
>           }
>   
> 

Nice catch! Although for code cleanliness I don't think that these calls 
belong here. They belong into callers where @vm is obtained (e.g. 
qemuDomainMigratePerform3()). Then we can also remove 
virDomainObjEndAPI() calls from other functions called from the one 
you're fixing (e.g. the call can be removed from 
qemuMigrationSrcPerformJob(), qemuMigrationSrcPerformPhase() and 
qemuMigrationSrcPerformJob()).

The reason that free is not done on the same level as alloc (or EndAPI() 
is not done at the same level as ObjFromDomain()) is the reason we have 
a bug like this in the first place. In my mind it should allways be like 
this:

foo()
{
   vm = qemuDomainObjFromDomain(dom);
   ...
   bar(vm);
   ...
   virDomainObjEndAPI(&vm);
}

and if bar() needs a reference, it can do virObjectRef(vm) + 
virObjectUnref(vm).

Michal