Re: [PATCH v3 01/13] block: return status from bdrv_append and friends

[Kevin Wolf <kwolf@redhat.com>]

Am 16.10.2020 um 19:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
> The recommended use of qemu error api assumes returning status together
> with setting errp and avoid void functions with errp parameter. Let's
> improve bdrv_append and some friends to reduce error-propagation
> overhead in further patches.
> 
> Choose int return status, because bdrv_replace_node() has call to
> bdrv_check_update_perm(), which reports int status, which seems correct
> to propagate.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  include/block/block.h | 12 ++++++------
>  block.c               | 39 ++++++++++++++++++++++++---------------
>  2 files changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index d16c401cb4..afb29cdbe4 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -346,10 +346,10 @@ int bdrv_create(BlockDriver *drv, const char* filename,
>  int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
>  
>  BlockDriverState *bdrv_new(void);
> -void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
> -                 Error **errp);
> -void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
> -                       Error **errp);
> +int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
> +                Error **errp);
> +int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
> +                      Error **errp);
>  
>  int bdrv_parse_aio(const char *mode, int *flags);
>  int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);
> @@ -361,8 +361,8 @@ BdrvChild *bdrv_open_child(const char *filename,
>                             BdrvChildRole child_role,
>                             bool allow_none, Error **errp);
>  BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
> -void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
> -                         Error **errp);
> +int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
> +                        Error **errp);
>  int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
>                             const char *bdref_key, Error **errp);
>  BlockDriverState *bdrv_open(const char *filename, const char *reference,
> diff --git a/block.c b/block.c
> index 430edf79bb..b05fbff42d 100644
> --- a/block.c
> +++ b/block.c
> @@ -2870,14 +2870,15 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState *bs)
>   * Sets the bs->backing link of a BDS. A new reference is created; callers
>   * which don't need their own reference any more must call bdrv_unref().
>   */
> -void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
> +int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
>                           Error **errp)
>  {
> +    int ret = 0;
>      bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
>          bdrv_inherits_from_recursive(backing_hd, bs);
>  
>      if (bdrv_is_backing_chain_frozen(bs, child_bs(bs->backing), errp)) {
> -        return;
> +        return -EPERM;
>      }
>  
>      if (backing_hd) {
> @@ -2896,15 +2897,22 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
>  
>      bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_of_bds,
>                                      bdrv_backing_role(bs), errp);
> +    if (!bs->backing) {
> +        ret = -EINVAL;

I think -EPERM describes the actual error cases better (though I'm not
sure if we're going to actually use the error code anywhere).

> +        goto out;
> +    }
> +
>      /* If backing_hd was already part of bs's backing chain, and
>       * inherits_from pointed recursively to bs then let's update it to
>       * point directly to bs (else it will become NULL). */
> -    if (bs->backing && update_inherits_from) {
> +    if (update_inherits_from) {
>          backing_hd->inherits_from = bs;
>      }
>  
>  out:

Please move the ret = 0 from the declaration to right above this line.
Otherwise we'd have to be careful in the future that the last assignment
of ret can't give it a non-zero (probably positive) value. Having
ret = 0 immediately before the label is the safe pattern.

Another possible advantage is that in some cases the compiler may then
be able to warn if you forget to set ret in an error path.

>      bdrv_refresh_limits(bs, NULL);
> +
> +    return ret;
>  }
>  
>  /*
> @@ -4554,8 +4562,8 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
>      return ret;
>  }
>  
> -void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
> -                       Error **errp)
> +int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
> +                      Error **errp)
>  {
>      BdrvChild *c, *next;
>      GSList *list = NULL, *p;
> @@ -4577,6 +4585,7 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
>              continue;
>          }
>          if (c->frozen) {
> +            ret = -EPERM;
>              error_setg(errp, "Cannot change '%s' link to '%s'",
>                         c->name, from->node_name);
>              goto out;
> @@ -4612,6 +4621,8 @@ out:
>      g_slist_free(list);
>      bdrv_drained_end(from);
>      bdrv_unref(from);
> +
> +    return ret;

Please add the ret = 0 before the label, too.

>  }
>  
>  /*
> @@ -4630,20 +4641,16 @@ out:
>   * 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,
> -                 Error **errp)
> +int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
> +                Error **errp)
>  {
> -    Error *local_err = NULL;
> -
> -    bdrv_set_backing_hd(bs_new, bs_top, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    int ret = bdrv_set_backing_hd(bs_new, bs_top, errp);
> +    if (ret < 0) {
>          goto out;
>      }
>  
> -    bdrv_replace_node(bs_top, bs_new, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    ret = bdrv_replace_node(bs_top, bs_new, errp);
> +    if (ret < 0) {
>          bdrv_set_backing_hd(bs_new, NULL, &error_abort);
>          goto out;
>      }
> @@ -4652,6 +4659,8 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
>       * additional reference any more. */
>  out:

And another one.

>      bdrv_unref(bs_new);
> +
> +    return ret;
>  }

Looks good to me otherwise.

Kevin