* [Qemu-devel] [PATCH v4 0/4] Introduce a new --only-migratable option
@ 2017-01-09 17:02 Ashijeet Acharya
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 1/4] migration: Add a new option to enable only-migratable Ashijeet Acharya
` (4 more replies)
0 siblings, 5 replies; 18+ messages in thread
From: Ashijeet Acharya @ 2017-01-09 17:02 UTC (permalink / raw)
To: dgilbert
Cc: jsnow, amit.shah, pbonzini, kwolf, armbru, quintela, mst,
marcandre.lureau, groug, aneesh.kumar, peter.maydell, qemu-devel,
Ashijeet Acharya
Previously posted series patches:
http://lists.nongnu.org/archive/html/qemu-devel/2017-01/msg00320.html
http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg02391.html
http://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg02062.html
This series adds a new command line option "--only-migratable" which will only
allow addition of those devices to a QEMU instance which are migratable and do
not abruptly fail QEMU after migration.
Patch 1 adds the new option "-only-migratable".
Patch 2 adds compatibility for various "device adding" options for both via
command line and hotplug methods.
Patch 3 helps to fail the migration blocker if the migration is already in
progress and thus cannot be blocked.
Note: This patch was originally written by John Snow and I have only made few
changes.
Patch 4 handles the special case of devices which become unmigratable
dynamically
by making call to "migrate_add_blocker". Here we fail the particular action of
the
device which results in an unmigratable VM.
Eg: 9pfs fails to mount the filesystem.
Note: I have not been able to test and compile the ARM drivers for KVM. They
are:
hw/intc/arm_gic_kvm.c
hw/intc/arm_gicv3_its_kvm.c
hw/intc/arm_gicv3_kvm.c
Changes in v4:
- drop diff in 9pfs for patch 4/4
- call clunk_fid() after freeing migration_blocker
- drop ret and use err directly
Changes in v3:
- set s->root_fid after migrate_add_blocker
- free migration_blocker inside v9fs_attach()
- change back ret<0 to just ret
- free local_err
Changes in v2:
- change the documentation for the new option
- add a NULL check for ObjectClass
- break patch 3 into patch 3 and 4
- use error_append_hint
- return -EACCES for only-migratable
- fix the error messages
Ashijeet Acharya (4):
migration: Add a new option to enable only-migratable
migration: Allow "device add" options to only add migratable devices
migration: disallow migrate_add_blocker during migration
migration: Fail migration blocker for --only-migratble
block/qcow.c | 9 ++++++++-
block/vdi.c | 9 ++++++++-
block/vhdx.c | 18 ++++++++++++------
block/vmdk.c | 10 +++++++++-
block/vpc.c | 13 ++++++++++---
block/vvfat.c | 22 ++++++++++++++--------
hw/9pfs/9p.c | 18 +++++++++++++-----
hw/display/virtio-gpu.c | 34 +++++++++++++++++++++-------------
hw/intc/arm_gic_kvm.c | 19 +++++++++++++------
hw/intc/arm_gicv3_its_kvm.c | 21 ++++++++++++++-------
hw/intc/arm_gicv3_kvm.c | 22 +++++++++++++++-------
hw/misc/ivshmem.c | 17 +++++++++++++----
hw/scsi/vhost-scsi.c | 27 +++++++++++++++++++++------
hw/usb/bus.c | 18 ++++++++++++++++++
hw/virtio/vhost.c | 10 +++++++++-
include/migration/migration.h | 9 ++++++++-
migration/migration.c | 42 ++++++++++++++++++++++++++++++++++++++++--
qdev-monitor.c | 9 +++++++++
qemu-options.hx | 9 +++++++++
stubs/migr-blocker.c | 3 ++-
target-i386/kvm.c | 19 ++++++++++++++++---
vl.c | 4 ++++
22 files changed, 286 insertions(+), 76 deletions(-)
--
2.6.2
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v4 1/4] migration: Add a new option to enable only-migratable
2017-01-09 17:02 [Qemu-devel] [PATCH v4 0/4] Introduce a new --only-migratable option Ashijeet Acharya
@ 2017-01-09 17:02 ` Ashijeet Acharya
2017-01-10 11:02 ` Dr. David Alan Gilbert
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 2/4] migration: Allow "device add" options to only add migratable devices Ashijeet Acharya
` (3 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Ashijeet Acharya @ 2017-01-09 17:02 UTC (permalink / raw)
To: dgilbert
Cc: jsnow, amit.shah, pbonzini, kwolf, armbru, quintela, mst,
marcandre.lureau, groug, aneesh.kumar, peter.maydell, qemu-devel,
Ashijeet Acharya
Add a new option "--only-migratable" in qemu which will allow to add
only those devices which will not fail qemu after migration. Devices
set with the flag 'unmigratable' cannot be added when this option will
be used.
Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
include/migration/migration.h | 3 +++
qemu-options.hx | 9 +++++++++
vl.c | 4 ++++
3 files changed, 16 insertions(+)
diff --git a/include/migration/migration.h b/include/migration/migration.h
index c309d23..40b3697 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -38,6 +38,9 @@
#define QEMU_VM_COMMAND 0x08
#define QEMU_VM_SECTION_FOOTER 0x7e
+/* for vl.c */
+extern int only_migratable;
+
struct MigrationParams {
bool blk;
bool shared;
diff --git a/qemu-options.hx b/qemu-options.hx
index c534a2f..1e16ae8 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3574,6 +3574,15 @@ be used to change settings (such as migration parameters) prior to issuing
the migrate_incoming to allow the migration to begin.
ETEXI
+DEF("only-migratable", 0, QEMU_OPTION_only_migratable, \
+ "-only-migratable allow only migratable devices\n", QEMU_ARCH_ALL)
+STEXI
+@item -only-migratable
+@findex -only-migratable
+Only allow migratable devices. Devices will not be allowed to enter an
+unmigratable state.
+ETEXI
+
DEF("nodefaults", 0, QEMU_OPTION_nodefaults, \
"-nodefaults don't create default devices\n", QEMU_ARCH_ALL)
STEXI
diff --git a/vl.c b/vl.c
index d77dd86..82bffb9 100644
--- a/vl.c
+++ b/vl.c
@@ -180,6 +180,7 @@ bool boot_strict;
uint8_t *boot_splash_filedata;
size_t boot_splash_filedata_size;
uint8_t qemu_extra_params_fw[2];
+int only_migratable = 0; /* turn it off unless user states otherwise */
int icount_align_option;
@@ -3914,6 +3915,9 @@ int main(int argc, char **argv, char **envp)
}
incoming = optarg;
break;
+ case QEMU_OPTION_only_migratable:
+ only_migratable = 1;
+ break;
case QEMU_OPTION_nodefaults:
has_defaults = 0;
break;
--
2.6.2
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v4 2/4] migration: Allow "device add" options to only add migratable devices
2017-01-09 17:02 [Qemu-devel] [PATCH v4 0/4] Introduce a new --only-migratable option Ashijeet Acharya
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 1/4] migration: Add a new option to enable only-migratable Ashijeet Acharya
@ 2017-01-09 17:02 ` Ashijeet Acharya
2017-01-10 11:23 ` Dr. David Alan Gilbert
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 3/4] migration: disallow migrate_add_blocker during migration Ashijeet Acharya
` (2 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Ashijeet Acharya @ 2017-01-09 17:02 UTC (permalink / raw)
To: dgilbert
Cc: jsnow, amit.shah, pbonzini, kwolf, armbru, quintela, mst,
marcandre.lureau, groug, aneesh.kumar, peter.maydell, qemu-devel,
Ashijeet Acharya
Introduce checks for the unmigratable flag in the VMStateDescription
structs of respective devices when user attempts to add them. If the
"--only-migratable" was specified, all unmigratable devices will
rightly fail to add. This feature is made compatible for both "-device"
and "-usbdevice" command line options and covers their hmp and qmp
counterparts as well.
Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
hw/usb/bus.c | 18 ++++++++++++++++++
qdev-monitor.c | 9 +++++++++
2 files changed, 27 insertions(+)
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 25913ad..3d26103 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -8,6 +8,7 @@
#include "monitor/monitor.h"
#include "trace.h"
#include "qemu/cutils.h"
+#include "migration/migration.h"
static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
@@ -686,6 +687,8 @@ USBDevice *usbdevice_create(const char *cmdline)
const char *params;
int len;
USBDevice *dev;
+ ObjectClass *klass;
+ DeviceClass *dc;
params = strchr(cmdline,':');
if (params) {
@@ -720,6 +723,21 @@ USBDevice *usbdevice_create(const char *cmdline)
return NULL;
}
+ klass = object_class_by_name(f->name);
+ if (klass == NULL) {
+ return NULL;
+ }
+
+ dc = DEVICE_CLASS(klass);
+
+ if (only_migratable) {
+ if (dc->vmsd->unmigratable) {
+ error_report("Device %s is not migratable, but --only-migratable "
+ "was specified", f->name);
+ return NULL;
+ }
+ }
+
if (f->usbdevice_init) {
dev = f->usbdevice_init(bus, params);
} else {
diff --git a/qdev-monitor.c b/qdev-monitor.c
index c73410c..81d01df 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -29,6 +29,7 @@
#include "qemu/error-report.h"
#include "qemu/help_option.h"
#include "sysemu/block-backend.h"
+#include "migration/migration.h"
/*
* Aliases were a bad idea from the start. Let's keep them
@@ -577,6 +578,14 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
return NULL;
}
+ if (only_migratable) {
+ if (dc->vmsd->unmigratable) {
+ error_setg(errp, "Device %s is not migratable, but "
+ "--only-migratable was specified", driver);
+ return NULL;
+ }
+ }
+
/* find bus */
path = qemu_opt_get(opts, "bus");
if (path != NULL) {
--
2.6.2
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v4 3/4] migration: disallow migrate_add_blocker during migration
2017-01-09 17:02 [Qemu-devel] [PATCH v4 0/4] Introduce a new --only-migratable option Ashijeet Acharya
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 1/4] migration: Add a new option to enable only-migratable Ashijeet Acharya
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 2/4] migration: Allow "device add" options to only add migratable devices Ashijeet Acharya
@ 2017-01-09 17:02 ` Ashijeet Acharya
2017-01-10 8:25 ` Greg Kurz
2017-01-10 12:01 ` Dr. David Alan Gilbert
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 4/4] migration: Fail migration blocker for --only-migratble Ashijeet Acharya
2017-01-09 17:27 ` [Qemu-devel] [PATCH v4 0/4] Introduce a new --only-migratable option no-reply
4 siblings, 2 replies; 18+ messages in thread
From: Ashijeet Acharya @ 2017-01-09 17:02 UTC (permalink / raw)
To: dgilbert
Cc: jsnow, amit.shah, pbonzini, kwolf, armbru, quintela, mst,
marcandre.lureau, groug, aneesh.kumar, peter.maydell, qemu-devel,
Ashijeet Acharya
If a migration is already in progress and somebody attempts
to add a migration blocker, this should rightly fail.
Add an errp parameter and a retcode return value to migrate_add_blocker.
Signed-off-by: John Snow <jsnow@redhat.com>
Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
block/qcow.c | 6 +++++-
block/vdi.c | 6 +++++-
block/vhdx.c | 16 ++++++++++------
block/vmdk.c | 7 ++++++-
block/vpc.c | 10 +++++++---
block/vvfat.c | 20 ++++++++++++--------
hw/9pfs/9p.c | 18 +++++++++++++-----
hw/display/virtio-gpu.c | 29 ++++++++++++++++-------------
hw/intc/arm_gic_kvm.c | 16 ++++++++++------
hw/intc/arm_gicv3_its_kvm.c | 18 +++++++++++-------
hw/intc/arm_gicv3_kvm.c | 19 ++++++++++++-------
hw/misc/ivshmem.c | 11 +++++++----
hw/scsi/vhost-scsi.c | 25 +++++++++++++++++++------
hw/virtio/vhost.c | 8 +++++++-
include/migration/migration.h | 6 +++++-
migration/migration.c | 35 +++++++++++++++++++++++++++++++++--
stubs/migr-blocker.c | 3 ++-
target-i386/kvm.c | 16 +++++++++++++---
18 files changed, 193 insertions(+), 76 deletions(-)
diff --git a/block/qcow.c b/block/qcow.c
index 7540f43..11526a1 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -252,7 +252,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
error_setg(&s->migration_blocker, "The qcow format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
- migrate_add_blocker(s->migration_blocker);
+ ret = migrate_add_blocker(s->migration_blocker, errp);
+ if (ret < 0) {
+ error_free(s->migration_blocker);
+ goto fail;
+ }
qemu_co_mutex_init(&s->lock);
return 0;
diff --git a/block/vdi.c b/block/vdi.c
index 96b78d5..2b56f52 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -471,7 +471,11 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
error_setg(&s->migration_blocker, "The vdi format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
- migrate_add_blocker(s->migration_blocker);
+ ret = migrate_add_blocker(s->migration_blocker, errp);
+ if (ret < 0) {
+ error_free(s->migration_blocker);
+ goto fail_free_bmap;
+ }
qemu_co_mutex_init(&s->write_lock);
diff --git a/block/vhdx.c b/block/vhdx.c
index 0ba2f0a..d81941b 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -991,6 +991,16 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
}
}
+ /* Disable migration when VHDX images are used */
+ error_setg(&s->migration_blocker, "The vhdx format used by node '%s' "
+ "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);
+ goto fail;
+ }
+
if (flags & BDRV_O_RDWR) {
ret = vhdx_update_headers(bs, s, false, NULL);
if (ret < 0) {
@@ -1000,12 +1010,6 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
/* TODO: differencing files */
- /* Disable migration when VHDX images are used */
- error_setg(&s->migration_blocker, "The vhdx format used by node '%s' "
- "does not support live migration",
- bdrv_get_device_or_node_name(bs));
- migrate_add_blocker(s->migration_blocker);
-
return 0;
fail:
vhdx_close(bs);
diff --git a/block/vmdk.c b/block/vmdk.c
index a11c27a..4a45a83 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -976,7 +976,12 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
error_setg(&s->migration_blocker, "The vmdk format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
- migrate_add_blocker(s->migration_blocker);
+ ret = migrate_add_blocker(s->migration_blocker, errp);
+ if (ret < 0) {
+ error_free(s->migration_blocker);
+ goto fail;
+ }
+
g_free(buf);
return 0;
diff --git a/block/vpc.c b/block/vpc.c
index 8d5886f..299a8c8 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -422,13 +422,17 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
#endif
}
- qemu_co_mutex_init(&s->lock);
-
/* Disable migration when VHD images are used */
error_setg(&s->migration_blocker, "The vpc format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
- migrate_add_blocker(s->migration_blocker);
+ ret = migrate_add_blocker(s->migration_blocker, errp);
+ if (ret < 0) {
+ error_free(s->migration_blocker);
+ goto fail;
+ }
+
+ qemu_co_mutex_init(&s->lock);
return 0;
diff --git a/block/vvfat.c b/block/vvfat.c
index ded2109..3de3320 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1185,22 +1185,26 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count;
- if (s->first_sectors_number == 0x40) {
- init_mbr(s, cyls, heads, secs);
- }
-
- // assert(is_consistent(s));
- qemu_co_mutex_init(&s->lock);
-
/* Disable migration when vvfat is used rw */
if (s->qcow) {
error_setg(&s->migration_blocker,
"The vvfat (rw) format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
- migrate_add_blocker(s->migration_blocker);
+ ret = migrate_add_blocker(s->migration_blocker, errp);
+ if (ret < 0) {
+ error_free(s->migration_blocker);
+ goto fail;
+ }
+ }
+
+ if (s->first_sectors_number == 0x40) {
+ init_mbr(s, cyls, heads, secs);
}
+ // assert(is_consistent(s));
+ qemu_co_mutex_init(&s->lock);
+
ret = 0;
fail:
qemu_opts_del(opts);
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index faebd91..c823a44 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1013,20 +1013,28 @@ static void coroutine_fn v9fs_attach(void *opaque)
goto out;
}
err += offset;
- memcpy(&s->root_qid, &qid, sizeof(qid));
- trace_v9fs_attach_return(pdu->tag, pdu->id,
- qid.type, qid.version, qid.path);
+
/*
* disable migration if we haven't done already.
* attach could get called multiple times for the same export.
*/
if (!s->migration_blocker) {
- 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);
- migrate_add_blocker(s->migration_blocker);
+ err = migrate_add_blocker(s->migration_blocker, NULL);
+ if (err < 0) {
+ error_free(s->migration_blocker);
+ s->migration_blocker = NULL;
+ clunk_fid(s, fid);
+ goto out;
+ }
+ s->root_fid = fid;
}
+
+ memcpy(&s->root_qid, &qid, sizeof(qid));
+ trace_v9fs_attach_return(pdu->tag, pdu->id,
+ qid.type, qid.version, qid.path);
out:
put_fid(pdu, fidp);
out_nofid:
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 5f32e1a..6e60b8c 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1108,14 +1108,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
return;
}
- g->config_size = sizeof(struct virtio_gpu_config);
- g->virtio_config.num_scanouts = g->conf.max_outputs;
- virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU,
- g->config_size);
-
- g->req_state[0].width = 1024;
- g->req_state[0].height = 768;
-
g->use_virgl_renderer = false;
#if !defined(CONFIG_VIRGL) || defined(HOST_WORDS_BIGENDIAN)
have_virgl = false;
@@ -1127,6 +1119,22 @@ 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);
+ return;
+ }
+ }
+
+ g->config_size = sizeof(struct virtio_gpu_config);
+ g->virtio_config.num_scanouts = g->conf.max_outputs;
+ virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU,
+ g->config_size);
+
+ g->req_state[0].width = 1024;
+ g->req_state[0].height = 768;
+
+ if (virtio_gpu_virgl_enabled(g->conf)) {
/* use larger control queue in 3d mode */
g->ctrl_vq = virtio_add_queue(vdev, 256, virtio_gpu_handle_ctrl_cb);
g->cursor_vq = virtio_add_queue(vdev, 16, virtio_gpu_handle_cursor_cb);
@@ -1152,11 +1160,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
dpy_gfx_replace_surface(g->scanout[i].con, NULL);
}
}
-
- if (virtio_gpu_virgl_enabled(g->conf)) {
- error_setg(&g->migration_blocker, "virgl is not yet migratable");
- migrate_add_blocker(g->migration_blocker);
- }
}
static void virtio_gpu_device_unrealize(DeviceState *qdev, Error **errp)
diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index 11729ee..3614daa 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -510,6 +510,16 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
return;
}
+ if (!kvm_arm_gic_can_save_restore(s)) {
+ 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);
+ return;
+ }
+ }
+
gic_init_irqs_and_mmio(s, kvm_arm_gicv2_set_irq, NULL);
for (i = 0; i < s->num_irq - GIC_INTERNAL; i++) {
@@ -558,12 +568,6 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
KVM_VGIC_V2_ADDR_TYPE_CPU,
s->dev_fd);
- if (!kvm_arm_gic_can_save_restore(s)) {
- error_setg(&s->migration_blocker, "This operating system kernel does "
- "not support vGICv2 migration");
- migrate_add_blocker(s->migration_blocker);
- }
-
if (kvm_has_gsi_routing()) {
/* set up irq routing */
kvm_init_irq_routing(kvm_state);
diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index fc246e0..950a02f 100644
--- a/hw/intc/arm_gicv3_its_kvm.c
+++ b/hw/intc/arm_gicv3_its_kvm.c
@@ -63,6 +63,17 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
return;
}
+ /*
+ * Block migration of a KVM GICv3 ITS device: the API for saving and
+ * restoring the state in the kernel is not yet available
+ */
+ 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);
+ return;
+ }
+
/* explicit init of the ITS */
kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
@@ -73,13 +84,6 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
gicv3_its_init_mmio(s, NULL);
- /*
- * Block migration of a KVM GICv3 ITS device: the API for saving and
- * restoring the state in the kernel is not yet available
- */
- error_setg(&s->migration_blocker, "vITS migration is not implemented");
- migrate_add_blocker(s->migration_blocker);
-
kvm_msi_use_devid = true;
kvm_gsi_direct_mapping = false;
kvm_msi_via_irqfd_allowed = kvm_irqfds_enabled();
diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 199a439..06f6f30 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -86,6 +86,7 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
Error *local_err = NULL;
int i;
+ int ret;
DPRINTF("kvm_arm_gicv3_realize\n");
@@ -110,6 +111,17 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
return;
}
+ /* Block migration of a KVM GICv3 device: the API for saving and restoring
+ * the state in the kernel is not yet finalised in the kernel or
+ * implemented in QEMU.
+ */
+ 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);
+ return;
+ }
+
kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
0, &s->num_irq, true);
@@ -122,13 +134,6 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
kvm_arm_register_device(&s->iomem_redist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd);
- /* Block migration of a KVM GICv3 device: the API for saving and restoring
- * the state in the kernel is not yet finalised in the kernel or
- * implemented in QEMU.
- */
- error_setg(&s->migration_blocker, "vGICv3 migration is not implemented");
- migrate_add_blocker(s->migration_blocker);
-
if (kvm_has_gsi_routing()) {
/* set up irq routing */
kvm_init_irq_routing(kvm_state);
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index abeaf3d..585cc5b 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -903,9 +903,6 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
}
}
- vmstate_register_ram(s->ivshmem_bar2, DEVICE(s));
- pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2);
-
if (s->master == ON_OFF_AUTO_AUTO) {
s->master = s->vm_id == 0 ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
}
@@ -913,8 +910,14 @@ 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'");
- migrate_add_blocker(s->migration_blocker);
+ if (migrate_add_blocker(s->migration_blocker, errp) < 0) {
+ error_free(s->migration_blocker);
+ return;
+ }
}
+
+ vmstate_register_ram(s->ivshmem_bar2, DEVICE(s));
+ pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2);
}
static void ivshmem_exit(PCIDevice *dev)
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 5b26946..ff503d0 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -238,8 +238,14 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
vhost_dummy_handle_output);
if (err != NULL) {
error_propagate(errp, err);
- close(vhostfd);
- return;
+ goto close_fd;
+ }
+
+ 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;
}
s->dev.nvqs = VHOST_SCSI_VQ_NUM_FIXED + vs->conf.num_queues;
@@ -252,7 +258,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
if (ret < 0) {
error_setg(errp, "vhost-scsi: vhost initialization failed: %s",
strerror(-ret));
- return;
+ goto free_vqs;
}
/* At present, channel and lun both are 0 for bootable vhost-scsi disk */
@@ -261,9 +267,16 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
/* Note: we can also get the minimum tpgt from kernel */
s->target = vs->conf.boot_tpgt;
- error_setg(&s->migration_blocker,
- "vhost-scsi does not support migration");
- migrate_add_blocker(s->migration_blocker);
+ return;
+
+ 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;
}
static void vhost_scsi_unrealize(DeviceState *dev, Error **errp)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index f7f7023..416fada 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1157,7 +1157,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
}
if (hdev->migration_blocker != NULL) {
- migrate_add_blocker(hdev->migration_blocker);
+ 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);
+ goto fail_busyloop;
+ }
}
hdev->mem = g_malloc0(offsetof(struct vhost_memory, regions));
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 40b3697..46a4bb5 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -243,6 +243,7 @@ void remove_migration_state_change_notifier(Notifier *notify);
MigrationState *migrate_init(const MigrationParams *params);
bool migration_is_blocked(Error **errp);
bool migration_in_setup(MigrationState *);
+bool migration_is_idle(MigrationState *s);
bool migration_has_finished(MigrationState *);
bool migration_has_failed(MigrationState *);
/* True if outgoing migration has entered postcopy phase */
@@ -287,8 +288,11 @@ int ram_postcopy_incoming_init(MigrationIncomingState *mis);
* @migrate_add_blocker - prevent migration from proceeding
*
* @reason - an error to be returned whenever migration is attempted
+ * @errp - [out] The reason (if any) we cannot block migration right now.
+ *
+ * @returns - 0 on success, -EBUSY on failure, with errp set.
*/
-void migrate_add_blocker(Error *reason);
+int migrate_add_blocker(Error *reason, Error **errp);
/**
* @migrate_del_blocker - remove a blocking error from migration
diff --git a/migration/migration.c b/migration/migration.c
index f498ab8..713e012 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1044,6 +1044,31 @@ bool migration_in_postcopy_after_devices(MigrationState *s)
return migration_in_postcopy(s) && s->postcopy_after_devices;
}
+bool migration_is_idle(MigrationState *s)
+{
+ if (!s) {
+ s = migrate_get_current();
+ }
+
+ switch (s->state) {
+ case MIGRATION_STATUS_NONE:
+ case MIGRATION_STATUS_CANCELLED:
+ case MIGRATION_STATUS_COMPLETED:
+ case MIGRATION_STATUS_FAILED:
+ return true;
+ case MIGRATION_STATUS_SETUP:
+ case MIGRATION_STATUS_CANCELLING:
+ case MIGRATION_STATUS_ACTIVE:
+ case MIGRATION_STATUS_POSTCOPY_ACTIVE:
+ case MIGRATION_STATUS_COLO:
+ return false;
+ case MIGRATION_STATUS__MAX:
+ g_assert_not_reached();
+ }
+
+ return false;
+}
+
MigrationState *migrate_init(const MigrationParams *params)
{
MigrationState *s = migrate_get_current();
@@ -1086,9 +1111,15 @@ MigrationState *migrate_init(const MigrationParams *params)
static GSList *migration_blockers;
-void migrate_add_blocker(Error *reason)
+int migrate_add_blocker(Error *reason, Error **errp)
{
- migration_blockers = g_slist_prepend(migration_blockers, reason);
+ if (migration_is_idle(NULL)) {
+ migration_blockers = g_slist_prepend(migration_blockers, reason);
+ return 0;
+ }
+
+ error_setg(errp, "Cannot block a migration already in-progress");
+ return -EBUSY;
}
void migrate_del_blocker(Error *reason)
diff --git a/stubs/migr-blocker.c b/stubs/migr-blocker.c
index 8ab3604..a5ba18f 100644
--- a/stubs/migr-blocker.c
+++ b/stubs/migr-blocker.c
@@ -2,8 +2,9 @@
#include "qemu-common.h"
#include "migration/migration.h"
-void migrate_add_blocker(Error *reason)
+int migrate_add_blocker(Error *reason, Error **errp)
{
+ return 0;
}
void migrate_del_blocker(Error *reason)
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index f62264a..6031a1c 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -961,7 +961,11 @@ int kvm_arch_init_vcpu(CPUState *cs)
error_setg(&invtsc_mig_blocker,
"State blocked by non-migratable CPU device"
" (invtsc flag)");
- migrate_add_blocker(invtsc_mig_blocker);
+ r = migrate_add_blocker(invtsc_mig_blocker, NULL);
+ if (r < 0) {
+ error_report("kvm: migrate_add_blocker failed");
+ goto efail;
+ }
/* for savevm */
vmstate_x86_cpu.unmigratable = 1;
}
@@ -969,12 +973,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
cpuid_data.cpuid.padding = 0;
r = kvm_vcpu_ioctl(cs, KVM_SET_CPUID2, &cpuid_data);
if (r) {
- return r;
+ goto fail;
}
r = kvm_arch_set_tsc_khz(cs);
if (r < 0) {
- return r;
+ goto fail;
}
/* vcpu's TSC frequency is either specified by user, or following
@@ -1001,6 +1005,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
}
return 0;
+
+ fail:
+ migrate_del_blocker(invtsc_mig_blocker);
+ efail:
+ error_free(invtsc_mig_blocker);
+ return r;
}
void kvm_arch_reset_vcpu(X86CPU *cpu)
--
2.6.2
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v4 4/4] migration: Fail migration blocker for --only-migratble
2017-01-09 17:02 [Qemu-devel] [PATCH v4 0/4] Introduce a new --only-migratable option Ashijeet Acharya
` (2 preceding siblings ...)
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 3/4] migration: disallow migrate_add_blocker during migration Ashijeet Acharya
@ 2017-01-09 17:02 ` Ashijeet Acharya
2017-01-09 21:01 ` Eric Blake
` (2 more replies)
2017-01-09 17:27 ` [Qemu-devel] [PATCH v4 0/4] Introduce a new --only-migratable option no-reply
4 siblings, 3 replies; 18+ messages in thread
From: Ashijeet Acharya @ 2017-01-09 17:02 UTC (permalink / raw)
To: dgilbert
Cc: jsnow, amit.shah, pbonzini, kwolf, armbru, quintela, mst,
marcandre.lureau, groug, aneesh.kumar, peter.maydell, qemu-devel,
Ashijeet Acharya
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");
+ }
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;
}
--
2.6.2
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/4] Introduce a new --only-migratable option
2017-01-09 17:02 [Qemu-devel] [PATCH v4 0/4] Introduce a new --only-migratable option Ashijeet Acharya
` (3 preceding siblings ...)
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 4/4] migration: Fail migration blocker for --only-migratble Ashijeet Acharya
@ 2017-01-09 17:27 ` no-reply
4 siblings, 0 replies; 18+ messages in thread
From: no-reply @ 2017-01-09 17:27 UTC (permalink / raw)
To: ashijeetacharya
Cc: famz, dgilbert, kwolf, peter.maydell, quintela, qemu-devel, mst,
armbru, groug, aneesh.kumar, marcandre.lureau, amit.shah,
pbonzini, jsnow
Hi,
Your series seems to have some coding style problems. See output below for
more information:
Type: series
Subject: [Qemu-devel] [PATCH v4 0/4] Introduce a new --only-migratable option
Message-id: 1483981368-9965-1-git-send-email-ashijeetacharya@gmail.com
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/1483981368-9965-1-git-send-email-ashijeetacharya@gmail.com -> patchew/1483981368-9965-1-git-send-email-ashijeetacharya@gmail.com
Switched to a new branch 'test'
757f972 migration: Fail migration blocker for --only-migratble
dd5e052 migration: disallow migrate_add_blocker during migration
bb70ea7 migration: Allow "device add" options to only add migratable devices
88b5312 migration: Add a new option to enable only-migratable
=== OUTPUT BEGIN ===
Checking PATCH 1/4: migration: Add a new option to enable only-migratable...
ERROR: do not initialise globals to 0 or NULL
#56: FILE: vl.c:183:
+int only_migratable = 0; /* turn it off unless user states otherwise */
total: 1 errors, 0 warnings, 40 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 2/4: migration: Allow "device add" options to only add migratable devices...
Checking PATCH 3/4: migration: disallow migrate_add_blocker during migration...
ERROR: do not use C99 // comments
#159: FILE: block/vvfat.c:1205:
+ // assert(is_consistent(s));
total: 1 errors, 0 warnings, 490 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 4/4: migration: Fail migration blocker for --only-migratble...
ERROR: space prohibited before that close parenthesis ')'
#159: FILE: hw/intc/arm_gic_kvm.c:517:
+ if (ret < 0 ) {
total: 1 errors, 0 warnings, 219 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] migration: Fail migration blocker for --only-migratble
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 4/4] migration: Fail migration blocker for --only-migratble Ashijeet Acharya
@ 2017-01-09 21:01 ` Eric Blake
2017-01-10 12:11 ` Dr. David Alan Gilbert
2017-01-10 17:15 ` Peter Maydell
2 siblings, 0 replies; 18+ messages in thread
From: Eric Blake @ 2017-01-09 21:01 UTC (permalink / raw)
To: Ashijeet Acharya, dgilbert
Cc: kwolf, peter.maydell, quintela, qemu-devel, mst, armbru, groug,
aneesh.kumar, marcandre.lureau, amit.shah, pbonzini, jsnow
[-- Attachment #1: Type: text/plain, Size: 538 bytes --]
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>
> ---
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v4 3/4] migration: disallow migrate_add_blocker during migration
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 3/4] migration: disallow migrate_add_blocker during migration Ashijeet Acharya
@ 2017-01-10 8:25 ` Greg Kurz
2017-01-10 12:01 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 18+ messages in thread
From: Greg Kurz @ 2017-01-10 8:25 UTC (permalink / raw)
To: Ashijeet Acharya
Cc: dgilbert, jsnow, amit.shah, pbonzini, kwolf, armbru, quintela,
mst, marcandre.lureau, aneesh.kumar, peter.maydell, qemu-devel
On Mon, 9 Jan 2017 22:32:47 +0530
Ashijeet Acharya <ashijeetacharya@gmail.com> wrote:
> [...]
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index faebd91..c823a44 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1013,20 +1013,28 @@ static void coroutine_fn v9fs_attach(void *opaque)
> goto out;
> }
> err += offset;
> - memcpy(&s->root_qid, &qid, sizeof(qid));
> - trace_v9fs_attach_return(pdu->tag, pdu->id,
> - qid.type, qid.version, qid.path);
> +
> /*
> * disable migration if we haven't done already.
> * attach could get called multiple times for the same export.
> */
> if (!s->migration_blocker) {
> - 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);
> - migrate_add_blocker(s->migration_blocker);
> + err = migrate_add_blocker(s->migration_blocker, NULL);
> + if (err < 0) {
> + error_free(s->migration_blocker);
> + s->migration_blocker = NULL;
> + clunk_fid(s, fid);
> + goto out;
> + }
> + s->root_fid = fid;
> }
Hmm... my suggestion was to deal with the migration blocker before the call
to pdu_marshal() since there's no point in copying the qid if we're about
to return an error back to the guest.
> +
> + memcpy(&s->root_qid, &qid, sizeof(qid));
> + trace_v9fs_attach_return(pdu->tag, pdu->id,
> + qid.type, qid.version, qid.path);
> out:
> put_fid(pdu, fidp);
> out_nofid:
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/4] migration: Add a new option to enable only-migratable
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 1/4] migration: Add a new option to enable only-migratable Ashijeet Acharya
@ 2017-01-10 11:02 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2017-01-10 11:02 UTC (permalink / raw)
To: Ashijeet Acharya
Cc: jsnow, amit.shah, pbonzini, kwolf, armbru, quintela, mst,
marcandre.lureau, groug, aneesh.kumar, peter.maydell, qemu-devel
* Ashijeet Acharya (ashijeetacharya@gmail.com) wrote:
> Add a new option "--only-migratable" in qemu which will allow to add
> only those devices which will not fail qemu after migration. Devices
> set with the flag 'unmigratable' cannot be added when this option will
> be used.
>
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
> include/migration/migration.h | 3 +++
> qemu-options.hx | 9 +++++++++
> vl.c | 4 ++++
> 3 files changed, 16 insertions(+)
>
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index c309d23..40b3697 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -38,6 +38,9 @@
> #define QEMU_VM_COMMAND 0x08
> #define QEMU_VM_SECTION_FOOTER 0x7e
>
> +/* for vl.c */
> +extern int only_migratable;
> +
> struct MigrationParams {
> bool blk;
> bool shared;
> diff --git a/qemu-options.hx b/qemu-options.hx
> index c534a2f..1e16ae8 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3574,6 +3574,15 @@ be used to change settings (such as migration parameters) prior to issuing
> the migrate_incoming to allow the migration to begin.
> ETEXI
>
> +DEF("only-migratable", 0, QEMU_OPTION_only_migratable, \
> + "-only-migratable allow only migratable devices\n", QEMU_ARCH_ALL)
> +STEXI
> +@item -only-migratable
> +@findex -only-migratable
> +Only allow migratable devices. Devices will not be allowed to enter an
> +unmigratable state.
> +ETEXI
> +
> DEF("nodefaults", 0, QEMU_OPTION_nodefaults, \
> "-nodefaults don't create default devices\n", QEMU_ARCH_ALL)
> STEXI
> diff --git a/vl.c b/vl.c
> index d77dd86..82bffb9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -180,6 +180,7 @@ bool boot_strict;
> uint8_t *boot_splash_filedata;
> size_t boot_splash_filedata_size;
> uint8_t qemu_extra_params_fw[2];
> +int only_migratable = 0; /* turn it off unless user states otherwise */
QEMU coding style is to omit the =0 for globals.
Other than that,
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Dave
> int icount_align_option;
>
> @@ -3914,6 +3915,9 @@ int main(int argc, char **argv, char **envp)
> }
> incoming = optarg;
> break;
> + case QEMU_OPTION_only_migratable:
> + only_migratable = 1;
> + break;
> case QEMU_OPTION_nodefaults:
> has_defaults = 0;
> break;
> --
> 2.6.2
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/4] migration: Allow "device add" options to only add migratable devices
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 2/4] migration: Allow "device add" options to only add migratable devices Ashijeet Acharya
@ 2017-01-10 11:23 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2017-01-10 11:23 UTC (permalink / raw)
To: Ashijeet Acharya
Cc: jsnow, amit.shah, pbonzini, kwolf, armbru, quintela, mst,
marcandre.lureau, groug, aneesh.kumar, peter.maydell, qemu-devel
* Ashijeet Acharya (ashijeetacharya@gmail.com) wrote:
> Introduce checks for the unmigratable flag in the VMStateDescription
> structs of respective devices when user attempts to add them. If the
> "--only-migratable" was specified, all unmigratable devices will
> rightly fail to add. This feature is made compatible for both "-device"
> and "-usbdevice" command line options and covers their hmp and qmp
> counterparts as well.
>
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
> hw/usb/bus.c | 18 ++++++++++++++++++
> qdev-monitor.c | 9 +++++++++
> 2 files changed, 27 insertions(+)
>
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index 25913ad..3d26103 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -8,6 +8,7 @@
> #include "monitor/monitor.h"
> #include "trace.h"
> #include "qemu/cutils.h"
> +#include "migration/migration.h"
>
> static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
>
> @@ -686,6 +687,8 @@ USBDevice *usbdevice_create(const char *cmdline)
> const char *params;
> int len;
> USBDevice *dev;
> + ObjectClass *klass;
> + DeviceClass *dc;
>
> params = strchr(cmdline,':');
> if (params) {
> @@ -720,6 +723,21 @@ USBDevice *usbdevice_create(const char *cmdline)
> return NULL;
> }
>
> + klass = object_class_by_name(f->name);
> + if (klass == NULL) {
> + return NULL;
All error paths should print something, even if it shouldn't happen.
Dave
> + }
> +
> + dc = DEVICE_CLASS(klass);
> +
> + if (only_migratable) {
> + if (dc->vmsd->unmigratable) {
> + error_report("Device %s is not migratable, but --only-migratable "
> + "was specified", f->name);
> + return NULL;
> + }
> + }
> +
> if (f->usbdevice_init) {
> dev = f->usbdevice_init(bus, params);
> } else {
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index c73410c..81d01df 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -29,6 +29,7 @@
> #include "qemu/error-report.h"
> #include "qemu/help_option.h"
> #include "sysemu/block-backend.h"
> +#include "migration/migration.h"
>
> /*
> * Aliases were a bad idea from the start. Let's keep them
> @@ -577,6 +578,14 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
> return NULL;
> }
>
> + if (only_migratable) {
> + if (dc->vmsd->unmigratable) {
> + error_setg(errp, "Device %s is not migratable, but "
> + "--only-migratable was specified", driver);
> + return NULL;
> + }
> + }
> +
> /* find bus */
> path = qemu_opt_get(opts, "bus");
> if (path != NULL) {
> --
> 2.6.2
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v4 3/4] migration: disallow migrate_add_blocker during migration
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 3/4] migration: disallow migrate_add_blocker during migration Ashijeet Acharya
2017-01-10 8:25 ` Greg Kurz
@ 2017-01-10 12:01 ` Dr. David Alan Gilbert
2017-01-10 18:33 ` Ashijeet Acharya
1 sibling, 1 reply; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2017-01-10 12:01 UTC (permalink / raw)
To: Ashijeet Acharya
Cc: jsnow, amit.shah, pbonzini, kwolf, armbru, quintela, mst,
marcandre.lureau, groug, aneesh.kumar, peter.maydell, qemu-devel
* Ashijeet Acharya (ashijeetacharya@gmail.com) wrote:
> If a migration is already in progress and somebody attempts
> to add a migration blocker, this should rightly fail.
>
> Add an errp parameter and a retcode return value to migrate_add_blocker.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
> block/qcow.c | 6 +++++-
> block/vdi.c | 6 +++++-
> block/vhdx.c | 16 ++++++++++------
> block/vmdk.c | 7 ++++++-
> block/vpc.c | 10 +++++++---
> block/vvfat.c | 20 ++++++++++++--------
> hw/9pfs/9p.c | 18 +++++++++++++-----
> hw/display/virtio-gpu.c | 29 ++++++++++++++++-------------
> hw/intc/arm_gic_kvm.c | 16 ++++++++++------
> hw/intc/arm_gicv3_its_kvm.c | 18 +++++++++++-------
> hw/intc/arm_gicv3_kvm.c | 19 ++++++++++++-------
> hw/misc/ivshmem.c | 11 +++++++----
> hw/scsi/vhost-scsi.c | 25 +++++++++++++++++++------
> hw/virtio/vhost.c | 8 +++++++-
> include/migration/migration.h | 6 +++++-
> migration/migration.c | 35 +++++++++++++++++++++++++++++++++--
> stubs/migr-blocker.c | 3 ++-
> target-i386/kvm.c | 16 +++++++++++++---
> 18 files changed, 193 insertions(+), 76 deletions(-)
>
> diff --git a/block/qcow.c b/block/qcow.c
> index 7540f43..11526a1 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -252,7 +252,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
> error_setg(&s->migration_blocker, "The qcow format used by node '%s' "
> "does not support live migration",
> bdrv_get_device_or_node_name(bs));
> - migrate_add_blocker(s->migration_blocker);
> + ret = migrate_add_blocker(s->migration_blocker, errp);
> + if (ret < 0) {
> + error_free(s->migration_blocker);
> + goto fail;
> + }
>
> qemu_co_mutex_init(&s->lock);
> return 0;
> diff --git a/block/vdi.c b/block/vdi.c
> index 96b78d5..2b56f52 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -471,7 +471,11 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
> error_setg(&s->migration_blocker, "The vdi format used by node '%s' "
> "does not support live migration",
> bdrv_get_device_or_node_name(bs));
> - migrate_add_blocker(s->migration_blocker);
> + ret = migrate_add_blocker(s->migration_blocker, errp);
> + if (ret < 0) {
> + error_free(s->migration_blocker);
> + goto fail_free_bmap;
> + }
>
> qemu_co_mutex_init(&s->write_lock);
>
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 0ba2f0a..d81941b 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -991,6 +991,16 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
> }
> }
>
> + /* Disable migration when VHDX images are used */
> + error_setg(&s->migration_blocker, "The vhdx format used by node '%s' "
> + "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);
> + goto fail;
> + }
> +
> if (flags & BDRV_O_RDWR) {
> ret = vhdx_update_headers(bs, s, false, NULL);
> if (ret < 0) {
> @@ -1000,12 +1010,6 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
>
> /* TODO: differencing files */
>
> - /* Disable migration when VHDX images are used */
> - error_setg(&s->migration_blocker, "The vhdx format used by node '%s' "
> - "does not support live migration",
> - bdrv_get_device_or_node_name(bs));
> - migrate_add_blocker(s->migration_blocker);
> -
> return 0;
> fail:
> vhdx_close(bs);
> diff --git a/block/vmdk.c b/block/vmdk.c
> index a11c27a..4a45a83 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -976,7 +976,12 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
> error_setg(&s->migration_blocker, "The vmdk format used by node '%s' "
> "does not support live migration",
> bdrv_get_device_or_node_name(bs));
> - migrate_add_blocker(s->migration_blocker);
> + ret = migrate_add_blocker(s->migration_blocker, errp);
> + if (ret < 0) {
> + error_free(s->migration_blocker);
> + goto fail;
> + }
> +
> g_free(buf);
> return 0;
>
> diff --git a/block/vpc.c b/block/vpc.c
> index 8d5886f..299a8c8 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -422,13 +422,17 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
> #endif
> }
>
> - qemu_co_mutex_init(&s->lock);
> -
> /* Disable migration when VHD images are used */
> error_setg(&s->migration_blocker, "The vpc format used by node '%s' "
> "does not support live migration",
> bdrv_get_device_or_node_name(bs));
> - migrate_add_blocker(s->migration_blocker);
> + ret = migrate_add_blocker(s->migration_blocker, errp);
> + if (ret < 0) {
> + error_free(s->migration_blocker);
> + goto fail;
> + }
> +
> + qemu_co_mutex_init(&s->lock);
>
> return 0;
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index ded2109..3de3320 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1185,22 +1185,26 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>
> s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count;
>
> - if (s->first_sectors_number == 0x40) {
> - init_mbr(s, cyls, heads, secs);
> - }
> -
> - // assert(is_consistent(s));
> - qemu_co_mutex_init(&s->lock);
> -
> /* Disable migration when vvfat is used rw */
> if (s->qcow) {
> error_setg(&s->migration_blocker,
> "The vvfat (rw) format used by node '%s' "
> "does not support live migration",
> bdrv_get_device_or_node_name(bs));
> - migrate_add_blocker(s->migration_blocker);
> + ret = migrate_add_blocker(s->migration_blocker, errp);
> + if (ret < 0) {
> + error_free(s->migration_blocker);
> + goto fail;
> + }
> + }
> +
> + if (s->first_sectors_number == 0x40) {
> + init_mbr(s, cyls, heads, secs);
> }
>
> + // assert(is_consistent(s));
> + qemu_co_mutex_init(&s->lock);
> +
> ret = 0;
> fail:
> qemu_opts_del(opts);
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index faebd91..c823a44 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1013,20 +1013,28 @@ static void coroutine_fn v9fs_attach(void *opaque)
> goto out;
> }
> err += offset;
> - memcpy(&s->root_qid, &qid, sizeof(qid));
> - trace_v9fs_attach_return(pdu->tag, pdu->id,
> - qid.type, qid.version, qid.path);
> +
> /*
> * disable migration if we haven't done already.
> * attach could get called multiple times for the same export.
> */
> if (!s->migration_blocker) {
> - 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);
> - migrate_add_blocker(s->migration_blocker);
> + err = migrate_add_blocker(s->migration_blocker, NULL);
> + if (err < 0) {
> + error_free(s->migration_blocker);
> + s->migration_blocker = NULL;
> + clunk_fid(s, fid);
> + goto out;
> + }
> + s->root_fid = fid;
> }
> +
> + memcpy(&s->root_qid, &qid, sizeof(qid));
> + trace_v9fs_attach_return(pdu->tag, pdu->id,
> + qid.type, qid.version, qid.path);
> out:
> put_fid(pdu, fidp);
> out_nofid:
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 5f32e1a..6e60b8c 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1108,14 +1108,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
> return;
> }
>
> - g->config_size = sizeof(struct virtio_gpu_config);
> - g->virtio_config.num_scanouts = g->conf.max_outputs;
> - virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU,
> - g->config_size);
> -
> - g->req_state[0].width = 1024;
> - g->req_state[0].height = 768;
> -
> g->use_virgl_renderer = false;
> #if !defined(CONFIG_VIRGL) || defined(HOST_WORDS_BIGENDIAN)
> have_virgl = false;
> @@ -1127,6 +1119,22 @@ 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);
> + return;
> + }
> + }
> +
> + g->config_size = sizeof(struct virtio_gpu_config);
> + g->virtio_config.num_scanouts = g->conf.max_outputs;
> + virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU,
> + g->config_size);
> +
> + g->req_state[0].width = 1024;
> + g->req_state[0].height = 768;
> +
> + if (virtio_gpu_virgl_enabled(g->conf)) {
> /* use larger control queue in 3d mode */
> g->ctrl_vq = virtio_add_queue(vdev, 256, virtio_gpu_handle_ctrl_cb);
> g->cursor_vq = virtio_add_queue(vdev, 16, virtio_gpu_handle_cursor_cb);
> @@ -1152,11 +1160,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
> dpy_gfx_replace_surface(g->scanout[i].con, NULL);
> }
> }
> -
> - if (virtio_gpu_virgl_enabled(g->conf)) {
> - error_setg(&g->migration_blocker, "virgl is not yet migratable");
> - migrate_add_blocker(g->migration_blocker);
> - }
> }
>
> static void virtio_gpu_device_unrealize(DeviceState *qdev, Error **errp)
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index 11729ee..3614daa 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -510,6 +510,16 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
> return;
> }
>
> + if (!kvm_arm_gic_can_save_restore(s)) {
> + 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);
> + return;
> + }
> + }
> +
> gic_init_irqs_and_mmio(s, kvm_arm_gicv2_set_irq, NULL);
>
> for (i = 0; i < s->num_irq - GIC_INTERNAL; i++) {
> @@ -558,12 +568,6 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
> KVM_VGIC_V2_ADDR_TYPE_CPU,
> s->dev_fd);
>
> - if (!kvm_arm_gic_can_save_restore(s)) {
> - error_setg(&s->migration_blocker, "This operating system kernel does "
> - "not support vGICv2 migration");
> - migrate_add_blocker(s->migration_blocker);
> - }
> -
> if (kvm_has_gsi_routing()) {
> /* set up irq routing */
> kvm_init_irq_routing(kvm_state);
> diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
> index fc246e0..950a02f 100644
> --- a/hw/intc/arm_gicv3_its_kvm.c
> +++ b/hw/intc/arm_gicv3_its_kvm.c
> @@ -63,6 +63,17 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
> return;
> }
>
> + /*
> + * Block migration of a KVM GICv3 ITS device: the API for saving and
> + * restoring the state in the kernel is not yet available
> + */
> + 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);
> + return;
> + }
> +
> /* explicit init of the ITS */
> kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> @@ -73,13 +84,6 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
>
> gicv3_its_init_mmio(s, NULL);
>
> - /*
> - * Block migration of a KVM GICv3 ITS device: the API for saving and
> - * restoring the state in the kernel is not yet available
> - */
> - error_setg(&s->migration_blocker, "vITS migration is not implemented");
> - migrate_add_blocker(s->migration_blocker);
> -
> kvm_msi_use_devid = true;
> kvm_gsi_direct_mapping = false;
> kvm_msi_via_irqfd_allowed = kvm_irqfds_enabled();
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index 199a439..06f6f30 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -86,6 +86,7 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
> KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
> Error *local_err = NULL;
> int i;
> + int ret;
>
> DPRINTF("kvm_arm_gicv3_realize\n");
>
> @@ -110,6 +111,17 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
> return;
> }
>
> + /* Block migration of a KVM GICv3 device: the API for saving and restoring
> + * the state in the kernel is not yet finalised in the kernel or
> + * implemented in QEMU.
> + */
> + 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);
> + return;
> + }
> +
Shouldn't this go a little earlier in the function - before the kvm_create_device call?
> kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
> 0, &s->num_irq, true);
>
> @@ -122,13 +134,6 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
> kvm_arm_register_device(&s->iomem_redist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
> KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd);
>
> - /* Block migration of a KVM GICv3 device: the API for saving and restoring
> - * the state in the kernel is not yet finalised in the kernel or
> - * implemented in QEMU.
> - */
> - error_setg(&s->migration_blocker, "vGICv3 migration is not implemented");
> - migrate_add_blocker(s->migration_blocker);
> -
> if (kvm_has_gsi_routing()) {
> /* set up irq routing */
> kvm_init_irq_routing(kvm_state);
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index abeaf3d..585cc5b 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -903,9 +903,6 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
> }
> }
>
> - vmstate_register_ram(s->ivshmem_bar2, DEVICE(s));
> - pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2);
> -
> if (s->master == ON_OFF_AUTO_AUTO) {
> s->master = s->vm_id == 0 ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> }
> @@ -913,8 +910,14 @@ 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'");
> - migrate_add_blocker(s->migration_blocker);
> + if (migrate_add_blocker(s->migration_blocker, errp) < 0) {
> + error_free(s->migration_blocker);
> + return;
> + }
> }
> +
> + vmstate_register_ram(s->ivshmem_bar2, DEVICE(s));
> + pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2);
> }
>
> static void ivshmem_exit(PCIDevice *dev)
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 5b26946..ff503d0 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -238,8 +238,14 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
> vhost_dummy_handle_output);
> if (err != NULL) {
> error_propagate(errp, err);
> - close(vhostfd);
> - return;
> + goto close_fd;
> + }
> +
> + 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;
> }
>
> s->dev.nvqs = VHOST_SCSI_VQ_NUM_FIXED + vs->conf.num_queues;
> @@ -252,7 +258,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
> if (ret < 0) {
> error_setg(errp, "vhost-scsi: vhost initialization failed: %s",
> strerror(-ret));
> - return;
> + goto free_vqs;
> }
>
> /* At present, channel and lun both are 0 for bootable vhost-scsi disk */
> @@ -261,9 +267,16 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
> /* Note: we can also get the minimum tpgt from kernel */
> s->target = vs->conf.boot_tpgt;
>
> - error_setg(&s->migration_blocker,
> - "vhost-scsi does not support migration");
> - migrate_add_blocker(s->migration_blocker);
> + return;
> +
> + 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;
> }
>
> static void vhost_scsi_unrealize(DeviceState *dev, Error **errp)
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index f7f7023..416fada 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1157,7 +1157,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> }
>
> if (hdev->migration_blocker != NULL) {
> - migrate_add_blocker(hdev->migration_blocker);
> + 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);
> + goto fail_busyloop;
> + }
> }
>
> hdev->mem = g_malloc0(offsetof(struct vhost_memory, regions));
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 40b3697..46a4bb5 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -243,6 +243,7 @@ void remove_migration_state_change_notifier(Notifier *notify);
> MigrationState *migrate_init(const MigrationParams *params);
> bool migration_is_blocked(Error **errp);
> bool migration_in_setup(MigrationState *);
> +bool migration_is_idle(MigrationState *s);
> bool migration_has_finished(MigrationState *);
> bool migration_has_failed(MigrationState *);
> /* True if outgoing migration has entered postcopy phase */
> @@ -287,8 +288,11 @@ int ram_postcopy_incoming_init(MigrationIncomingState *mis);
> * @migrate_add_blocker - prevent migration from proceeding
> *
> * @reason - an error to be returned whenever migration is attempted
> + * @errp - [out] The reason (if any) we cannot block migration right now.
> + *
> + * @returns - 0 on success, -EBUSY on failure, with errp set.
> */
> -void migrate_add_blocker(Error *reason);
> +int migrate_add_blocker(Error *reason, Error **errp);
>
> /**
> * @migrate_del_blocker - remove a blocking error from migration
> diff --git a/migration/migration.c b/migration/migration.c
> index f498ab8..713e012 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1044,6 +1044,31 @@ bool migration_in_postcopy_after_devices(MigrationState *s)
> return migration_in_postcopy(s) && s->postcopy_after_devices;
> }
>
> +bool migration_is_idle(MigrationState *s)
> +{
> + if (!s) {
> + s = migrate_get_current();
> + }
> +
> + switch (s->state) {
> + case MIGRATION_STATUS_NONE:
> + case MIGRATION_STATUS_CANCELLED:
> + case MIGRATION_STATUS_COMPLETED:
> + case MIGRATION_STATUS_FAILED:
> + return true;
> + case MIGRATION_STATUS_SETUP:
> + case MIGRATION_STATUS_CANCELLING:
> + case MIGRATION_STATUS_ACTIVE:
> + case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> + case MIGRATION_STATUS_COLO:
> + return false;
> + case MIGRATION_STATUS__MAX:
> + g_assert_not_reached();
> + }
> +
> + return false;
> +}
> +
> MigrationState *migrate_init(const MigrationParams *params)
> {
> MigrationState *s = migrate_get_current();
> @@ -1086,9 +1111,15 @@ MigrationState *migrate_init(const MigrationParams *params)
>
> static GSList *migration_blockers;
>
> -void migrate_add_blocker(Error *reason)
> +int migrate_add_blocker(Error *reason, Error **errp)
> {
> - migration_blockers = g_slist_prepend(migration_blockers, reason);
> + if (migration_is_idle(NULL)) {
> + migration_blockers = g_slist_prepend(migration_blockers, reason);
> + return 0;
> + }
> +
> + error_setg(errp, "Cannot block a migration already in-progress");
> + return -EBUSY;
> }
>
> void migrate_del_blocker(Error *reason)
> diff --git a/stubs/migr-blocker.c b/stubs/migr-blocker.c
> index 8ab3604..a5ba18f 100644
> --- a/stubs/migr-blocker.c
> +++ b/stubs/migr-blocker.c
> @@ -2,8 +2,9 @@
> #include "qemu-common.h"
> #include "migration/migration.h"
>
> -void migrate_add_blocker(Error *reason)
> +int migrate_add_blocker(Error *reason, Error **errp)
> {
> + return 0;
> }
>
> void migrate_del_blocker(Error *reason)
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index f62264a..6031a1c 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -961,7 +961,11 @@ int kvm_arch_init_vcpu(CPUState *cs)
> error_setg(&invtsc_mig_blocker,
> "State blocked by non-migratable CPU device"
> " (invtsc flag)");
> - migrate_add_blocker(invtsc_mig_blocker);
> + r = migrate_add_blocker(invtsc_mig_blocker, NULL);
> + if (r < 0) {
> + error_report("kvm: migrate_add_blocker failed");
> + goto efail;
> + }
> /* for savevm */
> vmstate_x86_cpu.unmigratable = 1;
> }
> @@ -969,12 +973,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
> cpuid_data.cpuid.padding = 0;
> r = kvm_vcpu_ioctl(cs, KVM_SET_CPUID2, &cpuid_data);
> if (r) {
> - return r;
> + goto fail;
> }
>
> r = kvm_arch_set_tsc_khz(cs);
> if (r < 0) {
> - return r;
> + goto fail;
> }
>
> /* vcpu's TSC frequency is either specified by user, or following
> @@ -1001,6 +1005,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
> }
>
> return 0;
> +
> + fail:
> + migrate_del_blocker(invtsc_mig_blocker);
> + efail:
> + error_free(invtsc_mig_blocker);
> + return r;
> }
Couldn't that be simplified just to always do the migrate_del_blocker?
I don't think there's any harm in calling migrate_del_blocker if the add failed.
Dave
> void kvm_arch_reset_vcpu(X86CPU *cpu)
> --
> 2.6.2
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] migration: Fail migration blocker for --only-migratble
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 4/4] migration: Fail migration blocker for --only-migratble Ashijeet Acharya
2017-01-09 21:01 ` Eric Blake
@ 2017-01-10 12:11 ` Dr. David Alan Gilbert
2017-01-10 17:15 ` Peter Maydell
2 siblings, 0 replies; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2017-01-10 12:11 UTC (permalink / raw)
To: Ashijeet Acharya
Cc: jsnow, amit.shah, pbonzini, kwolf, armbru, quintela, mst,
marcandre.lureau, groug, aneesh.kumar, peter.maydell, qemu-devel
* 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] migration: Fail migration blocker for --only-migratble
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 4/4] migration: Fail migration blocker for --only-migratble Ashijeet Acharya
2017-01-09 21:01 ` Eric Blake
2017-01-10 12:11 ` Dr. David Alan Gilbert
@ 2017-01-10 17:15 ` Peter Maydell
2017-01-11 5:43 ` Ashijeet Acharya
2017-01-12 10:45 ` Ashijeet Acharya
2 siblings, 2 replies; 18+ messages in thread
From: Peter Maydell @ 2017-01-10 17:15 UTC (permalink / raw)
To: Ashijeet Acharya
Cc: Dr. David Alan Gilbert, John Snow, Amit Shah, Paolo Bonzini,
Kevin Wolf, Markus Armbruster, Juan Quintela, Michael S. Tsirkin,
Marc-André Lureau, Greg Kurz, Aneesh Kumar K.V,
QEMU Developers
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v4 3/4] migration: disallow migrate_add_blocker during migration
2017-01-10 12:01 ` Dr. David Alan Gilbert
@ 2017-01-10 18:33 ` Ashijeet Acharya
0 siblings, 0 replies; 18+ messages in thread
From: Ashijeet Acharya @ 2017-01-10 18:33 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: John Snow, Amit Shah, Paolo Bonzini, Kevin Wolf,
Markus Armbruster, Juan Quintela, Michael S. Tsirkin,
Marc-André Lureau, Greg Kurz, Aneesh Kumar K.V,
Peter Maydell, QEMU Developers
On Tue, Jan 10, 2017 at 5:31 PM, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
> * Ashijeet Acharya (ashijeetacharya@gmail.com) wrote:
>> If a migration is already in progress and somebody attempts
>> to add a migration blocker, this should rightly fail.
>>
>> Add an errp parameter and a retcode return value to migrate_add_blocker.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>> ---
>> block/qcow.c | 6 +++++-
>> block/vdi.c | 6 +++++-
>> block/vhdx.c | 16 ++++++++++------
>> block/vmdk.c | 7 ++++++-
>> block/vpc.c | 10 +++++++---
>> block/vvfat.c | 20 ++++++++++++--------
>> hw/9pfs/9p.c | 18 +++++++++++++-----
>> hw/display/virtio-gpu.c | 29 ++++++++++++++++-------------
>> hw/intc/arm_gic_kvm.c | 16 ++++++++++------
>> hw/intc/arm_gicv3_its_kvm.c | 18 +++++++++++-------
>> hw/intc/arm_gicv3_kvm.c | 19 ++++++++++++-------
>> hw/misc/ivshmem.c | 11 +++++++----
>> hw/scsi/vhost-scsi.c | 25 +++++++++++++++++++------
>> hw/virtio/vhost.c | 8 +++++++-
>> include/migration/migration.h | 6 +++++-
>> migration/migration.c | 35 +++++++++++++++++++++++++++++++++--
>> stubs/migr-blocker.c | 3 ++-
>> target-i386/kvm.c | 16 +++++++++++++---
>> 18 files changed, 193 insertions(+), 76 deletions(-)
>>
>> diff --git a/block/qcow.c b/block/qcow.c
>> index 7540f43..11526a1 100644
>> --- a/block/qcow.c
>> +++ b/block/qcow.c
>> @@ -252,7 +252,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
>> error_setg(&s->migration_blocker, "The qcow format used by node '%s' "
>> "does not support live migration",
>> bdrv_get_device_or_node_name(bs));
>> - migrate_add_blocker(s->migration_blocker);
>> + ret = migrate_add_blocker(s->migration_blocker, errp);
>> + if (ret < 0) {
>> + error_free(s->migration_blocker);
>> + goto fail;
>> + }
>>
>> qemu_co_mutex_init(&s->lock);
>> return 0;
>> diff --git a/block/vdi.c b/block/vdi.c
>> index 96b78d5..2b56f52 100644
>> --- a/block/vdi.c
>> +++ b/block/vdi.c
>> @@ -471,7 +471,11 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
>> error_setg(&s->migration_blocker, "The vdi format used by node '%s' "
>> "does not support live migration",
>> bdrv_get_device_or_node_name(bs));
>> - migrate_add_blocker(s->migration_blocker);
>> + ret = migrate_add_blocker(s->migration_blocker, errp);
>> + if (ret < 0) {
>> + error_free(s->migration_blocker);
>> + goto fail_free_bmap;
>> + }
>>
>> qemu_co_mutex_init(&s->write_lock);
>>
>> diff --git a/block/vhdx.c b/block/vhdx.c
>> index 0ba2f0a..d81941b 100644
>> --- a/block/vhdx.c
>> +++ b/block/vhdx.c
>> @@ -991,6 +991,16 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
>> }
>> }
>>
>> + /* Disable migration when VHDX images are used */
>> + error_setg(&s->migration_blocker, "The vhdx format used by node '%s' "
>> + "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);
>> + goto fail;
>> + }
>> +
>> if (flags & BDRV_O_RDWR) {
>> ret = vhdx_update_headers(bs, s, false, NULL);
>> if (ret < 0) {
>> @@ -1000,12 +1010,6 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
>>
>> /* TODO: differencing files */
>>
>> - /* Disable migration when VHDX images are used */
>> - error_setg(&s->migration_blocker, "The vhdx format used by node '%s' "
>> - "does not support live migration",
>> - bdrv_get_device_or_node_name(bs));
>> - migrate_add_blocker(s->migration_blocker);
>> -
>> return 0;
>> fail:
>> vhdx_close(bs);
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index a11c27a..4a45a83 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -976,7 +976,12 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
>> error_setg(&s->migration_blocker, "The vmdk format used by node '%s' "
>> "does not support live migration",
>> bdrv_get_device_or_node_name(bs));
>> - migrate_add_blocker(s->migration_blocker);
>> + ret = migrate_add_blocker(s->migration_blocker, errp);
>> + if (ret < 0) {
>> + error_free(s->migration_blocker);
>> + goto fail;
>> + }
>> +
>> g_free(buf);
>> return 0;
>>
>> diff --git a/block/vpc.c b/block/vpc.c
>> index 8d5886f..299a8c8 100644
>> --- a/block/vpc.c
>> +++ b/block/vpc.c
>> @@ -422,13 +422,17 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>> #endif
>> }
>>
>> - qemu_co_mutex_init(&s->lock);
>> -
>> /* Disable migration when VHD images are used */
>> error_setg(&s->migration_blocker, "The vpc format used by node '%s' "
>> "does not support live migration",
>> bdrv_get_device_or_node_name(bs));
>> - migrate_add_blocker(s->migration_blocker);
>> + ret = migrate_add_blocker(s->migration_blocker, errp);
>> + if (ret < 0) {
>> + error_free(s->migration_blocker);
>> + goto fail;
>> + }
>> +
>> + qemu_co_mutex_init(&s->lock);
>>
>> return 0;
>>
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index ded2109..3de3320 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -1185,22 +1185,26 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>>
>> s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count;
>>
>> - if (s->first_sectors_number == 0x40) {
>> - init_mbr(s, cyls, heads, secs);
>> - }
>> -
>> - // assert(is_consistent(s));
>> - qemu_co_mutex_init(&s->lock);
>> -
>> /* Disable migration when vvfat is used rw */
>> if (s->qcow) {
>> error_setg(&s->migration_blocker,
>> "The vvfat (rw) format used by node '%s' "
>> "does not support live migration",
>> bdrv_get_device_or_node_name(bs));
>> - migrate_add_blocker(s->migration_blocker);
>> + ret = migrate_add_blocker(s->migration_blocker, errp);
>> + if (ret < 0) {
>> + error_free(s->migration_blocker);
>> + goto fail;
>> + }
>> + }
>> +
>> + if (s->first_sectors_number == 0x40) {
>> + init_mbr(s, cyls, heads, secs);
>> }
>>
>> + // assert(is_consistent(s));
>> + qemu_co_mutex_init(&s->lock);
>> +
>> ret = 0;
>> fail:
>> qemu_opts_del(opts);
>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
>> index faebd91..c823a44 100644
>> --- a/hw/9pfs/9p.c
>> +++ b/hw/9pfs/9p.c
>> @@ -1013,20 +1013,28 @@ static void coroutine_fn v9fs_attach(void *opaque)
>> goto out;
>> }
>> err += offset;
>> - memcpy(&s->root_qid, &qid, sizeof(qid));
>> - trace_v9fs_attach_return(pdu->tag, pdu->id,
>> - qid.type, qid.version, qid.path);
>> +
>> /*
>> * disable migration if we haven't done already.
>> * attach could get called multiple times for the same export.
>> */
>> if (!s->migration_blocker) {
>> - 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);
>> - migrate_add_blocker(s->migration_blocker);
>> + err = migrate_add_blocker(s->migration_blocker, NULL);
>> + if (err < 0) {
>> + error_free(s->migration_blocker);
>> + s->migration_blocker = NULL;
>> + clunk_fid(s, fid);
>> + goto out;
>> + }
>> + s->root_fid = fid;
>> }
>> +
>> + memcpy(&s->root_qid, &qid, sizeof(qid));
>> + trace_v9fs_attach_return(pdu->tag, pdu->id,
>> + qid.type, qid.version, qid.path);
>> out:
>> put_fid(pdu, fidp);
>> out_nofid:
>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>> index 5f32e1a..6e60b8c 100644
>> --- a/hw/display/virtio-gpu.c
>> +++ b/hw/display/virtio-gpu.c
>> @@ -1108,14 +1108,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
>> return;
>> }
>>
>> - g->config_size = sizeof(struct virtio_gpu_config);
>> - g->virtio_config.num_scanouts = g->conf.max_outputs;
>> - virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU,
>> - g->config_size);
>> -
>> - g->req_state[0].width = 1024;
>> - g->req_state[0].height = 768;
>> -
>> g->use_virgl_renderer = false;
>> #if !defined(CONFIG_VIRGL) || defined(HOST_WORDS_BIGENDIAN)
>> have_virgl = false;
>> @@ -1127,6 +1119,22 @@ 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);
>> + return;
>> + }
>> + }
>> +
>> + g->config_size = sizeof(struct virtio_gpu_config);
>> + g->virtio_config.num_scanouts = g->conf.max_outputs;
>> + virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU,
>> + g->config_size);
>> +
>> + g->req_state[0].width = 1024;
>> + g->req_state[0].height = 768;
>> +
>> + if (virtio_gpu_virgl_enabled(g->conf)) {
>> /* use larger control queue in 3d mode */
>> g->ctrl_vq = virtio_add_queue(vdev, 256, virtio_gpu_handle_ctrl_cb);
>> g->cursor_vq = virtio_add_queue(vdev, 16, virtio_gpu_handle_cursor_cb);
>> @@ -1152,11 +1160,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
>> dpy_gfx_replace_surface(g->scanout[i].con, NULL);
>> }
>> }
>> -
>> - if (virtio_gpu_virgl_enabled(g->conf)) {
>> - error_setg(&g->migration_blocker, "virgl is not yet migratable");
>> - migrate_add_blocker(g->migration_blocker);
>> - }
>> }
>>
>> static void virtio_gpu_device_unrealize(DeviceState *qdev, Error **errp)
>> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
>> index 11729ee..3614daa 100644
>> --- a/hw/intc/arm_gic_kvm.c
>> +++ b/hw/intc/arm_gic_kvm.c
>> @@ -510,6 +510,16 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
>> return;
>> }
>>
>> + if (!kvm_arm_gic_can_save_restore(s)) {
>> + 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);
>> + return;
>> + }
>> + }
>> +
>> gic_init_irqs_and_mmio(s, kvm_arm_gicv2_set_irq, NULL);
>>
>> for (i = 0; i < s->num_irq - GIC_INTERNAL; i++) {
>> @@ -558,12 +568,6 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
>> KVM_VGIC_V2_ADDR_TYPE_CPU,
>> s->dev_fd);
>>
>> - if (!kvm_arm_gic_can_save_restore(s)) {
>> - error_setg(&s->migration_blocker, "This operating system kernel does "
>> - "not support vGICv2 migration");
>> - migrate_add_blocker(s->migration_blocker);
>> - }
>> -
>> if (kvm_has_gsi_routing()) {
>> /* set up irq routing */
>> kvm_init_irq_routing(kvm_state);
>> diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
>> index fc246e0..950a02f 100644
>> --- a/hw/intc/arm_gicv3_its_kvm.c
>> +++ b/hw/intc/arm_gicv3_its_kvm.c
>> @@ -63,6 +63,17 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
>> return;
>> }
>>
>> + /*
>> + * Block migration of a KVM GICv3 ITS device: the API for saving and
>> + * restoring the state in the kernel is not yet available
>> + */
>> + 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);
>> + return;
>> + }
>> +
>> /* explicit init of the ITS */
>> kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
>> KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
>> @@ -73,13 +84,6 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
>>
>> gicv3_its_init_mmio(s, NULL);
>>
>> - /*
>> - * Block migration of a KVM GICv3 ITS device: the API for saving and
>> - * restoring the state in the kernel is not yet available
>> - */
>> - error_setg(&s->migration_blocker, "vITS migration is not implemented");
>> - migrate_add_blocker(s->migration_blocker);
>> -
>> kvm_msi_use_devid = true;
>> kvm_gsi_direct_mapping = false;
>> kvm_msi_via_irqfd_allowed = kvm_irqfds_enabled();
>> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
>> index 199a439..06f6f30 100644
>> --- a/hw/intc/arm_gicv3_kvm.c
>> +++ b/hw/intc/arm_gicv3_kvm.c
>> @@ -86,6 +86,7 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>> KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
>> Error *local_err = NULL;
>> int i;
>> + int ret;
>>
>> DPRINTF("kvm_arm_gicv3_realize\n");
>>
>> @@ -110,6 +111,17 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>> return;
>> }
>>
>> + /* Block migration of a KVM GICv3 device: the API for saving and restoring
>> + * the state in the kernel is not yet finalised in the kernel or
>> + * implemented in QEMU.
>> + */
>> + 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);
>> + return;
>> + }
>> +
>
> Shouldn't this go a little earlier in the function - before the kvm_create_device call?
Done.
>
>> kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
>> 0, &s->num_irq, true);
>>
>> @@ -122,13 +134,6 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>> kvm_arm_register_device(&s->iomem_redist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd);
>>
>> - /* Block migration of a KVM GICv3 device: the API for saving and restoring
>> - * the state in the kernel is not yet finalised in the kernel or
>> - * implemented in QEMU.
>> - */
>> - error_setg(&s->migration_blocker, "vGICv3 migration is not implemented");
>> - migrate_add_blocker(s->migration_blocker);
>> -
>> if (kvm_has_gsi_routing()) {
>> /* set up irq routing */
>> kvm_init_irq_routing(kvm_state);
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index abeaf3d..585cc5b 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -903,9 +903,6 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>> }
>> }
>>
>> - vmstate_register_ram(s->ivshmem_bar2, DEVICE(s));
>> - pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2);
>> -
>> if (s->master == ON_OFF_AUTO_AUTO) {
>> s->master = s->vm_id == 0 ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>> }
>> @@ -913,8 +910,14 @@ 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'");
>> - migrate_add_blocker(s->migration_blocker);
>> + if (migrate_add_blocker(s->migration_blocker, errp) < 0) {
>> + error_free(s->migration_blocker);
>> + return;
>> + }
>> }
>> +
>> + vmstate_register_ram(s->ivshmem_bar2, DEVICE(s));
>> + pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2);
>> }
>>
>> static void ivshmem_exit(PCIDevice *dev)
>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>> index 5b26946..ff503d0 100644
>> --- a/hw/scsi/vhost-scsi.c
>> +++ b/hw/scsi/vhost-scsi.c
>> @@ -238,8 +238,14 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>> vhost_dummy_handle_output);
>> if (err != NULL) {
>> error_propagate(errp, err);
>> - close(vhostfd);
>> - return;
>> + goto close_fd;
>> + }
>> +
>> + 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;
>> }
>>
>> s->dev.nvqs = VHOST_SCSI_VQ_NUM_FIXED + vs->conf.num_queues;
>> @@ -252,7 +258,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>> if (ret < 0) {
>> error_setg(errp, "vhost-scsi: vhost initialization failed: %s",
>> strerror(-ret));
>> - return;
>> + goto free_vqs;
>> }
>>
>> /* At present, channel and lun both are 0 for bootable vhost-scsi disk */
>> @@ -261,9 +267,16 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>> /* Note: we can also get the minimum tpgt from kernel */
>> s->target = vs->conf.boot_tpgt;
>>
>> - error_setg(&s->migration_blocker,
>> - "vhost-scsi does not support migration");
>> - migrate_add_blocker(s->migration_blocker);
>> + return;
>> +
>> + 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;
>> }
>>
>> static void vhost_scsi_unrealize(DeviceState *dev, Error **errp)
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index f7f7023..416fada 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1157,7 +1157,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>> }
>>
>> if (hdev->migration_blocker != NULL) {
>> - migrate_add_blocker(hdev->migration_blocker);
>> + 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);
>> + goto fail_busyloop;
>> + }
>> }
>>
>> hdev->mem = g_malloc0(offsetof(struct vhost_memory, regions));
>> diff --git a/include/migration/migration.h b/include/migration/migration.h
>> index 40b3697..46a4bb5 100644
>> --- a/include/migration/migration.h
>> +++ b/include/migration/migration.h
>> @@ -243,6 +243,7 @@ void remove_migration_state_change_notifier(Notifier *notify);
>> MigrationState *migrate_init(const MigrationParams *params);
>> bool migration_is_blocked(Error **errp);
>> bool migration_in_setup(MigrationState *);
>> +bool migration_is_idle(MigrationState *s);
>> bool migration_has_finished(MigrationState *);
>> bool migration_has_failed(MigrationState *);
>> /* True if outgoing migration has entered postcopy phase */
>> @@ -287,8 +288,11 @@ int ram_postcopy_incoming_init(MigrationIncomingState *mis);
>> * @migrate_add_blocker - prevent migration from proceeding
>> *
>> * @reason - an error to be returned whenever migration is attempted
>> + * @errp - [out] The reason (if any) we cannot block migration right now.
>> + *
>> + * @returns - 0 on success, -EBUSY on failure, with errp set.
>> */
>> -void migrate_add_blocker(Error *reason);
>> +int migrate_add_blocker(Error *reason, Error **errp);
>>
>> /**
>> * @migrate_del_blocker - remove a blocking error from migration
>> diff --git a/migration/migration.c b/migration/migration.c
>> index f498ab8..713e012 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1044,6 +1044,31 @@ bool migration_in_postcopy_after_devices(MigrationState *s)
>> return migration_in_postcopy(s) && s->postcopy_after_devices;
>> }
>>
>> +bool migration_is_idle(MigrationState *s)
>> +{
>> + if (!s) {
>> + s = migrate_get_current();
>> + }
>> +
>> + switch (s->state) {
>> + case MIGRATION_STATUS_NONE:
>> + case MIGRATION_STATUS_CANCELLED:
>> + case MIGRATION_STATUS_COMPLETED:
>> + case MIGRATION_STATUS_FAILED:
>> + return true;
>> + case MIGRATION_STATUS_SETUP:
>> + case MIGRATION_STATUS_CANCELLING:
>> + case MIGRATION_STATUS_ACTIVE:
>> + case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>> + case MIGRATION_STATUS_COLO:
>> + return false;
>> + case MIGRATION_STATUS__MAX:
>> + g_assert_not_reached();
>> + }
>> +
>> + return false;
>> +}
>> +
>> MigrationState *migrate_init(const MigrationParams *params)
>> {
>> MigrationState *s = migrate_get_current();
>> @@ -1086,9 +1111,15 @@ MigrationState *migrate_init(const MigrationParams *params)
>>
>> static GSList *migration_blockers;
>>
>> -void migrate_add_blocker(Error *reason)
>> +int migrate_add_blocker(Error *reason, Error **errp)
>> {
>> - migration_blockers = g_slist_prepend(migration_blockers, reason);
>> + if (migration_is_idle(NULL)) {
>> + migration_blockers = g_slist_prepend(migration_blockers, reason);
>> + return 0;
>> + }
>> +
>> + error_setg(errp, "Cannot block a migration already in-progress");
>> + return -EBUSY;
>> }
>>
>> void migrate_del_blocker(Error *reason)
>> diff --git a/stubs/migr-blocker.c b/stubs/migr-blocker.c
>> index 8ab3604..a5ba18f 100644
>> --- a/stubs/migr-blocker.c
>> +++ b/stubs/migr-blocker.c
>> @@ -2,8 +2,9 @@
>> #include "qemu-common.h"
>> #include "migration/migration.h"
>>
>> -void migrate_add_blocker(Error *reason)
>> +int migrate_add_blocker(Error *reason, Error **errp)
>> {
>> + return 0;
>> }
>>
>> void migrate_del_blocker(Error *reason)
>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>> index f62264a..6031a1c 100644
>> --- a/target-i386/kvm.c
>> +++ b/target-i386/kvm.c
>> @@ -961,7 +961,11 @@ int kvm_arch_init_vcpu(CPUState *cs)
>> error_setg(&invtsc_mig_blocker,
>> "State blocked by non-migratable CPU device"
>> " (invtsc flag)");
>> - migrate_add_blocker(invtsc_mig_blocker);
>> + r = migrate_add_blocker(invtsc_mig_blocker, NULL);
>> + if (r < 0) {
>> + error_report("kvm: migrate_add_blocker failed");
>> + goto efail;
>> + }
>> /* for savevm */
>> vmstate_x86_cpu.unmigratable = 1;
>> }
>> @@ -969,12 +973,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
>> cpuid_data.cpuid.padding = 0;
>> r = kvm_vcpu_ioctl(cs, KVM_SET_CPUID2, &cpuid_data);
>> if (r) {
>> - return r;
>> + goto fail;
>> }
>>
>> r = kvm_arch_set_tsc_khz(cs);
>> if (r < 0) {
>> - return r;
>> + goto fail;
>> }
>>
>> /* vcpu's TSC frequency is either specified by user, or following
>> @@ -1001,6 +1005,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
>> }
>>
>> return 0;
>> +
>> + fail:
>> + migrate_del_blocker(invtsc_mig_blocker);
>> + efail:
>> + error_free(invtsc_mig_blocker);
>> + return r;
>> }
>
> Couldn't that be simplified just to always do the migrate_del_blocker?
> I don't think there's any harm in calling migrate_del_blocker if the add failed.
Hmm, I will change it to just
fail:
migrate_del_blocker(invtsc_mig_blocker);
error_free(invtsc_mig_blocker);
return r;
Ashijeet
>
> Dave
>
>> void kvm_arch_reset_vcpu(X86CPU *cpu)
>> --
>> 2.6.2
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] migration: Fail migration blocker for --only-migratble
2017-01-10 17:15 ` Peter Maydell
@ 2017-01-11 5:43 ` Ashijeet Acharya
2017-01-12 10:45 ` Ashijeet Acharya
1 sibling, 0 replies; 18+ messages in thread
From: Ashijeet Acharya @ 2017-01-11 5:43 UTC (permalink / raw)
To: Peter Maydell
Cc: Dr. David Alan Gilbert, John Snow, Amit Shah, Paolo Bonzini,
Kevin Wolf, Markus Armbruster, Juan Quintela, Michael S. Tsirkin,
Marc-André Lureau, Greg Kurz, Aneesh Kumar K.V,
QEMU Developers
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] migration: Fail migration blocker for --only-migratble
2017-01-10 17:15 ` Peter Maydell
2017-01-11 5:43 ` Ashijeet Acharya
@ 2017-01-12 10:45 ` Ashijeet Acharya
2017-01-12 11:16 ` Greg Kurz
1 sibling, 1 reply; 18+ messages in thread
From: Ashijeet Acharya @ 2017-01-12 10:45 UTC (permalink / raw)
To: Peter Maydell
Cc: Dr. David Alan Gilbert, John Snow, Amit Shah, Paolo Bonzini,
Kevin Wolf, Markus Armbruster, Juan Quintela, Michael S. Tsirkin,
Marc-André Lureau, Greg Kurz, Aneesh Kumar K.V,
QEMU Developers
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] migration: Fail migration blocker for --only-migratble
2017-01-12 10:45 ` Ashijeet Acharya
@ 2017-01-12 11:16 ` Greg Kurz
2017-01-12 11:50 ` Ashijeet Acharya
0 siblings, 1 reply; 18+ messages in thread
From: Greg Kurz @ 2017-01-12 11:16 UTC (permalink / raw)
To: Ashijeet Acharya
Cc: Peter Maydell, Dr. David Alan Gilbert, John Snow, Amit Shah,
Paolo Bonzini, Kevin Wolf, Markus Armbruster, Juan Quintela,
Michael S. Tsirkin, Marc-André Lureau, Aneesh Kumar K.V,
QEMU Developers
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
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/4] migration: Fail migration blocker for --only-migratble
2017-01-12 11:16 ` Greg Kurz
@ 2017-01-12 11:50 ` Ashijeet Acharya
0 siblings, 0 replies; 18+ messages in thread
From: Ashijeet Acharya @ 2017-01-12 11:50 UTC (permalink / raw)
To: Greg Kurz
Cc: Peter Maydell, Dr. David Alan Gilbert, John Snow, Amit Shah,
Paolo Bonzini, Kevin Wolf, Markus Armbruster, Juan Quintela,
Michael S. Tsirkin, Marc-André Lureau, Aneesh Kumar K.V,
QEMU Developers
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
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-01-12 11:51 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-09 17:02 [Qemu-devel] [PATCH v4 0/4] Introduce a new --only-migratable option Ashijeet Acharya
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 1/4] migration: Add a new option to enable only-migratable Ashijeet Acharya
2017-01-10 11:02 ` Dr. David Alan Gilbert
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 2/4] migration: Allow "device add" options to only add migratable devices Ashijeet Acharya
2017-01-10 11:23 ` Dr. David Alan Gilbert
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 3/4] migration: disallow migrate_add_blocker during migration Ashijeet Acharya
2017-01-10 8:25 ` Greg Kurz
2017-01-10 12:01 ` Dr. David Alan Gilbert
2017-01-10 18:33 ` Ashijeet Acharya
2017-01-09 17:02 ` [Qemu-devel] [PATCH v4 4/4] migration: Fail migration blocker for --only-migratble Ashijeet Acharya
2017-01-09 21:01 ` Eric Blake
2017-01-10 12:11 ` Dr. David Alan Gilbert
2017-01-10 17:15 ` Peter Maydell
2017-01-11 5:43 ` Ashijeet Acharya
2017-01-12 10:45 ` Ashijeet Acharya
2017-01-12 11:16 ` Greg Kurz
2017-01-12 11:50 ` Ashijeet Acharya
2017-01-09 17:27 ` [Qemu-devel] [PATCH v4 0/4] Introduce a new --only-migratable option no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.