Re: [Qemu-devel] [PATCH 04/27] qcow2: Pass BlockdevCreateOptions to qcow2_create2()
On 2018-02-08 20:23, 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> --- > block/qcow2.c | 190 > ++ > 1 file changed, 152 insertions(+), 38 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 04/27] qcow2: Pass BlockdevCreateOptions to qcow2_create2()
Am 09.02.2018 um 00:29 hat Eric Blake geschrieben: > 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> > --- > > 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()? Probably because is_power_of_2() is newer than this code. Also, if we don't call ctz32(), we'd have to use (1 << MIN_CLUSTER_BITS) and (1 << MAX_CLUSTER_BITS), so I'm not sure if that would be better. > > @@ -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 = _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 You're ignoring the result of this command: $ ./qemu-img create -f qcow2 /tmp/test.qcow2 12345 Formatting '/tmp/test.qcow2', fmt=qcow2 size=12345 cluster_size=65536 lazy_refcounts=off refcount_bits=16 $ ./qemu-img info -f qcow2 /tmp/test.qcow2 image: /tmp/test.qcow2 file format: qcow2 virtual size: 13K (12800 bytes) disk size: 196K cluster_size: 65536 Format specific information: compat: 1.1 lazy refcounts: false refcount bits: 16 corrupt: false As you can see, the size got silently rounded up to the next 512 bytes. qcow2_create() still does the same even after this patch, but I chose not to extend the magic rounding to the QMP command. In the protocol drivers, I generally just allow byte granularity image sizes, but qcow2 does use sectors internally, so it felt safer to require 512 byte alignment in qcow2. > > +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'? Actually, it does accept 'v3' when the whole series is applied, and the message is changed in a later patch that enables this. Kevin
Re: [Qemu-devel] [PATCH 04/27] qcow2: Pass BlockdevCreateOptions to qcow2_create2()
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--- 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 = _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
[Qemu-devel] [PATCH 04/27] qcow2: Pass BlockdevCreateOptions to qcow2_create2()
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--- block/qcow2.c | 190 ++ 1 file changed, 152 insertions(+), 38 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index 6134c0d40c..4ab6ed15c2 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2638,19 +2638,26 @@ static int64_t qcow2_calc_prealloc_size(int64_t total_size, return meta_size + aligned_total_size; } -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) { error_setg(errp, "Cluster size must be a power of two between %d and " "%dk", 1 << MIN_CLUSTER_BITS, 1 << (MAX_CLUSTER_BITS - 10)); +return false; +} +return true; +} + +static size_t qcow2_opt_get_cluster_size_del(QemuOpts *opts, Error **errp) +{ +size_t cluster_size; + +cluster_size = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE, + DEFAULT_CLUSTER_SIZE); +if (!validate_cluster_size(cluster_size, errp)) { return 0; } return cluster_size; @@ -2698,12 +2705,11 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version, return refcount_bits; } -static int qcow2_create2(BlockDriverState *bs, int64_t total_size, - const char *backing_file, const char *backing_format, - int flags, size_t cluster_size, PreallocMode prealloc, - QemuOpts *opts, int version, int refcount_order, - const char *encryptfmt, Error **errp) +static int qcow2_create2(BlockDriverState *bs, + BlockdevCreateOptions *create_options, + QemuOpts *opts, const char *encryptfmt, Error **errp) { +BlockdevCreateOptionsQcow2 *qcow2_opts; QDict *options; /* @@ -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 = _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; +} + +if (qcow2_opts->has_version) { +switch (qcow2_opts->version) { +case BLOCKDEV_QCOW2_VERSION_V2: +version = 2; +break; +case BLOCKDEV_QCOW2_VERSION_V3: +version = 3; +break; +default: +g_assert_not_reached(); +} +} else { +version = 3; +} + +if (qcow2_opts->has_cluster_size) { +cluster_size = qcow2_opts->cluster_size; +} else { +cluster_size = DEFAULT_CLUSTER_SIZE; +} + +if (!validate_cluster_size(cluster_size, errp)) { +return -EINVAL; +} + +if (!qcow2_opts->has_preallocation) { +qcow2_opts->preallocation = PREALLOC_MODE_OFF; +} +if (qcow2_opts->has_backing_file && +qcow2_opts->preallocation != PREALLOC_MODE_OFF) +{ +error_setg(errp, "Backing file and preallocation cannot be used at " + "the same time"); +return -EINVAL; +} +if (qcow2_opts->has_backing_fmt && !qcow2_opts->has_backing_file) { +error_setg(errp, "Backing format cannot be used without backing file"); +return -EINVAL; +} + +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)"); +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 &&