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

[Ashijeet Acharya]

On Thursday, 12 January 2017, Greg Kurz <address@hidden> wrote:

> On Thu, 12 Jan 2017 16:15:28 +0530
> Ashijeet Acharya <address@hidden <javascript:;>> wrote:
>
> > On Tuesday, 10 January 2017, Peter Maydell <address@hidden
> <javascript:;>> wrote:
> >
> > > On 9 January 2017 at 17:02, Ashijeet Acharya <
> address@hidden <javascript:;>
> > > <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?
> >
>
> The need for 9pfs is just to return an error to the guest, which will end
> up being the errno for the failed mount(). And we don't want to print out
> any error message to avoid a stupid guest to fill the QEMU log in case it
> would loop over mount().


Yes, I completely understood that earlier :-)
I had a doubt further because atm I was passing NULL as an argument i.e.

migrate_add_blocker(s->migration_blocker, NULL);

which is why the whole 'local_err being returned as  NULL' situation
arrived. But I will make it pass &local_err now.

Still, I think we need to return an error value from migrate_add_blocker()
to avoid setting it manually and also avoid repetition of code at call
sites.

>
> The only reason I see for migration_add_blocker() to return an error
> would be to differentiate between the --only-migratable and the migration
> in progress cases... But honestly, I don't care that much and something
> like the following would be perfectly ok:
>
>     Error *local_err = NULL;
>     [...]
>     migrate_add_blocker(s->migration_blocker, &local_err);
>     if (local_err) {
>         error_free(local_err);
>         err = -EBUSY
>         goto out_nofid;
>     }
>
> Maybe yes, if everyone is okay with the same error value being set in both
the cases, then I will submit the patches like you proposed above :-)

Ashijeet