Re: [Qemu-block] [PATCH 04/27] qcow2: Pass BlockdevCreateOptions to qcow2_create2()

[Eric Blake]

On 02/08/2018 01:23 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 | 190 ++++++++++++++++++++++++++++++++++++++++++++++------------
>   1 file changed, 152 insertions(+), 38 deletions(-)

> -static size_t qcow2_opt_get_cluster_size_del(QemuOpts *opts, Error **errp)
> +static bool validate_cluster_size(size_t cluster_size, Error **errp)
>   {
> -    size_t cluster_size;
> -    int cluster_bits;
> -
> -    cluster_size = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE,
> -                                         DEFAULT_CLUSTER_SIZE);
> -    cluster_bits = ctz32(cluster_size);
> +    int cluster_bits = ctz32(cluster_size);
>       if (cluster_bits < MIN_CLUSTER_BITS || cluster_bits > MAX_CLUSTER_BITS ||
>           (1 << cluster_bits) != cluster_size)

Pre-existing, but why are we manually calling ctz32() instead of using 
is_power_of_2()?

> @@ -2720,10 +2726,92 @@ static int qcow2_create2(BlockDriverState *bs, int64_t 
> total_size,
>        */
>       BlockBackend *blk;
>       QCowHeader *header;
> +    size_t cluster_size;
> +    int version;
> +    int refcount_order;
>       uint64_t* refcount_table;
>       Error *local_err = NULL;
>       int ret;
>   
> +    /* Validate options and set default values */
> +    assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2);
> +    qcow2_opts = &create_options->u.qcow2;
> +
> +    if (!QEMU_IS_ALIGNED(qcow2_opts->size, BDRV_SECTOR_SIZE)) {
> +        error_setg(errp, "Image size must be a multiple of 512 bytes");
> +        ret = -EINVAL;
> +        goto out;
> +    }

This check looks new.  Does it really belong in this patch?  And it does 
NOT match what qemu-img can currently do, nor the fact that qcow2 
supports byte-based addressing:

$ qemu-img create -f qcow2 tmp 12345
Formatting 'tmp', fmt=qcow2 size=12345 cluster_size=65536 
lazy_refcounts=off refcount_bits=16

> +    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)");

Do we want to reword this error message at all, now that QMP spells it 
'v3'?  Should qemu-img be taught to accept 'compat=v3' as a synonym to 
'compat=1.1'?

> +        return -EINVAL;
> +    }
> +
> +    if (!qcow2_opts->has_refcount_bits) {
> +        qcow2_opts->refcount_bits = 16;
> +    }
> +    if (qcow2_opts->refcount_bits > 64 ||
> +        !is_power_of_2(qcow2_opts->refcount_bits))
> +    {
> +        error_setg(errp, "Refcount width must be a power of two and may not "
> +                   "exceed 64 bits");
> +        return -EINVAL;
> +    }
> +    if (version < 3 && qcow2_opts->refcount_bits != 16) {
> +        error_setg(errp, "Different refcount widths than 16 bits require "
> +                   "compatibility level 1.1 or above (use compat=1.1 or "
> +                   "greater)");

and again

> @@ -2978,9 +3068,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,
> +        .u.qcow2        = {
> +            .file               = &(BlockdevRef) {
> +                .type               = QTYPE_QSTRING,
> +                .u.reference        = bs->node_name,
> +            },
> +            .size               = size,
> +            .has_version        = true,
> +            .version            = version == 2
> +                                  ? BLOCKDEV_QCOW2_VERSION_V2
> +                                  : BLOCKDEV_QCOW2_VERSION_V3,
> +            .has_backing_file   = (backing_file != NULL),
> +            .backing_file       = backing_file,
> +            .has_backing_fmt    = (backing_fmt != NULL),

I might have spelled it '= !!backing_fmt', but your way is fine too.

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