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

[Markus Armbruster]

zhanghailiang <address@hidden> 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)

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.

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.