Daniel P. Berrangé <berra...@redhat.com> writes: > On Thu, Jan 30, 2020 at 09:03:00AM +0100, Markus Armbruster wrote: >> Juan Quintela <quint...@redhat.com> writes: >> >> > It will indicate which level use for compression. >> > >> > Signed-off-by: Juan Quintela <quint...@redhat.com> >> >> 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.