| Message ID | 1483981368-9965-5-git-send-email-ashijeetacharya@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/09/2017 11:02 AM, Ashijeet Acharya wrote: s/migratble/migratable/ in the subject > 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> > ---
* 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/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 | 8 +++++--- > hw/virtio/vhost.c | 6 ++++-- > migration/migration.c | 7 +++++++ > target-i386/kvm.c | 5 ++++- > 15 files changed, 72 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/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..2097808 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 < 0 ) { > + if (ret == -EACCES) { > + error_append_hint(errp, "Failed to realize vGICv2 as its" > + " migrataion is not implemented yet"); Typo of migrataion. Also is that message right? I think that should mention the OS kernel. > + } > 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"); In this case there are 2 different reasons that it might be disabled; we've lost that text. It seems best in this case to use the message from hdev->migration_blocker. > + } > 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); That free doesn't look right there; isn't that freeing the reason that is added to the list and will get used later if someone tries to start a migration? Dave > 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; > } > > -- > 2.6.2 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 9 January 2017 at 17:02, 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. > 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; > } > The error handling for these call sites should look just like that for any other function call that takes an Error**: Error *local_err = NULL; [...] migrate_add_blocker(s->migration_blocker, &local_err); if (local_err) { error_propagate(errp, local_err); return; // or otherwise handle failure appropriately } migrate_add_blocker() should just internally construct the error text and extra hint lines by looking at the text it can fish out of the s->migration_blocker argument and calling error_append_hint() itself. The patch is also a bit odd because the error_free() calls were only added in patch 3/4, right? Generally adding lines of code in one patch and deleting them in the next is a bad idea. thanks -- PMM
On Tue, Jan 10, 2017 at 10:45 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 9 January 2017 at 17:02, 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. > >> 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; >> } >> > > The error handling for these call sites should look just like > that for any other function call that takes an Error**: > > Error *local_err = NULL; > [...] > migrate_add_blocker(s->migration_blocker, &local_err); > if (local_err) { > error_propagate(errp, local_err); > return; // or otherwise handle failure appropriately > } > I think it will be better to make migrate_add_blocker() to return the error value as well, otherwise we will end up setting ret in all the callers manually and that will lead to a repetition of code at all call sites, right? Refer to qcow for an example... > migrate_add_blocker() should just internally construct > the error text and extra hint lines by looking at the > text it can fish out of the s->migration_blocker argument > and calling error_append_hint() itself. > Yes, I have done that now. > The patch is also a bit odd because the error_free() calls > were only added in patch 3/4, right? Generally adding > lines of code in one patch and deleting them in the next > is a bad idea. Yes, I have removed that as well. Ashijeet > > thanks > -- PMM
On Tuesday, 10 January 2017, Peter Maydell <peter.maydell@linaro.org> wrote: > On 9 January 2017 at 17:02, Ashijeet Acharya <ashijeetacharya@gmail.com > <javascript:;>> 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. > > > 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; > > } > > > > The error handling for these call sites should look just like > that for any other function call that takes an Error**: > > Error *local_err = NULL; > [...] > migrate_add_blocker(s->migration_blocker, &local_err); > if (local_err) { > error_propagate(errp, local_err); > return; // or otherwise handle failure appropriately > } One more reason Peter I found for returning an error value is that in cases like 9pfs where we do not set errp inside migrate_add_blocker() (as suggested by Greg) and other similar ones where we pass NULL for errp, we cannot rely on checking for "if (local_err)" as it will never be set and always be NULL. Thus we will never fail appropriately at the caller sites when we fail to add migration blocker. Sounds right? Ashijeet > > migrate_add_blocker() should just internally construct > the error text and extra hint lines by looking at the > text it can fish out of the s->migration_blocker argument > and calling error_append_hint() itself. > > The patch is also a bit odd because the error_free() calls > were only added in patch 3/4, right? Generally adding > lines of code in one patch and deleting them in the next > is a bad idea. > > thanks > -- PMM >
On Thu, 12 Jan 2017 16:15:28 +0530 Ashijeet Acharya <ashijeetacharya@gmail.com> wrote: > On Tuesday, 10 January 2017, Peter Maydell <peter.maydell@linaro.org> wrote: > > > On 9 January 2017 at 17:02, Ashijeet Acharya <ashijeetacharya@gmail.com > > <javascript:;>> 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. > > > > > 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; > > > } > > > > > > > The error handling for these call sites should look just like > > that for any other function call that takes an Error**: > > > > Error *local_err = NULL; > > [...] > > migrate_add_blocker(s->migration_blocker, &local_err); > > if (local_err) { > > error_propagate(errp, local_err); > > return; // or otherwise handle failure appropriately > > } > > > One more reason Peter I found for returning an error value is that in cases > like 9pfs where we do not set errp inside migrate_add_blocker() (as > suggested by Greg) and other similar ones where we pass NULL for errp, we > cannot rely on checking for > "if (local_err)" as it will never be set and always be NULL. > Thus we will never fail appropriately at the caller sites when we fail to > add migration blocker. > Sounds right? > The need for 9pfs is just to return an error to the guest, which will end up being the errno for the failed mount(). And we don't want to print out any error message to avoid a stupid guest to fill the QEMU log in case it would loop over mount(). The only reason I see for migration_add_blocker() to return an error would be to differentiate between the --only-migratable and the migration in progress cases... But honestly, I don't care that much and something like the following would be perfectly ok: Error *local_err = NULL; [...] migrate_add_blocker(s->migration_blocker, &local_err); if (local_err) { error_free(local_err); err = -EBUSY goto out_nofid; } > Ashijeet > > > > > migrate_add_blocker() should just internally construct > > the error text and extra hint lines by looking at the > > text it can fish out of the s->migration_blocker argument > > and calling error_append_hint() itself. > > > > The patch is also a bit odd because the error_free() calls > > were only added in patch 3/4, right? Generally adding > > lines of code in one patch and deleting them in the next > > is a bad idea. > > > > thanks > > -- PMM > >
On Thursday, 12 January 2017, Greg Kurz <groug@kaod.org> wrote: > On Thu, 12 Jan 2017 16:15:28 +0530 > Ashijeet Acharya <ashijeetacharya@gmail.com <javascript:;>> wrote: > > > On Tuesday, 10 January 2017, Peter Maydell <peter.maydell@linaro.org > <javascript:;>> wrote: > > > > > On 9 January 2017 at 17:02, Ashijeet Acharya < > ashijeetacharya@gmail.com <javascript:;> > > > <javascript:;>> 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. > > > > > > > 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; > > > > } > > > > > > > > > > The error handling for these call sites should look just like > > > that for any other function call that takes an Error**: > > > > > > Error *local_err = NULL; > > > [...] > > > migrate_add_blocker(s->migration_blocker, &local_err); > > > if (local_err) { > > > error_propagate(errp, local_err); > > > return; // or otherwise handle failure appropriately > > > } > > > > > > One more reason Peter I found for returning an error value is that in > cases > > like 9pfs where we do not set errp inside migrate_add_blocker() (as > > suggested by Greg) and other similar ones where we pass NULL for errp, we > > cannot rely on checking for > > "if (local_err)" as it will never be set and always be NULL. > > Thus we will never fail appropriately at the caller sites when we fail to > > add migration blocker. > > Sounds right? > > > > The need for 9pfs is just to return an error to the guest, which will end > up being the errno for the failed mount(). And we don't want to print out > any error message to avoid a stupid guest to fill the QEMU log in case it > would loop over mount(). Yes, I completely understood that earlier :-) I had a doubt further because atm I was passing NULL as an argument i.e. migrate_add_blocker(s->migration_blocker, NULL); which is why the whole 'local_err being returned as NULL' situation arrived. But I will make it pass &local_err now. Still, I think we need to return an error value from migrate_add_blocker() to avoid setting it manually and also avoid repetition of code at call sites. > > The only reason I see for migration_add_blocker() to return an error > would be to differentiate between the --only-migratable and the migration > in progress cases... But honestly, I don't care that much and something > like the following would be perfectly ok: > > Error *local_err = NULL; > [...] > migrate_add_blocker(s->migration_blocker, &local_err); > if (local_err) { > error_free(local_err); > err = -EBUSY > goto out_nofid; > } > > Maybe yes, if everyone is okay with the same error value being set in both the cases, then I will submit the patches like you proposed above :-) Ashijeet
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/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..2097808 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 < 0 ) { + 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; }
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/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 | 8 +++++--- hw/virtio/vhost.c | 6 ++++-- migration/migration.c | 7 +++++++ target-i386/kvm.c | 5 ++++- 15 files changed, 72 insertions(+), 22 deletions(-)