[Qemu-devel] [RFC PATCH 00/10] x-blockdev-create for qcow2

[Qemu-devel] [RFC PATCH 00/10] x-blockdev-create for qcow2

[Kevin Wolf]

This series implements a minimal QMP command that allows to create an
image format on a given block node. The interface is still going to
change to some kind of an async command (possibly a block job), so I
prefixed x- for now.

At this point, I'm mostly interested in comments about
BlockdevCreateOptions in the schema, the .bdrv_co_create callback and
the way that legacy .bdrv_create is implemented in qcow2 now.

It looks to me as if we will have to keep .bdrv_create in addition to
the new .bdrv_co_create for a while in all drivers, where the
implementation of .bdrv_create would call .bdrv_co_create like this
series does it for qcow2. We'll only be able to drop the old interface
after deprecating and eventually removing all of the driver-specific
compatibility work that remains. The example of qcow2 shows that the
"translation" from old to new is managable, but there are a few
differences.

Kevin Wolf (10):
  block/qapi: Introduce BlockdevCreateOptions
  block/qapi: Add qcow2 create options to schema
  qcow2: Let qcow2_create() handle protocol layer
  qcow2: Pass BlockdevCreateOptions to qcow2_create2()
  qcow2: Use BlockdevRef in qcow2_create2()
  qcow2: Use QCryptoBlockCreateOptions in qcow2_create2()
  qcow2: Handle full/falloc preallocation in qcow2_create2()
  util: Add qemu_opts_to_qdict_filtered()
  qcow2: Use visitor for options in qcow2_create()
  block: x-blockdev-create QMP command

 qapi/block-core.json       | 107 ++++++++++++
 include/block/block.h      |   1 +
 include/block/block_int.h  |   2 +
 include/qemu/option.h      |   2 +
 block.c                    |  87 ++++++++++
 block/qcow2.c              | 395 +++++++++++++++++++++++++++++----------------
 util/qemu-option.c         |  28 +++-
 tests/qemu-iotests/049.out |  10 +-
 8 files changed, 486 insertions(+), 146 deletions(-)

-- 
2.13.6

[Qemu-devel] [RFC PATCH 01/10] block/qapi: Introduce BlockdevCreateOptions

[Kevin Wolf]

This creates a BlockdevCreateOptions union type that will contain all of
the options for image creation. We'll start out with an empty struct
type BlockdevCreateDummy for all drivers.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index e94a6881b2..1749376c61 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3320,6 +3320,70 @@
 { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
 
 ##
+# @BlockdevCreateDummy:
+#
+# FIXME To be removed. Only there to make the QAPI generator happy while we're
+# adding driver by driver. Leaving out union branches is not allowed.
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateDummy', 'data': {}}
+
+##
+# @BlockdevCreateOptions:
+#
+# Options for creating an image format on a given node.
+#
+# @driver           block driver to create the image format
+# @node             node to create the image format on
+#
+# Since: 2.12
+##
+{ 'union': 'BlockdevCreateOptions',
+  'base': {
+      'driver':         'BlockdevDriver',
+      'node':           'BlockdevRef' },
+  'discriminator': 'driver',
+  'data': {
+      'blkdebug':       'BlockdevCreateDummy',
+      'blkverify':      'BlockdevCreateDummy',
+      'bochs':          'BlockdevCreateDummy',
+      'cloop':          'BlockdevCreateDummy',
+      'dmg':            'BlockdevCreateDummy',
+      'file':           'BlockdevCreateDummy',
+      'ftp':            'BlockdevCreateDummy',
+      'ftps':           'BlockdevCreateDummy',
+      'gluster':        'BlockdevCreateDummy',
+      'host_cdrom':     'BlockdevCreateDummy',
+      'host_device':    'BlockdevCreateDummy',
+      'http':           'BlockdevCreateDummy',
+      'https':          'BlockdevCreateDummy',
+      'iscsi':          'BlockdevCreateDummy',
+      'luks':           'BlockdevCreateDummy',
+      'nbd':            'BlockdevCreateDummy',
+      'nfs':            'BlockdevCreateDummy',
+      'null-aio':       'BlockdevCreateDummy',
+      'null-co':        'BlockdevCreateDummy',
+      'parallels':      'BlockdevCreateDummy',
+      'qcow2':          'BlockdevCreateDummy',
+      'qcow':           'BlockdevCreateDummy',
+      'qed':            'BlockdevCreateDummy',
+      'quorum':         'BlockdevCreateDummy',
+      'raw':            'BlockdevCreateDummy',
+      'rbd':            'BlockdevCreateDummy',
+      'replication':    'BlockdevCreateDummy',
+      'sheepdog':       'BlockdevCreateDummy',
+      'ssh':            'BlockdevCreateDummy',
+      'throttle':       'BlockdevCreateDummy',
+      'vdi':            'BlockdevCreateDummy',
+      'vhdx':           'BlockdevCreateDummy',
+      'vmdk':           'BlockdevCreateDummy',
+      'vpc':            'BlockdevCreateDummy',
+      'vvfat':          'BlockdevCreateDummy',
+      'vxhs':           'BlockdevCreateDummy'
+  } }
+
+##
 # @blockdev-open-tray:
 #
 # Opens a block device's tray. If there is a block driver state tree inserted as
-- 
2.13.6

Re: [Qemu-devel] [RFC PATCH 01/10] block/qapi: Introduce BlockdevCreateOptions

[Eric Blake]

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

On 01/11/2018 01:52 PM, Kevin Wolf wrote:
> This creates a BlockdevCreateOptions union type that will contain all of
> the options for image creation. We'll start out with an empty struct
> type BlockdevCreateDummy for all drivers.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/block-core.json | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e94a6881b2..1749376c61 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3320,6 +3320,70 @@
>  { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
>  
>  ##
> +# @BlockdevCreateDummy:
> +#
> +# FIXME To be removed. Only there to make the QAPI generator happy while we're
> +# adding driver by driver. Leaving out union branches is not allowed.
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockdevCreateDummy', 'data': {}}

At one point, I had a patch that let you do:

'data': { 'branch': {},
          ... }

for the branches that didn't need to add any additional types to the
flat union.  Hmm, looks like it is still sitting in my tree, unapplied;
would it help if I revived that one?

> +
> +##
> +# @BlockdevCreateOptions:
> +#
> +# Options for creating an image format on a given node.
> +#
> +# @driver           block driver to create the image format
> +# @node             node to create the image format on

Any restrictions we want to document about the node (for example, it
must not be in use by any backend device at the moment, particularly
since creation may change the node's format)?

> +#
> +# Since: 2.12
> +##
> +{ 'union': 'BlockdevCreateOptions',
> +  'base': {
> +      'driver':         'BlockdevDriver',
> +      'node':           'BlockdevRef' },
> +  'discriminator': 'driver',
> +  'data': {
> +      'blkdebug':       'BlockdevCreateDummy',

The QAPI itself looks sane for the future patches.

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

Re: [Qemu-devel] [RFC PATCH 01/10] block/qapi: Introduce BlockdevCreateOptions

[Kevin Wolf]

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

Am 16.01.2018 um 19:54 hat Eric Blake geschrieben:
> On 01/11/2018 01:52 PM, Kevin Wolf wrote:
> > This creates a BlockdevCreateOptions union type that will contain all of
> > the options for image creation. We'll start out with an empty struct
> > type BlockdevCreateDummy for all drivers.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/block-core.json | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index e94a6881b2..1749376c61 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3320,6 +3320,70 @@
> >  { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
> >  
> >  ##
> > +# @BlockdevCreateDummy:
> > +#
> > +# FIXME To be removed. Only there to make the QAPI generator happy while we're
> > +# adding driver by driver. Leaving out union branches is not allowed.
> > +#
> > +# Since: 2.12
> > +##
> > +{ 'struct': 'BlockdevCreateDummy', 'data': {}}
> 
> At one point, I had a patch that let you do:
> 
> 'data': { 'branch': {},
>           ... }
> 
> for the branches that didn't need to add any additional types to the
> flat union.  Hmm, looks like it is still sitting in my tree, unapplied;
> would it help if I revived that one?

I actually realised that this type won't go away because we have drivers
that don't support image creation. I'm not sure if I should prefer the
conciseness of this syntax or the documentation that comes with a full
type in this specific case. Though generally, I think allowing inline
definitions would be nice.

The infrastructure that I was actually looking for here was a union type
that doesn't accept all enum values, but only some. Maybe some kind of
sub-enum for the discriminator, so that I still get the usual
BLOCKDEV_DRIVER_* constants, but allow only some of them in the context
of the union.

> > +
> > +##
> > +# @BlockdevCreateOptions:
> > +#
> > +# Options for creating an image format on a given node.
> > +#
> > +# @driver           block driver to create the image format
> > +# @node             node to create the image format on
> 
> Any restrictions we want to document about the node (for example, it
> must not be in use by any backend device at the moment, particularly
> since creation may change the node's format)?

Hm... As long as we can get BLK_PERM_WRITE, we should be good, right?

By the way, I've moved this to the driver-specific options since because
protocol drivers don't need it.

> > +#
> > +# Since: 2.12
> > +##
> > +{ 'union': 'BlockdevCreateOptions',
> > +  'base': {
> > +      'driver':         'BlockdevDriver',
> > +      'node':           'BlockdevRef' },
> > +  'discriminator': 'driver',
> > +  'data': {
> > +      'blkdebug':       'BlockdevCreateDummy',
> 
> The QAPI itself looks sane for the future patches.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1749376c61..9341f6708d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3320,6 +3320,37 @@
 { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
 
 ##
+# @BlockdevQcow2CompatLevel:
+# @0_10:    The original QCOW2 format as introduced in qemu 0.10 (version 2)
+# @1_1:     The extended QCOW2 format as introduced in qemu 1.1 (version 3)
+#
+# Since: 2.10
+##
+{ 'enum': 'BlockdevQcow2CompatLevel',
+  'data': [ '0_10', '1_1' ] }
+
+
+##
+# @BlockdevCreateOptionsQcow2:
+#
+# Driver specific image creation options for qcow2.
+#
+# TODO Describe fields
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsQcow2',
+  'data': { 'size':             'size',
+            '*compat':          'BlockdevQcow2CompatLevel',
+            '*backing-file':    'str',
+            '*backing-fmt':     'BlockdevDriver',
+            '*encrypt':         'QCryptoBlockCreateOptions',
+            '*cluster-size':    'size',
+            '*preallocation':   'PreallocMode',
+            '*lazy-refcounts':  'bool',
+            '*refcount-bits':   'int' } }
+
+##
 # @BlockdevCreateDummy:
 #
 # FIXME To be removed. Only there to make the QAPI generator happy while we're
@@ -3365,7 +3396,7 @@
       'null-aio':       'BlockdevCreateDummy',
       'null-co':        'BlockdevCreateDummy',
       'parallels':      'BlockdevCreateDummy',
-      'qcow2':          'BlockdevCreateDummy',
+      'qcow2':          'BlockdevCreateOptionsQcow2',
       'qcow':           'BlockdevCreateDummy',
       'qed':            'BlockdevCreateDummy',
       'quorum':         'BlockdevCreateDummy',
-- 
2.13.6

Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema

[Daniel P. Berrange]

On Thu, Jan 11, 2018 at 08:52:17PM +0100, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 1749376c61..9341f6708d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3320,6 +3320,37 @@
>  { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
>  
>  ##
> +# @BlockdevQcow2CompatLevel:
> +# @0_10:    The original QCOW2 format as introduced in qemu 0.10 (version 2)
> +# @1_1:     The extended QCOW2 format as introduced in qemu 1.1 (version 3)
> +#
> +# Since: 2.10
> +##
> +{ 'enum': 'BlockdevQcow2CompatLevel',
> +  'data': [ '0_10', '1_1' ] }
> +
> +
> +##
> +# @BlockdevCreateOptionsQcow2:
> +#
> +# Driver specific image creation options for qcow2.
> +#
> +# TODO Describe fields
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockdevCreateOptionsQcow2',
> +  'data': { 'size':             'size',
> +            '*compat':          'BlockdevQcow2CompatLevel',
> +            '*backing-file':    'str',
> +            '*backing-fmt':     'BlockdevDriver',

For anything non-trivial, the caller is going to have to stuff a
JSON string into 'backing-file' value. It feels like we should
be referencing 'BlockdevOptions' here in some manner.

> +            '*encrypt':         'QCryptoBlockCreateOptions',
> +            '*cluster-size':    'size',
> +            '*preallocation':   'PreallocMode',
> +            '*lazy-refcounts':  'bool',
> +            '*refcount-bits':   'int' } }

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema

[Kevin Wolf]

Am 12.01.2018 um 11:53 hat Daniel P. Berrange geschrieben:
> On Thu, Jan 11, 2018 at 08:52:17PM +0100, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 1749376c61..9341f6708d 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3320,6 +3320,37 @@
> >  { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
> >  
> >  ##
> > +# @BlockdevQcow2CompatLevel:
> > +# @0_10:    The original QCOW2 format as introduced in qemu 0.10 (version 2)
> > +# @1_1:     The extended QCOW2 format as introduced in qemu 1.1 (version 3)
> > +#
> > +# Since: 2.10
> > +##
> > +{ 'enum': 'BlockdevQcow2CompatLevel',
> > +  'data': [ '0_10', '1_1' ] }
> > +
> > +
> > +##
> > +# @BlockdevCreateOptionsQcow2:
> > +#
> > +# Driver specific image creation options for qcow2.
> > +#
> > +# TODO Describe fields
> > +#
> > +# Since: 2.12
> > +##
> > +{ 'struct': 'BlockdevCreateOptionsQcow2',
> > +  'data': { 'size':             'size',
> > +            '*compat':          'BlockdevQcow2CompatLevel',
> > +            '*backing-file':    'str',
> > +            '*backing-fmt':     'BlockdevDriver',
> 
> For anything non-trivial, the caller is going to have to stuff a
> JSON string into 'backing-file' value. It feels like we should
> be referencing 'BlockdevOptions' here in some manner.

Hm, that's an interesting question. For the image creation, this is
really treated as a string that is directly written into the image file,
without being parsed, so 'str' is the more correct type in this context.
However, when the backing file gets loaded, that string is in fact
parsed and we expect it to describe the same thing as BlockdevOptions.

If we get BlockdevOptions here, qemu would have to convert them into a
json:{...} string before writing the header of the new image.
Compatibility code would become a bit more complex because we'd have to
convert the existing string into BlockdevOptions, only to convert it
back to a string before we write it to the image file. And finally, the
1023 character limit of qcow2 becomes kind of unpredicatble when you
don't pass the string yourself.

So considering all of that, I still think that 'str' is the better
option here.

Kevin

Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema

[Daniel P. Berrange]

On Mon, Jan 15, 2018 at 02:38:48PM +0100, Kevin Wolf wrote:
> Am 12.01.2018 um 11:53 hat Daniel P. Berrange geschrieben:
> > On Thu, Jan 11, 2018 at 08:52:17PM +0100, Kevin Wolf wrote:
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++-
> > >  1 file changed, 32 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > index 1749376c61..9341f6708d 100644
> > > --- a/qapi/block-core.json
> > > +++ b/qapi/block-core.json
> > > @@ -3320,6 +3320,37 @@
> > >  { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
> > >  
> > >  ##
> > > +# @BlockdevQcow2CompatLevel:
> > > +# @0_10:    The original QCOW2 format as introduced in qemu 0.10 (version 2)
> > > +# @1_1:     The extended QCOW2 format as introduced in qemu 1.1 (version 3)
> > > +#
> > > +# Since: 2.10
> > > +##
> > > +{ 'enum': 'BlockdevQcow2CompatLevel',
> > > +  'data': [ '0_10', '1_1' ] }
> > > +
> > > +
> > > +##
> > > +# @BlockdevCreateOptionsQcow2:
> > > +#
> > > +# Driver specific image creation options for qcow2.
> > > +#
> > > +# TODO Describe fields
> > > +#
> > > +# Since: 2.12
> > > +##
> > > +{ 'struct': 'BlockdevCreateOptionsQcow2',
> > > +  'data': { 'size':             'size',
> > > +            '*compat':          'BlockdevQcow2CompatLevel',
> > > +            '*backing-file':    'str',
> > > +            '*backing-fmt':     'BlockdevDriver',
> > 
> > For anything non-trivial, the caller is going to have to stuff a
> > JSON string into 'backing-file' value. It feels like we should
> > be referencing 'BlockdevOptions' here in some manner.
> 
> Hm, that's an interesting question. For the image creation, this is
> really treated as a string that is directly written into the image file,
> without being parsed, so 'str' is the more correct type in this context.
> However, when the backing file gets loaded, that string is in fact
> parsed and we expect it to describe the same thing as BlockdevOptions.
> 
> If we get BlockdevOptions here, qemu would have to convert them into a
> json:{...} string before writing the header of the new image.
> Compatibility code would become a bit more complex because we'd have to
> convert the existing string into BlockdevOptions, only to convert it
> back to a string before we write it to the image file. And finally, the
> 1023 character limit of qcow2 becomes kind of unpredicatble when you
> don't pass the string yourself.
> 
> So considering all of that, I still think that 'str' is the better
> option here.

Hmm, when we write the backing chain into the qcow2 header, we only want to
write the 1st level of the backing chain.

When we are creating the new qcow2 image, we could be pointing to a backing
chain that goes many levels deep. So the actual creation process potentially
needs to be given the full arbitrarily deep backing file eg in order that
we can set 'encrypt.secret' for any encrypted images at at arbitrary level.

IOW, I think we need to be able to pass BlockdevOptions here to specify the
full deep chain, but then only the 1st level of these BlockdevOptions should
get written into the qcow2 file header.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema

[Kevin Wolf]

Am 15.01.2018 um 14:51 hat Daniel P. Berrange geschrieben:
> On Mon, Jan 15, 2018 at 02:38:48PM +0100, Kevin Wolf wrote:
> > Am 12.01.2018 um 11:53 hat Daniel P. Berrange geschrieben:
> > > On Thu, Jan 11, 2018 at 08:52:17PM +0100, Kevin Wolf wrote:
> > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > > ---
> > > >  qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 32 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > index 1749376c61..9341f6708d 100644
> > > > --- a/qapi/block-core.json
> > > > +++ b/qapi/block-core.json
> > > > @@ -3320,6 +3320,37 @@
> > > >  { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
> > > >  
> > > >  ##
> > > > +# @BlockdevQcow2CompatLevel:
> > > > +# @0_10:    The original QCOW2 format as introduced in qemu 0.10 (version 2)
> > > > +# @1_1:     The extended QCOW2 format as introduced in qemu 1.1 (version 3)
> > > > +#
> > > > +# Since: 2.10
> > > > +##
> > > > +{ 'enum': 'BlockdevQcow2CompatLevel',
> > > > +  'data': [ '0_10', '1_1' ] }
> > > > +
> > > > +
> > > > +##
> > > > +# @BlockdevCreateOptionsQcow2:
> > > > +#
> > > > +# Driver specific image creation options for qcow2.
> > > > +#
> > > > +# TODO Describe fields
> > > > +#
> > > > +# Since: 2.12
> > > > +##
> > > > +{ 'struct': 'BlockdevCreateOptionsQcow2',
> > > > +  'data': { 'size':             'size',
> > > > +            '*compat':          'BlockdevQcow2CompatLevel',
> > > > +            '*backing-file':    'str',
> > > > +            '*backing-fmt':     'BlockdevDriver',
> > > 
> > > For anything non-trivial, the caller is going to have to stuff a
> > > JSON string into 'backing-file' value. It feels like we should
> > > be referencing 'BlockdevOptions' here in some manner.
> > 
> > Hm, that's an interesting question. For the image creation, this is
> > really treated as a string that is directly written into the image file,
> > without being parsed, so 'str' is the more correct type in this context.
> > However, when the backing file gets loaded, that string is in fact
> > parsed and we expect it to describe the same thing as BlockdevOptions.
> > 
> > If we get BlockdevOptions here, qemu would have to convert them into a
> > json:{...} string before writing the header of the new image.
> > Compatibility code would become a bit more complex because we'd have to
> > convert the existing string into BlockdevOptions, only to convert it
> > back to a string before we write it to the image file. And finally, the
> > 1023 character limit of qcow2 becomes kind of unpredicatble when you
> > don't pass the string yourself.
> > 
> > So considering all of that, I still think that 'str' is the better
> > option here.
> 
> Hmm, when we write the backing chain into the qcow2 header, we only
> want to write the 1st level of the backing chain.

That's a good point, too. References in BlockdevOptions are often
mandatory, which conflicts with this.

> When we are creating the new qcow2 image, we could be pointing to a backing
> chain that goes many levels deep. So the actual creation process potentially
> needs to be given the full arbitrarily deep backing file eg in order that
> we can set 'encrypt.secret' for any encrypted images at at arbitrary level.

But we don't even access the images in the backing chain during image
creation. Why would we need a secret for them?

> IOW, I think we need to be able to pass BlockdevOptions here to specify the
> full deep chain, but then only the 1st level of these BlockdevOptions should
> get written into the qcow2 file header.

But what's the point of even passing the full chain if only the first
layer is actually used?

Kevin

Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema

[Daniel P. Berrange]

On Mon, Jan 15, 2018 at 03:07:15PM +0100, Kevin Wolf wrote:
> Am 15.01.2018 um 14:51 hat Daniel P. Berrange geschrieben:
> > On Mon, Jan 15, 2018 at 02:38:48PM +0100, Kevin Wolf wrote:
> > > Am 12.01.2018 um 11:53 hat Daniel P. Berrange geschrieben:
> > > > On Thu, Jan 11, 2018 at 08:52:17PM +0100, Kevin Wolf wrote:
> > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > > > ---
> > > > >  qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++-
> > > > >  1 file changed, 32 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > > index 1749376c61..9341f6708d 100644
> > > > > --- a/qapi/block-core.json
> > > > > +++ b/qapi/block-core.json
> > > > > @@ -3320,6 +3320,37 @@
> > > > >  { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
> > > > >  
> > > > >  ##
> > > > > +# @BlockdevQcow2CompatLevel:
> > > > > +# @0_10:    The original QCOW2 format as introduced in qemu 0.10 (version 2)
> > > > > +# @1_1:     The extended QCOW2 format as introduced in qemu 1.1 (version 3)
> > > > > +#
> > > > > +# Since: 2.10
> > > > > +##
> > > > > +{ 'enum': 'BlockdevQcow2CompatLevel',
> > > > > +  'data': [ '0_10', '1_1' ] }
> > > > > +
> > > > > +
> > > > > +##
> > > > > +# @BlockdevCreateOptionsQcow2:
> > > > > +#
> > > > > +# Driver specific image creation options for qcow2.
> > > > > +#
> > > > > +# TODO Describe fields
> > > > > +#
> > > > > +# Since: 2.12
> > > > > +##
> > > > > +{ 'struct': 'BlockdevCreateOptionsQcow2',
> > > > > +  'data': { 'size':             'size',
> > > > > +            '*compat':          'BlockdevQcow2CompatLevel',
> > > > > +            '*backing-file':    'str',
> > > > > +            '*backing-fmt':     'BlockdevDriver',
> > > > 
> > > > For anything non-trivial, the caller is going to have to stuff a
> > > > JSON string into 'backing-file' value. It feels like we should
> > > > be referencing 'BlockdevOptions' here in some manner.
> > > 
> > > Hm, that's an interesting question. For the image creation, this is
> > > really treated as a string that is directly written into the image file,
> > > without being parsed, so 'str' is the more correct type in this context.
> > > However, when the backing file gets loaded, that string is in fact
> > > parsed and we expect it to describe the same thing as BlockdevOptions.
> > > 
> > > If we get BlockdevOptions here, qemu would have to convert them into a
> > > json:{...} string before writing the header of the new image.
> > > Compatibility code would become a bit more complex because we'd have to
> > > convert the existing string into BlockdevOptions, only to convert it
> > > back to a string before we write it to the image file. And finally, the
> > > 1023 character limit of qcow2 becomes kind of unpredicatble when you
> > > don't pass the string yourself.
> > > 
> > > So considering all of that, I still think that 'str' is the better
> > > option here.
> > 
> > Hmm, when we write the backing chain into the qcow2 header, we only
> > want to write the 1st level of the backing chain.
> 
> That's a good point, too. References in BlockdevOptions are often
> mandatory, which conflicts with this.
> 
> > When we are creating the new qcow2 image, we could be pointing to a backing
> > chain that goes many levels deep. So the actual creation process potentially
> > needs to be given the full arbitrarily deep backing file eg in order that
> > we can set 'encrypt.secret' for any encrypted images at at arbitrary level.
> 
> But we don't even access the images in the backing chain during image
> creation. Why would we need a secret for them?

Oh, i forgot that when qcow2 opens the just created image, it uses the
O_NO_IO and O_NO_BACKING flags. So yeah, we're probably ok in actual
fact.

> > IOW, I think we need to be able to pass BlockdevOptions here to specify the
> > full deep chain, but then only the 1st level of these BlockdevOptions should
> > get written into the qcow2 file header.
> 
> But what's the point of even passing the full chain if only the first
> layer is actually used?


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema

[Eric Blake]

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

On 01/11/2018 01:52 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 1749376c61..9341f6708d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3320,6 +3320,37 @@
>  { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
>  
>  ##
> +# @BlockdevQcow2CompatLevel:
> +# @0_10:    The original QCOW2 format as introduced in qemu 0.10 (version 2)
> +# @1_1:     The extended QCOW2 format as introduced in qemu 1.1 (version 3)
> +#
> +# Since: 2.10
> +##
> +{ 'enum': 'BlockdevQcow2CompatLevel',
> +  'data': [ '0_10', '1_1' ] }

Enums are allowed to start with digits while struct members are not; so
you can get away with this naming.  Do we really want the names 0_10 and
1_1, or are there better names we could come up with (it already
undergoes translation such that qemu-img reports 0.10 rather than 0_10).

> +
> +
> +##
> +# @BlockdevCreateOptionsQcow2:
> +#
> +# Driver specific image creation options for qcow2.
> +#
> +# TODO Describe fields

Hence this being RFC :)

> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockdevCreateOptionsQcow2',
> +  'data': { 'size':             'size',

Is size mandatory even when we have a backing file specification?  It is
not mandatory for qemu-img create; but on the other hand, I think I can
live with requiring the QMP caller to supply a size.

> +            '*compat':          'BlockdevQcow2CompatLevel',
> +            '*backing-file':    'str',

Given Dan's comments, perhaps name this one 'backing-str' to make it
obvious that it is the string written into the qcow2 header, rather than
the node we open as backing?  Or, maybe we support an optional
'*backing-node' that can be used for allowing a default size and backing
string if not explicitly overridden?

> +            '*backing-fmt':     'BlockdevDriver',
> +            '*encrypt':         'QCryptoBlockCreateOptions',
> +            '*cluster-size':    'size',
> +            '*preallocation':   'PreallocMode',
> +            '*lazy-refcounts':  'bool',
> +            '*refcount-bits':   'int' } }
> +
> +##
>  # @BlockdevCreateDummy:
>  #
>  # FIXME To be removed. Only there to make the QAPI generator happy while we're
> @@ -3365,7 +3396,7 @@
>        'null-aio':       'BlockdevCreateDummy',
>        'null-co':        'BlockdevCreateDummy',
>        'parallels':      'BlockdevCreateDummy',
> -      'qcow2':          'BlockdevCreateDummy',
> +      'qcow2':          'BlockdevCreateOptionsQcow2',
>        'qcow':           'BlockdevCreateDummy',
>        'qed':            'BlockdevCreateDummy',
>        'quorum':         'BlockdevCreateDummy',
> 

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

Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema

[Kevin Wolf]

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

Am 16.01.2018 um 19:59 hat Eric Blake geschrieben:
> On 01/11/2018 01:52 PM, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 1749376c61..9341f6708d 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3320,6 +3320,37 @@
> >  { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
> >  
> >  ##
> > +# @BlockdevQcow2CompatLevel:
> > +# @0_10:    The original QCOW2 format as introduced in qemu 0.10 (version 2)
> > +# @1_1:     The extended QCOW2 format as introduced in qemu 1.1 (version 3)
> > +#
> > +# Since: 2.10
> > +##
> > +{ 'enum': 'BlockdevQcow2CompatLevel',
> > +  'data': [ '0_10', '1_1' ] }
> 
> Enums are allowed to start with digits while struct members are not; so
> you can get away with this naming.  Do we really want the names 0_10 and
> 1_1, or are there better names we could come up with (it already
> undergoes translation such that qemu-img reports 0.10 rather than 0_10).

Yeah, I don't like 0_10/1_1 much.

Either we allow dots in enum values so that we can keep 0.10/1.1, or
something completely different. I was considering 'version': 'int' with
2 and 3 as possible values, after all QMP is already rather low-level.

The question is just what to do with the command line. Will we deprecate
compat=0.10/1.1 there, too, and tell users to switch to whatever new
syntax we invent for QMP? Or are we planning to keep the "translation"
from the old syntax forever?

query-block cheated and just exposed it as a string.

> > +
> > +
> > +##
> > +# @BlockdevCreateOptionsQcow2:
> > +#
> > +# Driver specific image creation options for qcow2.
> > +#
> > +# TODO Describe fields
> 
> Hence this being RFC :)
> 
> > +#
> > +# Since: 2.12
> > +##
> > +{ 'struct': 'BlockdevCreateOptionsQcow2',
> > +  'data': { 'size':             'size',
> 
> Is size mandatory even when we have a backing file specification?  It is
> not mandatory for qemu-img create; but on the other hand, I think I can
> live with requiring the QMP caller to supply a size.

The qemu-img create implementation of this is common code at least, but
we're in driver-specific definitions here, so every driver would have to
call some function to guess the size given a backing file string. With
the straightforward implementation of this series, it is really
mandatory because otherwise you'd get zero-sized images.

Accessing the backing file during image creation is also one of those
things that tend to cause surprises, so if we don't have to, I wouldn't
do that.

> > +            '*compat':          'BlockdevQcow2CompatLevel',
> > +            '*backing-file':    'str',
> 
> Given Dan's comments, perhaps name this one 'backing-str' to make it
> obvious that it is the string written into the qcow2 header, rather than
> the node we open as backing?

If you guys think that this is clearer, I can change it.

> Or, maybe we support an optional '*backing-node' that can be used for
> allowing a default size and backing string if not explicitly
> overridden?

Hm, it would make the interface a bit more complex. I'd try whether we
can do without it.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema

[Eric Blake]

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

On 01/16/2018 02:11 PM, Kevin Wolf wrote:

>>> +{ 'enum': 'BlockdevQcow2CompatLevel',
>>> +  'data': [ '0_10', '1_1' ] }
>>
>> Enums are allowed to start with digits while struct members are not; so
>> you can get away with this naming.  Do we really want the names 0_10 and
>> 1_1, or are there better names we could come up with (it already
>> undergoes translation such that qemu-img reports 0.10 rather than 0_10).
> 
> Yeah, I don't like 0_10/1_1 much.
> 
> Either we allow dots in enum values so that we can keep 0.10/1.1, or
> something completely different. I was considering 'version': 'int' with
> 2 and 3 as possible values, after all QMP is already rather low-level.
> 

I can live with a lower-level 'version':'int' for qcow2 creation over QMP.

> The question is just what to do with the command line. Will we deprecate
> compat=0.10/1.1 there, too, and tell users to switch to whatever new
> syntax we invent for QMP? Or are we planning to keep the "translation"
> from the old syntax forever?

At a minimum, we'll have to keep the translation syntax for as long as a
deprecation cycle with proper documentation is available (at least two
releases); keeping it longer than that depends on whether we think the
deprecation is worth the cleaner code in the long run.  But we do have a
deprecation policy, so we can start thinking about using that now so
that in another year we can do a release that gets rid of the
back-compat code.


>>> +  'data': { 'size':             'size',
>>
>> Is size mandatory even when we have a backing file specification?  It is
>> not mandatory for qemu-img create; but on the other hand, I think I can
>> live with requiring the QMP caller to supply a size.
> 
> The qemu-img create implementation of this is common code at least, but
> we're in driver-specific definitions here, so every driver would have to
> call some function to guess the size given a backing file string. With
> the straightforward implementation of this series, it is really
> mandatory because otherwise you'd get zero-sized images.
> 
> Accessing the backing file during image creation is also one of those
> things that tend to cause surprises, so if we don't have to, I wouldn't
> do that.

Good point.  So mandatory size at the QMP layer makes sense (qemu-img
can still open multiple images to determine what size to pass to QMP
under the hood).

> 
>>> +            '*compat':          'BlockdevQcow2CompatLevel',
>>> +            '*backing-file':    'str',
>>
>> Given Dan's comments, perhaps name this one 'backing-str' to make it
>> obvious that it is the string written into the qcow2 header, rather than
>> the node we open as backing?
> 
> If you guys think that this is clearer, I can change it.

Especially since you're convincing me that we DON'T want to open a
backing node during this operation, I think backing-str is a bit clearer
(of course, that's another place for command-line back-compat glue that
we may want to deprecate over time).

> 
>> Or, maybe we support an optional '*backing-node' that can be used for
>> allowing a default size and backing string if not explicitly
>> overridden?
> 
> Hm, it would make the interface a bit more complex. I'd try whether we
> can do without it.

I'm fine if you can manage the series without having a backing-node
argument.

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

Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema

[Max Reitz]

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

On 2018-01-11 20:52, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 1749376c61..9341f6708d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3320,6 +3320,37 @@
>  { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
>  
>  ##
> +# @BlockdevQcow2CompatLevel:
> +# @0_10:    The original QCOW2 format as introduced in qemu 0.10 (version 2)
> +# @1_1:     The extended QCOW2 format as introduced in qemu 1.1 (version 3)
> +#
> +# Since: 2.10
> +##
> +{ 'enum': 'BlockdevQcow2CompatLevel',
> +  'data': [ '0_10', '1_1' ] }

Just my two cents: I'd prefer 2 and 3 because I've never quite liked
that people are supposed to remember some pretty random qemu version
numbers anyway.

Max


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

Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema

[Kevin Wolf]

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

Am 29.01.2018 um 17:57 hat Max Reitz geschrieben:
> On 2018-01-11 20:52, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 1749376c61..9341f6708d 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3320,6 +3320,37 @@
> >  { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
> >  
> >  ##
> > +# @BlockdevQcow2CompatLevel:
> > +# @0_10:    The original QCOW2 format as introduced in qemu 0.10 (version 2)
> > +# @1_1:     The extended QCOW2 format as introduced in qemu 1.1 (version 3)
> > +#
> > +# Since: 2.10
> > +##
> > +{ 'enum': 'BlockdevQcow2CompatLevel',
> > +  'data': [ '0_10', '1_1' ] }
> 
> Just my two cents: I'd prefer 2 and 3 because I've never quite liked
> that people are supposed to remember some pretty random qemu version
> numbers anyway.

Yeah. An enum with '2' and '3' wouldn't work for Eric's desire to have
enum values starting in letters, though. I was originally planning to
replace it with an int, but then it's not introspectable.

An enum with 'v2' and 'v3' then?

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema

[Max Reitz]

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

On 2018-01-29 19:06, Kevin Wolf wrote:
> Am 29.01.2018 um 17:57 hat Max Reitz geschrieben:
>> On 2018-01-11 20:52, Kevin Wolf wrote:
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++-
>>>  1 file changed, 32 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 1749376c61..9341f6708d 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -3320,6 +3320,37 @@
>>>  { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
>>>  
>>>  ##
>>> +# @BlockdevQcow2CompatLevel:
>>> +# @0_10:    The original QCOW2 format as introduced in qemu 0.10 (version 2)
>>> +# @1_1:     The extended QCOW2 format as introduced in qemu 1.1 (version 3)
>>> +#
>>> +# Since: 2.10
>>> +##
>>> +{ 'enum': 'BlockdevQcow2CompatLevel',
>>> +  'data': [ '0_10', '1_1' ] }
>>
>> Just my two cents: I'd prefer 2 and 3 because I've never quite liked
>> that people are supposed to remember some pretty random qemu version
>> numbers anyway.
> 
> Yeah. An enum with '2' and '3' wouldn't work for Eric's desire to have
> enum values starting in letters, though. I was originally planning to
> replace it with an int, but then it's not introspectable.

Aw.

> An enum with 'v2' and 'v3' then?

Would work for me.

Max


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

[Qemu-devel] [RFC PATCH 03/10] qcow2: Let qcow2_create() handle protocol layer

[Kevin Wolf]

Currently, qcow2_create() only parses the QemuOpts and then calls
qcow2_create2() for the actual image creation, which includes both the
creation of the actual file on the file system and writing a valid empty
qcow2 image into that file.

The plan is that qcow2_create2() becomes the function that implements
the functionality for a future 'blockdev-create' QMP command, which only
creates the qcow2 layer on an already opened file node.

This is a first step towards that goal: Let's move out anything that
deals with the protocol layer from qcow2_create2() into qcow2_create().
This means that qcow2_create2() doesn't need a file name any more.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 64 +++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 26 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 4348b2c0c5..b02bc39a01 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2690,7 +2690,7 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version,
     return refcount_bits;
 }
 
-static int qcow2_create2(const char *filename, int64_t total_size,
+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,
@@ -2716,28 +2716,11 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     Error *local_err = NULL;
     int ret;
 
-    if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) {
-        int64_t prealloc_size =
-            qcow2_calc_prealloc_size(total_size, cluster_size, refcount_order);
-        qemu_opt_set_number(opts, BLOCK_OPT_SIZE, prealloc_size, &error_abort);
-        qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_str(prealloc),
-                     &error_abort);
-    }
-
-    ret = bdrv_create_file(filename, opts, &local_err);
+    blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
+    ret = blk_insert_bs(blk, bs, errp);
     if (ret < 0) {
-        error_propagate(errp, local_err);
-        return ret;
-    }
-
-    blk = blk_new_open(filename, NULL, NULL,
-                       BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
-                       &local_err);
-    if (blk == NULL) {
-        error_propagate(errp, local_err);
-        return -EIO;
+        goto out;
     }
-
     blk_set_allow_write_beyond_eof(blk, true);
 
     /* Write the header */
@@ -2792,7 +2775,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
      */
     options = qdict_new();
     qdict_put_str(options, "driver", "qcow2");
-    blk = blk_new_open(filename, NULL, options,
+    qdict_put_str(options, "file", bs->node_name);
+    blk = blk_new_open(NULL, NULL, options,
                        BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_NO_FLUSH,
                        &local_err);
     if (blk == NULL) {
@@ -2864,7 +2848,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
      */
     options = qdict_new();
     qdict_put_str(options, "driver", "qcow2");
-    blk = blk_new_open(filename, NULL, options,
+    qdict_put_str(options, "file", bs->node_name);
+    blk = blk_new_open(NULL, NULL, options,
                        BDRV_O_RDWR | BDRV_O_NO_BACKING | BDRV_O_NO_IO,
                        &local_err);
     if (blk == NULL) {
@@ -2894,6 +2879,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     uint64_t refcount_bits;
     int refcount_order;
     char *encryptfmt = NULL;
+    BlockDriverState *bs = NULL;
     Error *local_err = NULL;
     int ret;
 
@@ -2962,12 +2948,38 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 
     refcount_order = ctz32(refcount_bits);
 
-    ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
+    /* Create and open the file (protocol layer */
+    if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) {
+        int64_t prealloc_size =
+            qcow2_calc_prealloc_size(size, cluster_size, refcount_order);
+        qemu_opt_set_number(opts, BLOCK_OPT_SIZE, prealloc_size, &error_abort);
+        qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_str(prealloc),
+                     &error_abort);
+    }
+
+    ret = bdrv_create_file(filename, opts, errp);
+    if (ret < 0) {
+        goto finish;
+    }
+
+    bs = bdrv_open(filename, NULL, NULL,
+                   BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+    if (bs == NULL) {
+        ret = -EIO;
+        goto finish;
+    }
+
+    /* Create the qcow2 image (format layer) */
+    ret = qcow2_create2(bs, size, backing_file, backing_fmt, flags,
                         cluster_size, prealloc, opts, version, refcount_order,
-                        encryptfmt, &local_err);
-    error_propagate(errp, local_err);
+                        encryptfmt, errp);
+    if (ret < 0) {
+        goto finish;
+    }
 
 finish:
+    bdrv_unref(bs);
+
     g_free(backing_file);
     g_free(backing_fmt);
     g_free(encryptfmt);
-- 
2.13.6

Re: [Qemu-devel] [RFC PATCH 03/10] qcow2: Let qcow2_create() handle protocol layer

[Eric Blake]

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

On 01/11/2018 01:52 PM, Kevin Wolf wrote:
> Currently, qcow2_create() only parses the QemuOpts and then calls
> qcow2_create2() for the actual image creation, which includes both the
> creation of the actual file on the file system and writing a valid empty
> qcow2 image into that file.
> 
> The plan is that qcow2_create2() becomes the function that implements
> the functionality for a future 'blockdev-create' QMP command, which only
> creates the qcow2 layer on an already opened file node.
> 
> This is a first step towards that goal: Let's move out anything that
> deals with the protocol layer from qcow2_create2() into qcow2_create().
> This means that qcow2_create2() doesn't need a file name any more.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 64 +++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 38 insertions(+), 26 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 4348b2c0c5..b02bc39a01 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2690,7 +2690,7 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version,
>      return refcount_bits;
>  }
>  
> -static int qcow2_create2(const char *filename, int64_t total_size,
> +static int qcow2_create2(BlockDriverState *bs, int64_t total_size,

Might be worth adding a high-level comment before the two functions to
document the division of labor, rather than just the commit message.

> @@ -2962,12 +2948,38 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
>  
>      refcount_order = ctz32(refcount_bits);
>  
> -    ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
> +    /* Create and open the file (protocol layer */

Closing )

With the nit fixed,
Reviewed-by: Eric Blake <eblake@redhat.com>

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

[Qemu-devel] [RFC PATCH 04/10] 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 <kwolf@redhat.com>
---
 block/qcow2.c | 186 ++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 148 insertions(+), 38 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index b02bc39a01..09e567324d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2630,19 +2630,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;
@@ -2690,12 +2697,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;
 
     /*
@@ -2712,10 +2718,88 @@ 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;
+    }
+
+    if (qcow2_opts->has_compat) {
+        switch (qcow2_opts->compat) {
+        case BLOCKDEV_QCOW2_COMPAT_LEVEL_0_10:
+            version = 2;
+            break;
+        case BLOCKDEV_QCOW2_COMPAT_LEVEL_1_1:
+            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->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_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 && 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)");
+        return -EINVAL;
+    }
+    refcount_order = ctz32(qcow2_opts->refcount_bits);
+
+
+    /* Create BlockBackend to write to the image */
     blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
     ret = blk_insert_bs(blk, bs, errp);
     if (ret < 0) {
@@ -2742,7 +2826,7 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
     /* We'll update this to correct value later */
     header->crypt_method = cpu_to_be32(QCOW_CRYPT_NONE);
 
-    if (flags & BLOCK_FLAG_LAZY_REFCOUNTS) {
+    if (qcow2_opts->lazy_refcounts) {
         header->compatible_features |=
             cpu_to_be64(QCOW2_COMPAT_LAZY_REFCOUNTS);
     }
@@ -2804,18 +2888,26 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
     }
 
     /* Okay, now that we have a valid image, let's give it the right size */
-    ret = blk_truncate(blk, total_size, PREALLOC_MODE_OFF, errp);
+    ret = blk_truncate(blk, qcow2_opts->size, PREALLOC_MODE_OFF, errp);
     if (ret < 0) {
         error_prepend(errp, "Could not resize image: ");
         goto out;
     }
 
     /* 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);
+        }
+
+        ret = bdrv_change_backing_file(blk_bs(blk), qcow2_opts->backing_file,
+                                       backing_format);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not assign backing file '%s' "
-                             "with format '%s'", backing_file, backing_format);
+                             "with format '%s'", qcow2_opts->backing_file,
+                             backing_format);
             goto out;
         }
     }
@@ -2829,8 +2921,8 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
     }
 
     /* And if we're supposed to preallocate metadata, do that now */
-    if (prealloc != PREALLOC_MODE_OFF) {
-        ret = preallocate(blk_bs(blk), 0, total_size);
+    if (qcow2_opts->preallocation != PREALLOC_MODE_OFF) {
+        ret = preallocate(blk_bs(blk), 0, qcow2_opts->size);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not preallocate metadata");
             goto out;
@@ -2868,8 +2960,10 @@ out:
 
 static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 {
+    BlockdevCreateOptions create_options;
     char *backing_file = NULL;
     char *backing_fmt = NULL;
+    BlockdevDriver backing_drv;
     char *buf = NULL;
     uint64_t size = 0;
     int flags = 0;
@@ -2877,7 +2971,6 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     PreallocMode prealloc;
     int version;
     uint64_t refcount_bits;
-    int refcount_order;
     char *encryptfmt = NULL;
     BlockDriverState *bs = NULL;
     Error *local_err = NULL;
@@ -2888,6 +2981,13 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
                     BDRV_SECTOR_SIZE);
     backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
     backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
+    backing_drv = qapi_enum_parse(&BlockdevDriver_lookup, backing_fmt,
+                                  0, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto finish;
+    }
     encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
     if (encryptfmt) {
         if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) {
@@ -2925,20 +3025,6 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
         flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
     }
 
-    if (backing_file && prealloc != PREALLOC_MODE_OFF) {
-        error_setg(errp, "Backing file and preallocation cannot be used at "
-                   "the same time");
-        ret = -EINVAL;
-        goto finish;
-    }
-
-    if (version < 3 && (flags & BLOCK_FLAG_LAZY_REFCOUNTS)) {
-        error_setg(errp, "Lazy refcounts only supported with compatibility "
-                   "level 1.1 and above (use compat=1.1 or greater)");
-        ret = -EINVAL;
-        goto finish;
-    }
-
     refcount_bits = qcow2_opt_get_refcount_bits_del(opts, version, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -2946,10 +3032,10 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
         goto finish;
     }
 
-    refcount_order = ctz32(refcount_bits);
 
     /* Create and open the file (protocol layer */
     if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) {
+        int refcount_order = ctz32(refcount_bits);
         int64_t prealloc_size =
             qcow2_calc_prealloc_size(size, cluster_size, refcount_order);
         qemu_opt_set_number(opts, BLOCK_OPT_SIZE, prealloc_size, &error_abort);
@@ -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),
+            .backing_fmt        = backing_drv,
+            .has_cluster_size   = true,
+            .cluster_size       = cluster_size,
+            .has_preallocation  = true,
+            .preallocation      = prealloc,
+            .has_lazy_refcounts = true,
+            .lazy_refcounts     = (flags & BLOCK_FLAG_LAZY_REFCOUNTS),
+            .has_refcount_bits  = true,
+            .refcount_bits      = refcount_bits,
+        },
+    };
+    ret = qcow2_create2(bs, &create_options, opts, encryptfmt, errp);
     if (ret < 0) {
         goto finish;
     }
-- 
2.13.6

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

[Eric Blake]

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

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 <kwolf@redhat.com>
> ---
>  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 <eblake@redhat.com>

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

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

[Max Reitz]

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

On 2018-01-11 20:52, 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 <kwolf@redhat.com>
> ---
>  block/qcow2.c | 186 ++++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 148 insertions(+), 38 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index b02bc39a01..09e567324d 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c

[...]

> @@ -2690,12 +2697,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,

I'd personally really prefer this to be const...

> +                         QemuOpts *opts, const char *encryptfmt, Error **errp)
>  {
> +    BlockdevCreateOptionsQcow2 *qcow2_opts;
>      QDict *options;
>  
>      /*
> @@ -2712,10 +2718,88 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,

[...]

> +
> +    if (!qcow2_opts->has_preallocation) {
> +        qcow2_opts->preallocation = PREALLOC_MODE_OFF;
> +    }
> +    if (qcow2_opts->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_lazy_refcounts) {
> +        qcow2_opts->lazy_refcounts = false;

...because modifying some ideally QMP-provided objects just looks wrong.

[...]

> @@ -2804,18 +2888,26 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,

[...]

>      /* 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);
> +        }

has_backing_fmt && !has_backing_file should probably be an error.

Max

> +
> +        ret = bdrv_change_backing_file(blk_bs(blk), qcow2_opts->backing_file,
> +                                       backing_format);
>          if (ret < 0) {
>              error_setg_errno(errp, -ret, "Could not assign backing file '%s' "
> -                             "with format '%s'", backing_file, backing_format);
> +                             "with format '%s'", qcow2_opts->backing_file,
> +                             backing_format);
>              goto out;
>          }
>      }


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

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

[Kevin Wolf]

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

Am 29.01.2018 um 18:12 hat Max Reitz geschrieben:
> On 2018-01-11 20:52, 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 <kwolf@redhat.com>
> > ---
> >  block/qcow2.c | 186 ++++++++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 148 insertions(+), 38 deletions(-)
> > 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index b02bc39a01..09e567324d 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> 
> [...]
> 
> > @@ -2690,12 +2697,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,
> 
> I'd personally really prefer this to be const...
> 
> > +                         QemuOpts *opts, const char *encryptfmt, Error **errp)
> >  {
> > +    BlockdevCreateOptionsQcow2 *qcow2_opts;
> >      QDict *options;
> >  
> >      /*
> > @@ -2712,10 +2718,88 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
> 
> [...]
> 
> > +
> > +    if (!qcow2_opts->has_preallocation) {
> > +        qcow2_opts->preallocation = PREALLOC_MODE_OFF;
> > +    }
> > +    if (qcow2_opts->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_lazy_refcounts) {
> > +        qcow2_opts->lazy_refcounts = false;
> 
> ...because modifying some ideally QMP-provided objects just looks wrong.

Isn't this pretty standard, though? Most commands don't use boxed
options, so there they only modify stack variables, but if you look at
boxed ones like do_blockdev_backup() or qmp_drive_mirror(), they do the
same.

> [...]
> 
> > @@ -2804,18 +2888,26 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
> 
> [...]
> 
> >      /* 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);
> > +        }
> 
> has_backing_fmt && !has_backing_file should probably be an error.

Yes.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Max Reitz]

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

On 2018-01-29 19:10, Kevin Wolf wrote:
> Am 29.01.2018 um 18:12 hat Max Reitz geschrieben:
>> On 2018-01-11 20:52, 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 <kwolf@redhat.com>
>>> ---
>>>  block/qcow2.c | 186 ++++++++++++++++++++++++++++++++++++++++++++++------------
>>>  1 file changed, 148 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index b02bc39a01..09e567324d 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>
>> [...]
>>
>>> @@ -2690,12 +2697,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,
>>
>> I'd personally really prefer this to be const...
>>
>>> +                         QemuOpts *opts, const char *encryptfmt, Error **errp)
>>>  {
>>> +    BlockdevCreateOptionsQcow2 *qcow2_opts;
>>>      QDict *options;
>>>  
>>>      /*
>>> @@ -2712,10 +2718,88 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
>>
>> [...]
>>
>>> +
>>> +    if (!qcow2_opts->has_preallocation) {
>>> +        qcow2_opts->preallocation = PREALLOC_MODE_OFF;
>>> +    }
>>> +    if (qcow2_opts->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_lazy_refcounts) {
>>> +        qcow2_opts->lazy_refcounts = false;
>>
>> ...because modifying some ideally QMP-provided objects just looks wrong.
> 
> Isn't this pretty standard, though? Most commands don't use boxed
> options, so there they only modify stack variables, but if you look at
> boxed ones like do_blockdev_backup() or qmp_drive_mirror(), they do the
> same.

:C


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

[Qemu-devel] [RFC PATCH 05/10] qcow2: Use BlockdevRef in qcow2_create2()

[Kevin Wolf]

Instead of passing a separate BlockDriverState* into qcow2_create2(),
make use of the BlockdevRef that is included in BlockdevCreateOptions.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block.h |  1 +
 block.c               | 39 +++++++++++++++++++++++++++++++++++++++
 block/qcow2.c         | 33 +++++++++++++++++++++------------
 3 files changed, 61 insertions(+), 12 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 9b12774ddf..4b11f814a8 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -258,6 +258,7 @@ BdrvChild *bdrv_open_child(const char *filename,
                            BlockDriverState* parent,
                            const BdrvChildRole *child_role,
                            bool allow_none, Error **errp);
+BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
                          Error **errp);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
diff --git a/block.c b/block.c
index a8da4f2b25..c9b0e1d6d3 100644
--- a/block.c
+++ b/block.c
@@ -32,6 +32,8 @@
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qbool.h"
 #include "qapi/qmp/qjson.h"
+#include "qapi/qobject-output-visitor.h"
+#include "qapi-visit.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/sysemu.h"
 #include "qemu/notify.h"
@@ -2405,6 +2407,43 @@ BdrvChild *bdrv_open_child(const char *filename,
     return c;
 }
 
+/* TODO Future callers may need to specify parent/child_role in order for
+ * option inheritance to work. Existing callers use it for the root node. */
+BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp)
+{
+    BlockDriverState *bs = NULL;
+    Error *local_err = NULL;
+    QObject *obj = NULL;
+    QDict *qdict = NULL;
+    const char *reference = NULL;
+    Visitor *v = NULL;
+
+    if (ref->type == QTYPE_QSTRING) {
+        reference = ref->u.reference;
+    } else {
+        BlockdevOptions *options = &ref->u.definition;
+        assert(ref->type == QTYPE_QDICT);
+
+        v = qobject_output_visitor_new(&obj);
+        visit_type_BlockdevOptions(v, NULL, &options, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            goto fail;
+        }
+        visit_complete(v, &obj);
+
+        qdict = qobject_to_qdict(obj);
+        qdict_flatten(qdict);
+    }
+
+    bs = bdrv_open_inherit(NULL, reference, qdict, 0, NULL, NULL, errp);
+
+fail:
+    qobject_decref(obj);
+    visit_free(v);
+    return bs;
+}
+
 static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
                                                    int flags,
                                                    QDict *snapshot_options,
diff --git a/block/qcow2.c b/block/qcow2.c
index 09e567324d..1a0f8f2e6d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2697,8 +2697,7 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version,
     return refcount_bits;
 }
 
-static int qcow2_create2(BlockDriverState *bs,
-                         BlockdevCreateOptions *create_options,
+static int qcow2_create2(BlockdevCreateOptions *create_options,
                          QemuOpts *opts, const char *encryptfmt, Error **errp)
 {
     BlockdevCreateOptionsQcow2 *qcow2_opts;
@@ -2716,7 +2715,8 @@ static int qcow2_create2(BlockDriverState *bs,
      * 2 GB for 64k clusters, and we don't want to have a 2 GB initial file
      * size for any qcow2 image.
      */
-    BlockBackend *blk;
+    BlockBackend *blk = NULL;
+    BlockDriverState *bs = NULL;
     QCowHeader *header;
     size_t cluster_size;
     int version;
@@ -2725,6 +2725,11 @@ static int qcow2_create2(BlockDriverState *bs,
     Error *local_err = NULL;
     int ret;
 
+    bs = bdrv_open_blockdev_ref(create_options->node, errp);
+    if (bs == NULL) {
+        return -EIO;
+    }
+
     /* Validate options and set default values */
     assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2);
     qcow2_opts = &create_options->u.qcow2;
@@ -2757,7 +2762,8 @@ static int qcow2_create2(BlockDriverState *bs,
     }
 
     if (!validate_cluster_size(cluster_size, errp)) {
-        return -EINVAL;
+        ret = -EINVAL;
+        goto out;
     }
 
     if (!qcow2_opts->has_preallocation) {
@@ -2768,7 +2774,8 @@ static int qcow2_create2(BlockDriverState *bs,
     {
         error_setg(errp, "Backing file and preallocation cannot be used at "
                    "the same time");
-        return -EINVAL;
+        ret = -EINVAL;
+        goto out;
     }
 
     if (!qcow2_opts->has_lazy_refcounts) {
@@ -2777,7 +2784,8 @@ static int qcow2_create2(BlockDriverState *bs,
     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;
+        ret = -EINVAL;
+        goto out;
     }
 
     if (!qcow2_opts->has_refcount_bits) {
@@ -2788,13 +2796,15 @@ static int qcow2_create2(BlockDriverState *bs,
     {
         error_setg(errp, "Refcount width must be a power of two and may not "
                    "exceed 64 bits");
-        return -EINVAL;
+        ret = -EINVAL;
+        goto out;
     }
     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)");
-        return -EINVAL;
+        ret = -EINVAL;
+        goto out;
     }
     refcount_order = ctz32(qcow2_opts->refcount_bits);
 
@@ -2952,9 +2962,8 @@ static int qcow2_create2(BlockDriverState *bs,
 
     ret = 0;
 out:
-    if (blk) {
-        blk_unref(blk);
-    }
+    blk_unref(blk);
+    bdrv_unref(bs);
     return ret;
 }
 
@@ -3082,7 +3091,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
             .refcount_bits      = refcount_bits,
         },
     };
-    ret = qcow2_create2(bs, &create_options, opts, encryptfmt, errp);
+    ret = qcow2_create2(&create_options, opts, encryptfmt, errp);
     if (ret < 0) {
         goto finish;
     }
-- 
2.13.6

Re: [Qemu-devel] [RFC PATCH 05/10] qcow2: Use BlockdevRef in qcow2_create2()

[Eric Blake]

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

On 01/11/2018 01:52 PM, Kevin Wolf wrote:
> Instead of passing a separate BlockDriverState* into qcow2_create2(),
> make use of the BlockdevRef that is included in BlockdevCreateOptions.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block.h |  1 +
>  block.c               | 39 +++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c         | 33 +++++++++++++++++++++------------
>  3 files changed, 61 insertions(+), 12 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [Qemu-devel] [RFC PATCH 05/10] qcow2: Use BlockdevRef in qcow2_create2()

[Max Reitz]

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

On 2018-01-11 20:52, Kevin Wolf wrote:
> Instead of passing a separate BlockDriverState* into qcow2_create2(),
> make use of the BlockdevRef that is included in BlockdevCreateOptions.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block.h |  1 +
>  block.c               | 39 +++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c         | 33 +++++++++++++++++++++------------
>  3 files changed, 61 insertions(+), 12 deletions(-)

[...]

> diff --git a/block.c b/block.c
> index a8da4f2b25..c9b0e1d6d3 100644
> --- a/block.c
> +++ b/block.c

[...]

> @@ -2405,6 +2407,43 @@ BdrvChild *bdrv_open_child(const char *filename,
>      return c;
>  }
>  
> +/* TODO Future callers may need to specify parent/child_role in order for
> + * option inheritance to work. Existing callers use it for the root node. */
> +BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp)
> +{
> +    BlockDriverState *bs = NULL;
> +    Error *local_err = NULL;
> +    QObject *obj = NULL;
> +    QDict *qdict = NULL;
> +    const char *reference = NULL;
> +    Visitor *v = NULL;
> +
> +    if (ref->type == QTYPE_QSTRING) {
> +        reference = ref->u.reference;
> +    } else {
> +        BlockdevOptions *options = &ref->u.definition;
> +        assert(ref->type == QTYPE_QDICT);
> +
> +        v = qobject_output_visitor_new(&obj);
> +        visit_type_BlockdevOptions(v, NULL, &options, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            goto fail;
> +        }
> +        visit_complete(v, &obj);
> +
> +        qdict = qobject_to_qdict(obj);
> +        qdict_flatten(qdict);
> +    }
> +
> +    bs = bdrv_open_inherit(NULL, reference, qdict, 0, NULL, NULL, errp);
> +
> +fail:
> +    qobject_decref(obj);

I'd prefer QDECREF(qdict), myself, although I can't quite explain why.
Probably because @qdict is the object you're actually using and @obj is
just some temporary designation.

Also, doesn't bdrv_open_inherit() take ownership of @qdict and thus @obj?

Max

> +    visit_free(v);
> +    return bs;
> +}
> +
>  static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
>                                                     int flags,
>                                                     QDict *snapshot_options,


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

Re: [Qemu-devel] [RFC PATCH 05/10] qcow2: Use BlockdevRef in qcow2_create2()

[Kevin Wolf]

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

Am 29.01.2018 um 18:30 hat Max Reitz geschrieben:
> On 2018-01-11 20:52, Kevin Wolf wrote:
> > Instead of passing a separate BlockDriverState* into qcow2_create2(),
> > make use of the BlockdevRef that is included in BlockdevCreateOptions.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  include/block/block.h |  1 +
> >  block.c               | 39 +++++++++++++++++++++++++++++++++++++++
> >  block/qcow2.c         | 33 +++++++++++++++++++++------------
> >  3 files changed, 61 insertions(+), 12 deletions(-)
> 
> [...]
> 
> > diff --git a/block.c b/block.c
> > index a8da4f2b25..c9b0e1d6d3 100644
> > --- a/block.c
> > +++ b/block.c
> 
> [...]
> 
> > @@ -2405,6 +2407,43 @@ BdrvChild *bdrv_open_child(const char *filename,
> >      return c;
> >  }
> >  
> > +/* TODO Future callers may need to specify parent/child_role in order for
> > + * option inheritance to work. Existing callers use it for the root node. */
> > +BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp)
> > +{
> > +    BlockDriverState *bs = NULL;
> > +    Error *local_err = NULL;
> > +    QObject *obj = NULL;
> > +    QDict *qdict = NULL;
> > +    const char *reference = NULL;
> > +    Visitor *v = NULL;
> > +
> > +    if (ref->type == QTYPE_QSTRING) {
> > +        reference = ref->u.reference;
> > +    } else {
> > +        BlockdevOptions *options = &ref->u.definition;
> > +        assert(ref->type == QTYPE_QDICT);
> > +
> > +        v = qobject_output_visitor_new(&obj);
> > +        visit_type_BlockdevOptions(v, NULL, &options, &local_err);
> > +        if (local_err) {
> > +            error_propagate(errp, local_err);
> > +            goto fail;
> > +        }
> > +        visit_complete(v, &obj);
> > +
> > +        qdict = qobject_to_qdict(obj);
> > +        qdict_flatten(qdict);
> > +    }
> > +
> > +    bs = bdrv_open_inherit(NULL, reference, qdict, 0, NULL, NULL, errp);
> > +
> > +fail:
> > +    qobject_decref(obj);
> 
> I'd prefer QDECREF(qdict), myself, although I can't quite explain why.
> Probably because @qdict is the object you're actually using and @obj is
> just some temporary designation.

Hm... If visit_type_BlockdevOptions() fails, qdict is not yet assigned.
Are we sure that obj remains NULL in this case?

qobject_decref(obj) looks more obviously correct to me.

> Also, doesn't bdrv_open_inherit() take ownership of @qdict and thus @obj?

That's a very good point...

Kevin

> Max
> 
> > +    visit_free(v);
> > +    return bs;
> > +}
> > +
> >  static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
> >                                                     int flags,
> >                                                     QDict *snapshot_options,
> 




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[Qemu-devel] [RFC PATCH 06/10] qcow2: Use QCryptoBlockCreateOptions in qcow2_create2()

[Kevin Wolf]

Instead of passing the encryption format name and the QemuOpts down, use
the QCryptoBlockCreateOptions contained in BlockdevCreateOptions.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 62 +++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 1a0f8f2e6d..686b765c06 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2379,13 +2379,10 @@ static int qcow2_crypt_method_from_format(const char *encryptfmt)
     }
 }
 
-static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt,
-                                   QemuOpts *opts, Error **errp)
+static QCryptoBlockCreateOptions *
+qcow2_parse_encryption(const char *encryptfmt, QemuOpts *opts, Error **errp)
 {
-    BDRVQcow2State *s = bs->opaque;
     QCryptoBlockCreateOptions *cryptoopts = NULL;
-    QCryptoBlock *crypto = NULL;
-    int ret = -EINVAL;
     QDict *options, *encryptopts;
     int fmt;
 
@@ -2408,10 +2405,31 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt,
         error_setg(errp, "Unknown encryption format '%s'", encryptfmt);
         break;
     }
-    if (!cryptoopts) {
-        ret = -EINVAL;
-        goto out;
+
+    QDECREF(encryptopts);
+    return cryptoopts;
+}
+
+static int qcow2_set_up_encryption(BlockDriverState *bs,
+                                   QCryptoBlockCreateOptions *cryptoopts,
+                                   Error **errp)
+{
+    BDRVQcow2State *s = bs->opaque;
+    QCryptoBlock *crypto = NULL;
+    int fmt, ret;
+
+    switch (cryptoopts->format) {
+    case Q_CRYPTO_BLOCK_FORMAT_LUKS:
+        fmt = QCOW_CRYPT_LUKS;
+        break;
+    case Q_CRYPTO_BLOCK_FORMAT_QCOW:
+        fmt = QCOW_CRYPT_AES;
+        break;
+    default:
+        error_setg(errp, "Crypto format not supported in qcow2");
+        return -EINVAL;
     }
+
     s->crypt_method_header = fmt;
 
     crypto = qcrypto_block_create(cryptoopts, "encrypt.",
@@ -2419,8 +2437,7 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt,
                                   qcow2_crypto_hdr_write_func,
                                   bs, errp);
     if (!crypto) {
-        ret = -EINVAL;
-        goto out;
+        return -EINVAL;
     }
 
     ret = qcow2_update_header(bs);
@@ -2429,10 +2446,9 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt,
         goto out;
     }
 
+    ret = 0;
  out:
-    QDECREF(encryptopts);
     qcrypto_block_free(crypto);
-    qapi_free_QCryptoBlockCreateOptions(cryptoopts);
     return ret;
 }
 
@@ -2697,8 +2713,7 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version,
     return refcount_bits;
 }
 
-static int qcow2_create2(BlockdevCreateOptions *create_options,
-                         QemuOpts *opts, const char *encryptfmt, Error **errp)
+static int qcow2_create2(BlockdevCreateOptions *create_options, Error **errp)
 {
     BlockdevCreateOptionsQcow2 *qcow2_opts;
     QDict *options;
@@ -2923,8 +2938,8 @@ static int qcow2_create2(BlockdevCreateOptions *create_options,
     }
 
     /* Want encryption? There you go. */
-    if (encryptfmt) {
-        ret = qcow2_set_up_encryption(blk_bs(blk), encryptfmt, opts, errp);
+    if (qcow2_opts->has_encrypt) {
+        ret = qcow2_set_up_encryption(blk_bs(blk), qcow2_opts->encrypt, errp);
         if (ret < 0) {
             goto out;
         }
@@ -2981,6 +2996,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     int version;
     uint64_t refcount_bits;
     char *encryptfmt = NULL;
+    QCryptoBlockCreateOptions *cryptoopts = NULL;
     BlockDriverState *bs = NULL;
     Error *local_err = NULL;
     int ret;
@@ -2997,6 +3013,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
         ret = -EINVAL;
         goto finish;
     }
+
     encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
     if (encryptfmt) {
         if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) {
@@ -3008,6 +3025,14 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
         encryptfmt = g_strdup("aes");
     }
+    if (encryptfmt) {
+        cryptoopts = qcow2_parse_encryption(encryptfmt, opts, errp);
+        if (cryptoopts == NULL) {
+            ret = -EINVAL;
+            goto finish;
+        }
+    }
+
     cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -3081,6 +3106,8 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
             .backing_file       = backing_file,
             .has_backing_fmt    = (backing_fmt != NULL),
             .backing_fmt        = backing_drv,
+            .has_encrypt        = (encryptfmt != NULL),
+            .encrypt            = cryptoopts,
             .has_cluster_size   = true,
             .cluster_size       = cluster_size,
             .has_preallocation  = true,
@@ -3091,7 +3118,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
             .refcount_bits      = refcount_bits,
         },
     };
-    ret = qcow2_create2(&create_options, opts, encryptfmt, errp);
+    ret = qcow2_create2(&create_options, errp);
     if (ret < 0) {
         goto finish;
     }
@@ -3099,6 +3126,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 finish:
     bdrv_unref(bs);
 
+    qapi_free_QCryptoBlockCreateOptions(cryptoopts);
     g_free(backing_file);
     g_free(backing_fmt);
     g_free(encryptfmt);
-- 
2.13.6

Re: [Qemu-devel] [RFC PATCH 06/10] qcow2: Use QCryptoBlockCreateOptions in qcow2_create2()

[Eric Blake]

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

On 01/11/2018 01:52 PM, Kevin Wolf wrote:
> Instead of passing the encryption format name and the QemuOpts down, use
> the QCryptoBlockCreateOptions contained in BlockdevCreateOptions.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 62 +++++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 45 insertions(+), 17 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[Qemu-devel] [RFC PATCH 07/10] qcow2: Handle full/falloc preallocation in qcow2_create2()

[Kevin Wolf]

Once qcow2_create2() can be called directly on an already existing node,
we must provide the 'full' and 'falloc' preallocation modes outside of
creating the image on the protocol layer. Fortunately, we have
preallocated truncate now which can provide this functionality.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 686b765c06..868e0e8a62 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2832,6 +2832,25 @@ static int qcow2_create2(BlockdevCreateOptions *create_options, Error **errp)
     }
     blk_set_allow_write_beyond_eof(blk, true);
 
+    /* Clear the protocol layer and preallocate it if necessary */
+    ret = blk_truncate(blk, 0, PREALLOC_MODE_OFF, errp);
+    if (ret < 0) {
+        goto out;
+    }
+
+    if (qcow2_opts->preallocation == PREALLOC_MODE_FULL ||
+        qcow2_opts->preallocation == PREALLOC_MODE_FALLOC)
+    {
+        int64_t prealloc_size =
+            qcow2_calc_prealloc_size(qcow2_opts->size, cluster_size,
+                                     refcount_order);
+
+        ret = blk_truncate(blk, prealloc_size, qcow2_opts->preallocation, errp);
+        if (ret < 0) {
+            goto out;
+        }
+    }
+
     /* Write the header */
     QEMU_BUILD_BUG_ON((1 << MIN_CLUSTER_BITS) < sizeof(*header));
     header = g_malloc0(cluster_size);
@@ -3068,15 +3087,6 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 
 
     /* Create and open the file (protocol layer */
-    if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) {
-        int refcount_order = ctz32(refcount_bits);
-        int64_t prealloc_size =
-            qcow2_calc_prealloc_size(size, cluster_size, refcount_order);
-        qemu_opt_set_number(opts, BLOCK_OPT_SIZE, prealloc_size, &error_abort);
-        qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_str(prealloc),
-                     &error_abort);
-    }
-
     ret = bdrv_create_file(filename, opts, errp);
     if (ret < 0) {
         goto finish;
-- 
2.13.6

Re: [Qemu-devel] [RFC PATCH 07/10] qcow2: Handle full/falloc preallocation in qcow2_create2()

[Eric Blake]

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

On 01/11/2018 01:52 PM, Kevin Wolf wrote:
> Once qcow2_create2() can be called directly on an already existing node,
> we must provide the 'full' and 'falloc' preallocation modes outside of
> creating the image on the protocol layer. Fortunately, we have
> preallocated truncate now which can provide this functionality.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[Qemu-devel] [RFC PATCH 08/10] util: Add qemu_opts_to_qdict_filtered()

[Kevin Wolf]

This allows, given a QemuOpts for a QemuOptsList that was merged from
multiple QemuOptsList, to only consider those options that exist in one
specific list. Block drivers need this to separate format-layer create
options from protocol-level options.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qemu/option.h |  2 ++
 util/qemu-option.c    | 28 +++++++++++++++++++++++++---
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index a88c5f02b1..197f80e79d 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -125,6 +125,8 @@ void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
                             int permit_abbrev);
 QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
                                Error **errp);
+QDict *qemu_opts_to_qdict_filtered(QemuOpts *opts, QDict *qdict,
+                                   QemuOptsList *list, bool del);
 QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
 void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp);
 
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 553d3dc552..76dc5bedf8 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -1009,9 +1009,10 @@ void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp)
  * TODO We'll want to use types appropriate for opt->desc->type, but
  * this is enough for now.
  */
-QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict)
+QDict *qemu_opts_to_qdict_filtered(QemuOpts *opts, QDict *qdict,
+                                   QemuOptsList *list, bool del)
 {
-    QemuOpt *opt;
+    QemuOpt *opt, *next;
 
     if (!qdict) {
         qdict = qdict_new();
@@ -1019,12 +1020,33 @@ QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict)
     if (opts->id) {
         qdict_put_str(qdict, "id", opts->id);
     }
-    QTAILQ_FOREACH(opt, &opts->head, next) {
+    QTAILQ_FOREACH_SAFE(opt, &opts->head, next, next) {
+        if (list) {
+            QemuOptDesc *desc;
+            bool found = false;
+            for (desc = list->desc; desc->name; desc++) {
+                if (!strcmp(desc->name, opt->name)) {
+                    found = true;
+                    break;
+                }
+            }
+            if (!found) {
+                continue;
+            }
+        }
         qdict_put_str(qdict, opt->name, opt->str);
+        if (del) {
+            qemu_opt_del_all(opts, opt->name);
+        }
     }
     return qdict;
 }
 
+QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict)
+{
+    return qemu_opts_to_qdict_filtered(opts, qdict, NULL, false);
+}
+
 /* Validate parsed opts against descriptions where no
  * descriptions were provided in the QemuOptsList.
  */
-- 
2.13.6

Re: [Qemu-devel] [RFC PATCH 08/10] util: Add qemu_opts_to_qdict_filtered()

[Eric Blake]

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

On 01/11/2018 01:52 PM, Kevin Wolf wrote:
> This allows, given a QemuOpts for a QemuOptsList that was merged from
> multiple QemuOptsList, to only consider those options that exist in one
> specific list. Block drivers need this to separate format-layer create
> options from protocol-level options.

Someday it would be nice to not have to rely so much on QemuOpts. But in
the meantime, this glue makes sense.

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/qemu/option.h |  2 ++
>  util/qemu-option.c    | 28 +++++++++++++++++++++++++---
>  2 files changed, 27 insertions(+), 3 deletions(-)
> 

It would be nice to add direct testsuite coverage of the new function,
in addition to the indirect coverage it gets when list is NULL.


> +++ b/util/qemu-option.c
> @@ -1009,9 +1009,10 @@ void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp)
>   * TODO We'll want to use types appropriate for opt->desc->type, but
>   * this is enough for now.
>   */

Does this comment need any tweaking...

> -QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict)
> +QDict *qemu_opts_to_qdict_filtered(QemuOpts *opts, QDict *qdict,
> +                                   QemuOptsList *list, bool del)
>  {
> -    QemuOpt *opt;
> +    QemuOpt *opt, *next;
>  
>      if (!qdict) {
>          qdict = qdict_new();

>  
> +QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict)

...or this moved declaration need a comment?

> +{
> +    return qemu_opts_to_qdict_filtered(opts, qdict, NULL, false);
> +}
> +
>  /* Validate parsed opts against descriptions where no
>   * descriptions were provided in the QemuOptsList.
>   */
> 

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

[Qemu-devel] [RFC PATCH 09/10] qcow2: Use visitor for options in qcow2_create()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c              | 227 ++++++++++++++++++---------------------------
 tests/qemu-iotests/049.out |  10 +-
 2 files changed, 93 insertions(+), 144 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 868e0e8a62..4031a18a77 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -36,7 +36,7 @@
 #include "qemu/option_int.h"
 #include "qemu/cutils.h"
 #include "qemu/bswap.h"
-#include "qapi/opts-visitor.h"
+#include "qapi/qobject-input-visitor.h"
 #include "qapi-visit.h"
 #include "block/crypto.h"
 
@@ -2379,37 +2379,6 @@ static int qcow2_crypt_method_from_format(const char *encryptfmt)
     }
 }
 
-static QCryptoBlockCreateOptions *
-qcow2_parse_encryption(const char *encryptfmt, QemuOpts *opts, Error **errp)
-{
-    QCryptoBlockCreateOptions *cryptoopts = NULL;
-    QDict *options, *encryptopts;
-    int fmt;
-
-    options = qemu_opts_to_qdict(opts, NULL);
-    qdict_extract_subqdict(options, &encryptopts, "encrypt.");
-    QDECREF(options);
-
-    fmt = qcow2_crypt_method_from_format(encryptfmt);
-
-    switch (fmt) {
-    case QCOW_CRYPT_LUKS:
-        cryptoopts = block_crypto_create_opts_init(
-            Q_CRYPTO_BLOCK_FORMAT_LUKS, encryptopts, errp);
-        break;
-    case QCOW_CRYPT_AES:
-        cryptoopts = block_crypto_create_opts_init(
-            Q_CRYPTO_BLOCK_FORMAT_QCOW, encryptopts, errp);
-        break;
-    default:
-        error_setg(errp, "Unknown encryption format '%s'", encryptfmt);
-        break;
-    }
-
-    QDECREF(encryptopts);
-    return cryptoopts;
-}
-
 static int qcow2_set_up_encryption(BlockDriverState *bs,
                                    QCryptoBlockCreateOptions *cryptoopts,
                                    Error **errp)
@@ -3003,144 +2972,124 @@ out:
 
 static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 {
-    BlockdevCreateOptions create_options;
-    char *backing_file = NULL;
-    char *backing_fmt = NULL;
-    BlockdevDriver backing_drv;
-    char *buf = NULL;
-    uint64_t size = 0;
-    int flags = 0;
-    size_t cluster_size = DEFAULT_CLUSTER_SIZE;
-    PreallocMode prealloc;
-    int version;
-    uint64_t refcount_bits;
-    char *encryptfmt = NULL;
-    QCryptoBlockCreateOptions *cryptoopts = NULL;
+    BlockdevCreateOptions *create_options;
+    QDict *qdict = NULL;
+    QObject *qobj;
+    Visitor *v;
     BlockDriverState *bs = NULL;
+    const char *val;
+    int i, ret;
     Error *local_err = NULL;
-    int ret;
 
-    /* Read out options */
-    size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
-                    BDRV_SECTOR_SIZE);
-    backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
-    backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
-    backing_drv = qapi_enum_parse(&BlockdevDriver_lookup, backing_fmt,
-                                  0, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        ret = -EINVAL;
-        goto finish;
-    }
+    /* Only the keyval visitor supports the dotted syntax needed for
+     * encryption, so go through a QDict before getting a QAPI type. Ignore
+     * options meant for the protocol layer so that the visitor doesn't
+     * complain. */
+    qdict = qemu_opts_to_qdict_filtered(opts, NULL, bdrv_qcow2.create_opts,
+                                        true);
+
+    /* Handle encryption options */
+    val = qdict_get_try_str(qdict, BLOCK_OPT_ENCRYPT);
+    if (val && !strcmp(val, "on")) {
+        qdict_put_str(qdict, BLOCK_OPT_ENCRYPT, "qcow");
+    } else if (val && !strcmp(val, "off")) {
+        qdict_del(qdict, BLOCK_OPT_ENCRYPT);
+    }
+
+    val = qdict_get_try_str(qdict, BLOCK_OPT_ENCRYPT_FORMAT);
+    if (val && !strcmp(val, "aes")) {
+        qdict_put_str(qdict, BLOCK_OPT_ENCRYPT_FORMAT, "qcow");
+    }
+
+    /* TODO QAPI doesn't allow dots in enum values. Either change QAPI or
+     * decide on the proper new representation for blockdev-create */
+    val = qdict_get_try_str(qdict, BLOCK_OPT_COMPAT_LEVEL);
+    if (val && !strcmp(val, "0.10")) {
+        qdict_put_str(qdict, BLOCK_OPT_COMPAT_LEVEL, "0_10");
+    } else if (val && !strcmp(val, "1.1")) {
+        qdict_put_str(qdict, BLOCK_OPT_COMPAT_LEVEL, "1_1");
+    }
+
+    /* Change legacy command line options into QMP ones */
+    static const struct {
+        const char *from;
+        const char *to;
+    } opt_renames[] = {
+        { BLOCK_OPT_BACKING_FILE,       "backing-file" },
+        { BLOCK_OPT_BACKING_FMT,        "backing-fmt" },
+        { BLOCK_OPT_CLUSTER_SIZE,       "cluster-size" },
+        { BLOCK_OPT_LAZY_REFCOUNTS,     "lazy-refcounts" },
+        { BLOCK_OPT_REFCOUNT_BITS,      "refcount-bits" },
+        { BLOCK_OPT_ENCRYPT,            BLOCK_OPT_ENCRYPT_FORMAT },
+    };
 
-    encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
-    if (encryptfmt) {
-        if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) {
-            error_setg(errp, "Options " BLOCK_OPT_ENCRYPT " and "
-                       BLOCK_OPT_ENCRYPT_FORMAT " are mutually exclusive");
-            ret = -EINVAL;
-            goto finish;
-        }
-    } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
-        encryptfmt = g_strdup("aes");
-    }
-    if (encryptfmt) {
-        cryptoopts = qcow2_parse_encryption(encryptfmt, opts, errp);
-        if (cryptoopts == NULL) {
-            ret = -EINVAL;
-            goto finish;
+    for (i = 0; i < ARRAY_SIZE(opt_renames); i++) {
+        if (qdict_haskey(qdict, opt_renames[i].from)) {
+            if (qdict_haskey(qdict, opt_renames[i].to)) {
+                error_setg(errp, "'%s' and its alias '%s' can't be used at the "
+                           "same time", opt_renames[i].to, opt_renames[i].from);
+                ret = -EINVAL;
+                goto finish;
+            }
+
+            qobj = qdict_get(qdict, opt_renames[i].from);
+            qobject_incref(qobj);
+            qdict_put_obj(qdict, opt_renames[i].to, qobj);
+            qdict_del(qdict, opt_renames[i].from);
         }
     }
 
-    cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        ret = -EINVAL;
+    /* Create and open the file (protocol layer) */
+    ret = bdrv_create_file(filename, opts, errp);
+    if (ret < 0) {
         goto finish;
     }
-    buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
-    prealloc = qapi_enum_parse(&PreallocMode_lookup, buf,
-                               PREALLOC_MODE_OFF, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        ret = -EINVAL;
+
+    bs = bdrv_open(filename, NULL, NULL,
+                   BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+    if (bs == NULL) {
+        ret = -EIO;
         goto finish;
     }
 
-    version = qcow2_opt_get_version_del(opts, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    /* Set 'driver' and 'node' options */
+    qdict_put_str(qdict, "driver", "qcow2");
+    qdict_put_str(qdict, "node", bs->node_name);
+
+    /* Now get the QAPI type BlockdevCreateOptions */
+    qobj = qdict_crumple(qdict, errp);
+    QDECREF(qdict);
+    qdict = qobject_to_qdict(qobj);
+    if (qdict == NULL) {
         ret = -EINVAL;
         goto finish;
     }
 
-    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_LAZY_REFCOUNTS, false)) {
-        flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
-    }
+    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
+    visit_free(v);
 
-    refcount_bits = qcow2_opt_get_refcount_bits_del(opts, version, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
         goto finish;
     }
 
-
-    /* Create and open the file (protocol layer */
-    ret = bdrv_create_file(filename, opts, errp);
-    if (ret < 0) {
-        goto finish;
-    }
-
-    bs = bdrv_open(filename, NULL, NULL,
-                   BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
-    if (bs == NULL) {
-        ret = -EIO;
-        goto finish;
-    }
+    /* Silently round up size */
+    create_options->u.qcow2.size = ROUND_UP(create_options->u.qcow2.size,
+                                            BDRV_SECTOR_SIZE);
 
     /* Create the qcow2 image (format layer) */
-    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),
-            .backing_fmt        = backing_drv,
-            .has_encrypt        = (encryptfmt != NULL),
-            .encrypt            = cryptoopts,
-            .has_cluster_size   = true,
-            .cluster_size       = cluster_size,
-            .has_preallocation  = true,
-            .preallocation      = prealloc,
-            .has_lazy_refcounts = true,
-            .lazy_refcounts     = (flags & BLOCK_FLAG_LAZY_REFCOUNTS),
-            .has_refcount_bits  = true,
-            .refcount_bits      = refcount_bits,
-        },
-    };
-    ret = qcow2_create2(&create_options, errp);
+    ret = qcow2_create2(create_options, errp);
     if (ret < 0) {
         goto finish;
     }
 
+    ret = 0;
 finish:
+    QDECREF(qdict);
     bdrv_unref(bs);
-
-    qapi_free_QCryptoBlockCreateOptions(cryptoopts);
-    g_free(backing_file);
-    g_free(backing_fmt);
-    g_free(encryptfmt);
-    g_free(buf);
+    qapi_free_BlockdevCreateOptions(create_options);
     return ret;
 }
 
diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out
index 003247023e..1115c27ccf 100644
--- a/tests/qemu-iotests/049.out
+++ b/tests/qemu-iotests/049.out
@@ -106,7 +106,7 @@ qemu-img: Value '-1k' is out of range for parameter 'size'
 qemu-img: TEST_DIR/t.qcow2: Invalid options for file format 'qcow2'
 
 qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- 1kilobyte
-qemu-img: Invalid image size specified! You may use k, M, G, T, P or E suffixes for
+qemu-img: Invalid image size specified! You may use k, M, G, T, P or E suffixes for 
 qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes.
 
 qemu-img create -f qcow2 -o size=1kilobyte TEST_DIR/t.qcow2
@@ -116,7 +116,7 @@ and exabytes, respectively.
 qemu-img: TEST_DIR/t.qcow2: Invalid options for file format 'qcow2'
 
 qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- foobar
-qemu-img: Invalid image size specified! You may use k, M, G, T, P or E suffixes for
+qemu-img: Invalid image size specified! You may use k, M, G, T, P or E suffixes for 
 qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes.
 
 qemu-img create -f qcow2 -o size=foobar TEST_DIR/t.qcow2
@@ -166,11 +166,11 @@ qemu-img create -f qcow2 -o compat=1.1 TEST_DIR/t.qcow2 64M
 Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=1.1 cluster_size=65536 lazy_refcounts=off refcount_bits=16
 
 qemu-img create -f qcow2 -o compat=0.42 TEST_DIR/t.qcow2 64M
-qemu-img: TEST_DIR/t.qcow2: Invalid compatibility level: '0.42'
+qemu-img: TEST_DIR/t.qcow2: Invalid parameter '0.42'
 Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=0.42 cluster_size=65536 lazy_refcounts=off refcount_bits=16
 
 qemu-img create -f qcow2 -o compat=foobar TEST_DIR/t.qcow2 64M
-qemu-img: TEST_DIR/t.qcow2: Invalid compatibility level: 'foobar'
+qemu-img: TEST_DIR/t.qcow2: Invalid parameter 'foobar'
 Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=foobar cluster_size=65536 lazy_refcounts=off refcount_bits=16
 
 == Check preallocation option ==
@@ -182,7 +182,7 @@ qemu-img create -f qcow2 -o preallocation=metadata TEST_DIR/t.qcow2 64M
 Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 cluster_size=65536 preallocation=metadata lazy_refcounts=off refcount_bits=16
 
 qemu-img create -f qcow2 -o preallocation=1234 TEST_DIR/t.qcow2 64M
-qemu-img: TEST_DIR/t.qcow2: invalid parameter value: 1234
+qemu-img: TEST_DIR/t.qcow2: Invalid parameter '1234'
 Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 cluster_size=65536 preallocation=1234 lazy_refcounts=off refcount_bits=16
 
 == Check encryption option ==
-- 
2.13.6

Re: [Qemu-devel] [RFC PATCH 09/10] qcow2: Use visitor for options in qcow2_create()

[Eric Blake]

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

On 01/11/2018 01:52 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c              | 227 ++++++++++++++++++---------------------------
>  tests/qemu-iotests/049.out |  10 +-
>  2 files changed, 93 insertions(+), 144 deletions(-)
> 


> +    /* Only the keyval visitor supports the dotted syntax needed for
> +     * encryption, so go through a QDict before getting a QAPI type. Ignore
> +     * options meant for the protocol layer so that the visitor doesn't
> +     * complain. */
> +    qdict = qemu_opts_to_qdict_filtered(opts, NULL, bdrv_qcow2.create_opts,
> +                                        true);

Whether we like it or not, the keyval visitor is getting more and more
important!

> +
> +    /* TODO QAPI doesn't allow dots in enum values. Either change QAPI or
> +     * decide on the proper new representation for blockdev-create */
> +    val = qdict_get_try_str(qdict, BLOCK_OPT_COMPAT_LEVEL);
> +    if (val && !strcmp(val, "0.10")) {
> +        qdict_put_str(qdict, BLOCK_OPT_COMPAT_LEVEL, "0_10");
> +    } else if (val && !strcmp(val, "1.1")) {
> +        qdict_put_str(qdict, BLOCK_OPT_COMPAT_LEVEL, "1_1");

Yeah, I flagged that in the earlier QAPI patch. The fact that it starts
with a number at all is also a bit worrying (we have to support leading
digits for legacy reasons in QKeyCode, but maybe naming things just 'v2'
and 'v3' might be cleaner than '0_10' and '1_1'?)


> +    /* Now get the QAPI type BlockdevCreateOptions */
> +    qobj = qdict_crumple(qdict, errp);
> +    QDECREF(qdict);
> +    qdict = qobject_to_qdict(qobj);
> +    if (qdict == NULL) {
>          ret = -EINVAL;
>          goto finish;
>      }
>  
> -    if (qemu_opt_get_bool_del(opts, BLOCK_OPT_LAZY_REFCOUNTS, false)) {
> -        flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
> -    }
> +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
> +    visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err);
> +    visit_free(v);

Feels like a lot of round-tripping between formats to get things this
compact - but it's as good as anything else I can come up with.


> +++ b/tests/qemu-iotests/049.out
> @@ -106,7 +106,7 @@ qemu-img: Value '-1k' is out of range for parameter 'size'
>  qemu-img: TEST_DIR/t.qcow2: Invalid options for file format 'qcow2'
>  
>  qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- 1kilobyte
> -qemu-img: Invalid image size specified! You may use k, M, G, T, P or E suffixes for
> +qemu-img: Invalid image size specified! You may use k, M, G, T, P or E suffixes for 

Why are you re-adding trailing whitespace?

Overall seems sane.

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

[Qemu-devel] [RFC PATCH 10/10] block: x-blockdev-create QMP command

[Kevin Wolf]

This adds a synchronous x-blockdev-create QMP command that can create
qcow2 images on a given node name.

We don't want to block while creating an image, so this is not the final
interface in all aspects, but BlockdevCreateOptionsQcow2 and
.bdrv_co_create() are what they actually might look like in the end. In
any case, this should be good enough to test whether we interpret
BlockdevCreateOptions as we should.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json      | 12 ++++++++++++
 include/block/block_int.h |  2 ++
 block.c                   | 48 +++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c             |  3 ++-
 4 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9341f6708d..93357a4d5d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3415,6 +3415,18 @@
   } }
 
 ##
+# @x-blockdev-create:
+#
+# Create an image format on a given node.
+# TODO Replace with something asynchronous (block job?)
+#
+# Since: 2.12
+##
+{ 'command': 'x-blockdev-create',
+  'data': 'BlockdevCreateOptions',
+  'boxed': true }
+
+##
 # @blockdev-open-tray:
 #
 # Opens a block device's tray. If there is a block driver state tree inserted as
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 29cafa4236..a9f144d7bd 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -130,6 +130,8 @@ struct BlockDriver {
     int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp);
     void (*bdrv_close)(BlockDriverState *bs);
+    int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
+                                       Error **errp);
     int (*bdrv_create)(const char *filename, QemuOpts *opts, Error **errp);
     int (*bdrv_make_empty)(BlockDriverState *bs);
 
diff --git a/block.c b/block.c
index c9b0e1d6d3..7521884de8 100644
--- a/block.c
+++ b/block.c
@@ -485,6 +485,54 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
     return ret;
 }
 
+typedef struct BlockdevCreateCo {
+    BlockDriver *drv;
+    BlockdevCreateOptions *opts;
+    int ret;
+    Error **errp;
+} BlockdevCreateCo;
+
+static void coroutine_fn bdrv_co_create_co_entry(void *opaque)
+{
+    BlockdevCreateCo *cco = opaque;
+    cco->ret = cco->drv->bdrv_co_create(cco->opts, cco->errp);
+}
+
+void qmp_x_blockdev_create(BlockdevCreateOptions *options, Error **errp)
+{
+    const char *fmt = BlockdevDriver_str(options->driver);
+    BlockDriver* drv = bdrv_find_format(fmt);
+    Coroutine *co;
+    BlockdevCreateCo cco;
+
+    /* If the driver is in the schema, we know that it exists. But it may not
+     * be whitelisted. */
+    assert(drv);
+    if (bdrv_uses_whitelist() && !bdrv_is_whitelisted(drv, true)) {
+        error_setg(errp, "Driver is not whitelisted");
+        return;
+    }
+
+    /* Call callback if it exists */
+    if (!drv->bdrv_co_create) {
+        error_setg(errp, "Driver does not support blockdev-create");
+        return;
+    }
+
+    cco = (BlockdevCreateCo) {
+        .drv = drv,
+        .opts = options,
+        .ret = NOT_DONE,
+        .errp = errp,
+    };
+
+    co = qemu_coroutine_create(bdrv_co_create_co_entry, &cco);
+    qemu_coroutine_enter(co);
+    while (cco.ret == NOT_DONE) {
+        aio_poll(qemu_get_aio_context(), true);
+    }
+}
+
 /**
  * Try to get @bs's logical and physical block size.
  * On success, store them in @bsz struct and return 0.
diff --git a/block/qcow2.c b/block/qcow2.c
index 4031a18a77..5c5386e0b6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4436,7 +4436,8 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_reopen_abort    = qcow2_reopen_abort,
     .bdrv_join_options    = qcow2_join_options,
     .bdrv_child_perm      = bdrv_format_default_perms,
-    .bdrv_create        = qcow2_create,
+    .bdrv_create          = qcow2_create,
+    .bdrv_co_create       = qcow2_create2,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
     .bdrv_co_get_block_status = qcow2_co_get_block_status,
 
-- 
2.13.6

Re: [Qemu-devel] [RFC PATCH 10/10] block: x-blockdev-create QMP command

[Eric Blake]

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

On 01/11/2018 01:52 PM, Kevin Wolf wrote:
> This adds a synchronous x-blockdev-create QMP command that can create
> qcow2 images on a given node name.
> 
> We don't want to block while creating an image, so this is not the final
> interface in all aspects, but BlockdevCreateOptionsQcow2 and
> .bdrv_co_create() are what they actually might look like in the end. In
> any case, this should be good enough to test whether we interpret
> BlockdevCreateOptions as we should.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/block-core.json      | 12 ++++++++++++
>  include/block/block_int.h |  2 ++
>  block.c                   | 48 +++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c             |  3 ++-
>  4 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 9341f6708d..93357a4d5d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3415,6 +3415,18 @@
>    } }
>  
>  ##
> +# @x-blockdev-create:
> +#
> +# Create an image format on a given node.
> +# TODO Replace with something asynchronous (block job?)
> +#
> +# Since: 2.12
> +##
> +{ 'command': 'x-blockdev-create',
> +  'data': 'BlockdevCreateOptions',
> +  'boxed': true }
> +

So simple, compared to all the prep work in earlier patches ;)

I like the approach.  As you say, there's still more work before we can
remove the x- prefix, but I think you're headed on a good track.

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

Re: [Qemu-devel] [RFC PATCH 10/10] block: x-blockdev-create QMP command

[Kevin Wolf]

Am 11.01.2018 um 20:52 hat Kevin Wolf geschrieben:
> This adds a synchronous x-blockdev-create QMP command that can create
> qcow2 images on a given node name.
> 
> We don't want to block while creating an image, so this is not the final
> interface in all aspects, but BlockdevCreateOptionsQcow2 and
> .bdrv_co_create() are what they actually might look like in the end. In
> any case, this should be good enough to test whether we interpret
> BlockdevCreateOptions as we should.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/block-core.json      | 12 ++++++++++++
>  include/block/block_int.h |  2 ++
>  block.c                   | 48 +++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c             |  3 ++-
>  4 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 9341f6708d..93357a4d5d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3415,6 +3415,18 @@
>    } }
>  
>  ##
> +# @x-blockdev-create:
> +#
> +# Create an image format on a given node.
> +# TODO Replace with something asynchronous (block job?)

I just realised that this won't be a block job. It will be a job without
block, because there isn't necessarily any BlockDriverState involved
(when you create the protocol layer).

So it looks like my first job there is to make block jobs work with
bs == NULL... Or actually, we should probably try and do the proper
thing with the whole new job API that we were planning anyway?

Kevin

Re: [Qemu-devel] [RFC PATCH 00/10] x-blockdev-create for qcow2

[no-reply]

Hi,

This series failed build test on ppc host. Please find the details below.

Message-id: 20180111195225.4226-1-kwolf@redhat.com
Subject: [Qemu-devel] [RFC PATCH 00/10] x-blockdev-create for qcow2
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e
echo "=== ENV ==="
env
echo "=== PACKAGES ==="
rpm -qa
echo "=== TEST BEGIN ==="
INSTALL=$PWD/install
BUILD=$PWD/build
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --prefix=$INSTALL
make -j100
# XXX: we need reliable clean up
# make check -j100 V=1
make install
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1515700960-26388-1-git-send-email-thuth@redhat.com -> patchew/1515700960-26388-1-git-send-email-thuth@redhat.com
 - [tag update]      patchew/20180111183714.22834-2-malbarbo@gmail.com -> patchew/20180111183714.22834-2-malbarbo@gmail.com
 * [new tag]         patchew/20180111195225.4226-1-kwolf@redhat.com -> patchew/20180111195225.4226-1-kwolf@redhat.com
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Submodule 'pixman' (git://anongit.freedesktop.org/pixman) registered for path 'pixman'
Submodule 'roms/SLOF' (git://git.qemu-project.org/SLOF.git) registered for path 'roms/SLOF'
Submodule 'roms/ipxe' (git://git.qemu-project.org/ipxe.git) registered for path 'roms/ipxe'
Submodule 'roms/openbios' (git://git.qemu-project.org/openbios.git) registered for path 'roms/openbios'
Submodule 'roms/openhackware' (git://git.qemu-project.org/openhackware.git) registered for path 'roms/openhackware'
Submodule 'roms/qemu-palcode' (git://github.com/rth7680/qemu-palcode.git) registered for path 'roms/qemu-palcode'
Submodule 'roms/seabios' (git://git.qemu-project.org/seabios.git/) registered for path 'roms/seabios'
Submodule 'roms/sgabios' (git://git.qemu-project.org/sgabios.git) registered for path 'roms/sgabios'
Submodule 'roms/u-boot' (git://git.qemu-project.org/u-boot.git) registered for path 'roms/u-boot'
Submodule 'roms/vgabios' (git://git.qemu-project.org/vgabios.git/) registered for path 'roms/vgabios'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf'
fatal: remote error: access denied or repository not exported: /pixman
Cloning into 'pixman'...
Clone of 'git://anongit.freedesktop.org/pixman' into submodule path 'pixman' failed
Traceback (most recent call last):
  File "/home/patchew/patchew/patchew-cli", line 504, in test_one
    git_clone_repo(clone, r["repo"], r["head"], logf)
  File "/home/patchew/patchew/patchew-cli", line 50, in git_clone_repo
    stderr=logf, stdout=logf)
  File "/usr/lib64/python3.4/subprocess.py", line 558, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'clone', '-q', '--recursive', '/home/patchew/.cache/patchew-git-cache/httpsgithubcompatchewprojectqemu-3c8cf5a9c21ff8782164d1def7f44bd888713384', '/var/tmp/patchew-tester-tmp-ksowqfsy/src']' returned non-zero exit status 1



---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [RFC PATCH 00/10] x-blockdev-create for qcow2

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180111195225.4226-1-kwolf@redhat.com
Subject: [Qemu-devel] [RFC PATCH 00/10] x-blockdev-create for qcow2

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
54f470b74b block: x-blockdev-create QMP command
1aab7a7129 qcow2: Use visitor for options in qcow2_create()
a669aa74c7 util: Add qemu_opts_to_qdict_filtered()
9397f3fd59 qcow2: Handle full/falloc preallocation in qcow2_create2()
f54ee96af1 qcow2: Use QCryptoBlockCreateOptions in qcow2_create2()
b0955f8fa3 qcow2: Use BlockdevRef in qcow2_create2()
583499d48c qcow2: Pass BlockdevCreateOptions to qcow2_create2()
6c4762a900 qcow2: Let qcow2_create() handle protocol layer
715887c079 block/qapi: Add qcow2 create options to schema
84a5b6de96 block/qapi: Introduce BlockdevCreateOptions

=== OUTPUT BEGIN ===
Checking PATCH 1/10: block/qapi: Introduce BlockdevCreateOptions...
Checking PATCH 2/10: block/qapi: Add qcow2 create options to schema...
Checking PATCH 3/10: qcow2: Let qcow2_create() handle protocol layer...
Checking PATCH 4/10: qcow2: Pass BlockdevCreateOptions to qcow2_create2()...
ERROR: space prohibited after that '&' (ctx:WxW)
#283: FILE: block/qcow2.c:3061:
+        .node           = & (BlockdevRef) {
                           ^

total: 1 errors, 0 warnings, 282 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 5/10: qcow2: Use BlockdevRef in qcow2_create2()...
Checking PATCH 6/10: qcow2: Use QCryptoBlockCreateOptions in qcow2_create2()...
Checking PATCH 7/10: qcow2: Handle full/falloc preallocation in qcow2_create2()...
Checking PATCH 8/10: util: Add qemu_opts_to_qdict_filtered()...
Checking PATCH 9/10: qcow2: Use visitor for options in qcow2_create()...
ERROR: trailing whitespace
#301: FILE: tests/qemu-iotests/049.out:109:
+qemu-img: Invalid image size specified! You may use k, M, G, T, P or E suffixes for $

ERROR: trailing whitespace
#310: FILE: tests/qemu-iotests/049.out:119:
+qemu-img: Invalid image size specified! You may use k, M, G, T, P or E suffixes for $

total: 2 errors, 0 warnings, 313 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 10/10: block: x-blockdev-create QMP command...
ERROR: "foo* bar" should be "foo *bar"
#42: FILE: block.c:504:
+    BlockDriver* drv = bdrv_find_format(fmt);

total: 1 errors, 0 warnings, 89 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [RFC PATCH 00/10] x-blockdev-create for qcow2

[Kevin Wolf]

Am 11.01.2018 um 20:52 hat Kevin Wolf geschrieben:
> This series implements a minimal QMP command that allows to create an
> image format on a given block node. The interface is still going to
> change to some kind of an async command (possibly a block job), so I
> prefixed x- for now.
> 
> At this point, I'm mostly interested in comments about
> BlockdevCreateOptions in the schema, the .bdrv_co_create callback and
> the way that legacy .bdrv_create is implemented in qcow2 now.
> 
> It looks to me as if we will have to keep .bdrv_create in addition to
> the new .bdrv_co_create for a while in all drivers, where the
> implementation of .bdrv_create would call .bdrv_co_create like this
> series does it for qcow2. We'll only be able to drop the old interface
> after deprecating and eventually removing all of the driver-specific
> compatibility work that remains. The example of qcow2 shows that the
> "translation" from old to new is managable, but there are a few
> differences.

Peter? Eric? Any opinions so far?

Kevin

Re: [Qemu-devel] [RFC PATCH 00/10] x-blockdev-create for qcow2

[Max Reitz]

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

On 2018-01-11 20:52, Kevin Wolf wrote:
> This series implements a minimal QMP command that allows to create an
> image format on a given block node. The interface is still going to
> change to some kind of an async command (possibly a block job), so I
> prefixed x- for now.
> 
> At this point, I'm mostly interested in comments about
> BlockdevCreateOptions in the schema, the .bdrv_co_create callback and
> the way that legacy .bdrv_create is implemented in qcow2 now.
> 
> It looks to me as if we will have to keep .bdrv_create in addition to
> the new .bdrv_co_create for a while in all drivers, where the
> implementation of .bdrv_create would call .bdrv_co_create like this
> series does it for qcow2. We'll only be able to drop the old interface
> after deprecating and eventually removing all of the driver-specific
> compatibility work that remains. The example of qcow2 shows that the
> "translation" from old to new is managable, but there are a few
> differences.

Looks good. :-)

Max


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