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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 8369 bytes --]

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.

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

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

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

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

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

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


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]