On Thursday, 12 January 2017, Greg Kurz <gr...@kaod.org> wrote:
> On Thu, 12 Jan 2017 16:15:28 +0530
> Ashijeet Acharya <ashijeetacha...@gmail.com <javascript:;>> wrote:
>
> > On Tuesday, 10 January 2017, Peter Maydell <peter.mayd...@linaro.org
> <javascript:;>> wrote:
> >
> > > On 9 January 2017 at 17:02, Ashijeet Acharya <
> ashijeetacha...@gmail.com <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
