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

[Peter Maydell]

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
    }

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.

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.

thanks
-- PMM