Re: [Qemu-devel] [PATCH RFC] migration: Re-implement 'migrate-set-parameters' to make it easily for extension

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

On 2015/6/9 18:49, Eric Blake wrote:
> On 06/09/2015 01:18 AM, zhanghailiang wrote:
>
> In the subject: s/easily/easier/
>
>> 'migrate-set-parameters' was specially used for setting the compression related
>> parameters of migration. Here we re-implement it so that it can be easily extended when
>> we want to add other parameters for migration.
>
> Might want to make it clear that this is an ABI change, but on an
> unreleased command; it is safe to take this patch only if it makes it
> into 2.4.
>
> You claim that this makes it easier for extension, but don't go into
> details of why that is so; I guess it is that segregating related
> parameters into sub-structs makes it possible to pass sub-structs around
> instead of a list of parameters.
>

Yes, this is the reason, i want to use this new command to set parameter for
COLO, and Dave reminded me that there already has a discussion on it:
https://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg01709.html
and i think maybe he wants to use this command for postcopy too?

>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>   hmp.c                 | 23 +++++++++++--------
>>   migration/migration.c | 63 +++++++++++++++++++++++++++------------------------
>>   qapi-schema.json      | 20 ++++++++++++----
>>   qmp-commands.hx       |  4 ++--
>>   4 files changed, 65 insertions(+), 45 deletions(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index e17852d..4305ae1 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -1213,27 +1213,32 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>>       const char *param = qdict_get_str(qdict, "parameter");
>>       int value = qdict_get_int(qdict, "value");
>>       Error *err = NULL;
>> -    bool has_compress_level = false;
>> -    bool has_compress_threads = false;
>> -    bool has_decompress_threads = false;
>> +    struct MigrationCompressParameter *compress_para =
>> +                                    g_malloc0(sizeof(*compress_para));
>
> Do you really need to malloc this? Just stack-allocate it.  Particularly
> since...
>

Hmm, yes, stack-allocate seems more better.

>> +    bool has_compress_parameter = false;
>>       int i;
>>
>>       for (i = 0; i < MIGRATION_PARAMETER_MAX; i++) {
>>           if (strcmp(param, MigrationParameter_lookup[i]) == 0) {
>>               switch (i) {
>>               case MIGRATION_PARAMETER_COMPRESS_LEVEL:
>> -                has_compress_level = true;
>> +                compress_para->has_compress_level = true;
>> +                compress_para->compress_level = value;
>> +                has_compress_parameter = true;
>>                   break;
>>               case MIGRATION_PARAMETER_COMPRESS_THREADS:
>> -                has_compress_threads = true;
>> +                compress_para->has_compress_threads = true;
>> +                compress_para->compress_threads = value;
>> +                has_compress_parameter = true;
>>                   break;
>>               case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
>> -                has_decompress_threads = true;
>> +                compress_para->has_decompress_threads = true;
>> +                compress_para->decompress_threads = value;
>> +                has_compress_parameter = true;
>>                   break;
>>               }
>> -            qmp_migrate_set_parameters(has_compress_level, value,
>> -                                       has_compress_threads, value,
>> -                                       has_decompress_threads, value,
>> +            qmp_migrate_set_parameters(has_compress_parameter,
>> +                                       compress_para,
>>                                          &err);
>>               break;
>>           }
>
> ...you forgot to free it, and therefore have a leak.
>

oops~

> I also think you can get away without using has_compress_parameter, and
> just call qmp_migrate_set_parameters(true, &compress_para, &err).  Since
> all the members are optional, that should still do the right thing (in
> QMP terms, omitting the argument and passing 'compress-parameter':{}
> will do the same thing).
>

Good catch.

>> diff --git a/migration/migration.c b/migration/migration.c
>> index 732d229..eef0e5c 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -316,44 +316,49 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>>       }
>>   }
>>
>> -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, Error **errp)
>> +void qmp_migrate_set_parameters(bool has_compress_parameter,
>> +                            struct MigrationCompressParameter *compress_para,
>> +                            Error **errp)
>
> Sounds verbose, and makes you run up against 80-column limits. I wonder
> if any shorter names will work.
>

Yes, maybe should  change it to 'struct CompressParameter'.

>>   {
>>       MigrationState *s = migrate_get_current();
>>
>> -    if (has_compress_level && (compress_level < 0 || compress_level > 9)) {
>> -        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
>> +    if (has_compress_parameter) {
>> +        if (compress_para->has_compress_level &&
>> +            (compress_para->compress_level < 0 ||
>> +             compress_para->compress_level > 9)) {
>> +            error_set(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
>>                     "is invalid, it should be in the range of 0 to 9");
>
> A lot of verbosity.  This could read as:
>
> if (compression->has_level &&
>      (compression->level < 0 || compression->level > 9)) {
>
> if you do some renaming...
>
>
>> -        return;
>> -    }
>> -    if (has_compress_threads &&
>> -            (compress_threads < 1 || compress_threads > 255)) {
>> -        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> +            return;
>> +        }
>> +        if (compress_para->has_compress_threads &&
>> +            (compress_para->compress_threads < 1 ||
>> +             compress_para->compress_threads > 255)) {
>
> ...except then we have a problem with compress_threads vs.
> decompress_threads.
>
>> +++ b/qapi-schema.json
>> @@ -592,10 +592,8 @@
>>   { 'enum': 'MigrationParameter',
>>     'data': ['compress-level', 'compress-threads', 'decompress-threads'] }
>>
>> -#
>> -# @migrate-set-parameters
>> -#
>> -# Set the following migration parameters
>> +##
>> +# @MigrationCompressParameter
>>   #
>>   # @compress-level: compression level
>
> Missing an overview of the purpose of this struct, which is to collect
> compression-related parameters into one spot.
>
> Back to the renaming bit, it feels redundant to name all parameters
> beginning 'compress-*' if the struct itself has Compress in the name
> (but that pesky decompress-threads bucks the trend).  On the other hand,
> if you call this 'MigrationCompression'...
>
>>   #
>> @@ -605,12 +603,24 @@
>>   #
>>   # Since: 2.4
>>   ##
>> -{ 'command': 'migrate-set-parameters',
>> +{ 'struct': 'MigrationCompressParameter',
>>     'data': { '*compress-level': 'int',
>>               '*compress-threads': 'int',
>>               '*decompress-threads': 'int'} }
>>
>>   #
>> +# @migrate-set-parameters
>> +#
>> +# Set the migration parameters
>> +#
>> +# @compress-parameter: compression related parameters
>> +#
>> +# Since: 2.4
>> +##
>> +{ 'command': 'migrate-set-parameters',
>> +  'data': { '*compress-parameter': 'MigrationCompressParameter'} }
>
> ...and name the member '*compression':'MigrationCompression', that will
> still be legible, and buy you back some screen width.
>
>> +
>> +#
>>   # @MigrationParameters
>>   #
>>   # @compress-level: compression level
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 14e109e..ce721ba 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -3460,14 +3460,14 @@ Arguments:
>>   Example:
>>
>>   -> { "execute": "migrate-set-parameters" , "arguments":
>> -      { "compress-level": 1 } }
>> +      { "compress-parameter": { "compress-level": 1 } } }
>
> and back to my argument of verbosity, doesn't:
>
> { "compression": { "level": 1 } } }
>
> look better?
>

Yes，that will be better.

>>
>>   EQMP
>>
>>       {
>>           .name       = "migrate-set-parameters",
>>           .args_type  =
>> -            "compress-level:i?,compress-threads:i?,decompress-threads:i?",
>> +            "compress-parameter:q?",
>>   	.mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters,
>>       },
>>   SQMP
>>
>
> Overall, I think the idea has merit, but it will need a v2.

Er, this is a 'RFC' patch, i don't know if this modification is appropriate.
Since there is already a discussion about this command. I think it's better to
wait for more comments before deciding whether to send v2 or not. :)


Thanks,
zhanghailiang