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