On 9 January 2017 at 17:02, Ashijeet Acharya <ashijeetacha...@gmail.com> 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
