x-blockdev-create for qcow2
cover x-blockdev-create for qcow2
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(-)
Signature · 1 line
2.13.6
RFC PATCH 1/10 block/qapi: Introduce BlockdevCreateOptions
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.
--- qapi/block-core.json | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)
qapi/block-core.json +64 -0
@@ -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
Signature · 1 line
2.13.6
Re: [Qemu-devel] [RFC PATCH 01/10] block/qapi: Introduce BlockdevCreateOptions
Quoted 15 lines · 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 <[email protected]>--- qapi/block-core.json | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)qapi/block-core.json +0
-0@@ -3320,6 +3320,70 @@ { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
qapi/block-core.json line 3330
Quoted 10 lines
qapi/block-core.json +8
-0## +# @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?
qapi/block-core.json line 3338
Quoted 8 lines
qapi/block-core.json +8
-0+ +## +# @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)?
qapi/block-core.json line 3348
Quoted 10 lines
qapi/block-core.json +10
-0+# +# Since: 2.12 +## +{ 'union': 'BlockdevCreateOptions', + 'base': { + 'driver': 'BlockdevDriver', + 'node': 'BlockdevRef' }, + 'discriminator': 'driver', + 'data': { + 'blkdebug': 'BlockdevCreateDummy',
The QAPI itself looks sane for the future patches.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [RFC PATCH 01/10] block/qapi: Introduce BlockdevCreateOptions
Am 16.01.2018 um 19:54 hat Eric Blake geschrieben:
Quoted 29 lines · Kevin Wolf
Quoted 15 lines
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 <[email protected]>--- qapi/block-core.json | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)qapi/block-core.json +0
-0@@ -3320,6 +3320,70 @@ { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }qapi/block-core.json line 3330
Quoted 10 lines
qapi/block-core.json +8
-0## +# @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:
Quoted 6 lines
'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.
Quoted 4 lines
Quoted 4 lines
qapi/block-core.json +4
-0+ +## +# @BlockdevCreateOptions: +#
Quoted 8 lines
qapi/block-core.json line 3338
Quoted 4 lines
qapi/block-core.json +4
-0+# 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 onAny 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.
Quoted 4 lines
Quoted 4 lines
qapi/block-core.json +4
-0+# +# Since: 2.12 +## +{ 'union': 'BlockdevCreateOptions',
Quoted 8 lines
qapi/block-core.json line 3348
Quoted 6 lines
qapi/block-core.json +6
-0+ 'base': { + 'driver': 'BlockdevDriver', + 'node': 'BlockdevRef' }, + 'discriminator': 'driver', + 'data': { + 'blkdebug': 'BlockdevCreateDummy',The QAPI itself looks sane for the future patches.
Kevin
RFC PATCH 2/10 block/qapi: Add qcow2 create options to schema
In reply to Kevin Wolf--- qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)
qapi/block-core.json +32 -1
@@ -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',
Signature · 1 line
2.13.6
Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema
Quoted 27 lines · Kevin Wolf
Signed-off-by Kevin Wolf <[email protected]>--- qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)qapi/block-core.json +14
-0@@ -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.
qapi/block-core.json line 3346
Quoted 10 lines
qapi/block-core.json +10
-0+# +# 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.
qapi/block-core.json line 3351
Quoted 5 lines
qapi/block-core.json +5
-0+ '*encrypt': 'QCryptoBlockCreateOptions', + '*cluster-size': 'size', + '*preallocation': 'PreallocMode', + '*lazy-refcounts': 'bool', + '*refcount-bits': 'int' } }
Regards,
Daniel
Signature · 3 lines
|: 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
Am 12.01.2018 um 11:53 hat Daniel P. Berrange geschrieben:
Quoted 34 lines · Kevin Wolf
Quoted 33 lines
Signed-off-by Kevin Wolf <[email protected]>--- qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)qapi/block-core.json +20
-0@@ -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',
Quoted 8 lines
qapi/block-core.json line 3346
Quoted 4 lines
qapi/block-core.json +4
-0+ '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
Quoted 58 lines · Kevin Wolf
Am 12.01.2018 um 11:53 hat Daniel P. Berrange geschrieben:
Quoted 34 lines · Kevin Wolf
Quoted 33 lines
Signed-off-by Kevin Wolf <[email protected]>--- qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)qapi/block-core.json +20
-0@@ -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',Quoted 8 lines
qapi/block-core.json line 3346
Quoted 4 lines
qapi/block-core.json +4
-0+ '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.
Quoted 2 lines
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
Signature · 3 lines
|: 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
Am 15.01.2018 um 14:51 hat Daniel P. Berrange geschrieben:
Quoted 59 lines · Kevin Wolf
Quoted 58 lines
Am 12.01.2018 um 11:53 hat Daniel P. Berrange geschrieben:
Quoted 34 lines · Kevin Wolf
Quoted 33 lines
Signed-off-by Kevin Wolf <[email protected]>--- qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)qapi/block-core.json +20
-0@@ -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',Quoted 8 lines
qapi/block-core.json line 3346
Quoted 4 lines
qapi/block-core.json +4
-0+ '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.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 4 lines
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?
Quoted 3 lines
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
Quoted 69 lines · Kevin Wolf
Am 15.01.2018 um 14:51 hat Daniel P. Berrange geschrieben:
Quoted 59 lines · Kevin Wolf
Quoted 58 lines
Am 12.01.2018 um 11:53 hat Daniel P. Berrange geschrieben:
Quoted 34 lines · Kevin Wolf
Quoted 33 lines
Signed-off-by Kevin Wolf <[email protected]>--- qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)qapi/block-core.json +20
-0@@ -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',Quoted 8 lines
qapi/block-core.json line 3346
Quoted 4 lines
qapi/block-core.json +4
-0+ '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.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 6 lines
Quoted 3 lines
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
Signature · 3 lines
|: 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
In reply to Kevin WolfQuoted 11 lines · Kevin Wolf
Signed-off-by Kevin Wolf <[email protected]>--- qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)qapi/block-core.json +0
-0@@ -3320,6 +3320,37 @@ { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
qapi/block-core.json line 3330
Quoted 10 lines
qapi/block-core.json +8
-0## +# @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).
qapi/block-core.json line 3338
Quoted 8 lines
qapi/block-core.json +8
-0+ + +## +# @BlockdevCreateOptionsQcow2: +# +# Driver specific image creation options for qcow2. +# +# TODO Describe fields
Hence this being RFC :)
qapi/block-core.json line 3343
Quoted 5 lines
qapi/block-core.json +5
-0+# +# 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.
qapi/block-core.json line 3345
Quoted 2 lines
qapi/block-core.json +2
-0+ '*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?
Quoted 21 lines
qapi/block-core.json +9
-1+ '*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',
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema
Am 16.01.2018 um 19:59 hat Eric Blake geschrieben:
Quoted 13 lines · Kevin Wolf
Quoted 12 lines
Signed-off-by Kevin Wolf <[email protected]>--- qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)qapi/block-core.json +0
-0@@ -3320,6 +3320,37 @@ { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
Quoted 14 lines
qapi/block-core.json line 3330
Quoted 9 lines
qapi/block-core.json +8
-0## +# @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.
Quoted 11 lines
qapi/block-core.json line 3338
Quoted 8 lines
qapi/block-core.json +8
-0+ + +## +# @BlockdevCreateOptionsQcow2: +# +# Driver specific image creation options for qcow2. +# +# TODO Describe fieldsHence this being RFC :)
Quoted 9 lines
qapi/block-core.json line 3343
Quoted 5 lines
qapi/block-core.json +5
-0+# +# 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.
Quoted 6 lines
qapi/block-core.json line 3345
Quoted 2 lines
qapi/block-core.json +2
-0+ '*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.
Quoted 3 lines
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
Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema
Quoted 8 lines · Kevin Wolf
Quoted 7 lines
qapi/block-core.json line 3330
Quoted 2 lines
qapi/block-core.json +2
-0+{ '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).
Quoted 6 lines
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.
Quoted 4 lines
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.
Quoted 12 lines
Quoted 5 lines
Quoted 1 line
+ '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.
Quoted 3 lines
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).
Quoted 9 lines
Quoted 7 lines
qapi/block-core.json line 3345
Quoted 3 lines
qapi/block-core.json +2
-0+ '*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).
Quoted 7 lines
Quoted 4 lines
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.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema
In reply to Kevin WolfQuoted 11 lines · Kevin Wolf
Signed-off-by Kevin Wolf <[email protected]>--- qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)qapi/block-core.json +0
-0@@ -3320,6 +3320,37 @@ { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
qapi/block-core.json line 3330
Quoted 10 lines
qapi/block-core.json +8
-0## +# @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
Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema
Am 29.01.2018 um 17:57 hat Max Reitz geschrieben:
Quoted 13 lines · Kevin Wolf
Quoted 12 lines
Signed-off-by Kevin Wolf <[email protected]>--- qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)qapi/block-core.json +0
-0@@ -3320,6 +3320,37 @@ { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
Quoted 13 lines
qapi/block-core.json line 3330
Quoted 9 lines
qapi/block-core.json +8
-0## +# @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
Re: [Qemu-devel] [RFC PATCH 02/10] block/qapi: Add qcow2 create options to schema
Quoted 28 lines · Kevin Wolf
Am 29.01.2018 um 17:57 hat Max Reitz geschrieben:
Quoted 13 lines · Kevin Wolf
Quoted 12 lines
Signed-off-by Kevin Wolf <[email protected]>--- qapi/block-core.json | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)qapi/block-core.json +0
-0@@ -3320,6 +3320,37 @@ { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }Quoted 13 lines
qapi/block-core.json line 3330
Quoted 9 lines
qapi/block-core.json +8
-0## +# @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.
Quoted 3 lines
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.
Quoted 1 line
An enum with 'v2' and 'v3' then?
Would work for me.
Max
RFC PATCH 3/10 qcow2: Let qcow2_create() handle protocol layer
In reply to Kevin WolfCurrently, 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.
--- block/qcow2.c | 64 +++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 38 insertions(+), 26 deletions(-)
block/qcow2.c +38 -26
@@ -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);
Signature · 1 line
2.13.6
Re: [Qemu-devel] [RFC PATCH 03/10] qcow2: Let qcow2_create() handle protocol layer
Quoted 23 lines · Kevin Wolf
Currently,
qcow2_create()only parses the QemuOpts and then callsqcow2_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()intoqcow2_create(). This means thatqcow2_create2()doesn't need a file name any more.Signed-off-by Kevin Wolf <[email protected]>--- block/qcow2.c | 64 +++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 38 insertions(+), 26 deletions(-)block/qcow2.c +0
-0
block/qcow2.c static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version, · line 2693
Quoted 5 lines
block/qcow2.c +1
-1@@ -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.
block/qcow2.c static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp) · line 2951
Quoted 6 lines
block/qcow2.c +1
-1@@ -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,
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
RFC PATCH 4/10 qcow2: Pass BlockdevCreateOptions to qcow2_create2()
In reply to Kevin WolfAll of the simple options are now passed to qcow2_create2() in a BlockdevCreateOptions object. Still missing: node-name and the encryption options.
--- block/qcow2.c | 186 ++++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 148 insertions(+), 38 deletions(-)
block/qcow2.c +148 -38
@@ -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; }
Signature · 1 line
2.13.6
Re: [Qemu-devel] [RFC PATCH 04/10] qcow2: Pass BlockdevCreateOptions to qcow2_create2()
Quoted 10 lines · 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 <[email protected]>--- block/qcow2.c | 186 ++++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 148 insertions(+), 38 deletions(-)
block/qcow2.c static int qcow2_create2(BlockDriverState *bs, int64_t total_size, · line 2779
Quoted 6 lines
block/qcow2.c +6
-0+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?
block/qcow2.c static int qcow2_create2(BlockDriverState *bs, int64_t total_size, · line 2903
Quoted 10 lines
/* Want a backing file? There you go.*/block/qcow2.c +6
-2-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)?
Quoted 12 lines
block/qcow2.c +5
-3@@ -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,
block/qcow2.c static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp) · line 3073
Quoted 10 lines
block/qcow2.c +10
-0+ }, + .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.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [RFC PATCH 04/10] qcow2: Pass BlockdevCreateOptions to qcow2_create2()
In reply to Kevin WolfQuoted 13 lines · 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 <[email protected]>--- block/qcow2.c | 186 ++++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 148 insertions(+), 38 deletions(-)block/qcow2.c +0
-0
[...]
block/qcow2.c static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version, · line 2701
Quoted 11 lines
block/qcow2.c +2
-5@@ -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...
Quoted 7 lines
block/qcow2.c +2
-0+ 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,
[...]
Quoted 4 lines
block/qcow2.c +4
-0+ +if (!qcow2_opts->has_preallocation) { + qcow2_opts->preallocation =PREALLOC_MODE_OFF ; + }
block/qcow2.c static int qcow2_create2(BlockDriverState *bs, int64_t total_size, · line 2775
Quoted 10 lines
block/qcow2.c +10
-0+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.
[...]
Quoted 1 line
block/qcow2.c +0
-0@@ -2804,18 +2888,26 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,
[...]
block/qcow2.c static int qcow2_create2(BlockDriverState *bs, int64_t total_size, · line 2903
Quoted 9 lines
/* Want a backing file? There you go.*/block/qcow2.c +6
-2-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
Quoted 11 lines
block/qcow2.c +5
-1+ + 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; } }
Re: [Qemu-devel] [RFC PATCH 04/10] qcow2: Pass BlockdevCreateOptions to qcow2_create2()
Am 29.01.2018 um 18:12 hat Max Reitz geschrieben:
Quoted 41 lines · Kevin Wolf
Quoted 13 lines
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 <[email protected]>--- block/qcow2.c | 186 ++++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 148 insertions(+), 38 deletions(-)block/qcow2.c +0
-0[...]
block/qcow2.c static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version, · line 2701
Quoted 11 lines
block/qcow2.c +2
-5@@ -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...
Quoted 7 lines
block/qcow2.c +2
-0+ 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,[...]
Quoted 16 lines
Quoted 4 lines
block/qcow2.c +4
-0+ +if (!qcow2_opts->has_preallocation) { + qcow2_opts->preallocation =PREALLOC_MODE_OFF ; + }block/qcow2.c static int qcow2_create2(BlockDriverState *bs, int64_t total_size, · line 2775
Quoted 10 lines
block/qcow2.c +10
-0+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.
Quoted 16 lines
[...]
Quoted 1 line
block/qcow2.c +0
-0@@ -2804,18 +2888,26 @@ static int qcow2_create2(BlockDriverState *bs, int64_t total_size,[...]
block/qcow2.c static int qcow2_create2(BlockDriverState *bs, int64_t total_size, · line 2903
Quoted 9 lines
/* Want a backing file? There you go.*/block/qcow2.c +6
-2-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); + }
Quoted 1 line
has_backing_fmt&& !has_backing_file should probably be an error.
Yes.
Kevin
Re: [Qemu-devel] [RFC PATCH 04/10] qcow2: Pass BlockdevCreateOptions to qcow2_create2()
Quoted 59 lines · Kevin Wolf
Am 29.01.2018 um 18:12 hat Max Reitz geschrieben:
Quoted 41 lines · Kevin Wolf
Quoted 13 lines
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 <[email protected]>--- block/qcow2.c | 186 ++++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 148 insertions(+), 38 deletions(-)block/qcow2.c +0
-0[...]
block/qcow2.c static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version, · line 2701
Quoted 11 lines
block/qcow2.c +2
-5@@ -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...
Quoted 7 lines
block/qcow2.c +2
-0+ 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,[...]
Quoted 16 lines
Quoted 4 lines
block/qcow2.c +4
-0+ +if (!qcow2_opts->has_preallocation) { + qcow2_opts->preallocation =PREALLOC_MODE_OFF ; + }block/qcow2.c static int qcow2_create2(BlockDriverState *bs, int64_t total_size, · line 2775
Quoted 10 lines
block/qcow2.c +10
-0+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.
Quoted 4 lines
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()orqmp_drive_mirror(), they do the same.
:C
RFC PATCH 5/10 qcow2: Use BlockdevRef in qcow2_create2()
In reply to Kevin WolfInstead of passing a separate BlockDriverState* into qcow2_create2(), make use of the BlockdevRef that is included in BlockdevCreateOptions.
--- include/block/block.h | 1 + block.c | 39 +++++++++++++++++++++++++++++++++++++++ block/qcow2.c | 33 +++++++++++++++++++++------------ 3 files changed, 61 insertions(+), 12 deletions(-)
include/block/block.h +1 -0
@@ -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,
block.c +39 -0
@@ -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,
block/qcow2.c +21 -12
@@ -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; }
Signature · 1 line
2.13.6
Re: [Qemu-devel] [RFC PATCH 05/10] qcow2: Use BlockdevRef in qcow2_create2()
Quoted 10 lines · 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 <[email protected]>--- include/block/block.h | 1 + block.c | 39 +++++++++++++++++++++++++++++++++++++++ block/qcow2.c | 33 +++++++++++++++++++++------------ 3 files changed, 61 insertions(+), 12 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [RFC PATCH 05/10] qcow2: Use BlockdevRef in qcow2_create2()
In reply to Kevin WolfQuoted 9 lines · 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 <[email protected]>--- include/block/block.h | 1 + block.c | 39 +++++++++++++++++++++++++++++++++++++++ block/qcow2.c | 33 +++++++++++++++++++++------------ 3 files changed, 61 insertions(+), 12 deletions(-)
[...]
Quoted 4 lines
block.c +0
-0
[...]
Quoted 27 lines
block.c +23
-0@@ -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; + }
block.c BdrvChild *bdrv_open_child(const char *filename, · line 2442
Quoted 10 lines
block.c +10
-0+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
Quoted 7 lines
block.c +4
-0+visit_free (v); +return bs; +} + static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, QDict *snapshot_options,
Re: [Qemu-devel] [RFC PATCH 05/10] qcow2: Use BlockdevRef in qcow2_create2()
Am 29.01.2018 um 18:30 hat Max Reitz geschrieben:
Quoted 53 lines · Kevin Wolf
Quoted 9 lines
Instead of passing a separate BlockDriverState* into
qcow2_create2(), make use of the BlockdevRef that is included in BlockdevCreateOptions.Signed-off-by Kevin Wolf <[email protected]>--- include/block/block.h | 1 + block.c | 39 +++++++++++++++++++++++++++++++++++++++ block/qcow2.c | 33 +++++++++++++++++++++------------ 3 files changed, 61 insertions(+), 12 deletions(-)[...]
Quoted 4 lines
block.c +0
-0[...]
Quoted 33 lines
block.c +29
-0@@ -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); + } +
Quoted 8 lines
block.c BdrvChild *bdrv_open_child(const char *filename, · line 2442
Quoted 4 lines
block.c +4
-0+ 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.
Quoted 1 line
Also, doesn't
bdrv_open_inherit()take ownership of @qdict and thus @obj?
That's a very good point...
Kevin
Quoted 10 lines
Max
Quoted 7 lines
block.c +4
-0+visit_free (v); +return bs; +} + static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, QDict *snapshot_options,
RFC PATCH 6/10 qcow2: Use QCryptoBlockCreateOptions in qcow2_create2()
In reply to Kevin WolfInstead of passing the encryption format name and the QemuOpts down, use the QCryptoBlockCreateOptions contained in BlockdevCreateOptions.
--- block/qcow2.c | 62 +++++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 17 deletions(-)
block/qcow2.c +45 -17
@@ -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);
Signature · 1 line
2.13.6
Re: [Qemu-devel] [RFC PATCH 06/10] qcow2: Use QCryptoBlockCreateOptions in qcow2_create2()
Quoted 8 lines · Kevin Wolf
Instead of passing the encryption format name and the QemuOpts down, use the QCryptoBlockCreateOptions contained in BlockdevCreateOptions.
Signed-off-by Kevin Wolf <[email protected]>--- block/qcow2.c | 62 +++++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 17 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
RFC PATCH 7/10 qcow2: Handle full/falloc preallocation in qcow2_create2()
In reply to Kevin WolfOnce 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.
--- block/qcow2.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-)
block/qcow2.c +19 -9
@@ -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;
Signature · 1 line
2.13.6
Re: [Qemu-devel] [RFC PATCH 07/10] qcow2: Handle full/falloc preallocation in qcow2_create2()
Quoted 10 lines · 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 <[email protected]>--- block/qcow2.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
RFC PATCH 8/10 util: Add qemu_opts_to_qdict_filtered()
In reply to Kevin WolfThis 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.
--- include/qemu/option.h | 2 ++ util/qemu-option.c | 28 +++++++++++++++++++++++++--- 2 files changed, 27 insertions(+), 3 deletions(-)
include/qemu/option.h +2 -0
@@ -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);
util/qemu-option.c +25 -3
@@ -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. */
Signature · 1 line
2.13.6
Re: [Qemu-devel] [RFC PATCH 08/10] util: Add qemu_opts_to_qdict_filtered()
Quoted 4 lines · 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.
Someday it would be nice to not have to rely so much on QemuOpts. But in the meantime, this glue makes sense.
Quoted 7 lines
Signed-off-by Kevin Wolf <[email protected]>--- 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.
util/qemu-option.c void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp) · line 1011
Quoted 5 lines
+++
b/util/qemu-option.cutil/qemu-option.c +0
-0@@ -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...
util/qemu-option.c void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp)
Quoted 12 lines
util/qemu-option.c +4
-2-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?
Quoted 8 lines
util/qemu-option.c +4
-0+{ +return qemu_opts_to_qdict_filtered (opts, qdict,NULL , false); +} +/* Validate parsed opts against descriptions where no * descriptions were provided in the QemuOptsList. */
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
RFC PATCH 9/10 qcow2: Use visitor for options in qcow2_create()
In reply to Kevin Wolf--- block/qcow2.c | 227 ++++++++++++++++++--------------------------- tests/qemu-iotests/049.out | 10 +- 2 files changed, 93 insertions(+), 144 deletions(-)
block/qcow2.c +88 -139
@@ -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; }
tests/qemu-iotests/049.out +5 -5
@@ -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 ==
Signature · 1 line
2.13.6
Re: [Qemu-devel] [RFC PATCH 09/10] qcow2: Use visitor for options in qcow2_create()
Quoted 8 lines · Kevin Wolf
Signed-off-by Kevin Wolf <[email protected]>--- block/qcow2.c | 227 ++++++++++++++++++--------------------------- tests/qemu-iotests/049.out | 10 +- 2 files changed, 93 insertions(+), 144 deletions(-)
block/qcow2.c out: · line 2989
Quoted 6 lines
block/qcow2.c +6
-0+/* 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!
block/qcow2.c out: · line 3010
Quoted 8 lines
block/qcow2.c +8
-0+ +/* 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'?)
Quoted 5 lines
block/qcow2.c +5
-0+/* Now get the QAPI type BlockdevCreateOptions */ + qobj =qdict_crumple (qdict, errp); +QDECREF (qdict); + qdict =qobject_to_qdict (qobj); +if (qdict ==NULL ) {
block/qcow2.c out: · line 3070
Quoted 10 lines
block/qcow2.c +3
-3ret = -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.
tests/qemu-iotests/049.out qemu-img: Value '-1k' is out of range for parameter 'size' · line 109
Quoted 7 lines
+++ b/tests/qemu-iotests/049.out
tests/qemu-iotests/049.out +1
-1@@ -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.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
RFC PATCH 10/10 block: x-blockdev-create QMP command
In reply to Kevin WolfThis 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.
--- 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(-)
qapi/block-core.json +12 -0
@@ -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
include/block/block_int.h +2 -0
@@ -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);
block.c +48 -0
54 lines not on this page · raw message
block/qcow2.c +2 -1
@@ -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,
Signature · 1 line
2.13.6
Re: [Qemu-devel] [RFC PATCH 10/10] block: x-blockdev-create QMP command
Quoted 26 lines · 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 <[email protected]>--- 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(-)qapi/block-core.json +1
-0@@ -3415,6 +3415,18 @@ } } ## +# @x-blockdev-create:
qapi/block-core.json line 3428
Quoted 10 lines
qapi/block-core.json +10
-0+# +# 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.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [RFC PATCH 10/10] block: x-blockdev-create QMP command
In reply to Kevin WolfAm 11.01.2018 um 20:52 hat Kevin Wolf geschrieben:
Quoted 22 lines
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 <[email protected]>--- 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(-)qapi/block-core.json +0
-0
qapi/block-core.json line 3421
Quoted 7 lines
qapi/block-core.json +4
-0@@ -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
This series failed build test on ppc host. Please find the details below.
Hi,
This series failed build test on ppc host. Please find the details below.
Message-id: [email protected]
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/[email protected] -> patchew/[email protected] - [tag update] patchew/[email protected] -> patchew/[email protected] * [new tag] patchew/[email protected] -> patchew/[email protected]
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 [email protected]
This series seems to have some coding style problems. See output below for more information:
Hi,
This series seems to have some coding style problems. See output below for more information:
Type: series
Message-id: [email protected]
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 command1aab7a7129 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 [email protected]
Am 11.01.2018 um 20:52 hat Kevin Wolf geschrieben:
Quoted 9 lines
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.
Quoted 8 lines
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
Quoted 9 lines · 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.
Quoted 8 lines
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