Re: [PATCH v5 4/8] multifd: Add multifd-zlib-level parameter

[Daniel P . Berrangé]

On Thu, Jan 30, 2020 at 09:03:00AM +0100, Markus Armbruster wrote:
> Juan Quintela <address@hidden> writes:
> 
> > It will indicate which level use for compression.
> >
> > Signed-off-by: Juan Quintela <address@hidden>
> 
> This is slightly confusing (there is no zlib compression), unless you
> peek at the next patch (which adds zlib compression).
> 
> Three ways to make it less confusing:
> 
> * Squash the two commits
> 
> * Swap them: first add zlib compression with level hardcoded to 1, then
>   make the level configurable.
> 
> * Have the first commit explain itself better.  Something like
> 
>     multifd: Add multifd-zlib-level parameter
> 
>     This parameter specifies zlib compression level.  The next patch
>     will put it to use.

Wouldn't the "normal" best practice for QAPI design be to use a
enum and discriminated union. eg

  { 'enum': 'MigrationCompression',
     'data': ['none', 'zlib'] }

  { 'struct': 'MigrationCompressionParamsZLib',
    'data': { 'compression-level' } }

  { 'union':  'MigrationCompressionParams',
    'base': { 'mode': 'MigrationCompression' },
    'discriminator': 'mode',
    'data': {
      'zlib': 'MigrationCompressionParamsZLib',
    }

Of course this is quite different from how migration parameters are
done today. Maybe it makes sense to stick with the flat list of
migration parameters for consistency & ignore normal QAPI design
practice ?


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 :|