[54/54] block: Add Error parameter to bdrv_append()

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 <kwolf@redhat.com>
---
 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(-)

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 <kwolf@redhat.com>
> ---
>  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 ===
>  
>