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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>
Attachment:
signature.asc
Description: PGP signature
