Re: [Qemu-block] [RFC PATCH 03/41] block: Add Error argument to bdrv_attach_child()

[Max Reitz]

On 13.02.2017 18:22, Kevin Wolf wrote:
> It will have to return an error soon, so prepare the callers for it.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block.c               | 13 ++++++++++---
>  block/quorum.c        |  8 +++++++-
>  include/block/block.h |  3 ++-
>  3 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 0618f4b..9895aaf 100644
> --- a/block.c
> +++ b/block.c
> @@ -1323,7 +1323,8 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState 
> *child_bs,
>  BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
>                               BlockDriverState *child_bs,
>                               const char *child_name,
> -                             const BdrvChildRole *child_role)
> +                             const BdrvChildRole *child_role,
> +                             Error **errp)
>  {
>      BdrvChild *child = bdrv_root_attach_child(child_bs, child_name, 
> child_role,
>                                                parent_bs);
> @@ -1424,7 +1425,9 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
> BlockDriverState *backing_hd)
>          bs->backing = NULL;
>          goto out;
>      }
> -    bs->backing = bdrv_attach_child(bs, backing_hd, "backing", 
> &child_backing);
> +    /* FIXME Error handling */
> +    bs->backing = bdrv_attach_child(bs, backing_hd, "backing", 
> &child_backing,
> +                                    &error_abort);

Not sure if this counts as "prepare the callers for it". ;-)

>      bs->open_flags &= ~BDRV_O_NO_BACKING;
>      pstrcpy(bs->backing_file, sizeof(bs->backing_file), 
> backing_hd->filename);
>      pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> @@ -1594,7 +1597,11 @@ BdrvChild *bdrv_open_child(const char *filename,
>          goto done;
>      }
>  
> -    c = bdrv_attach_child(parent, bs, bdref_key, child_role);
> +    c = bdrv_attach_child(parent, bs, bdref_key, child_role, errp);
> +    if (!c) {
> +        bdrv_unref(bs);
> +        goto done;
> +    }
>  
>  done:
>      qdict_del(options, bdref_key);
> diff --git a/block/quorum.c b/block/quorum.c
> index 86e2072..45bf010 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -1032,10 +1032,16 @@ static void quorum_add_child(BlockDriverState *bs, 
> BlockDriverState *child_bs,
>  
>      /* We can safely add the child now */
>      bdrv_ref(child_bs);
> -    child = bdrv_attach_child(bs, child_bs, indexstr, &child_format);
> +
> +    child = bdrv_attach_child(bs, child_bs, indexstr, &child_format, errp);
> +    if (child == NULL) {
> +        bdrv_unref(child_bs);
> +        goto out;

Might make sense to decrement next_child_index here, but it's not necessary.

(That means: Patch is fine, I'm just nagging because I like to nag.)

Max

> +    }
>      s->children = g_renew(BdrvChild *, s->children, s->num_children + 1);
>      s->children[s->num_children++] = child;
>  
> +out:
>      bdrv_drained_end(bs);
>  }
>  
> diff --git a/include/block/block.h b/include/block/block.h
> index 4e81f20..93812df 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -482,7 +482,8 @@ void bdrv_unref_child(BlockDriverState *parent, BdrvChild 
> *child);
>  BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
>                               BlockDriverState *child_bs,
>                               const char *child_name,
> -                             const BdrvChildRole *child_role);
> +                             const BdrvChildRole *child_role,
> +                             Error **errp);
>  
>  bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
>  void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);
>

signature.asc
Description: OpenPGP digital signature