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

[Ashijeet Acharya]

On Tuesday, 10 January 2017, Peter Maydell <address@hidden> wrote:

> On 9 January 2017 at 17:02, Ashijeet Acharya <address@hidden
> <javascript:;>> 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
>     }


One more reason Peter I found for returning an error value is that in cases
like 9pfs where we do not set errp inside migrate_add_blocker() (as
suggested by Greg) and other similar ones where we pass NULL for errp, we
cannot rely on checking for
"if (local_err)" as it will never be set and always be NULL.
Thus we will never fail appropriately at the caller sites when we fail to
add migration blocker.
Sounds right?

Ashijeet

>
> 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
>