Re: [Qemu-devel] [PATCH v14 3/5] qapi: introduce PreallocMode and new PreallocModes full and falloc.

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2111 bytes --]

On 09/08/2014 09:54 PM, Hu Tao wrote:
> This patch prepares for the subsequent patches.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c              | 23 +++++++++++++++--------
>  qapi/block-core.json       | 17 +++++++++++++++++
>  tests/qemu-iotests/049.out |  2 +-
>  3 files changed, 33 insertions(+), 9 deletions(-)
> 

>      buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
> -    if (!buf || !strcmp(buf, "off")) {
> -        prealloc = 0;
> -    } else if (!strcmp(buf, "metadata")) {
> -        prealloc = 1;
> -    } else {
> -        error_setg(errp, "Invalid preallocation mode: '%s'", buf);
> +    prealloc = qapi_enum_parse(PreallocMode_lookup, buf,
> +                               PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
> +                               &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
>          ret = -EINVAL;
>          goto finish;
>      }
> @@ -1958,6 +1958,13 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
>          flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
>      }
>  
> +    if (prealloc && prealloc != PREALLOC_MODE_METADATA) {
> +        ret = -EINVAL;
> +        error_setg(errp, "Unsupported preallocate mode: %s",
> +                   PreallocMode_lookup[prealloc]);
> +        goto finish;
> +    }

I _still_ think this looks weird, and would be better as either:

if (prealloc != PREALLOC_MODE_NONE &&
    prealloc != PREALLOC_MODE_METADATA) {

to make it obvious that you are filtering for two acceptable modes, or as:

if (prealloc == PREALLOC_MODE_FALLOC ||
    prealloc == PREALLOC_MODE_FULL) {

to make it obvious the modes that you do not support.  But my complaint
is not strong enough to prevent this patch, especially if later in the
series revisits this code.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]