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

[Peter Maydell]

On 16 December 2016 at 09:53, Ashijeet Acharya
<address@hidden> 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 <address@hidden>

> diff --git a/block/qcow.c b/block/qcow.c
> index 11526a1..7e5003ac 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -253,8 +253,11 @@ static int qcow_open(BlockDriverState *bs, QDict 
> *options, int flags,
>                 "does not support live migration",
>                 bdrv_get_device_or_node_name(bs));
>      ret = migrate_add_blocker(s->migration_blocker, errp);
> -    if (ret < 0) {
> -        error_free(s->migration_blocker);
> +    if (ret) {
> +        if (ret == -EACCES) {
> +            error_append_hint(errp, "Cannot use a node with qcow format as "
> +                              "it does not support live migration");
> +        }
>          goto fail;
>      }

> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1102,6 +1102,7 @@ static void virtio_gpu_device_realize(DeviceState 
> *qdev, Error **errp)
>      VirtIOGPU *g = VIRTIO_GPU(qdev);
>      bool have_virgl;
>      int i;
> +    int ret;
>
>      if (g->conf.max_outputs > VIRTIO_GPU_MAX_SCANOUTS) {
>          error_setg(errp, "invalid max_outputs > %d", 
> VIRTIO_GPU_MAX_SCANOUTS);
> @@ -1120,8 +1121,12 @@ static void virtio_gpu_device_realize(DeviceState 
> *qdev, Error **errp)
>
>      if (virtio_gpu_virgl_enabled(g->conf)) {
>          error_setg(&g->migration_blocker, "virgl is not yet migratable");
> -        if (migrate_add_blocker(g->migration_blocker, errp) < 0) {
> -            error_free(g->migration_blocker);
> +        ret = migrate_add_blocker(g->migration_blocker, errp);
> +        if (ret) {
> +            if (ret == -EACCES) {
> +                error_append_hint(errp, "Cannot use virgl as it does not "
> +                                  "support live migration yet");
> +            }
>              return;
>          }
>      }
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index 3614daa..b40ad5f 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -514,8 +514,11 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error 
> **errp)
>          error_setg(&s->migration_blocker, "This operating system kernel does 
> "
>                                            "not support vGICv2 migration");
>          ret = migrate_add_blocker(s->migration_blocker, errp);
> -        if (ret < 0) {
> -            error_free(s->migration_blocker);
> +        if (ret) {
> +            if (ret == -EACCES) {
> +                error_append_hint(errp, "Failed to realize vGICv2 as its"
> +                                  " migrataion is not implemented yet");
> +            }
>              return;
>          }
>      }

So if you look at these diff hunks you can see that we effectively
end up duplicating the "why doesn't this device let you migrate"
user message -- once in the error_setg(&s->migration_blocker, ...)
and once with the error_append_hint() call.

Is there some way we can get migrate_add_blocker() to automatically
incorporate the message when it's failing for the only-migratable case?
(It's already being passed the errp.)
That would avoid this duplication of the messages, and should also
mean we don't need to modify the callers at all.

If the error messages come out looking a bit strangely worded we
could edit the text in the callers so it works in both situations
when it will be used.

thanks
-- PMM