| Message ID | 1481882024-10016-5-git-send-email-ashijeetacharya@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 16 Dec 2016 15:23:44 +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 | 7 +++++-- > block/vdi.c | 7 +++++-- > block/vhdx.c | 8 +++++--- > block/vmdk.c | 7 +++++-- > block/vpc.c | 7 +++++-- > block/vvfat.c | 8 +++++--- > hw/9pfs/9p.c | 9 +++++++-- > hw/display/virtio-gpu.c | 9 +++++++-- > hw/intc/arm_gic_kvm.c | 7 +++++-- > 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 | 10 ++++++---- > hw/virtio/vhost.c | 8 +++++--- > migration/migration.c | 7 +++++++ > target-i386/kvm.c | 7 +++++-- > 16 files changed, 88 insertions(+), 33 deletions(-) > > diff --git a/block/qcow.c b/block/qcow.c > index 11526a1..7e5003ac 100644 > --- a/block/qcow.c > +++ b/block/qcow.c > @@ -253,8 +253,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, > "does not support live migration", > 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) { > + 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..df45df4 100644 > --- a/block/vdi.c > +++ b/block/vdi.c > @@ -472,8 +472,11 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, > "does not support live migration", > 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) { > + 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..118134e 100644 > --- a/block/vhdx.c > +++ b/block/vhdx.c > @@ -996,11 +996,13 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags, > "does not support live migration", > 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) { > + 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..cd6a641 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -977,8 +977,11 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags, > "does not support live migration", > 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) { > + 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..c935962 100644 > --- a/block/vpc.c > +++ b/block/vpc.c > @@ -427,8 +427,11 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > "does not support live migration", > 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) { > + 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..f9a5f9b 100644 > --- a/block/vvfat.c > +++ b/block/vvfat.c > @@ -1192,8 +1192,11 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, > "does not support live migration", > 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) { > + 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 e72ef43..01dd67f 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -1020,12 +1020,17 @@ static void coroutine_fn v9fs_attach(void *opaque) > */ > if (!s->migration_blocker) { > int ret; > + Error *local_err; > s->root_fid = fid; > 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); > - if (ret < 0) { > + ret = migrate_add_blocker(s->migration_blocker, &local_err); > + if (ret) { Why s/ret < 0/ret/ ? The migrate_add_blocker() function in this v2 only returns 0 or a negative errno... > + if (ret == -EACCES) { > + error_append_hint(&local_err, "Failed to mount VirtFS as it " > + "does not support live migration"); And then local_err is leaked... BTW, I'm not even sure if this useful since this is a guest originated action. > + } This errno will be ultimately be returned to the mount(2) caller in the guest. When reading the manpage, it looks like EBUSY definitely makes more sense than EACCES: EACCES A component of a path was not searchable. (See also path_reso‐ lution(7).) Or, mounting a read-only filesystem was attempted without giving the MS_RDONLY flag. Or, the block device source is located on a filesystem mounted with the MS_NODEV option. EBUSY source is already mounted. Or, it cannot be remounted read- only, because it still holds files open for writing. Or, it cannot be mounted on target because target is still busy (it is the working directory of some thread, the mount point of another device, has open files, etc.). > err = ret; > clunk_fid(s, fid); > goto out; > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > index 6e60b8c..1b02f4b 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) { > + 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..b40ad5f 100644 > --- a/hw/intc/arm_gic_kvm.c > +++ b/hw/intc/arm_gic_kvm.c > @@ -514,8 +514,11 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp) > error_setg(&s->migration_blocker, "This operating system kernel does " > "not support vGICv2 migration"); > ret = migrate_add_blocker(s->migration_blocker, errp); > - if (ret < 0) { > - error_free(s->migration_blocker); > + if (ret) { > + 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..58f27b1 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) { > + 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..0c571f8 100644 > --- a/hw/scsi/vhost-scsi.c > +++ b/hw/scsi/vhost-scsi.c > @@ -244,8 +244,12 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > error_setg(&s->migration_blocker, > "vhost-scsi does not support migration"); > ret = migrate_add_blocker(s->migration_blocker, errp); > - if (ret < 0) { > - goto free_blocker; > + if (ret) { > + 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..1c4a4f9 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1159,9 +1159,11 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, > if (hdev->migration_blocker != NULL) { > 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) { > + 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..05c8365 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -962,7 +962,11 @@ int kvm_arch_init_vcpu(CPUState *cs) > "State blocked by non-migratable CPU device" > " (invtsc flag)"); > r = migrate_add_blocker(invtsc_mig_blocker, NULL); > - if (r < 0) { > + if (r) { > + 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; > } >
On 16 December 2016 at 09:53, 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> > diff --git a/block/qcow.c b/block/qcow.c > index 11526a1..7e5003ac 100644 > --- a/block/qcow.c > +++ b/block/qcow.c > @@ -253,8 +253,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, > "does not support live migration", > 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) { > + if (ret == -EACCES) { > + error_append_hint(errp, "Cannot use a node with qcow format as " > + "it does not support live migration"); > + } > goto fail; > } > --- 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) { > + 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..b40ad5f 100644 > --- a/hw/intc/arm_gic_kvm.c > +++ b/hw/intc/arm_gic_kvm.c > @@ -514,8 +514,11 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp) > error_setg(&s->migration_blocker, "This operating system kernel does " > "not support vGICv2 migration"); > ret = migrate_add_blocker(s->migration_blocker, errp); > - if (ret < 0) { > - error_free(s->migration_blocker); > + if (ret) { > + if (ret == -EACCES) { > + error_append_hint(errp, "Failed to realize vGICv2 as its" > + " migrataion is not implemented yet"); > + } > return; > } > } So if you look at these diff hunks you can see that we effectively end up duplicating the "why doesn't this device let you migrate" user message -- once in the error_setg(&s->migration_blocker, ...) and once with the error_append_hint() call. Is there some way we can get migrate_add_blocker() to automatically incorporate the message when it's failing for the only-migratable case? (It's already being passed the errp.) That would avoid this duplication of the messages, and should also mean we don't need to modify the callers at all. If the error messages come out looking a bit strangely worded we could edit the text in the callers so it works in both situations when it will be used. thanks -- PMM
On Fri, Dec 16, 2016 at 4:44 PM, Greg Kurz <groug@kaod.org> wrote: > On Fri, 16 Dec 2016 15:23:44 +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 | 7 +++++-- >> block/vdi.c | 7 +++++-- >> block/vhdx.c | 8 +++++--- >> block/vmdk.c | 7 +++++-- >> block/vpc.c | 7 +++++-- >> block/vvfat.c | 8 +++++--- >> hw/9pfs/9p.c | 9 +++++++-- >> hw/display/virtio-gpu.c | 9 +++++++-- >> hw/intc/arm_gic_kvm.c | 7 +++++-- >> 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 | 10 ++++++---- >> hw/virtio/vhost.c | 8 +++++--- >> migration/migration.c | 7 +++++++ >> target-i386/kvm.c | 7 +++++-- >> 16 files changed, 88 insertions(+), 33 deletions(-) >> >> diff --git a/block/qcow.c b/block/qcow.c >> index 11526a1..7e5003ac 100644 >> --- a/block/qcow.c >> +++ b/block/qcow.c >> @@ -253,8 +253,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, >> "does not support live migration", >> 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) { >> + 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..df45df4 100644 >> --- a/block/vdi.c >> +++ b/block/vdi.c >> @@ -472,8 +472,11 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, >> "does not support live migration", >> 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) { >> + 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..118134e 100644 >> --- a/block/vhdx.c >> +++ b/block/vhdx.c >> @@ -996,11 +996,13 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags, >> "does not support live migration", >> 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) { >> + 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..cd6a641 100644 >> --- a/block/vmdk.c >> +++ b/block/vmdk.c >> @@ -977,8 +977,11 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags, >> "does not support live migration", >> 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) { >> + 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..c935962 100644 >> --- a/block/vpc.c >> +++ b/block/vpc.c >> @@ -427,8 +427,11 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, >> "does not support live migration", >> 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) { >> + 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..f9a5f9b 100644 >> --- a/block/vvfat.c >> +++ b/block/vvfat.c >> @@ -1192,8 +1192,11 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, >> "does not support live migration", >> 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) { >> + 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 e72ef43..01dd67f 100644 >> --- a/hw/9pfs/9p.c >> +++ b/hw/9pfs/9p.c >> @@ -1020,12 +1020,17 @@ static void coroutine_fn v9fs_attach(void *opaque) >> */ >> if (!s->migration_blocker) { >> int ret; >> + Error *local_err; >> s->root_fid = fid; >> 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); >> - if (ret < 0) { >> + ret = migrate_add_blocker(s->migration_blocker, &local_err); >> + if (ret) { > > Why s/ret < 0/ret/ ? The migrate_add_blocker() function in this v2 only > returns 0 or a negative errno... No reason, maybe I forgot to change it back because I got no logical error after compiling but I have fixed this at all occurrences. > >> + if (ret == -EACCES) { >> + error_append_hint(&local_err, "Failed to mount VirtFS as it " >> + "does not support live migration"); > > And then local_err is leaked... Yes, I have freed it with error_free(). > > BTW, I'm not even sure if this useful since this is a guest originated action. > >> + } > > This errno will be ultimately be returned to the mount(2) caller in the guest. > > When reading the manpage, it looks like EBUSY definitely makes more sense than > EACCES: > > EACCES A component of a path was not searchable. (See also path_reso‐ > lution(7).) Or, mounting a read-only filesystem was attempted > without giving the MS_RDONLY flag. Or, the block device source > is located on a filesystem mounted with the MS_NODEV option. > > EBUSY source is already mounted. Or, it cannot be remounted read- > only, because it still holds files open for writing. Or, it > cannot be mounted on target because target is still busy (it is > the working directory of some thread, the mount point of another > device, has open files, etc.). > Reading the manpages definitely make your argument stronger but I am not sure if I will be able to differentiate between what caused the migrate_add_blocker to fail if I get -EBUSY for both the cases. Ashijeet >> err = ret; >> clunk_fid(s, fid); >> goto out; >> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c >> index 6e60b8c..1b02f4b 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) { >> + 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..b40ad5f 100644 >> --- a/hw/intc/arm_gic_kvm.c >> +++ b/hw/intc/arm_gic_kvm.c >> @@ -514,8 +514,11 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp) >> error_setg(&s->migration_blocker, "This operating system kernel does " >> "not support vGICv2 migration"); >> ret = migrate_add_blocker(s->migration_blocker, errp); >> - if (ret < 0) { >> - error_free(s->migration_blocker); >> + if (ret) { >> + 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..58f27b1 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) { >> + 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..0c571f8 100644 >> --- a/hw/scsi/vhost-scsi.c >> +++ b/hw/scsi/vhost-scsi.c >> @@ -244,8 +244,12 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) >> error_setg(&s->migration_blocker, >> "vhost-scsi does not support migration"); >> ret = migrate_add_blocker(s->migration_blocker, errp); >> - if (ret < 0) { >> - goto free_blocker; >> + if (ret) { >> + 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..1c4a4f9 100644 >> --- a/hw/virtio/vhost.c >> +++ b/hw/virtio/vhost.c >> @@ -1159,9 +1159,11 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, >> if (hdev->migration_blocker != NULL) { >> 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) { >> + 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..05c8365 100644 >> --- a/target-i386/kvm.c >> +++ b/target-i386/kvm.c >> @@ -962,7 +962,11 @@ int kvm_arch_init_vcpu(CPUState *cs) >> "State blocked by non-migratable CPU device" >> " (invtsc flag)"); >> r = migrate_add_blocker(invtsc_mig_blocker, NULL); >> - if (r < 0) { >> + if (r) { >> + 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; >> } >> >
On Fri, Dec 16, 2016 at 5:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 16 December 2016 at 09:53, 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> > >> diff --git a/block/qcow.c b/block/qcow.c >> index 11526a1..7e5003ac 100644 >> --- a/block/qcow.c >> +++ b/block/qcow.c >> @@ -253,8 +253,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, >> "does not support live migration", >> 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) { >> + if (ret == -EACCES) { >> + error_append_hint(errp, "Cannot use a node with qcow format as " >> + "it does not support live migration"); >> + } >> goto fail; >> } > >> --- 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) { >> + 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..b40ad5f 100644 >> --- a/hw/intc/arm_gic_kvm.c >> +++ b/hw/intc/arm_gic_kvm.c >> @@ -514,8 +514,11 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp) >> error_setg(&s->migration_blocker, "This operating system kernel does " >> "not support vGICv2 migration"); >> ret = migrate_add_blocker(s->migration_blocker, errp); >> - if (ret < 0) { >> - error_free(s->migration_blocker); >> + if (ret) { >> + if (ret == -EACCES) { >> + error_append_hint(errp, "Failed to realize vGICv2 as its" >> + " migrataion is not implemented yet"); >> + } >> return; >> } >> } > > So if you look at these diff hunks you can see that we effectively > end up duplicating the "why doesn't this device let you migrate" > user message -- once in the error_setg(&s->migration_blocker, ...) > and once with the error_append_hint() call. Maybe changing the error message in error_setg(&s->migration_blocker, ...) to: "Trying to add migration blocker but will fail if --only-migratable is specified" or something like that do the trick? > > Is there some way we can get migrate_add_blocker() to automatically > incorporate the message when it's failing for the only-migratable case? > (It's already being passed the errp.) Sorry! I am not sure how I can use that but if you have something more I am ready to work on it to fix this. > That would avoid this duplication of the messages, and should also > mean we don't need to modify the callers at all. Right. I also thought so while writing this patch but wasn't able to come up with a solution. Ashijeet > > If the error messages come out looking a bit strangely worded we > could edit the text in the callers so it works in both situations > when it will be used. > > thanks > -- PMM
diff --git a/block/qcow.c b/block/qcow.c index 11526a1..7e5003ac 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -253,8 +253,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, "does not support live migration", 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) { + 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..df45df4 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -472,8 +472,11 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, "does not support live migration", 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) { + 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..118134e 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -996,11 +996,13 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags, "does not support live migration", 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) { + 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..cd6a641 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -977,8 +977,11 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags, "does not support live migration", 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) { + 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..c935962 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -427,8 +427,11 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, "does not support live migration", 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) { + 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..f9a5f9b 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -1192,8 +1192,11 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, "does not support live migration", 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) { + 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 e72ef43..01dd67f 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -1020,12 +1020,17 @@ static void coroutine_fn v9fs_attach(void *opaque) */ if (!s->migration_blocker) { int ret; + Error *local_err; s->root_fid = fid; 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); - if (ret < 0) { + ret = migrate_add_blocker(s->migration_blocker, &local_err); + if (ret) { + 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); goto out; diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index 6e60b8c..1b02f4b 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) { + 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..b40ad5f 100644 --- a/hw/intc/arm_gic_kvm.c +++ b/hw/intc/arm_gic_kvm.c @@ -514,8 +514,11 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp) error_setg(&s->migration_blocker, "This operating system kernel does " "not support vGICv2 migration"); ret = migrate_add_blocker(s->migration_blocker, errp); - if (ret < 0) { - error_free(s->migration_blocker); + if (ret) { + 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..58f27b1 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) { + 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..0c571f8 100644 --- a/hw/scsi/vhost-scsi.c +++ b/hw/scsi/vhost-scsi.c @@ -244,8 +244,12 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) error_setg(&s->migration_blocker, "vhost-scsi does not support migration"); ret = migrate_add_blocker(s->migration_blocker, errp); - if (ret < 0) { - goto free_blocker; + if (ret) { + 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..1c4a4f9 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1159,9 +1159,11 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, if (hdev->migration_blocker != NULL) { 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) { + 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..05c8365 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -962,7 +962,11 @@ int kvm_arch_init_vcpu(CPUState *cs) "State blocked by non-migratable CPU device" " (invtsc flag)"); r = migrate_add_blocker(invtsc_mig_blocker, NULL); - if (r < 0) { + if (r) { + 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; }
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 | 7 +++++-- block/vdi.c | 7 +++++-- block/vhdx.c | 8 +++++--- block/vmdk.c | 7 +++++-- block/vpc.c | 7 +++++-- block/vvfat.c | 8 +++++--- hw/9pfs/9p.c | 9 +++++++-- hw/display/virtio-gpu.c | 9 +++++++-- hw/intc/arm_gic_kvm.c | 7 +++++-- 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 | 10 ++++++---- hw/virtio/vhost.c | 8 +++++--- migration/migration.c | 7 +++++++ target-i386/kvm.c | 7 +++++-- 16 files changed, 88 insertions(+), 33 deletions(-)