[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v11 7/9] blkdebug: Simplify override logic
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v11 7/9] blkdebug: Simplify override logic |
Date: |
Wed, 3 May 2017 21:01:19 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0.1 |
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 <address@hidden>
>>
>> ---
>> 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 <address@hidden>
signature.asc
Description: OpenPGP digital signature