qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH V2 5/8] block/qcow2: read and write the compress


From: Peter Lieven
Subject: Re: [Qemu-block] [PATCH V2 5/8] block/qcow2: read and write the compress format extension
Date: Thu, 13 Jul 2017 16:18:13 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1

Am 13.07.2017 um 16:07 schrieb Daniel P. Berrange:
On Thu, Jul 13, 2017 at 04:03:47PM +0200, Peter Lieven wrote:
Am 13.07.2017 um 16:00 schrieb Daniel P. Berrange:
On Thu, Jul 13, 2017 at 03:49:09PM +0200, Peter Lieven wrote:
Am 13.07.2017 um 11:21 schrieb Daniel P. Berrange:
On Thu, Jul 13, 2017 at 10:44:53AM +0200, Peter Lieven wrote:
Am 10.07.2017 um 15:55 schrieb Daniel P. Berrange:
On Mon, Jul 10, 2017 at 03:52:18PM +0200, Kevin Wolf wrote:
Am 10.07.2017 um 15:44 hat Daniel P. Berrange geschrieben:
On Mon, Jul 10, 2017 at 03:34:59PM +0200, Kevin Wolf wrote:
Am 10.07.2017 um 15:29 hat Peter Lieven geschrieben:
Am 10.07.2017 um 15:25 schrieb Kevin Wolf:
Am 29.06.2017 um 12:57 hat Peter Lieven geschrieben:
we now read the extension on open and write it on update, but
do not yet use it.

Signed-off-by: Peter Lieven <address@hidden>
---
     block/qcow2.c | 100 
++++++++++++++++++++++++++++++++++++++++++++++++++--------
     block/qcow2.h |  23 +++++++++++---
     2 files changed, 104 insertions(+), 19 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 308121a..39a8afc 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -63,6 +63,7 @@ typedef struct {
     #define  QCOW2_EXT_MAGIC_END 0
     #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
     #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
+#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
     static int qcow2_probe(const uint8_t *buf, int buf_size, const char 
*filename)
     {
@@ -76,6 +77,26 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, 
const char *filename)
             return 0;
     }
+static int qcow2_compress_format_from_name(char *fmt)
+{
+    if (!fmt || !fmt[0]) {
+        return QCOW2_COMPRESS_ZLIB_COMPAT;
+    } else if (g_str_equal(fmt, "zlib")) {
+        return QCOW2_COMPRESS_ZLIB;
+    } else {
+        return -EINVAL;
+    }
+}
It might make sense to allow specifying the old compression format in
the compression header. But I'm not so sure about using the empty string
rather than a proper name for it, and even less about not documenting
it.
The old format is used if and only if the compression header is absent.
It makes no sense to write a header and use the old format. The old
settings are suboptimal and old versions can't open a qcow2 with a
compression header anyway.
Your code allows an empty string in the header extension. If you don't
want it there, you need to reject it.
This is a good reason to use the QAPI schema to parse the create options
instead of doing it using qemu_opts - the QAPI schema will generate correct
code to parse & validate enum values
I'd really like to see QAPI used for bdrv_create(), but here we're in
the bdrv_open() path and parsing qcow2 headers. So either way this one
specifically wouldn't be fixed.
Sorry, yes, I was getting mixed up in the two different patches with
similar bits of code.

We could still replace the qcow2_compress_format_from_name() method
though with a call to the qapi_enum_parse() passing in the
'QcowCompressFormat_lookup' table.
Can you advise how exactly I would do that? Sorry, but this is new stuff
for me.
In your line of code:

             s->compress_format_id =
                  qcow2_compress_format_from_name(s->compress_format.name);

Instead do

              s->compress_format_id =
                  qapi_enum_parse(QcowCompressFormat_lookup, 
s->compress_format.name);


The 'QcowCompressFormat_lookup' symbol is a global variable that is an
array of strings, automatically created from the 'enum' you define in
the QAPI schema.
Is it possible to limit the valid integer values for an integer in the QAPI 
specification?

I would like to do that for the compress.level stuff.
Not that I know of.
If I specify an enum, what values are assigned to them? I am thinking of
specifying an enum {0, 1, 2, 3, 4, 5, 6, 7, 8, 9} for the valid
compress levels of zlib.
It should assume enum values starting from zero, but I think that is not
considered ABI from QAPI POV. IOW, you shouldn't persist the enum values
anywhere - only the enum string representation is considered ABI stable.

Also you can't have enum string names starting with a digit. So you would
need enum {level0, level1, level2, etc.... }, and store the string
'level0', etc in the qcow2 header if you went this way. I think this is
kind of ugly compared to just using an int and doing range checks at time
of use.

Okay, so it has to be a mix of QAPI parsing and manual parameter checking, 
right?

I currently have the following:

    options = qemu_opts_to_qdict(opts, NULL);
    qdict_extract_subqdict(options, &compressopts, "compress.");
    v = qobject_input_visitor_new_keyval(QOBJECT(compressopts));
    visit_start_struct(v, NULL, NULL, 0, &local_err);
    if (local_err) {
        ret= -EINVAL;
        goto finish;
    }
    visit_type_Qcow2Compress_members(v, &compress, &local_err);
    if (local_err) {
        ret= -EINVAL;
        goto finish;
    }
    visit_end_struct(v, NULL);
    visit_free(v);
    QDECREF(compressopts);
    QDECREF(options);

And I have the following 2 questions:
 a) I have to specifiy compress.format and compress.level otherwise I will get 
an error. How can I fix that the settings are optional?
 b) If I just specify a compress.format can I default the compress.level to 0 
without an error?

Thanks,
Peter




reply via email to

[Prev in Thread] Current Thread [Next in Thread]