Re: [PATCH v3 1/3] block: Add blk_new_with_bs() helper

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3599 bytes --]

On 24.04.20 21:09, Eric Blake wrote:
> There are several callers that need to create a new block backend from
> an existing BDS; make the task slightly easier with a common helper
> routine.
> 
> Suggested-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/sysemu/block-backend.h |  2 ++
>  block/block-backend.c          | 23 +++++++++++++++++++++++
>  block/crypto.c                 | 10 ++++------
>  block/parallels.c              |  8 ++++----
>  block/qcow.c                   |  8 ++++----
>  block/qcow2.c                  | 18 ++++++++----------
>  block/qed.c                    |  9 ++++-----
>  block/sheepdog.c               | 11 +++++------
>  block/vdi.c                    |  8 ++++----
>  block/vhdx.c                   |  9 ++++-----
>  block/vmdk.c                   |  9 ++++-----
>  block/vpc.c                    |  8 ++++----
>  blockdev.c                     |  8 +++-----
>  blockjob.c                     |  7 ++-----
>  14 files changed, 75 insertions(+), 63 deletions(-)

[...]

> diff --git a/block/crypto.c b/block/crypto.c
> index d577f89659fa..a4d77f07fe8c 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -256,16 +256,14 @@ static int block_crypto_co_create_generic(BlockDriverState *bs,
>                                            PreallocMode prealloc,
>                                            Error **errp)
>  {
> -    int ret;
> +    int ret = -EPERM;

I’m not sure I’m a fan of this, because I feel like it makes the code
harder to read, due to having to look in three places (here, around the
blk_new_with_bs() call, and under the cleanup label) instead of in two
(not here) to verify that the error handling code is correct.

There’s also the fact that this is not really a default return value,
but one very specific error code for if one very specific function call
fails.

I suppose it comes down to whether one considers LoC a complexity
problem.  (I don’t, necessarily.)

(Also I realize it seems rather common in the kernel to set error return
variables before the function call, but I think the more common pattern
in qemu is to set it in the error path.)

[...]

> diff --git a/block/qed.c b/block/qed.c
> index 1af9b3cb1db1..7a31495d293b 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -610,7 +610,7 @@ static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts,
>      QEDHeader le_header;
>      uint8_t *l1_table = NULL;
>      size_t l1_size;
> -    int ret = 0;
> +    int ret = -EPERM;

(Same here; well, especially here, because there is so much other code
between the initialization and the “goto out” this is for)

[...]

> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 59f7ebb1710f..2b53cd950d20 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1801,14 +1801,13 @@ static int sd_prealloc(BlockDriverState *bs, int64_t old_size, int64_t new_size,
>      uint32_t idx, max_idx;
>      uint32_t object_size;
>      void *buf = NULL;
> -    int ret;
> +    int ret = -EPERM;

(and here)

[...]

> diff --git a/block/vhdx.c b/block/vhdx.c
> index 33e57cd6567a..bdf5d05cc018 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1891,7 +1891,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
>      BlockBackend *blk = NULL;
>      BlockDriverState *bs = NULL;
> 
> -    int ret = 0;
> +    int ret = -EPERM;

(and again especially here)

But it does look like you got all cases covered this time.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]