Assume that QEMU_CAPS_BLOCKDEV is present and remove all code executed
when it's not.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_migration.c | 127 ++++++++------------------------------
 1 file changed, 25 insertions(+), 102 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 8e9428a5bb..ef24a1dedf 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1080,46 +1080,6 @@ qemuMigrationSrcNBDStorageCopyBlockdev(virQEMUDriver *driver,
 }


-static int
-qemuMigrationSrcNBDStorageCopyDriveMirror(virQEMUDriver *driver,
-                                          virDomainObj *vm,
-                                          const char *diskAlias,
-                                          const char *host,
-                                          int port,
-                                          const char *socket,
-                                          unsigned long long mirror_speed,
-                                          bool mirror_shallow)
-{
-    g_autofree char *nbd_dest = NULL;
-    int mon_ret;
-
-    if (socket) {
-        nbd_dest = g_strdup_printf("nbd+unix:///%s?socket=%s",
-                                   diskAlias, socket);
-    } else if (strchr(host, ':')) {
-        nbd_dest = g_strdup_printf("nbd:[%s]:%d:exportname=%s", host, port,
-                                   diskAlias);
-    } else {
-        nbd_dest = g_strdup_printf("nbd:%s:%d:exportname=%s", host, port,
-                                   diskAlias);
-    }
-
-    if (qemuDomainObjEnterMonitorAsync(driver, vm,
-                                       VIR_ASYNC_JOB_MIGRATION_OUT) < 0)
-        return -1;
-
-    mon_ret = qemuMonitorDriveMirror(qemuDomainGetMonitor(vm),
-                                     diskAlias, nbd_dest, "raw",
-                                     mirror_speed, 0, 0, mirror_shallow, true);
-
-    qemuDomainObjExitMonitor(vm);
-    if (mon_ret < 0)
-        return -1;
-
-    return 0;
-}
-
-
 static int
 qemuMigrationSrcNBDStorageCopyOne(virQEMUDriver *driver,
                                   virDomainObj *vm,
@@ -1133,13 +1093,9 @@ qemuMigrationSrcNBDStorageCopyOne(virQEMUDriver *driver,
                                   const char *tlsHostname,
                                   unsigned int flags)
 {
-    qemuDomainObjPrivate *priv = vm->privateData;
     qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
     qemuBlockJobData *job = NULL;
     g_autofree char *diskAlias = NULL;
-    const char *jobname = NULL;
-    const char *sourcename = NULL;
-    bool persistjob = false;
     bool syncWrites = !!(flags & VIR_MIGRATE_NON_SHARED_SYNCHRONOUS_WRITES);
     int rc;
     int ret = -1;
@@ -1150,35 +1106,18 @@ qemuMigrationSrcNBDStorageCopyOne(virQEMUDriver *driver,
     if (!(job = qemuBlockJobDiskNew(vm, disk, QEMU_BLOCKJOB_TYPE_COPY, diskAlias)))
         return -1;

-    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
-        jobname = diskAlias;
-        sourcename = qemuDomainDiskGetTopNodename(disk);
-        persistjob = true;
-    } else {
-        jobname = NULL;
-        sourcename = diskAlias;
-        persistjob = false;
-    }
-
     qemuBlockJobSyncBegin(job);

-    if (flags & VIR_MIGRATE_TLS ||
-        virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
-        rc = qemuMigrationSrcNBDStorageCopyBlockdev(driver, vm,
-                                                    disk, jobname,
-                                                    sourcename, persistjob,
-                                                    host, port, socket,
-                                                    mirror_speed,
-                                                    mirror_shallow,
-                                                    tlsAlias,
-                                                    tlsHostname,
-                                                    syncWrites);
-    } else {
-        rc = qemuMigrationSrcNBDStorageCopyDriveMirror(driver, vm, diskAlias,
-                                                       host, port, socket,
-                                                       mirror_speed,
-                                                       mirror_shallow);
-    }
+    rc = qemuMigrationSrcNBDStorageCopyBlockdev(driver, vm,
+                                                disk, diskAlias,
+                                                qemuDomainDiskGetTopNodename(disk),
+                                                true,
+                                                host, port, socket,
+                                                mirror_speed,
+                                                mirror_shallow,
+                                                tlsAlias,
+                                                tlsHostname,
+                                                syncWrites);

     if (rc == 0) {
         diskPriv->migrating = true;
@@ -2687,41 +2626,25 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver,
     }

     if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)) {
-        if (flags & VIR_MIGRATE_NON_SHARED_SYNCHRONOUS_WRITES &&
-            !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
+        if (flags & VIR_MIGRATE_TUNNELLED) {
             virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
-                           _("VIR_MIGRATE_NON_SHARED_SYNCHRONOUS_WRITES is not supported by this QEMU"));
-            return NULL;
+                           _("migration of non-shared storage is not supported with tunnelled migration and this QEMU"));
         }

-        if (flags & VIR_MIGRATE_TUNNELLED) {
-            if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
-                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
-                               _("migration of non-shared storage is not supported with tunnelled migration and this QEMU"));
-                return NULL;
-            }
-
-            if (nmigrate_disks) {
-                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
-                               _("Selecting disks to migrate is not implemented for tunnelled migration"));
-                return NULL;
-            }
-        } else {
-            if (nmigrate_disks) {
-                size_t i, j;
-                /* Check user requested only known disk targets. */
-                for (i = 0; i < nmigrate_disks; i++) {
-                    for (j = 0; j < vm->def->ndisks; j++) {
-                        if (STREQ(vm->def->disks[j]->dst, migrate_disks[i]))
-                            break;
-                    }
+        if (nmigrate_disks) {
+            size_t i, j;
+            /* Check user requested only known disk targets. */
+            for (i = 0; i < nmigrate_disks; i++) {
+                for (j = 0; j < vm->def->ndisks; j++) {
+                    if (STREQ(vm->def->disks[j]->dst, migrate_disks[i]))
+                        break;
+                }

-                    if (j == vm->def->ndisks) {
-                        virReportError(VIR_ERR_INVALID_ARG,
-                                       _("disk target %s not found"),
-                                       migrate_disks[i]);
-                        return NULL;
-                    }
+                if (j == vm->def->ndisks) {
+                    virReportError(VIR_ERR_INVALID_ARG,
+                                   _("disk target %s not found"),
+                                   migrate_disks[i]);
+                    return NULL;
                 }
             }

-- 
2.36.1

On Tue, Jul 26, 2022 at 04:36:59PM +0200, Peter Krempa wrote:
> Assume that QEMU_CAPS_BLOCKDEV is present and remove all code executed
> when it's not.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_migration.c | 127 ++++++++------------------------------
>  1 file changed, 25 insertions(+), 102 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 8e9428a5bb..ef24a1dedf 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c

[...]

> @@ -2687,41 +2626,25 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver,
>      }
> 
>      if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)) {
> -        if (flags & VIR_MIGRATE_NON_SHARED_SYNCHRONOUS_WRITES &&
> -            !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
> +        if (flags & VIR_MIGRATE_TUNNELLED) {
>              virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> -                           _("VIR_MIGRATE_NON_SHARED_SYNCHRONOUS_WRITES is not supported by this QEMU"));
> -            return NULL;
> +                           _("migration of non-shared storage is not supported with tunnelled migration and this QEMU"));
>          }
> 
> -        if (flags & VIR_MIGRATE_TUNNELLED) {
> -            if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
> -                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> -                               _("migration of non-shared storage is not supported with tunnelled migration and this QEMU"));
> -                return NULL;
> -            }
> -
> -            if (nmigrate_disks) {
> -                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> -                               _("Selecting disks to migrate is not implemented for tunnelled migration"));
> -                return NULL;
> -            }
> -        } else {
> -            if (nmigrate_disks) {
> -                size_t i, j;
> -                /* Check user requested only known disk targets. */
> -                for (i = 0; i < nmigrate_disks; i++) {
> -                    for (j = 0; j < vm->def->ndisks; j++) {
> -                        if (STREQ(vm->def->disks[j]->dst, migrate_disks[i]))
> -                            break;
> -                    }
> +        if (nmigrate_disks) {
> +            size_t i, j;
> +            /* Check user requested only known disk targets. */
> +            for (i = 0; i < nmigrate_disks; i++) {
> +                for (j = 0; j < vm->def->ndisks; j++) {
> +                    if (STREQ(vm->def->disks[j]->dst, migrate_disks[i]))
> +                        break;
> +                }
> 
> -                    if (j == vm->def->ndisks) {
> -                        virReportError(VIR_ERR_INVALID_ARG,
> -                                       _("disk target %s not found"),
> -                                       migrate_disks[i]);
> -                        return NULL;
> -                    }
> +                if (j == vm->def->ndisks) {
> +                    virReportError(VIR_ERR_INVALID_ARG,
> +                                   _("disk target %s not found"),
> +                                   migrate_disks[i]);
> +                    return NULL;
>                  }
>              }

This changes doesn't look equivalent.

Before this patch the `for` loop to check `nmigrate_disks` would be done
only for non-tunneled migration but after this changes it is done even
for tunneled migration.

In addition the new code dropped the error path for tunneled migration
if `nmigrate_disks` is not NULL.

Not sure if this was intended or is based on some other knowledge and
code that is not in scope of this patch.

Pavel

On Wed, Aug 03, 2022 at 17:45:07 +0200, Pavel Hrdina wrote:
> On Tue, Jul 26, 2022 at 04:36:59PM +0200, Peter Krempa wrote:
> > Assume that QEMU_CAPS_BLOCKDEV is present and remove all code executed
> > when it's not.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  src/qemu/qemu_migration.c | 127 ++++++++------------------------------
> >  1 file changed, 25 insertions(+), 102 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 8e9428a5bb..ef24a1dedf 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> 
> [...]
> 
> > @@ -2687,41 +2626,25 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver,
> >      }
> > 
> >      if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)) {
> > -        if (flags & VIR_MIGRATE_NON_SHARED_SYNCHRONOUS_WRITES &&
> > -            !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
> > +        if (flags & VIR_MIGRATE_TUNNELLED) {
> >              virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> > -                           _("VIR_MIGRATE_NON_SHARED_SYNCHRONOUS_WRITES is not supported by this QEMU"));
> > -            return NULL;
> > +                           _("migration of non-shared storage is not supported with tunnelled migration and this QEMU"));

[1]

> >          }
> > 
> > -        if (flags & VIR_MIGRATE_TUNNELLED) {

Even prior to this patch, when tunnelled migration is requested with the
VIR_MIGRATE_NON_SHARED* flag

> > -            if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
> > -                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> > -                               _("migration of non-shared storage is not supported with tunnelled migration and this QEMU"));
> > -                return NULL;
> > -            }

... and blockdev is enabled, we simply reject it right away without
consulting anything else. This is what is now done right at the
beginning [1] ...

> > -
> > -            if (nmigrate_disks) {
> > -                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> > -                               _("Selecting disks to migrate is not implemented for tunnelled migration"));
> > -                return NULL;
> > -            }
> > -        } else {
> > -            if (nmigrate_disks) {
> > -                size_t i, j;
> > -                /* Check user requested only known disk targets. */
> > -                for (i = 0; i < nmigrate_disks; i++) {
> > -                    for (j = 0; j < vm->def->ndisks; j++) {
> > -                        if (STREQ(vm->def->disks[j]->dst, migrate_disks[i]))
> > -                            break;
> > -                    }
> > +        if (nmigrate_disks) {

... so that we don't have to worry about any further checks.

> > +            size_t i, j;
> > +            /* Check user requested only known disk targets. */
> > +            for (i = 0; i < nmigrate_disks; i++) {
> > +                for (j = 0; j < vm->def->ndisks; j++) {
> > +                    if (STREQ(vm->def->disks[j]->dst, migrate_disks[i]))
> > +                        break;
> > +                }
> > 
> > -                    if (j == vm->def->ndisks) {
> > -                        virReportError(VIR_ERR_INVALID_ARG,
> > -                                       _("disk target %s not found"),
> > -                                       migrate_disks[i]);
> > -                        return NULL;
> > -                    }
> > +                if (j == vm->def->ndisks) {
> > +                    virReportError(VIR_ERR_INVALID_ARG,
> > +                                   _("disk target %s not found"),
> > +                                   migrate_disks[i]);
> > +                    return NULL;
> >                  }
> >              }
> 
> This changes doesn't look equivalent.
> 
> Before this patch the `for` loop to check `nmigrate_disks` would be done
> only for non-tunneled migration but after this changes it is done even
> for tunneled migration.

As explained above, even before this patch we rejected tunnelled
migration with storage when blockdev was enabled.

After this patch, it's checked at the beginning and thus the rest of the
code doesn't need to be conditional.

> In addition the new code dropped the error path for tunneled migration
> if `nmigrate_disks` is not NULL.

Once again, we simply reject tunneled migration with storage
unconditionally now, so the check would be dead code.

> 
> Not sure if this was intended or is based on some other knowledge and
> code that is not in scope of this patch.
> 
> Pavel

On Tue, Aug 09, 2022 at 02:03:03PM +0200, Peter Krempa wrote:
> On Wed, Aug 03, 2022 at 17:45:07 +0200, Pavel Hrdina wrote:
> > On Tue, Jul 26, 2022 at 04:36:59PM +0200, Peter Krempa wrote:
> > > Assume that QEMU_CAPS_BLOCKDEV is present and remove all code executed
> > > when it's not.
> > > 
> > > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > > ---
> > >  src/qemu/qemu_migration.c | 127 ++++++++------------------------------
> > >  1 file changed, 25 insertions(+), 102 deletions(-)
> > > 
> > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > > index 8e9428a5bb..ef24a1dedf 100644
> > > --- a/src/qemu/qemu_migration.c
> > > +++ b/src/qemu/qemu_migration.c
> > 
> > [...]
> > 
> > > @@ -2687,41 +2626,25 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver,
> > >      }
> > > 
> > >      if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)) {
> > > -        if (flags & VIR_MIGRATE_NON_SHARED_SYNCHRONOUS_WRITES &&
> > > -            !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
> > > +        if (flags & VIR_MIGRATE_TUNNELLED) {
> > >              virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> > > -                           _("VIR_MIGRATE_NON_SHARED_SYNCHRONOUS_WRITES is not supported by this QEMU"));
> > > -            return NULL;
> > > +                           _("migration of non-shared storage is not supported with tunnelled migration and this QEMU"));
> 
> [1]
> 
> > >          }
> > > 
> > > -        if (flags & VIR_MIGRATE_TUNNELLED) {
> 
> Even prior to this patch, when tunnelled migration is requested with the
> VIR_MIGRATE_NON_SHARED* flag
> 
> > > -            if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
> > > -                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> > > -                               _("migration of non-shared storage is not supported with tunnelled migration and this QEMU"));
> > > -                return NULL;
> > > -            }
> 
> ... and blockdev is enabled, we simply reject it right away without
> consulting anything else. This is what is now done right at the
> beginning [1] ...
> 
> > > -
> > > -            if (nmigrate_disks) {
> > > -                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> > > -                               _("Selecting disks to migrate is not implemented for tunnelled migration"));
> > > -                return NULL;
> > > -            }
> > > -        } else {
> > > -            if (nmigrate_disks) {
> > > -                size_t i, j;
> > > -                /* Check user requested only known disk targets. */
> > > -                for (i = 0; i < nmigrate_disks; i++) {
> > > -                    for (j = 0; j < vm->def->ndisks; j++) {
> > > -                        if (STREQ(vm->def->disks[j]->dst, migrate_disks[i]))
> > > -                            break;
> > > -                    }
> > > +        if (nmigrate_disks) {
> 
> ... so that we don't have to worry about any further checks.
> 
> > > +            size_t i, j;
> > > +            /* Check user requested only known disk targets. */
> > > +            for (i = 0; i < nmigrate_disks; i++) {
> > > +                for (j = 0; j < vm->def->ndisks; j++) {
> > > +                    if (STREQ(vm->def->disks[j]->dst, migrate_disks[i]))
> > > +                        break;
> > > +                }
> > > 
> > > -                    if (j == vm->def->ndisks) {
> > > -                        virReportError(VIR_ERR_INVALID_ARG,
> > > -                                       _("disk target %s not found"),
> > > -                                       migrate_disks[i]);
> > > -                        return NULL;
> > > -                    }
> > > +                if (j == vm->def->ndisks) {
> > > +                    virReportError(VIR_ERR_INVALID_ARG,
> > > +                                   _("disk target %s not found"),
> > > +                                   migrate_disks[i]);
> > > +                    return NULL;
> > >                  }
> > >              }
> > 
> > This changes doesn't look equivalent.
> > 
> > Before this patch the `for` loop to check `nmigrate_disks` would be done
> > only for non-tunneled migration but after this changes it is done even
> > for tunneled migration.
> 
> As explained above, even before this patch we rejected tunnelled
> migration with storage when blockdev was enabled.
> 
> After this patch, it's checked at the beginning and thus the rest of the
> code doesn't need to be conditional.
> 
> > In addition the new code dropped the error path for tunneled migration
> > if `nmigrate_disks` is not NULL.
> 
> Once again, we simply reject tunneled migration with storage
> unconditionally now, so the check would be dead code.

I guess brain fog or what is the term used these days :) no idea how
I've missed that.

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>