Re: [Qemu-devel] [PATCH 3/3] migration: disallow migrate_add_blocker during migration

[Ashijeet Acharya]

On Thu, Dec 15, 2016 at 11:21 PM, John Snow  wrote:
>
>
> On 12/15/2016 12:11 PM, Dr. David Alan Gilbert wrote:
>>  if (virtio_gpu_virgl_enabled(g->conf)) {
>> +error_setg(>migration_blocker, "virgl is not yet migratable");
>> +ret = migrate_add_blocker(g->migration_blocker, errp);
>> +if (ret) {
>> +if (ret > 0) {
>> +error_setg(errp, "Cannot use virgl as it does not support 
>> live"
>> +   " migration yet and --only-migratable was "
>> +   "specified");
>> +}
>> +return;
>> +}
>> +}
>
> It may be best to leave this patch as a generic "Failed to add a
> migration blocker" type of patch.
>
> Then, in a fourth patch, you add the --only-migratable specific stuff as
> an enhancement. (Basically, add your changes on top of my patch in your
> own, separate patch.)

No problem, I will separate them. Although I will add changes for the
ARM drivers you first missed out on.

>
> I'd also add the "--only-migratable" stuff only inside
> migrate_add_blocker, and whatever the reason happens to be, you can
> append a hint to the error message in the caller above.
>
> e.g.
>
> migrate_add_blocker(..) {
> ..
>   error_setg("Failed to add migration blocker: --only-migratable was
> specified")
> ..
> }
>
> And in the caller:
>
> r = migrate_add_blocker(.., errp)
> if (r) {
>   error_append_hint(errp, "Failed to initialize virgl, couldn't add
> migration blocker")
> }
>
> Or something along those lines. Maybe even error_prepend in this case
> makes sense.

Yes, I will make this change and make the error message more like:
error_append_hint(errp, "Failed to initialize virgl as it does not
support live migration, couldn't add migration blocker");

>
> I'd try to keep all of the --only-migratable logic localized and in a
> separate patch, leaving this patch strictly to deal with the case where
> a migration is already in progress. Of course, you'll be right to notice
> that in many of these initialization cases the error path could never
> trigger, but that's OK. It adds the generic handling necessary to cope
> with an error from a lower layer.
>
> I'd also certainly advise to never return a custom error code from
> migrate_add_blocker if we also wish to return a 'real' error code. Stick
> with POSIX entirely or avoid it entirely.

Right, as discussed with Dave on the IRC, I will make use of EACCES.

Ashijeet
>
> --js

Re: [Qemu-devel] [PATCH 3/3] migration: disallow migrate_add_blocker during migration

[John Snow]

On 12/15/2016 12:11 PM, Dr. David Alan Gilbert wrote:
>  if (virtio_gpu_virgl_enabled(g->conf)) {
> +error_setg(>migration_blocker, "virgl is not yet migratable");
> +ret = migrate_add_blocker(g->migration_blocker, errp);
> +if (ret) {
> +if (ret > 0) {
> +error_setg(errp, "Cannot use virgl as it does not support 
> live"
> +   " migration yet and --only-migratable was "
> +   "specified");
> +}
> +return;
> +}
> +}

It may be best to leave this patch as a generic "Failed to add a
migration blocker" type of patch.

Then, in a fourth patch, you add the --only-migratable specific stuff as
an enhancement. (Basically, add your changes on top of my patch in your
own, separate patch.)

I'd also add the "--only-migratable" stuff only inside
migrate_add_blocker, and whatever the reason happens to be, you can
append a hint to the error message in the caller above.

e.g.

migrate_add_blocker(..) {
..
  error_setg("Failed to add migration blocker: --only-migratable was
specified")
..
}

And in the caller:

r = migrate_add_blocker(.., errp)
if (r) {
  error_append_hint(errp, "Failed to initialize virgl, couldn't add
migration blocker")
}

Or something along those lines. Maybe even error_prepend in this case
makes sense.

I'd try to keep all of the --only-migratable logic localized and in a
separate patch, leaving this patch strictly to deal with the case where
a migration is already in progress. Of course, you'll be right to notice
that in many of these initialization cases the error path could never
trigger, but that's OK. It adds the generic handling necessary to cope
with an error from a lower layer.

I'd also certainly advise to never return a custom error code from
migrate_add_blocker if we also wish to return a 'real' error code. Stick
with POSIX entirely or avoid it entirely.

--js

Re: [Qemu-devel] [PATCH 3/3] migration: disallow migrate_add_blocker during migration

[Dr. David Alan Gilbert]

* Ashijeet Acharya (ashijeetacha...@gmail.com) wrote:
> If a migration is already in progress and somebody attempts
> to add a migration blocker, this should rightly fail.
> 
> Also, it should 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.
> 
> Add an errp parameter and a retcode return value to migrate_add_blocker.
> 
> Signed-off-by: John Snow 
> Signed-off-by: Ashijeet Acharya 
> ---
>  block/qcow.c  | 11 ++-
>  block/vdi.c   | 11 ++-
>  block/vhdx.c  | 20 ++--
>  block/vmdk.c  | 12 +++-
>  block/vpc.c   | 15 ---
>  block/vvfat.c | 24 
>  hw/9pfs/9p.c  | 22 ++
>  hw/display/virtio-gpu.c   | 35 ++-
>  hw/intc/arm_gic_kvm.c | 20 ++--
>  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/virtio/vhost.c | 11 ++-
>  include/migration/migration.h |  6 +-
>  migration/migration.c | 42 --
>  stubs/migr-blocker.c  |  3 ++-
>  target-i386/kvm.c | 19 ---
>  18 files changed, 263 insertions(+), 75 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index 7540f43..7228fc8 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -252,7 +252,16 @@ static int qcow_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  error_setg(>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) {
> +if (ret > 0) {
> +error_setg(errp, "Cannot use a node with qcow format as it does"
> +   " not support live migration and --only-migratable 
> was "
> +   "specified");
> +}
> +
> +goto fail;
> +}
>  
>  qemu_co_mutex_init(>lock);
>  return 0;
> diff --git a/block/vdi.c b/block/vdi.c
> index 96b78d5..bb25fba 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -471,7 +471,16 @@ static int vdi_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  error_setg(>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) {
> +if (ret > 0) {
> +error_setg(errp, "Cannot use a node with vdi format as it does"
> +   " not support live migration and --only-migratable 
> was "
> +   "specified");
> +}
> +
> +goto fail;
> +}
>  
>  qemu_co_mutex_init(>write_lock);
>  
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 0ba2f0a..89c3d54 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -991,6 +991,20 @@ static int vhdx_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  }
>  }
>  
> +/* Disable migration when VHDX images are used */
> +error_setg(>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) {
> +if (ret > 0) {
> +error_setg(errp, "Cannot use a node with vhdx format as it does"
> +   " not support live migration and --only-migratable 
> was "
> +   "specified");
> +}
> +
> +goto fail;
> +}
>  if (flags & BDRV_O_RDWR) {
>  ret = vhdx_update_headers(bs, s, false, NULL);
>  if (ret < 0) {
> @@ -1000,12 +1014,6 @@ static int vhdx_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  
>  /* TODO: differencing files */
>  
> -/* Disable migration when VHDX images are used */
> -error_setg(>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..ee53aca 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -976,7 +976,17 @@ static int vmdk_open(BlockDriverState *bs, QDict 
> *options, int