Re: [PATCH 21/80] qemu: migration: Remove pre-blockdev code paths

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(a)redhat.com&gt; > > --- > > 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.