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 <ebl...@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
signature.asc
Description: OpenPGP digital signature
