| Message ID | 1498733831-15254-6-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
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 <pl@kamp.de> > --- > 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. Kevin
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 <pl@kamp.de> >> --- >> 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. Peter
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 <pl@kamp.de> > >>--- > >> 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. Kevin
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 <pl@kamp.de> > > >>--- > > >> 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 Regards, Daniel
Am 10.07.2017 um 15:44 schrieb Daniel P. Berrange: > 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 <pl@kamp.de> >>>>> --- >>>>> 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 Can you then please advise where to add the schematas and which functions to use for parsing it? You wrote that it is a bit hackish at the moment as not everything is available in qcow2_create path. Thank you, Peter
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 <pl@kamp.de> > > > >>--- > > > >> 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. Kevin
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 <pl@kamp.de> > > > > >>--- > > > > >> 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. Regards, Daniel
On Mon, Jul 10, 2017 at 03:46:21PM +0200, Peter Lieven wrote: > Am 10.07.2017 um 15:44 schrieb Daniel P. Berrange: > > 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 <pl@kamp.de> > > > > > > --- > > > > > > 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 > > Can you then please advise where to add the schematas and which functions > to use for parsing it? You wrote that it is a bit hackish at the moment as not > everything is available in qcow2_create path. I just sent an (untested) example for patch 3 that should help. If you still have problems let me know. Regards, Daniel
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 <pl@kamp.de> >>>>>>> --- >>>>>>> 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. Thanks, Peter
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 <pl@kamp.de> > > > > > > > > --- > > > > > > > > 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. Regards, Daniel
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 <pl@kamp.de> >>>>>>>>> --- >>>>>>>>> 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. Thanks, Peter
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 <pl@kamp.de> > > > > > > > > > > --- > > > > > > > > > > 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. Regards, Daniel
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 <pl@kamp.de> >>>>>>>>>>> --- >>>>>>>>>>> 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. Peter
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 <pl@kamp.de> > > > > > > > > > > > > --- > > > > > > > > > > > > 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. Regards, Daniel
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 <pl@kamp.de> >>>>>>>>>>>>> --- >>>>>>>>>>>>> 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
On Thu, Jul 13, 2017 at 04:18:13PM +0200, Peter Lieven wrote: > > Okay, so it has to be a mix of QAPI parsing and manual parameter checking, > right? Yeah. It does feel like a valid RFE for QAPI to add a permitted range to 'int' types though, which would simplify the code in future. > 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) Looks good. > 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? Put an '*' as the first character of any field name if it should be optional. > b) If I just specify a compress.format can I default the compress.level to 0 without an error? I believe you'd get compress.level as 0 automatically for an 'int' type. Regards, Daniel
Am 13.07.2017 um 16:58 schrieb Daniel P. Berrange: > On Thu, Jul 13, 2017 at 04:18:13PM +0200, Peter Lieven wrote: >> Okay, so it has to be a mix of QAPI parsing and manual parameter checking, >> right? > Yeah. It does feel like a valid RFE for QAPI to add a permitted range to > 'int' types though, which would simplify the code in future. > >> 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) > Looks good. > >> 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? > Put an '*' as the first character of any field name if it should be optional. > >> b) If I just specify a compress.format can I default the compress.level to 0 without an error? > I believe you'd get compress.level as 0 automatically for an 'int' type. I still face the issue that I now always have to specify a compress.format. I tried to solve it like this: int compress_format = -1; uint8_t compress_level = 0; if (qemu_opt_find(opts, BLOCK_OPT_COMPRESS_FORMAT) || qemu_opt_find(opts, BLOCK_OPT_COMPRESS_LEVEL)) { QDict *options, *compressopts = NULL; Qcow2Compress compress; Visitor *v; 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) { error_propagate(errp, local_err); ret = -EINVAL; goto finish; } visit_type_Qcow2Compress_members(v, &compress, &local_err); if (local_err) { error_propagate(errp, local_err); ret = -EINVAL; goto finish; } visit_end_struct(v, NULL); visit_free(v); QDECREF(compressopts); QDECREF(options); compress_format = compress.format; compress_level = compress.level; if (compress_format == QCOW2_COMPRESS_FORMAT_ZLIB && compress_level > 9) { error_setg(errp, "Compress level %" PRIu8 " is not supported for" " format '%s'", compress_level, qemu_opt_get(opts, BLOCK_OPT_COMPRESS_FORMAT)); ret = -EINVAL; goto finish; } } Maybe there is a better option? Peter
On Thu, Jul 13, 2017 at 05:00:39PM +0200, Peter Lieven wrote: > Am 13.07.2017 um 16:58 schrieb Daniel P. Berrange: > > On Thu, Jul 13, 2017 at 04:18:13PM +0200, Peter Lieven wrote: > > > Okay, so it has to be a mix of QAPI parsing and manual parameter checking, > > > right? > > Yeah. It does feel like a valid RFE for QAPI to add a permitted range to > > 'int' types though, which would simplify the code in future. > > > > > 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) > > Looks good. > > > > > 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? > > Put an '*' as the first character of any field name if it should be optional. > > > > > b) If I just specify a compress.format can I default the compress.level to 0 without an error? > > I believe you'd get compress.level as 0 automatically for an 'int' type. > > I still face the issue that I now always have to specify a compress.format. > I tried to solve it like this: [snip] that's not needed if you name the parameter '*level' in the QAPI schema Regards, Daniel
Am 13.07.2017 um 17:01 schrieb Daniel P. Berrange: > On Thu, Jul 13, 2017 at 05:00:39PM +0200, Peter Lieven wrote: >> Am 13.07.2017 um 16:58 schrieb Daniel P. Berrange: >>> On Thu, Jul 13, 2017 at 04:18:13PM +0200, Peter Lieven wrote: >>>> Okay, so it has to be a mix of QAPI parsing and manual parameter checking, >>>> right? >>> Yeah. It does feel like a valid RFE for QAPI to add a permitted range to >>> 'int' types though, which would simplify the code in future. >>> >>>> 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) >>> Looks good. >>> >>>> 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? >>> Put an '*' as the first character of any field name if it should be optional. >>> >>>> b) If I just specify a compress.format can I default the compress.level to 0 without an error? >>> I believe you'd get compress.level as 0 automatically for an 'int' type. >> I still face the issue that I now always have to specify a compress.format. >> I tried to solve it like this: > [snip] > > that's not needed if you name the parameter '*level' in the QAPI schema its not needed for the *level, but I still get the error that the format parameter is missing otherwise. And I can't make format optional. Peter
On Thu, Jul 13, 2017 at 05:02:51PM +0200, Peter Lieven wrote: > Am 13.07.2017 um 17:01 schrieb Daniel P. Berrange: > > On Thu, Jul 13, 2017 at 05:00:39PM +0200, Peter Lieven wrote: > > > Am 13.07.2017 um 16:58 schrieb Daniel P. Berrange: > > > > On Thu, Jul 13, 2017 at 04:18:13PM +0200, Peter Lieven wrote: > > > > > Okay, so it has to be a mix of QAPI parsing and manual parameter checking, > > > > > right? > > > > Yeah. It does feel like a valid RFE for QAPI to add a permitted range to > > > > 'int' types though, which would simplify the code in future. > > > > > > > > > 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) > > > > Looks good. > > > > > > > > > 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? > > > > Put an '*' as the first character of any field name if it should be optional. > > > > > > > > > b) If I just specify a compress.format can I default the compress.level to 0 without an error? > > > > I believe you'd get compress.level as 0 automatically for an 'int' type. > > > I still face the issue that I now always have to specify a compress.format. > > > I tried to solve it like this: > > [snip] > > > > that's not needed if you name the parameter '*level' in the QAPI schema > > its not needed for the *level, but I still get the error that the format > parameter is missing otherwise. And I can't make format optional. Surely specifying compress.format is how you turn on compression in the first place, so making that optional should not be necessary. eg the simple case becomes: qemu-img create -f qcow2 -o compress.format=zlib foo.qcow2 1G Yes, there is no notion of a "default" format, but IMHO that's a good thing - same way with encryption - it requires an explict encrypt.format=luks to turn on encryption Regards, Daniel
Am 13.07.2017 um 17:06 schrieb Daniel P. Berrange: > On Thu, Jul 13, 2017 at 05:02:51PM +0200, Peter Lieven wrote: >> Am 13.07.2017 um 17:01 schrieb Daniel P. Berrange: >>> On Thu, Jul 13, 2017 at 05:00:39PM +0200, Peter Lieven wrote: >>>> Am 13.07.2017 um 16:58 schrieb Daniel P. Berrange: >>>>> On Thu, Jul 13, 2017 at 04:18:13PM +0200, Peter Lieven wrote: >>>>>> Okay, so it has to be a mix of QAPI parsing and manual parameter checking, >>>>>> right? >>>>> Yeah. It does feel like a valid RFE for QAPI to add a permitted range to >>>>> 'int' types though, which would simplify the code in future. >>>>> >>>>>> 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) >>>>> Looks good. >>>>> >>>>>> 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? >>>>> Put an '*' as the first character of any field name if it should be optional. >>>>> >>>>>> b) If I just specify a compress.format can I default the compress.level to 0 without an error? >>>>> I believe you'd get compress.level as 0 automatically for an 'int' type. >>>> I still face the issue that I now always have to specify a compress.format. >>>> I tried to solve it like this: >>> [snip] >>> >>> that's not needed if you name the parameter '*level' in the QAPI schema >> its not needed for the *level, but I still get the error that the format >> parameter is missing otherwise. And I can't make format optional. > Surely specifying compress.format is how you turn on compression in the > first place, so making that optional should not be necessary. eg the > simple case becomes: > > qemu-img create -f qcow2 -o compress.format=zlib foo.qcow2 1G > > Yes, there is no notion of a "default" format, but IMHO that's a good > thing - same way with encryption - it requires an explict encrypt.format=luks > to turn on encryption Yes, but visit_type_Qcow2Compress_members always returns an error if compress.format is missing from the options. So I should not call it an error out if compress.format is not in the options. Thus the check if either compress.format or compress.level is specified at all. Peter
On Thu, Jul 13, 2017 at 05:13:23PM +0200, Peter Lieven wrote: > Am 13.07.2017 um 17:06 schrieb Daniel P. Berrange: > > On Thu, Jul 13, 2017 at 05:02:51PM +0200, Peter Lieven wrote: > > > Am 13.07.2017 um 17:01 schrieb Daniel P. Berrange: > > > > On Thu, Jul 13, 2017 at 05:00:39PM +0200, Peter Lieven wrote: > > > > > Am 13.07.2017 um 16:58 schrieb Daniel P. Berrange: > > > > > > On Thu, Jul 13, 2017 at 04:18:13PM +0200, Peter Lieven wrote: > > > > > > > Okay, so it has to be a mix of QAPI parsing and manual parameter checking, > > > > > > > right? > > > > > > Yeah. It does feel like a valid RFE for QAPI to add a permitted range to > > > > > > 'int' types though, which would simplify the code in future. > > > > > > > > > > > > > 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) > > > > > > Looks good. > > > > > > > > > > > > > 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? > > > > > > Put an '*' as the first character of any field name if it should be optional. > > > > > > > > > > > > > b) If I just specify a compress.format can I default the compress.level to 0 without an error? > > > > > > I believe you'd get compress.level as 0 automatically for an 'int' type. > > > > > I still face the issue that I now always have to specify a compress.format. > > > > > I tried to solve it like this: > > > > [snip] > > > > > > > > that's not needed if you name the parameter '*level' in the QAPI schema > > > its not needed for the *level, but I still get the error that the format > > > parameter is missing otherwise. And I can't make format optional. > > Surely specifying compress.format is how you turn on compression in the > > first place, so making that optional should not be necessary. eg the > > simple case becomes: > > > > qemu-img create -f qcow2 -o compress.format=zlib foo.qcow2 1G > > > > Yes, there is no notion of a "default" format, but IMHO that's a good > > thing - same way with encryption - it requires an explict encrypt.format=luks > > to turn on encryption > > Yes, but visit_type_Qcow2Compress_members always returns an error if compress.format > is missing from the options. So I should not call it an error out if compress.format is not in the > options. Thus the check if either compress.format or compress.level is specified at all. Oh yes, I see what you mean now. Yes, just wrapping the whole qapi block in an if(qemu_opt_get(opts, "compress.format")) is the right way to handle that. Regards, Daniel
Am 13.07.2017 um 17:17 schrieb Daniel P. Berrange: > On Thu, Jul 13, 2017 at 05:13:23PM +0200, Peter Lieven wrote: >> Am 13.07.2017 um 17:06 schrieb Daniel P. Berrange: >>> On Thu, Jul 13, 2017 at 05:02:51PM +0200, Peter Lieven wrote: >>>> Am 13.07.2017 um 17:01 schrieb Daniel P. Berrange: >>>>> On Thu, Jul 13, 2017 at 05:00:39PM +0200, Peter Lieven wrote: >>>>>> Am 13.07.2017 um 16:58 schrieb Daniel P. Berrange: >>>>>>> On Thu, Jul 13, 2017 at 04:18:13PM +0200, Peter Lieven wrote: >>>>>>>> Okay, so it has to be a mix of QAPI parsing and manual parameter checking, >>>>>>>> right? >>>>>>> Yeah. It does feel like a valid RFE for QAPI to add a permitted range to >>>>>>> 'int' types though, which would simplify the code in future. >>>>>>> >>>>>>>> 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) >>>>>>> Looks good. >>>>>>> >>>>>>>> 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? >>>>>>> Put an '*' as the first character of any field name if it should be optional. >>>>>>> >>>>>>>> b) If I just specify a compress.format can I default the compress.level to 0 without an error? >>>>>>> I believe you'd get compress.level as 0 automatically for an 'int' type. >>>>>> I still face the issue that I now always have to specify a compress.format. >>>>>> I tried to solve it like this: >>>>> [snip] >>>>> >>>>> that's not needed if you name the parameter '*level' in the QAPI schema >>>> its not needed for the *level, but I still get the error that the format >>>> parameter is missing otherwise. And I can't make format optional. >>> Surely specifying compress.format is how you turn on compression in the >>> first place, so making that optional should not be necessary. eg the >>> simple case becomes: >>> >>> qemu-img create -f qcow2 -o compress.format=zlib foo.qcow2 1G >>> >>> Yes, there is no notion of a "default" format, but IMHO that's a good >>> thing - same way with encryption - it requires an explict encrypt.format=luks >>> to turn on encryption >> Yes, but visit_type_Qcow2Compress_members always returns an error if compress.format >> is missing from the options. So I should not call it an error out if compress.format is not in the >> options. Thus the check if either compress.format or compress.level is specified at all. > Oh yes, I see what you mean now. Yes, just wrapping the whole qapi block > in an if(qemu_opt_get(opts, "compress.format")) is the right way to > handle that. qemu_opt_get doesn't work, because it seems to return an empty string. I now also always see compress.format and compress.level in the qemu-img output even if I don't specify it. Have to figure out why this is... ~/git/qemu$ ./qemu-img create -f qcow2 /tmp/1G.qcow2 1G Formatting '/tmp/1G.qcow2', fmt=qcow2 size=1073741824 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16 compress.format= compress.level=0 I hope I have the new patchset ready tomorrow. I would appreciate if you could have a look at it. Thank you, Peter
On 07/13/2017 10:13 AM, Peter Lieven wrote: >>>>>> >>>>>>> I currently have the following: >>>>>>> >>>>>>> options = qemu_opts_to_qdict(opts, NULL); >>>>>>> qdict_extract_subqdict(options, &compressopts, "compress."); Can you show your .json patches as well? > Yes, but visit_type_Qcow2Compress_members always returns an error if > compress.format > is missing from the options. So I should not call it an error out if > compress.format is not in the > options. Thus the check if either compress.format or compress.level is > specified at all. In the JSON, you want '*compress':'CompressUnion' (or some such name) (overall, the user does not have to specify compression); but within CompressUnion, you want 'format' to be unconditional (it is the discriminator), and then branches of the union to add whatever other fields match the enum members covered by 'format'. { 'enum': 'CompressionType', 'data': [ 'gzip', 'lzo' ] } { 'union': 'CompressUnion', 'discriminator': 'format', 'base': { 'format': 'CompressionType', '*level': 'int' }, 'data': { 'gzip': {...any gzip-specific extras}, 'lzo': {...any lzo-specific extras} } } [hmm, I think I'm STILL missing my patches to allow for an anonymous base... I should rebase and send those; but in the meantime, you'll have to create a separate 'CompressBase' struct and use 'base':'CompressBase' instead]
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; + } +} + +static int qcow2_compress_level_supported(int id, uint64_t level) +{ + if ((id == QCOW2_COMPRESS_ZLIB_COMPAT && level > 0) || + (id == QCOW2_COMPRESS_ZLIB && level > 9) || + level > 0xff) { + return -EINVAL; + } + return 0; +} /* * read qcow2 extension and fill bs @@ -148,6 +169,43 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, #endif break; + case QCOW2_EXT_MAGIC_COMPRESS_FORMAT: + if (ext.len != sizeof(s->compress_format)) { + error_setg(errp, "ERROR: ext_compress_format: len=%" + PRIu32 " invalid (!=%zu)", ext.len, + sizeof(s->compress_format)); + return 2; + } + ret = bdrv_pread(bs->file, offset, &s->compress_format, + ext.len); + if (ret < 0) { + error_setg_errno(errp, -ret, "ERROR: ext_compress_fromat:" + " Could not read extension"); + return 3; + } + s->compress_format_id = + qcow2_compress_format_from_name(s->compress_format.name); + if (s->compress_format_id < 0) { + error_setg(errp, "ERROR: compression algorithm '%s' is " + " unsupported", s->compress_format.name); + return 4; + } + if (qcow2_compress_level_supported(s->compress_format_id, + s->compress_format.level) < 0) { + error_setg(errp, "ERROR: compress level %" PRIu8 " is not" + " supported for format '%s'", + s->compress_format.level, s->compress_format.name); + return 5; + } + +#ifdef DEBUG_EXT + printf("Qcow2: Got compress format %s with compress level %" + PRIu8 "\n", s->compress_format.name, + s->compress_format.level); +#endif + break; + + case QCOW2_EXT_MAGIC_FEATURE_TABLE: if (p_feature_table != NULL) { void* feature_table = g_malloc0(ext.len + 2 * sizeof(Qcow2Feature)); @@ -1981,6 +2039,20 @@ int qcow2_update_header(BlockDriverState *bs) buflen -= ret; } + /* Compress Format header extension */ + if (s->compress_format.name[0]) { + assert(!s->compress_format.extra_data_size); + ret = header_ext_add(buf, QCOW2_EXT_MAGIC_COMPRESS_FORMAT, + &s->compress_format, sizeof(s->compress_format), + buflen); + if (ret < 0) { + goto fail; + } + buf += ret; + buflen -= ret; + header->incompatible_features |= cpu_to_be64(QCOW2_INCOMPAT_COMPRESS); + } + /* Feature table */ if (s->qcow_version >= 3) { Qcow2Feature features[] = { @@ -1995,6 +2067,11 @@ int qcow2_update_header(BlockDriverState *bs) .name = "corrupt bit", }, { + .type = QCOW2_FEAT_TYPE_INCOMPATIBLE, + .bit = QCOW2_INCOMPAT_COMPRESS_BITNR, + .name = "compress format bit", + }, + { .type = QCOW2_FEAT_TYPE_COMPATIBLE, .bit = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR, .name = "lazy refcounts", @@ -2333,6 +2410,13 @@ static int qcow2_create2(const char *filename, int64_t total_size, abort(); } + if (compress_format_name[0]) { + BDRVQcow2State *s = blk_bs(blk)->opaque; + memcpy(s->compress_format.name, compress_format_name, + strlen(compress_format_name)); + s->compress_format.level = compress_level; + } + /* Create a full header (including things like feature table) */ ret = qcow2_update_header(blk_bs(blk)); if (ret < 0) { @@ -2391,17 +2475,6 @@ out: return ret; } -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; - } -} - static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp) { char *backing_file = NULL; @@ -2505,11 +2578,10 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp) ret = -EINVAL; goto finish; } - if ((ret == QCOW2_COMPRESS_ZLIB && compress_level > 9) || - compress_level > 0xff) { + ret = qcow2_compress_level_supported(ret, compress_level); + if (ret < 0) { error_setg(errp, "Compress level %" PRIu64 " is not supported for" " format '%s'", compress_level, compress_format_name); - ret = -EINVAL; goto finish; } diff --git a/block/qcow2.h b/block/qcow2.h index d21da33..4ceaba1 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -187,13 +187,16 @@ enum { /* Incompatible feature bits */ enum { - QCOW2_INCOMPAT_DIRTY_BITNR = 0, - QCOW2_INCOMPAT_CORRUPT_BITNR = 1, - QCOW2_INCOMPAT_DIRTY = 1 << QCOW2_INCOMPAT_DIRTY_BITNR, - QCOW2_INCOMPAT_CORRUPT = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR, + QCOW2_INCOMPAT_DIRTY_BITNR = 0, + QCOW2_INCOMPAT_CORRUPT_BITNR = 1, + QCOW2_INCOMPAT_COMPRESS_BITNR = 2, + QCOW2_INCOMPAT_DIRTY = 1 << QCOW2_INCOMPAT_DIRTY_BITNR, + QCOW2_INCOMPAT_CORRUPT = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR, + QCOW2_INCOMPAT_COMPRESS = 1 << QCOW2_INCOMPAT_COMPRESS_BITNR, QCOW2_INCOMPAT_MASK = QCOW2_INCOMPAT_DIRTY - | QCOW2_INCOMPAT_CORRUPT, + | QCOW2_INCOMPAT_CORRUPT + | QCOW2_INCOMPAT_COMPRESS, }; /* Compatible feature bits */ @@ -219,6 +222,13 @@ typedef struct Qcow2Feature { char name[46]; } QEMU_PACKED Qcow2Feature; +typedef struct Qcow2CompressFormatExt { + char name[16]; + uint8_t level; + char res[3]; + uint32_t extra_data_size; +} QEMU_PACKED Qcow2CompressFormatExt; + typedef struct Qcow2DiscardRegion { BlockDriverState *bs; uint64_t offset; @@ -303,6 +313,9 @@ typedef struct BDRVQcow2State { * override) */ char *image_backing_file; char *image_backing_format; + + Qcow2CompressFormatExt compress_format; + int compress_format_id; } BDRVQcow2State; typedef struct Qcow2COWRegion {
we now read the extension on open and write it on update, but do not yet use it. Signed-off-by: Peter Lieven <pl@kamp.de> --- block/qcow2.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++-------- block/qcow2.h | 23 +++++++++++--- 2 files changed, 104 insertions(+), 19 deletions(-)