[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 01/13] block: return status from bdrv_append and friends
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v3 01/13] block: return status from bdrv_append and friends |
Date: |
Mon, 19 Oct 2020 13:50:35 +0200 |
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
- [PATCH v3 00/13] block: deal with errp: part I, Vladimir Sementsov-Ogievskiy, 2020/10/16
- [PATCH v3 04/13] blockdev: fix drive_backup_prepare() missed error, Vladimir Sementsov-Ogievskiy, 2020/10/16
- [PATCH v3 05/13] block: drop extra error propagation for bdrv_set_backing_hd, Vladimir Sementsov-Ogievskiy, 2020/10/16
- [PATCH v3 07/13] blockjob: return status from block_job_set_speed(), Vladimir Sementsov-Ogievskiy, 2020/10/16
- [PATCH v3 03/13] block: check return value of bdrv_open_child and drop error propagation, Vladimir Sementsov-Ogievskiy, 2020/10/16
- [PATCH v3 06/13] block/mirror: drop extra error propagation in commit_active_start(), Vladimir Sementsov-Ogievskiy, 2020/10/16
- [PATCH v3 08/13] block/qcow2: qcow2_get_specific_info(): drop error propagation, Vladimir Sementsov-Ogievskiy, 2020/10/16