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

[Pavel Hrdina]

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 at 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 at redhat.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20220809/a7084268/attachment.sig>