Re: [Qemu-devel] [PATCH v11 7/9] blkdebug: Simplify override logic

[Max Reitz]

On 29.04.2017 21:14, Eric Blake wrote:
> Rather than store into a local variable, then copy to the struct
> if the value is valid, then reporting errors otherwise, it is
> simpler to just store into the struct and report errors if the
> value is invalid.  This however requires that the struct store
> a 64-bit number, rather than a narrower type.  Likewise, setting
> a sane errno value in ret prior to the sequence of parsing and
> jumping to out: on error makes it easier for the next patch to
> add a chain of similar checks.
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> v11: rebase to master, enough changed to drop R-b
> v10: no change
> v9: no change
> v7-v8: not submitted (earlier half of series sent for 2.9)
> v6: tweak error message, but R-b kept
> v5: no change
> v4: fix typo in commit message, move errno assignment
> v3: new patch
> ---
>  block/blkdebug.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 6414f1c..8e0f596 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -38,7 +38,7 @@
>  typedef struct BDRVBlkdebugState {
>      int state;
>      int new_state;
> -    int align;
> +    uint64_t align;
> 
>      /* For blkdebug_refresh_filename() */
>      char *config_file;
> @@ -353,7 +353,6 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      BDRVBlkdebugState *s = bs->opaque;
>      QemuOpts *opts;
>      Error *local_err = NULL;
> -    uint64_t align;
>      int ret;
> 
>      opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> @@ -387,20 +386,17 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
> *options, int flags,
>          bs->file->bs->supported_write_flags;
>      bs->supported_zero_flags = (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) &
>          bs->file->bs->supported_zero_flags;
> +    ret = -EINVAL;
> 
>      /* Set request alignment */
> -    align = qemu_opt_get_size(opts, "align", 0);
> -    if (align < INT_MAX && is_power_of_2(align)) {
> -        s->align = align;
> -    } else if (align) {
> -        error_setg(errp, "Invalid alignment");
> -        ret = -EINVAL;
> +    s->align = qemu_opt_get_size(opts, "align", 0);
> +    if (s->align && (s->align >= INT_MAX || !is_power_of_2(s->align))) {
> +        error_setg(errp, "Cannot meet constraints with align %" PRIu64,
> +                   s->align);

Why don't you just keep the ret = -EINVAL here and not add it above?
It's not wrong, I just think it makes the code a bit weird to read.

Max

>          goto out;
>      }
> 
>      ret = 0;
> -    goto out;
> -
>  out:
>      if (ret < 0) {
>          g_free(s->config_file);
>

signature.asc
Description: OpenPGP digital signature