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

[Ashijeet Acharya]

On Fri, Dec 16, 2016 at 5:03 PM, Peter Maydell <address@hidden> wrote:
> 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.

Maybe changing the error message in error_setg(&s->migration_blocker, ...) to:
"Trying to add migration blocker but will fail if --only-migratable is
specified"
or something like that do the trick?

>
> Is there some way we can get migrate_add_blocker() to automatically
> incorporate the message when it's failing for the only-migratable case?
> (It's already being passed the errp.)

Sorry! I am not sure how I can use that but if you have something more
I am ready to work on it to fix this.

> That would avoid this duplication of the messages, and should also
> mean we don't need to modify the callers at all.

Right. I also thought so while writing this patch but wasn't able to
come up with a solution.

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