On Tuesday, 10 January 2017, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 9 January 2017 at 17:02, Ashijeet Acharya <ashijeetacha...@gmail.com
> <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
>
