On 21.02.2017 15:58, Kevin Wolf wrote: > Aborting on error in bdrv_append() isn't correct. This patch fixes it > and lets the callers handle failures. > > Test case 085 needs a reference output update. This is caused by the > reversed order of bdrv_set_backing_hd() and change_parent_backing_link() > in bdrv_append(): When the backing file of the new node is set, the > parent nodes are still pointing to the old top, so the backing blocker > is now initialised with the node name rather than the BlockBackend name. > > Signed-off-by: Kevin Wolf <[email protected]> > --- > block.c | 23 +++++++++++++++++------ > block/mirror.c | 9 ++++++++- > blockdev.c | 15 +++++++++++++-- > include/block/block.h | 3 ++- > tests/qemu-iotests/085.out | 2 +- > 5 files changed, 41 insertions(+), 11 deletions(-) > > diff --git a/block.c b/block.c > index b3f03a4..33edbda 100644 > --- a/block.c > +++ b/block.c > @@ -2060,6 +2060,7 @@ static BlockDriverState > *bdrv_append_temp_snapshot(BlockDriverState *bs, > int64_t total_size; > QemuOpts *opts = NULL; > BlockDriverState *bs_snapshot; > + Error *local_err = NULL; > int ret; > > /* if snapshot, we create a temporary backing file and open it > @@ -2109,7 +2110,12 @@ static BlockDriverState > *bdrv_append_temp_snapshot(BlockDriverState *bs, > * call bdrv_unref() on it), so in order to be able to return one, we > have > * to increase bs_snapshot's refcount here */ > bdrv_ref(bs_snapshot); > - bdrv_append(bs_snapshot, bs); > + bdrv_append(bs_snapshot, bs, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + ret = -EINVAL; > + goto out; > + } > > g_free(tmp_filename); > return bs_snapshot; > @@ -2900,20 +2906,25 @@ static void > change_parent_backing_link(BlockDriverState *from, > * parents of bs_top after bdrv_append() returns. If the caller needs to > keep a > * reference of its own, it must call bdrv_ref(). > */ > -void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top) > +void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, > + Error **errp) > { > + Error *local_err = NULL; > + > assert(!atomic_read(&bs_top->in_flight)); > assert(!atomic_read(&bs_new->in_flight)); > > - bdrv_ref(bs_top); > + bdrv_set_backing_hd(bs_new, bs_top, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + goto out; > + } > > change_parent_backing_link(bs_top, bs_new); > - /* FIXME Error handling */ > - bdrv_set_backing_hd(bs_new, bs_top, &error_abort); > - bdrv_unref(bs_top); > > /* bs_new is now referenced by its new parents, we don't need the > * additional reference any more. */ > +out: > bdrv_unref(bs_new); > } > > diff --git a/block/mirror.c b/block/mirror.c > index abbd847..4e35d85 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1066,6 +1066,7 @@ static void mirror_start_job(const char *job_id, > BlockDriverState *bs, > BlockDriverState *mirror_top_bs; > bool target_graph_mod; > bool target_is_backing; > + Error *local_err = NULL; > int ret; > > if (granularity == 0) { > @@ -1096,9 +1097,15 @@ static void mirror_start_job(const char *job_id, > BlockDriverState *bs, > * it alive until block_job_create() even if bs has no parent. */ > bdrv_ref(mirror_top_bs); > bdrv_drained_begin(bs); > - bdrv_append(mirror_top_bs, bs); > + bdrv_append(mirror_top_bs, bs, &local_err); > bdrv_drained_end(bs); > > + if (local_err) { > + bdrv_unref(mirror_top_bs); > + error_propagate(errp, local_err); > + return; > + } > + > /* Make sure that the source is not resized while the job is running */ > s = block_job_create(job_id, driver, mirror_top_bs, > BLK_PERM_CONSISTENT_READ, > diff --git a/blockdev.c b/blockdev.c > index 63b1ac4..580fa66 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1764,6 +1764,16 @@ static void external_snapshot_prepare(BlkActionState > *common, > > if (!state->new_bs->drv->supports_backing) { > error_setg(errp, "The snapshot does not support backing images"); > + return; > + } > + > + /* This removes our old bs and adds the new bs. This is an operation that > + * can fail, so we need to do it in .prepare; undoing it for abort is > + * always possible. */ > + bdrv_append(state->new_bs, state->old_bs, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > }
After bdrv_append(), the state->new_bs reference is no longer valid,
necessarily; especially if the operation failed.
> }
>
> @@ -1774,8 +1784,6 @@ static void external_snapshot_commit(BlkActionState
> *common)
>
> bdrv_set_aio_context(state->new_bs, state->aio_context);
>
> - /* This removes our old bs and adds the new bs */
> - bdrv_append(state->new_bs, state->old_bs);
> /* We don't need (or want) to use the transactional
> * bdrv_reopen_multiple() across all the entries at once, because we
> * don't want to abort all of them if one of them fails the reopen */
> @@ -1791,6 +1799,9 @@ static void external_snapshot_abort(BlkActionState
> *common)
> DO_UPCAST(ExternalSnapshotState, common,
> common);
> if (state->new_bs) {
> bdrv_unref(state->new_bs);
So this is not necessarily a good idea. I guess the solution would be a
bdrv_ref() before bdrv_append().
(At which point we might just remove the bdrv_unref() from bdrv_append()
because all of its callers would invoke bdrv_ref() before, so it's
definitely no longer "what the callers commonly need.")
> + if (state->new_bs->backing) {
> + bdrv_replace_in_backing_chain(state->new_bs, state->old_bs);
> + }
And in any case, this should be before the bdrv_unref(), because that
may have dropped the last reference to state->new_bs.
Max
> }
> }
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 6a6408e..6ce8b26 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -235,7 +235,8 @@ int bdrv_create(BlockDriver *drv, const char* filename,
> QemuOpts *opts, Error **errp);
> int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
> BlockDriverState *bdrv_new(void);
> -void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
> +void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
> + Error **errp);
> void bdrv_replace_in_backing_chain(BlockDriverState *old,
> BlockDriverState *new);
>
> diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
> index 08e4bb7..182acb4 100644
> --- a/tests/qemu-iotests/085.out
> +++ b/tests/qemu-iotests/085.out
> @@ -74,7 +74,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> backing_file=TEST_DIR/
>
> === Invalid command - snapshot node used as backing hd ===
>
> -{"error": {"class": "GenericError", "desc": "Node 'snap_11' is busy: node is
> used as backing hd of 'virtio0'"}}
> +{"error": {"class": "GenericError", "desc": "Node 'snap_11' is busy: node is
> used as backing hd of 'snap_12'"}}
>
> === Invalid command - snapshot node has a backing image ===
>
>
signature.asc
Description: OpenPGP digital signature
