On Fri, Dec 16, 2016 at 5:03 PM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 16 December 2016 at 09:53, Ashijeet Acharya
> <ashijeetacha...@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 <ashijeetacha...@gmail.com>
>
>> 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
