Re: [Qemu-devel] [PATCH V5 03/10] block/qcow2: parse compress create options

[Eric Blake <eblake@redhat.com>]

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

On 07/25/2017 09:41 AM, Peter Lieven wrote:
> this adds parsing and validation for the compress create
> options. They are only validated but not yet used.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/qcow2.c             | 86 +++++++++++++++++++++++++++++++++++++++++++++--
>  include/block/block_int.h | 39 +++++++++++----------
>  2 files changed, 105 insertions(+), 20 deletions(-)
> 

> +static void
> +qcow2_check_compress_settings(Qcow2Compress *compress, Error **errp)
> +{
> +    if (compress->format == QCOW2_COMPRESS_FORMAT_DEFLATE) {
> +        if (!compress->u.deflate.has_level) {
> +            compress->u.deflate.has_level = true;
> +            compress->u.deflate.level = 0;
> +        }
> +        if (compress->u.deflate.level > 9) {
> +            error_setg(errp, "Compress level %" PRIu8 " is not supported for"
> +                       " format '%s'", compress->u.deflate.level,
> +                       Qcow2CompressFormat_lookup[compress->format]);
> +            return;
> +        }
> +        if (!compress->u.deflate.has_window_size) {
> +            compress->u.deflate.has_window_size = true;
> +            compress->u.deflate.window_size = 15;

Maybe for 2.11 we should finally get around to adding parameter defaults
directly in QMP (you would then not need a has_FOO for any parameter FOO
that can specify its own default).  Not your problem to solve, though.

> +        }
> +        if (compress->u.deflate.window_size < 8 ||
> +            compress->u.deflate.window_size > 15) {
> +            error_setg(errp, "Compress window size %" PRIu8 " is not supported"
> +                       " for format '%s'", compress->u.deflate.window_size,
> +                       Qcow2CompressFormat_lookup[compress->format]);

Should we allow a window_size of 0 in the UI (meaning pick a sane
non-zero default)?  It's odd that you allow 0 for level but not for
window-size.

> +            return;
> +        }
> +    }
> +}

Dead return statement, since the end of the function occurs anyways.
But arguably it helps future maintenance if more tests were to be added
later?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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