qemu-devel
[Top][All Lists]
Advanced

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

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


From: Markus Armbruster
Subject: Re: [PATCH v5 4/8] multifd: Add multifd-zlib-level parameter
Date: Thu, 13 Feb 2020 14:27:42 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux)

Daniel P. Berrangé <address@hidden> writes:

> 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',
>     }

This expresses the connection between compression mode and level.  In
general, we prefer that to a bunch of optional members where the
comments say things like "member A can be present only when member B has
value V", or worse "member A is silently ignored unless member B has
value V".  However:

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

Unsure.  It's certainly ugly now.  Each parameter is defined in three
places: enum MigrationParameter (for HMP), struct MigrateSetParameters
(for QMP migrate-set-parameters), and struct MigrationParameters (for
QMP query-migrate-parameters).

I don't know how to make this better other than by starting over.
I don't know whether starting over would result in enough of an
improvement to make it worthwhile.




reply via email to

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