diff mbox

[V2,2/8] qapi: add compress parameters to Qcow2 Blockdev options

Message ID 1498733831-15254-3-git-send-email-pl@kamp.de (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Lieven June 29, 2017, 10:57 a.m. UTC
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

Comments

Kevin Wolf July 10, 2017, 1:10 p.m. UTC | #1
Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)

How does this make sense as a runtime option? What would happen if the
image contains one compression format and I specify another one on the
command line or in blockdev-add?

Shouldn't it just be a create-time option and when you run qemu, it uses
whatever format that image has?

Kevin
Peter Lieven July 10, 2017, 1:24 p.m. UTC | #2
Am 10.07.2017 um 15:10 schrieb Kevin Wolf:
> Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 43 insertions(+), 1 deletion(-)
> How does this make sense as a runtime option? What would happen if the
> image contains one compression format and I specify another one on the
> command line or in blockdev-add?
>
> Shouldn't it just be a create-time option and when you run qemu, it uses
> whatever format that image has?

I was asked to add it here. It is indeed only a create option and has no other effect.

Peter
Daniel P. Berrangé July 10, 2017, 1:27 p.m. UTC | #3
On Mon, Jul 10, 2017 at 03:24:02PM +0200, Peter Lieven wrote:
> Am 10.07.2017 um 15:10 schrieb Kevin Wolf:
> > Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> > > Signed-off-by: Peter Lieven <pl@kamp.de>
> > > ---
> > >   qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 43 insertions(+), 1 deletion(-)
> > How does this make sense as a runtime option? What would happen if the
> > image contains one compression format and I specify another one on the
> > command line or in blockdev-add?
> > 
> > Shouldn't it just be a create-time option and when you run qemu, it uses
> > whatever format that image has?
> 
> I was asked to add it here. It is indeed only a create option and has no
> other effect.

I think there was some mis-understanding. I suggested adding QAPI schema
to define the options needed during create, and then use QAPI functions
when processing the create. In patch 4 you've just used the traditional
qemu opts functions to query the parameters, so there's no validation
that what's being processed actually matches the QAPI schema. The QAPI
schema additions for create would need to be defined indepenantly of
the runtime options too.

Regards,
Daniel
Kevin Wolf July 10, 2017, 1:30 p.m. UTC | #4
Am 10.07.2017 um 15:24 hat Peter Lieven geschrieben:
> Am 10.07.2017 um 15:10 schrieb Kevin Wolf:
> >Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> >>Signed-off-by: Peter Lieven <pl@kamp.de>
> >>---
> >>  qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 43 insertions(+), 1 deletion(-)
> >How does this make sense as a runtime option? What would happen if the
> >image contains one compression format and I specify another one on the
> >command line or in blockdev-add?
> >
> >Shouldn't it just be a create-time option and when you run qemu, it uses
> >whatever format that image has?
> 
> I was asked to add it here. It is indeed only a create option and has
> no other effect.

But if it's only a create option, why is it added to blockdev-add?

Kevin
Peter Lieven July 10, 2017, 1:32 p.m. UTC | #5
Am 10.07.2017 um 15:30 schrieb Kevin Wolf:
> Am 10.07.2017 um 15:24 hat Peter Lieven geschrieben:
>> Am 10.07.2017 um 15:10 schrieb Kevin Wolf:
>>> Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>>   qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
>>>>   1 file changed, 43 insertions(+), 1 deletion(-)
>>> How does this make sense as a runtime option? What would happen if the
>>> image contains one compression format and I specify another one on the
>>> command line or in blockdev-add?
>>>
>>> Shouldn't it just be a create-time option and when you run qemu, it uses
>>> whatever format that image has?
>> I was asked to add it here. It is indeed only a create option and has
>> no other effect.
> But if it's only a create option, why is it added to blockdev-add?

As Daniel wrote its maybe a misunderstanding. He pointed me to the crypto extension
that was recently added and the structs there. As I said I might need help with the
QAPI stuff...

Peter
Peter Lieven July 13, 2017, 8:45 a.m. UTC | #6
Am 10.07.2017 um 15:30 schrieb Kevin Wolf:
> Am 10.07.2017 um 15:24 hat Peter Lieven geschrieben:
>> Am 10.07.2017 um 15:10 schrieb Kevin Wolf:
>>> Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>>   qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
>>>>   1 file changed, 43 insertions(+), 1 deletion(-)
>>> How does this make sense as a runtime option? What would happen if the
>>> image contains one compression format and I specify another one on the
>>> command line or in blockdev-add?
>>>
>>> Shouldn't it just be a create-time option and when you run qemu, it uses
>>> whatever format that image has?
>> I was asked to add it here. It is indeed only a create option and has
>> no other effect.
> But if it's only a create option, why is it added to blockdev-add?

Is there already a separate schemata for the create options?

Thanks,
Peter
Kevin Wolf July 13, 2017, 8:52 a.m. UTC | #7
Am 13.07.2017 um 10:45 hat Peter Lieven geschrieben:
> Am 10.07.2017 um 15:30 schrieb Kevin Wolf:
> >Am 10.07.2017 um 15:24 hat Peter Lieven geschrieben:
> >>Am 10.07.2017 um 15:10 schrieb Kevin Wolf:
> >>>Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> >>>>Signed-off-by: Peter Lieven <pl@kamp.de>
> >>>>---
> >>>>  qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
> >>>>  1 file changed, 43 insertions(+), 1 deletion(-)
> >>>How does this make sense as a runtime option? What would happen if the
> >>>image contains one compression format and I specify another one on the
> >>>command line or in blockdev-add?
> >>>
> >>>Shouldn't it just be a create-time option and when you run qemu, it uses
> >>>whatever format that image has?
> >>I was asked to add it here. It is indeed only a create option and has
> >>no other effect.
> >But if it's only a create option, why is it added to blockdev-add?
> 
> Is there already a separate schemata for the create options?

If you mean a separate schema file, no. Putting it in the same file
isn't a problem either, but hooking it up in structs used by
blockdev-add (or other QMP commands) probably is.

So if you want to follow Dan's suggestion, I think you just need to put
the new type into the schema, without referencing it anywhere except in
qcow2's .bdrv_create C implementation.

Kevin
Daniel P. Berrangé July 13, 2017, 9:18 a.m. UTC | #8
On Thu, Jul 13, 2017 at 10:52:41AM +0200, Kevin Wolf wrote:
> Am 13.07.2017 um 10:45 hat Peter Lieven geschrieben:
> > Am 10.07.2017 um 15:30 schrieb Kevin Wolf:
> > >Am 10.07.2017 um 15:24 hat Peter Lieven geschrieben:
> > >>Am 10.07.2017 um 15:10 schrieb Kevin Wolf:
> > >>>Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
> > >>>>Signed-off-by: Peter Lieven <pl@kamp.de>
> > >>>>---
> > >>>>  qapi/block-core.json | 44 +++++++++++++++++++++++++++++++++++++++++++-
> > >>>>  1 file changed, 43 insertions(+), 1 deletion(-)
> > >>>How does this make sense as a runtime option? What would happen if the
> > >>>image contains one compression format and I specify another one on the
> > >>>command line or in blockdev-add?
> > >>>
> > >>>Shouldn't it just be a create-time option and when you run qemu, it uses
> > >>>whatever format that image has?
> > >>I was asked to add it here. It is indeed only a create option and has
> > >>no other effect.
> > >But if it's only a create option, why is it added to blockdev-add?
> > 
> > Is there already a separate schemata for the create options?
> 
> If you mean a separate schema file, no. Putting it in the same file
> isn't a problem either, but hooking it up in structs used by
> blockdev-add (or other QMP commands) probably is.
> 
> So if you want to follow Dan's suggestion, I think you just need to put
> the new type into the schema, without referencing it anywhere except in
> qcow2's .bdrv_create C implementation.

Yep, that's exactly what I intended by my suggestion

Regards,
Daniel
diff mbox

Patch

diff --git a/qapi/block-core.json b/qapi/block-core.json
index f85c223..1574ffb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2282,6 +2282,43 @@ 
             'mode':  'Qcow2OverlapCheckMode' } }
 
 ##
+# @Qcow2CompressFormat:
+# @zlib: standard zlib deflate compression
+#
+# Since: 2.10
+##
+{ 'enum': 'Qcow2CompressFormat',
+  'data': [ 'zlib' ] }
+
+##
+# @Qcow2CompressZLib:
+#
+# Since: 2.10
+##
+{ 'struct': 'Qcow2CompressZLib',
+  'data': { } }
+
+##
+# @Qcow2Compress:
+#
+# Specifies the compression format and compression level that should
+# be used for compressed Qcow2 clusters.
+#
+# @format: specifies the compression format to use. (defaults to zlib)
+#
+# @level: specifies the compression level. 0 = default compression,
+#         1 = fastest compression, x = highest compresion (x may very between
+#         different compression formats)
+#
+# Since: 2.10
+##
+{ 'union': 'Qcow2Compress',
+  'base': { 'format': 'Qcow2CompressFormat',
+            'level': 'uint8' },
+  'discriminator': 'format',
+  'data': { 'zlib': 'Qcow2CompressZLib' } }
+
+##
 # @BlockdevOptionsQcow2:
 #
 # Driver specific block device options for qcow2.
@@ -2316,6 +2353,10 @@ 
 #                         caches. The interval is in seconds. The default value
 #                         is 0 and it disables this feature (since 2.5)
 #
+# @compress:              which format and compression level to use for
+#                         compressed clusters. Defaults to zlib with default
+#                         compression level (since 2.10)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsQcow2',
@@ -2328,7 +2369,8 @@ 
             '*cache-size': 'int',
             '*l2-cache-size': 'int',
             '*refcount-cache-size': 'int',
-            '*cache-clean-interval': 'int' } }
+            '*cache-clean-interval': 'int',
+            '*compress': 'Qcow2Compress' } }
 
 
 ##