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.
Max
> goto out;
> }
>
> ret = 0;
> - goto out;
> -
> out:
> if (ret < 0) {
> g_free(s->config_file);
>
signature.asc
Description: OpenPGP digital signature
