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