Re: [Qemu-devel] [RFC PATCH 04/10] qcow2: Pass BlockdevCreateOptions to qcow2_create2()

[Eric Blake]

On 01/11/2018 01:52 PM, Kevin Wolf wrote:
> All of the simple options are now passed to qcow2_create2() in a
> BlockdevCreateOptions object. Still missing: node-name and the
> encryption options.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/qcow2.c | 186 
> ++++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 148 insertions(+), 38 deletions(-)
> 

> +    if (!qcow2_opts->has_lazy_refcounts) {
> +        qcow2_opts->lazy_refcounts = false;
> +    }
> +    if (version < 3 && qcow2_opts->lazy_refcounts) {
> +        error_setg(errp, "Lazy refcounts only supported with compatibility "
> +                   "level 1.1 and above (use compat=1.1 or greater)");

Does this error message need tweaking given that the QMP spelling for
the compat level is slightly different?


>  
>      /* Want a backing file? There you go.*/
> -    if (backing_file) {
> -        ret = bdrv_change_backing_file(blk_bs(blk), backing_file, 
> backing_format);
> +    if (qcow2_opts->has_backing_file) {
> +        const char *backing_format = NULL;
> +
> +        if (qcow2_opts->has_backing_fmt) {
> +            backing_format = BlockdevDriver_str(qcow2_opts->backing_fmt);
> +        }

Do we want to declare it an error to specify backing_fmt without a
backing file (string)?

> @@ -2970,9 +3056,33 @@ static int qcow2_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>      }
>  
>      /* Create the qcow2 image (format layer) */
> -    ret = qcow2_create2(bs, size, backing_file, backing_fmt, flags,
> -                        cluster_size, prealloc, opts, version, 
> refcount_order,
> -                        encryptfmt, errp);
> +    create_options = (BlockdevCreateOptions) {
> +        .driver         = BLOCKDEV_DRIVER_QCOW2,
> +        .node           = & (BlockdevRef) {
> +            .type               = QTYPE_QSTRING,
> +            .u.reference        = bs->node_name,
> +        },
> +        .u.qcow2        = {
> +            .size               = size,
> +            .has_compat         = true,
> +            .compat             = version == 2
> +                                  ? BLOCKDEV_QCOW2_COMPAT_LEVEL_0_10
> +                                  : BLOCKDEV_QCOW2_COMPAT_LEVEL_1_1,
> +            .has_backing_file   = (backing_file != NULL),
> +            .backing_file       = backing_file,
> +            .has_backing_fmt    = (backing_fmt != NULL),

I would have used .has_backing_fmt = !!backing_fmt; but your way works too.

Overall looks like a good refactoring.

Reviewed-by: Eric Blake <address@hidden>

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

signature.asc
Description: OpenPGP digital signature