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

[Eric Blake <eblake@redhat.com>]

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

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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