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

[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.


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