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

Nikolay Shirokovskiy posted 1 patch 4 years, 12 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1554112980-131799-1-git-send-email-nshirokovskiy@virtuozzo.com
There is a newer version of this series
src/qemu/qemu_migration.c | 2 ++
1 file changed, 2 insertions(+)
[libvirt] [PATCH] qemu: fix domain unlock/unref in qemuMigrationSrcPerform
Posted by Nikolay Shirokovskiy 4 years, 12 months ago
qemuMigrationSrcPerform callers expect it to call virDomainObjEndAPI
in any case.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@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;
         }
 
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: fix domain unlock/unref in qemuMigrationSrcPerform
Posted by Michal Privoznik 4 years, 12 months ago
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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: fix domain unlock/unref in qemuMigrationSrcPerform
Posted by Ján Tomko 4 years, 12 months ago
On Mon, Apr 01, 2019 at 01:03:00PM +0300, Nikolay Shirokovskiy wrote:
>qemuMigrationSrcPerform callers expect it to call virDomainObjEndAPI
>in any case.
>
>Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>---
> src/qemu/qemu_migration.c | 2 ++
> 1 file changed, 2 insertions(+)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list