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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2941 bytes --]

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>


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