Re: [Qemu-devel] Adding new migration-parameters - any easier way?

[zhanghailiang <zhang.zhanghailiang@huawei.com>]

On 2015/6/19 16:11, Markus Armbruster wrote:
> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>
>> Hi,
>>
>> Is there any news about this discussion?
>> Is anyone working on it? ;)
>>
>> Since the 'hard feature freeze' time is closer, we'd better to fix it in 2.4
>> before libvirt uses it.
>>
>> I have sent a RFC patch "[RFC] migration: Re-implement 'migrate-set-parameters' to make it easily for extension"
>> http://patchwork.ozlabs.org/patch/482125/
>> Which just changes qmp command to
>>   '{ "execute": "migrate-set-parameters" , "arguments":
>>       { "compression": { "compress-level": 1 } } }',
>> Compared with its original way, it seems to be more easy for reading and extension, but
>> it can't address most problems discussed here...
>
>>From a QMP wire protocol point of view, @migrate-set-parameters is just
> fine as it is:
>
>      #
>      # @migrate-set-parameters
>      #
>      # Set the following migration parameters
>      #
>      # @compress-level: compression level
>      #
>      # @compress-threads: compression thread count
>      #
>      # @decompress-threads: decompression thread count
>      #
>      # Since: 2.4
>      ##
>      { 'command': 'migrate-set-parameters',
>        'data': { '*compress-level': 'int',
>                  '*compress-threads': 'int',
>                  '*decompress-threads': 'int'} }
>
> However, as the number of parameters grows, the generated C interface
> becomes more and more unwieldy:
>
>      void qmp_migrate_set_parameters(bool has_compress_level,
>                                      int64_t compress_level,
>                                      bool has_compress_threads,
>                                      int64_t compress_threads,
>                                      bool has_decompress_threads,
>                                      int64_t decompress_threads,
>                                      ... more ...
>                                      Error **errp)
>

Yes, this is the problem.

> Your [RFC PATCH] hides away some of the parameters in a separate
> MigrationCompressParameter (misnamed, should be plural).  Leads to
>
>      void qmp_migrate_set_parameters(bool has_compress,
>                                      MigrationCompressParameters *compress,
>                                      ... more ...
>                                      Error **errp)
>
> Makes the QMP wire protocol more complex, because now you have to
> collect some parameters in extra curlies.
>
> Note the two levels of optionalness: one for the wrapping struct, one
> for each of its members.  If you made the members non-optional, we'd
> have to specify none or all, which is not what we want.  If you made the
> wrapping struct non-optional, you'd have to add a stupid 'compress': {}
> when you want to set something else.  So we need both.
>
> In C, the relatively simple test has_compress_level becomes compress &&
> compress->has_level.
>

Hmm, i see, thanks for your explanation.

> If we ever want to set migration parameters from the command line, the
> nesting will be in the way.  Our tool to parse command line into
> QAPI-generated data types (OptsVisitor) by design doesn't cope with
> nested structs.
>
> Perhaps the extra complexity is worthwhile, but it's certainly not
> obvious.
>
> If all we want is making adding new parameters easier (this thread's
> stated subject), then I guess having the function take a single struct
> parameter for all its arguments would do:
>
>      void qmp_migrate_set_parameters(MigrationParameters *parms,
>                                      Error **errp)
>
> To get that from the current QAPI code generator, we'd have to do
> something like
>
>      { 'command': 'migrate-set-parameters',
>        'data': { 'parms': 'MigrationParameters' } }
>
> Trouble is this messes up the QMP wire interface: you now have to wrap
> the actual arguments in two curlies instead of one.
>
> Not a backward-compatibility issue, because the command is new.
>
> Ugly all the same.
>
> To avoid the ugliness, we could change the QAPI generator.  Currently,
>
>      { 'command': 'migrate-set-parameters',
>        'data': 'MigrationParameters' }
>
> generates the same interface as when you inline MigrationParameters,
> namely
>
>      void qmp_migrate_set_parameters(bool has_compress_level,
>                                      int64_t compress_level,
>                                      bool has_compress_threads,
>                                      int64_t compress_threads,
>                                      bool has_decompress_threads,
>                                      int64_t decompress_threads,
>                                      ... more ...
>                                      Error **errp)
>
> It could instead generate
>
>      void qmp_migrate_set_parameters(MigrationParameters *parms,
>                                      Error **errp)
>
> No change to the wire protocol.  Fairly big, but relatively mechanical
> change to the handler functions.  I'd be willing to give it a shot and
> see how it turns out, but I can't do it for 2.4, sorry.
>

Great, that would be fine~ Thanks.