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

[Juan Quintela]

Daniel P. Berrangé <address@hidden> wrote:
> 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',
>     }

How is this translate into HMP?

Markus says to start over, so lets see the dependencies:

Announce: Allawys there

announce-initial
announce-max
announce-rounds
announce-step

Osd compression (deprecated)

compress-level
compress-threads
compress-wait-thread
decompress-threads

cpu-throttles-initial
cpu-throottle-incroment
max-cpu-throotle

tls-creds
tls-hostname
tls-auth


Real params

max-bandwidth
downtime-limit


colo

x-checkpoint-delay

block-incremental

multifd-channels

xbzrle-cache-size

max-postcopy-bandwidth

New things:
- multifd method
- multifd-zlib-level
- multifd-zstd-level

What is a good way to define them?

Why do I ask, because the current method is as bad as it can be.
To add a new parameter:
- for qapi, add it in three places (as Markus said)
- go to hmp-cmds.c and do things by hand
- qemu_migrate_set_parameters
- migrate_params_check
- migrate_params_apply
(last three functions are almost identical in structure, not in
content).

So, if you can give me something that is _easier_ of maintaining, I am
all ears.

Later, Juan.

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