[v11,7/9] blkdebug: Simplify override logic

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 <eblake@redhat.com>

---
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(-)

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 <eblake@redhat.com>
> 
> ---
> 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);
>

On 03.05.2017 20:59, Max Reitz wrote:
> 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 <eblake@redhat.com>
>>
>> ---
>> 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.

Ah. Read the next patch, that is why you don't. Well, I would've
probably still added it to each goto out;, but it does make sense to
just set it once here, so:

Reviewed-by: Max Reitz <mreitz@redhat.com>

On 05/03/2017 01:59 PM, Max Reitz wrote:
> 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 <eblake@redhat.com>
>>
>> ---

>> @@ -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.

Because the next patch would then have to add a 'ret = -EINVAL;' prior
to ever other added 'goto out;'.  It's easier to prep ret once, have a
bunch of intermediate gotos, and then have the final...

> 
> Max
> 
>>          goto out;
>>      }
>>
>>      ret = 0;
>> -    goto out;
>> -
>>  out:

...'ret = 0' reachable only on success, but it takes repeating the
pattern in the next patch to see that.

>>      if (ret < 0) {
>>          g_free(s->config_file);
>>
> 
>