[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v11 7/9] blkdebug: Simplify override logic
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH v11 7/9] blkdebug: Simplify override logic |
Date: |
Wed, 3 May 2017 14:03:16 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0 |
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 <address@hidden>
>>
>> ---
>> @@ -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