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

[Ashijeet Acharya]

On Tue, Jan 10, 2017 at 10:45 PM, Peter Maydell
<address@hidden> wrote:
> On 9 January 2017 at 17:02, 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.
>
>> 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