Re: [PATCH v3 02/13] block: use return status of bdrv_append()

From: Kevin Wolf <kwolf@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Cc: berto@igalia.com, pavel.dovgaluk@ispras.ru,
	qemu-block@nongnu.org, qemu-devel@nongnu.org, armbru@redhat.com,
	Greg Kurz <groug@kaod.org>,
	stefanha@redhat.com, den@openvz.org, pbonzini@redhat.com,
	mreitz@redhat.com, jsnow@redhat.com, ari@tuxera.com
Subject: Re: [PATCH v3 02/13] block: use return status of bdrv_append()
Date: Mon, 19 Oct 2020 14:13:02 +0200	[thread overview]
Message-ID: <20201019121302.GD6508@merkur.fritz.box> (raw)
In-Reply-To: <20201016171057.6182-3-vsementsov@virtuozzo.com>

Am 16.10.2020 um 19:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Now bdrv_append returns status and we can drop all the local_err things
> around it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c                     |  5 +----
>  block/backup-top.c          | 20 ++++++++------------
>  block/commit.c              |  5 +----
>  block/mirror.c              |  6 ++----
>  blockdev.c                  |  4 +---
>  tests/test-bdrv-graph-mod.c |  6 +++---
>  6 files changed, 16 insertions(+), 30 deletions(-)
> 
> diff --git a/block.c b/block.c
> index b05fbff42d..7b6818c681 100644
> --- a/block.c
> +++ b/block.c
> @@ -3161,7 +3161,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
>      int64_t total_size;
>      QemuOpts *opts = NULL;
>      BlockDriverState *bs_snapshot = NULL;
> -    Error *local_err = NULL;
>      int ret;
>  
>      /* if snapshot, we create a temporary backing file and open it
> @@ -3208,9 +3207,7 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
>       * 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, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    if (bdrv_append(bs_snapshot, bs, errp) < 0) {

We generally avoid calling functions with side effects inside a
comparison. Let's use the usual pattern:

    ret = bdrv_append(bs_snapshot, bs, errp);
    if (ret < 0) {
        ...
    }

(I mention it only once, but there are multiple instances in this
patch.)

>          bs_snapshot = NULL;
>          goto out;
>      }

> diff --git a/tests/test-bdrv-graph-mod.c b/tests/test-bdrv-graph-mod.c
> index 8cff13830e..2b71601c24 100644
> --- a/tests/test-bdrv-graph-mod.c
> +++ b/tests/test-bdrv-graph-mod.c
> @@ -101,7 +101,7 @@ static BlockDriverState *pass_through_node(const char *name)
>   */
>  static void test_update_perm_tree(void)
>  {
> -    Error *local_err = NULL;
> +    int ret;
>  
>      BlockBackend *root = blk_new(qemu_get_aio_context(),
>                                   BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ,
> @@ -114,8 +114,8 @@ static void test_update_perm_tree(void)
>      bdrv_attach_child(filter, bs, "child", &child_of_bds,
>                        BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, &error_abort);
>  
> -    bdrv_append(filter, bs, &local_err);
> -    error_free_or_abort(&local_err);
> +    ret = bdrv_append(filter, bs, NULL);
> +    assert(ret < 0);

Usually, test cases use g_assert_cmpint() which prints the expected and
actual value if it fails.

Kevin