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

[Max Reitz]

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()

[Kevin Wolf]

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()

[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 
---
  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()

[Kevin Wolf]

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 &&