Re: [Qemu-devel] [PATCH v2 4/4] migration: Fail migration blocker for --only-migratble

[Greg Kurz <groug@kaod.org>]

Again v3 turned into v2... what happened ?

On Sun, 8 Jan 2017 12:55:51 +0530
Ashijeet Acharya <ashijeetacharya@gmail.com> wrote:

> On Friday, January 6, 2017, Greg Kurz <groug@kaod.org
> <javascript:_e(%7B%7D,'cvml','groug@kaod.org');>> wrote:
> 
> > On Wed,  4 Jan 2017 18:02:29 +0530
> > Ashijeet Acharya <ashijeetacharya@gmail.com> wrote:
> >
> > > migrate_add_blocker should rightly fail if the '--only-migratable'
> > > option was specified and the device in use should not be able to
> > > perform the action which results in an unmigratable VM.
> > >
> > > Make migrate_add_blocker return -EACCES in this case.
> > >
> > > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> > > ---
> > >  block/qcow.c                |  5 ++++-
> > >  block/vdi.c                 |  5 ++++-
> > >  block/vhdx.c                |  6 ++++--
> > >  block/vmdk.c                |  5 ++++-
> > >  block/vpc.c                 |  5 ++++-
> > >  block/vvfat.c               |  6 ++++--
> > >  hw/9pfs/9p.c                |  8 +++++++-
> > >  hw/display/virtio-gpu.c     |  9 +++++++--
> > >  hw/intc/arm_gic_kvm.c       |  5 ++++-
> > >  hw/intc/arm_gicv3_its_kvm.c |  5 ++++-
> > >  hw/intc/arm_gicv3_kvm.c     |  5 ++++-
> > >  hw/misc/ivshmem.c           | 10 ++++++++--
> > >  hw/scsi/vhost-scsi.c        |  8 +++++---
> > >  hw/virtio/vhost.c           |  6 ++++--
> > >  migration/migration.c       |  7 +++++++
> > >  target-i386/kvm.c           |  5 ++++-
> > >  16 files changed, 78 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/block/qcow.c b/block/qcow.c
> > > index 11526a1..bdc6446 100644
> > > --- a/block/qcow.c
> > > +++ b/block/qcow.c
> > > @@ -254,7 +254,10 @@ static int qcow_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > >                 bdrv_get_device_or_node_name(bs));
> > >      ret = migrate_add_blocker(s->migration_blocker, errp);
> > >      if (ret < 0) {
> > > -        error_free(s->migration_blocker);
> > > +        if (ret == -EACCES) {
> > > +            error_append_hint(errp, "Cannot use a node with qcow format
> > as "
> > > +                              "it does not support live migration");
> > > +        }
> > >          goto fail;
> > >      }
> > >
> > > diff --git a/block/vdi.c b/block/vdi.c
> > > index 2b56f52..0b011ea 100644
> > > --- a/block/vdi.c
> > > +++ b/block/vdi.c
> > > @@ -473,7 +473,10 @@ static int vdi_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > >                 bdrv_get_device_or_node_name(bs));
> > >      ret = migrate_add_blocker(s->migration_blocker, errp);
> > >      if (ret < 0) {
> > > -        error_free(s->migration_blocker);
> > > +        if (ret == -EACCES) {
> > > +            error_append_hint(errp, "Cannot use a node with vdi format
> > as "
> > > +                              "it does not support live migration");
> > > +        }
> > >          goto fail_free_bmap;
> > >      }
> > >
> > > diff --git a/block/vhdx.c b/block/vhdx.c
> > > index d81941b..b8d53ec 100644
> > > --- a/block/vhdx.c
> > > +++ b/block/vhdx.c
> > > @@ -997,10 +997,12 @@ static int vhdx_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > >                 bdrv_get_device_or_node_name(bs));
> > >      ret = migrate_add_blocker(s->migration_blocker, errp);
> > >      if (ret < 0) {
> > > -        error_free(s->migration_blocker);
> > > +        if (ret == -EACCES) {
> > > +            error_append_hint(errp, "Cannot use a node with vhdx format
> > as "
> > > +                              "it does not support live migration");
> > > +        }
> > >          goto fail;
> > >      }
> > > -
> > >      if (flags & BDRV_O_RDWR) {
> > >          ret = vhdx_update_headers(bs, s, false, NULL);
> > >          if (ret < 0) {
> > > diff --git a/block/vmdk.c b/block/vmdk.c
> > > index 4a45a83..defe400 100644
> > > --- a/block/vmdk.c
> > > +++ b/block/vmdk.c
> > > @@ -978,7 +978,10 @@ static int vmdk_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > >                 bdrv_get_device_or_node_name(bs));
> > >      ret = migrate_add_blocker(s->migration_blocker, errp);
> > >      if (ret < 0) {
> > > -        error_free(s->migration_blocker);
> > > +        if (ret == -EACCES) {
> > > +            error_append_hint(errp, "Cannot use a node with vmdk format
> > as "
> > > +                              "it does not support live migration");
> > > +        }
> > >          goto fail;
> > >      }
> > >
> > > diff --git a/block/vpc.c b/block/vpc.c
> > > index 299a8c8..5e58d77 100644
> > > --- a/block/vpc.c
> > > +++ b/block/vpc.c
> > > @@ -428,7 +428,10 @@ static int vpc_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > >                 bdrv_get_device_or_node_name(bs));
> > >      ret = migrate_add_blocker(s->migration_blocker, errp);
> > >      if (ret < 0) {
> > > -        error_free(s->migration_blocker);
> > > +        if (ret == -EACCES) {
> > > +            error_append_hint(errp, "Cannot use a node with vpc format
> > as "
> > > +                              "it does not support live migration");
> > > +        }
> > >          goto fail;
> > >      }
> > >
> > > diff --git a/block/vvfat.c b/block/vvfat.c
> > > index 3de3320..5a6e038 100644
> > > --- a/block/vvfat.c
> > > +++ b/block/vvfat.c
> > > @@ -1193,7 +1193,10 @@ static int vvfat_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > >                     bdrv_get_device_or_node_name(bs));
> > >          ret = migrate_add_blocker(s->migration_blocker, errp);
> > >          if (ret < 0) {
> > > -            error_free(s->migration_blocker);
> > > +            if (ret == -EACCES) {
> > > +                error_append_hint(errp, "Cannot use a node with vvfat
> > format "
> > > +                                  "as it does not support live
> > migration");
> > > +            }
> > >              goto fail;
> > >          }
> > >      }
> > > @@ -1202,7 +1205,6 @@ static int vvfat_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > >          init_mbr(s, cyls, heads, secs);
> > >      }
> > >
> > > -    //    assert(is_consistent(s));
> > >      qemu_co_mutex_init(&s->lock);
> > >
> > >      ret = 0;
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index 43cb065..3b4fd24 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -1020,17 +1020,23 @@ static void coroutine_fn v9fs_attach(void
> > *opaque)
> > >       */
> > >      if (!s->migration_blocker) {
> > >          int ret;
> > > +        Error *local_err;
> > >          error_setg(&s->migration_blocker,
> > >                     "Migration is disabled when VirtFS export path '%s'
> > is mounted in the guest using mount_tag '%s'",
> > >                     s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag);
> > > -        ret = migrate_add_blocker(s->migration_blocker, NULL);
> > > +        ret = migrate_add_blocker(s->migration_blocker, &local_err);
> > >          if (ret < 0) {
> > > +            if (ret == -EACCES) {
> > > +                error_append_hint(&local_err, "Failed to mount VirtFS
> > as it "
> > > +                                  "does not support live migration");
> > > +            }
> > >              err = ret;
> > >              clunk_fid(s, fid);
> > >              error_free(s->migration_blocker);
> > >              s->migration_blocker = NULL;
> > >              goto out;
> > >          }
> > > +        error_free(local_err);
> >
> > Thinking again, I suggest you simply drop these changes: v9fs_attach() is
> > triggered by the guest and doesn't cause QEMU to fail, so we don't really
> > care.
> >
> > This might be naive but I didn't quite understand which changes you are
> implying here.  V2 to V3 or the entire diff in 9pfs?
> 

The entire diff of 4/4 in 9pfs, which has no effect actually since local_err
is not even passed to error_report_err() :) and anyway, this would allow
a guest to fill the QEMU log with error messages if it deviced to call
mount(2) repeatedly.

> Ashijeet
> 
> > Cheers.
> >
> > --
> > Greg
> >
> > >          s->root_fid = fid;
> > >      }
> > >
> > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > > index 6e60b8c..fad95bf 100644
> > > --- a/hw/display/virtio-gpu.c
> > > +++ b/hw/display/virtio-gpu.c
> > > @@ -1102,6 +1102,7 @@ static void virtio_gpu_device_realize(DeviceState
> > *qdev, Error **errp)
> > >      VirtIOGPU *g = VIRTIO_GPU(qdev);
> > >      bool have_virgl;
> > >      int i;
> > > +    int ret;
> > >
> > >      if (g->conf.max_outputs > VIRTIO_GPU_MAX_SCANOUTS) {
> > >          error_setg(errp, "invalid max_outputs > %d",
> > VIRTIO_GPU_MAX_SCANOUTS);
> > > @@ -1120,8 +1121,12 @@ static void virtio_gpu_device_realize(DeviceState
> > *qdev, Error **errp)
> > >
> > >      if (virtio_gpu_virgl_enabled(g->conf)) {
> > >          error_setg(&g->migration_blocker, "virgl is not yet
> > migratable");
> > > -        if (migrate_add_blocker(g->migration_blocker, errp) < 0) {
> > > -            error_free(g->migration_blocker);
> > > +        ret = migrate_add_blocker(g->migration_blocker, errp);
> > > +        if (ret < 0) {
> > > +            if (ret == -EACCES) {
> > > +                error_append_hint(errp, "Cannot use virgl as it does
> > not "
> > > +                                  "support live migration yet");
> > > +            }
> > >              return;
> > >          }
> > >      }
> > > diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> > > index 3614daa..c2ef236 100644
> > > --- a/hw/intc/arm_gic_kvm.c
> > > +++ b/hw/intc/arm_gic_kvm.c
> > > @@ -515,7 +515,10 @@ static void kvm_arm_gic_realize(DeviceState *dev,
> > Error **errp)
> > >                                            "not support vGICv2
> > migration");
> > >          ret = migrate_add_blocker(s->migration_blocker, errp);
> > >          if (ret < 0) {
> > > -            error_free(s->migration_blocker);
> > > +            if (ret == -EACCES) {
> > > +                error_append_hint(errp, "Failed to realize vGICv2 as
> > its"
> > > +                                  " migrataion is not implemented yet");
> > > +            }
> > >              return;
> > >          }
> > >      }
> > > diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
> > > index 950a02f..3474642 100644
> > > --- a/hw/intc/arm_gicv3_its_kvm.c
> > > +++ b/hw/intc/arm_gicv3_its_kvm.c
> > > @@ -70,7 +70,10 @@ static void kvm_arm_its_realize(DeviceState *dev,
> > Error **errp)
> > >      error_setg(&s->migration_blocker, "vITS migration is not
> > implemented");
> > >      ret = migrate_add_blocker(s->migration_blocker, errp);
> > >      if (ret < 0) {
> > > -        error_free(s->migration_blocker);
> > > +        if (ret == -EACCES) {
> > > +            error_append_hint(errp, "Failed to realize vITS as its
> > migration "
> > > +                              "is not implemented yet");
> > > +        }
> > >          return;
> > >      }
> > >
> > > diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> > > index 06f6f30..be7b97c 100644
> > > --- a/hw/intc/arm_gicv3_kvm.c
> > > +++ b/hw/intc/arm_gicv3_kvm.c
> > > @@ -118,7 +118,10 @@ static void kvm_arm_gicv3_realize(DeviceState
> > *dev, Error **errp)
> > >      error_setg(&s->migration_blocker, "vGICv3 migration is not
> > implemented");
> > >      ret = migrate_add_blocker(s->migration_blocker, errp);
> > >      if (ret < 0) {
> > > -        error_free(s->migration_blocker);
> > > +        if (ret == -EACCES) {
> > > +            error_append_hint(errp, "Failed to realize vGICv3 as its
> > migration"
> > > +                              "is not implemented yet");
> > > +        }
> > >          return;
> > >      }
> > >
> > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> > > index 585cc5b..14ed60d 100644
> > > --- a/hw/misc/ivshmem.c
> > > +++ b/hw/misc/ivshmem.c
> > > @@ -837,6 +837,7 @@ static void ivshmem_common_realize(PCIDevice *dev,
> > Error **errp)
> > >  {
> > >      IVShmemState *s = IVSHMEM_COMMON(dev);
> > >      Error *err = NULL;
> > > +    int ret;
> > >      uint8_t *pci_conf;
> > >      uint8_t attr = PCI_BASE_ADDRESS_SPACE_MEMORY |
> > >          PCI_BASE_ADDRESS_MEM_PREFETCH;
> > > @@ -910,8 +911,13 @@ static void ivshmem_common_realize(PCIDevice *dev,
> > Error **errp)
> > >      if (!ivshmem_is_master(s)) {
> > >          error_setg(&s->migration_blocker,
> > >                     "Migration is disabled when using feature 'peer
> > mode' in device 'ivshmem'");
> > > -        if (migrate_add_blocker(s->migration_blocker, errp) < 0) {
> > > -            error_free(s->migration_blocker);
> > > +        ret = migrate_add_blocker(s->migration_blocker, errp);
> > > +        if (ret < 0) {
> > > +            if (ret == -EACCES) {
> > > +                error_append_hint(errp, "Cannot use the 'peer mode'
> > feature "
> > > +                                  "in device 'ivshmem' as it does not "
> > > +                                  "support live migration");
> > > +            }
> > >              return;
> > >          }
> > >      }
> > > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> > > index ff503d0..bb731b2 100644
> > > --- a/hw/scsi/vhost-scsi.c
> > > +++ b/hw/scsi/vhost-scsi.c
> > > @@ -245,7 +245,11 @@ static void vhost_scsi_realize(DeviceState *dev,
> > Error **errp)
> > >                 "vhost-scsi does not support migration");
> > >      ret = migrate_add_blocker(s->migration_blocker, errp);
> > >      if (ret < 0) {
> > > -        goto free_blocker;
> > > +        if (ret == -EACCES) {
> > > +            error_append_hint(errp, "Cannot use vhost-scsi as it does
> > not "
> > > +                              "support live migration");
> > > +        }
> > > +        goto close_fd;
> > >      }
> > >
> > >      s->dev.nvqs = VHOST_SCSI_VQ_NUM_FIXED + vs->conf.num_queues;
> > > @@ -272,8 +276,6 @@ static void vhost_scsi_realize(DeviceState *dev,
> > Error **errp)
> > >   free_vqs:
> > >      migrate_del_blocker(s->migration_blocker);
> > >      g_free(s->dev.vqs);
> > > - free_blocker:
> > > -    error_free(s->migration_blocker);
> > >   close_fd:
> > >      close(vhostfd);
> > >      return;
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index 416fada..6d2375a 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -1160,8 +1160,10 @@ int vhost_dev_init(struct vhost_dev *hdev, void
> > *opaque,
> > >          Error *local_err;
> > >          r = migrate_add_blocker(hdev->migration_blocker, &local_err);
> > >          if (r < 0) {
> > > -            error_report_err(local_err);
> > > -            error_free(hdev->migration_blocker);
> > > +            if (r == -EACCES) {
> > > +                error_append_hint(&local_err, "Cannot use vhost drivers
> > as "
> > > +                                  "it does not support live migration");
> > > +            }
> > >              goto fail_busyloop;
> > >          }
> > >      }
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index 713e012..ab34328 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -1115,9 +1115,16 @@ int migrate_add_blocker(Error *reason, Error
> > **errp)
> > >  {
> > >      if (migration_is_idle(NULL)) {
> > >          migration_blockers = g_slist_prepend(migration_blockers,
> > reason);
> > > +        error_free(reason);
> > >          return 0;
> > >      }
> > >
> > > +    if (only_migratable) {
> > > +        error_setg(errp, "Failed to add migration blocker:
> > --only-migratable "
> > > +                   "was specified");
> > > +        return -EACCES;
> > > +    }
> > > +
> > >      error_setg(errp, "Cannot block a migration already in-progress");
> > >      return -EBUSY;
> > >  }
> > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > > index 6031a1c..f2c35d8 100644
> > > --- a/target-i386/kvm.c
> > > +++ b/target-i386/kvm.c
> > > @@ -963,6 +963,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
> > >                     " (invtsc flag)");
> > >          r = migrate_add_blocker(invtsc_mig_blocker, NULL);
> > >          if (r < 0) {
> > > +            if (r == -EACCES) {
> > > +                error_report("kvm: non-migratable CPU device but"
> > > +                        " --only-migratable was specified");
> > > +            }
> > >              error_report("kvm: migrate_add_blocker failed");
> > >              goto efail;
> > >          }
> > > @@ -1009,7 +1013,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
> > >   fail:
> > >      migrate_del_blocker(invtsc_mig_blocker);
> > >   efail:
> > > -    error_free(invtsc_mig_blocker);
> > >      return r;
> > >  }
> > >
> >
> >