The function which started a migration phase should also finish it by
calling qemuMigrationJobFinish/qemuMigrationJobContinue so that the code
is easier to follow.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/qemu/qemu_migration.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index d02e8132e4..b62f7256c4 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -6017,7 +6017,8 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver,
                              int retcode,
                              bool v3proto,
                              unsigned long long timeReceived,
-                             virErrorPtr *orig_err)
+                             virErrorPtr *orig_err,
+                             bool *finishJob)
 {
     virDomainPtr dom = NULL;
     g_autoptr(qemuMigrationCookie) mig = NULL;
@@ -6065,8 +6066,6 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver,
     qemuMigrationDstComplete(driver, vm, inPostCopy,
                              VIR_ASYNC_JOB_MIGRATION_IN);
 
-    qemuMigrationJobFinish(vm);
-
     return dom;
 
  error:
@@ -6092,12 +6091,10 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver,
     if (virDomainObjIsFailedPostcopy(vm)) {
         qemuProcessAutoDestroyRemove(driver, vm);
         qemuDomainCleanupAdd(vm, qemuProcessCleanupMigrationJob);
-        qemuMigrationJobContinue(vm);
+        *finishJob = false;
     } else {
         qemuMigrationParamsReset(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN,
                                  jobPriv->migParams, priv->job.apiFlags);
-
-        qemuMigrationJobFinish(vm);
     }
 
     if (!virDomainObjIsActive(vm))
@@ -6162,18 +6159,21 @@ qemuMigrationDstFinish(virQEMUDriver *driver,
                                                 cookiein, cookieinlen,
                                                 cookieout, cookieoutlen);
         }
+    } else {
+        bool finishJob = true;
 
-        qemuMigrationJobFinish(vm);
-        goto cleanup;
+        dom = qemuMigrationDstFinishActive(driver, dconn, vm, cookie_flags,
+                                           cookiein, cookieinlen,
+                                           cookieout, cookieoutlen,
+                                           flags, retcode, v3proto, timeReceived,
+                                           &orig_err, &finishJob);
+        if (!finishJob) {
+            qemuMigrationJobContinue(vm);
+            goto cleanup;
+        }
     }
 
-    dom = qemuMigrationDstFinishActive(driver, dconn, vm, cookie_flags,
-                                       cookiein, cookieinlen,
-                                       cookieout, cookieoutlen,
-                                       flags, retcode, v3proto, timeReceived,
-                                       &orig_err);
-    if (!dom)
-        goto cleanup;
+    qemuMigrationJobFinish(vm);
 
  cleanup:
     virPortAllocatorRelease(port);
-- 
2.35.1

On Tue, May 10, 2022 at 17:20:57 +0200, Jiri Denemark wrote:
> The function which started a migration phase should also finish it by
> calling qemuMigrationJobFinish/qemuMigrationJobContinue so that the code
> is easier to follow.

[1]

> 
> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> ---
>  src/qemu/qemu_migration.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index d02e8132e4..b62f7256c4 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -6017,7 +6017,8 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver,
>                               int retcode,
>                               bool v3proto,
>                               unsigned long long timeReceived,
> -                             virErrorPtr *orig_err)
> +                             virErrorPtr *orig_err,
> +                             bool *finishJob)

Come on! Really?!? [1]

>  {
>      virDomainPtr dom = NULL;
>      g_autoptr(qemuMigrationCookie) mig = NULL;

[....]

> @@ -6162,18 +6159,21 @@ qemuMigrationDstFinish(virQEMUDriver *driver,
>                                                  cookiein, cookieinlen,
>                                                  cookieout, cookieoutlen);
>          }
> +    } else {
> +        bool finishJob = true;
>  
> -        qemuMigrationJobFinish(vm);
> -        goto cleanup;
> +        dom = qemuMigrationDstFinishActive(driver, dconn, vm, cookie_flags,
> +                                           cookiein, cookieinlen,
> +                                           cookieout, cookieoutlen,
> +                                           flags, retcode, v3proto, timeReceived,
> +                                           &orig_err, &finishJob);
> +        if (!finishJob) {
> +            qemuMigrationJobContinue(vm);
> +            goto cleanup;
> +        }
>      }

If I overlook the fact that you have to remember what qemuMigrationDstFinishActive
you used probably the least easy variant of seeing what is happening
with that extra jump. Do it without that jump:

    if (finishJob)
        qemuMigrationJobFinish(vm);
    else
        qemuMigrationJobContinue(vm);

With that I'll maybe buy the argument from the commit message, thus:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>


>  
> -    dom = qemuMigrationDstFinishActive(driver, dconn, vm, cookie_flags,
> -                                       cookiein, cookieinlen,
> -                                       cookieout, cookieoutlen,
> -                                       flags, retcode, v3proto, timeReceived,
> -                                       &orig_err);
> -    if (!dom)
> -        goto cleanup;
> +    qemuMigrationJobFinish(vm);
>  
>   cleanup:
>      virPortAllocatorRelease(port);
> -- 
> 2.35.1
>