[v5,12/12] migration: Add commands to set and query parameter

Add the qmp and hmp commands to tune and query the parameters used in
live migration.

Signed-off-by: Liang Li <liang.z.li@intel.com>
Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
---
 hmp-commands.hx               | 17 ++++++++
 hmp.c                         | 56 +++++++++++++++++++++++++
 hmp.h                         |  4 ++
 include/migration/migration.h |  4 +-
 migration/migration.c         | 96 +++++++++++++++++++++++++++++++++++++------
 monitor.c                     | 25 +++++++++++
 qapi-schema.json              | 86 ++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx               | 66 +++++++++++++++++++++++++++++
 8 files changed, 339 insertions(+), 15 deletions(-)

Liang Li <liang.z.li@intel.com> wrote:
> Add the qmp and hmp commands to tune and query the parameters used in
> live migration.
>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>

I agree with the existence of this changes, and we could use them for
existing paramenters like migration_speed (independent of this patch).

About the qmp bits of the patch, I let others to comment on it.

I like the appreach, though.

Later, Juan.

* Liang Li (liang.z.li@intel.com) wrote:
> Add the qmp and hmp commands to tune and query the parameters used in
> live migration.
> 
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>

Hi,
   You might like to split this patch into two; one with no actual
parameters in the lists, and then a second one that adds your
compression options.

Dave

> ---
>  hmp-commands.hx               | 17 ++++++++
>  hmp.c                         | 56 +++++++++++++++++++++++++
>  hmp.h                         |  4 ++
>  include/migration/migration.h |  4 +-
>  migration/migration.c         | 96 +++++++++++++++++++++++++++++++++++++------
>  monitor.c                     | 25 +++++++++++
>  qapi-schema.json              | 86 ++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx               | 66 +++++++++++++++++++++++++++++
>  8 files changed, 339 insertions(+), 15 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index e37bc8b..ed0c06a 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -985,6 +985,21 @@ Enable/Disable the usage of a capability @var{capability} for migration.
>  ETEXI
>  
>      {
> +        .name       = "migrate_set_parameter",
> +        .args_type  = "parameter:s,value:i",
> +        .params     = "parameter value",
> +        .help       = "Set the parameter for migration",
> +        .mhandler.cmd = hmp_migrate_set_parameter,
> +        .command_completion = migrate_set_parameter_completion,
> +    },
> +
> +STEXI
> +@item migrate_set_parameter @var{parameter} @var{value}
> +@findex migrate_set_parameter
> +Set the parameter @var{parameter} for migration.
> +ETEXI
> +
> +    {
>          .name       = "client_migrate_info",
>          .args_type  = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
>          .params     = "protocol hostname port tls-port cert-subject",
> @@ -1764,6 +1779,8 @@ show user network stack connection states
>  show migration status
>  @item info migrate_capabilities
>  show current migration capabilities
> +@item info migrate_parameters
> +show current migration parameters
>  @item info migrate_cache_size
>  show current migration XBZRLE cache size
>  @item info balloon
> diff --git a/hmp.c b/hmp.c
> index b47f331..1f67651 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -246,6 +246,27 @@ void hmp_info_migrate_capabilities(Monitor *mon, const QDict *qdict)
>      qapi_free_MigrationCapabilityStatusList(caps);
>  }
>  
> +void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
> +{
> +    MigrationParameterStatusList *params, *p;
> +    MigrationParameterInt *data;
> +
> +    params = qmp_query_migrate_parameters(NULL);
> +
> +    if (params) {
> +        monitor_printf(mon, "parameters:");
> +        for (p = params; p; p = p->next) {
> +            data = (MigrationParameterInt *)p->value->data;
> +            monitor_printf(mon, " %s: %" PRId64,
> +                           MigrationParameter_lookup[p->value->kind],
> +                           data->value);
> +        }
> +        monitor_printf(mon, "\n");
> +    }
> +
> +    qapi_free_MigrationParameterStatusList(params);
> +}
> +
>  void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict)
>  {
>      monitor_printf(mon, "xbzrel cache size: %" PRId64 " kbytes\n",
> @@ -1140,6 +1161,41 @@ void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +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;
> +    MigrationParameterStatusList *params = g_malloc0(sizeof(*params));
> +    MigrationParameterInt *data;
> +    int i;
> +
> +    for (i = 0; i < MIGRATION_PARAMETER_MAX; i++) {
> +        if (strcmp(param, MigrationParameter_lookup[i]) == 0) {
> +            params->value = g_malloc0(sizeof(*params->value));
> +            params->value->kind = i;
> +            params->value->data = g_malloc0(sizeof(MigrationParameterInt));
> +            data = (MigrationParameterInt *)params->value->data;
> +            data->value = value;
> +            params->next = NULL;
> +            qmp_migrate_set_parameters(params, &err);
> +            break;
> +        }
> +    }
> +
> +    if (i == MIGRATION_PARAMETER_MAX) {
> +        error_set(&err, QERR_INVALID_PARAMETER, param);
> +    }
> +
> +    qapi_free_MigrationParameterStatusList(params);
> +
> +    if (err) {
> +        monitor_printf(mon, "migrate_set_parameter: %s\n",
> +                       error_get_pretty(err));
> +        error_free(err);
> +    }
> +}
> +
>  void hmp_set_password(Monitor *mon, const QDict *qdict)
>  {
>      const char *protocol  = qdict_get_str(qdict, "protocol");
> diff --git a/hmp.h b/hmp.h
> index 4bb5dca..b2b2d2c 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -28,6 +28,7 @@ void hmp_info_chardev(Monitor *mon, const QDict *qdict);
>  void hmp_info_mice(Monitor *mon, const QDict *qdict);
>  void hmp_info_migrate(Monitor *mon, const QDict *qdict);
>  void hmp_info_migrate_capabilities(Monitor *mon, const QDict *qdict);
> +void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict);
>  void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict);
>  void hmp_info_cpus(Monitor *mon, const QDict *qdict);
>  void hmp_info_block(Monitor *mon, const QDict *qdict);
> @@ -63,6 +64,7 @@ void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict);
> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict);
>  void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict);
>  void hmp_set_password(Monitor *mon, const QDict *qdict);
>  void hmp_expire_password(Monitor *mon, const QDict *qdict);
> @@ -111,6 +113,8 @@ void watchdog_action_completion(ReadLineState *rs, int nb_args,
>                                  const char *str);
>  void migrate_set_capability_completion(ReadLineState *rs, int nb_args,
>                                         const char *str);
> +void migrate_set_parameter_completion(ReadLineState *rs, int nb_args,
> +                                      const char *str);
>  void host_net_add_completion(ReadLineState *rs, int nb_args, const char *str);
>  void host_net_remove_completion(ReadLineState *rs, int nb_args,
>                                  const char *str);
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 9ac1b23..434cc96 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -51,9 +51,7 @@ struct MigrationState
>      QEMUBH *cleanup_bh;
>      QEMUFile *file;
>      QemuThread *compress_thread;
> -    int compress_thread_count;
> -    int decompress_thread_count;
> -    int compress_level;
> +    int parameters[MIGRATION_PARAMETER_MAX];
>  
>      int state;
>      MigrationParams params;
> diff --git a/migration/migration.c b/migration/migration.c
> index 55f749e..89750ba 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -66,9 +66,12 @@ MigrationState *migrate_get_current(void)
>          .bandwidth_limit = MAX_THROTTLE,
>          .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
>          .mbps = -1,
> -        .compress_thread_count = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
> -        .decompress_thread_count = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
> -        .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
> +        .parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL] =
> +                DEFAULT_MIGRATE_COMPRESS_LEVEL,
> +        .parameters[MIGRATION_PARAMETER_COMPRESS_THREADS] =
> +                DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
> +        .parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] =
> +                DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
>      };
>  
>      return &current_migration;
> @@ -178,6 +181,33 @@ MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
>      return head;
>  }
>  
> +MigrationParameterStatusList *qmp_query_migrate_parameters(Error **errp)
> +{
> +    MigrationParameterStatusList *head = NULL;
> +    MigrationParameterStatusList *params;
> +    MigrationState *s = migrate_get_current();
> +    MigrationParameterInt *data;
> +    int i;
> +
> +    params = NULL; /* silence compiler warning */
> +    for (i = 0; i < MIGRATION_PARAMETER_MAX; i++) {
> +        if (head == NULL) {
> +            head = g_malloc0(sizeof(*params));
> +            params = head;
> +        } else {
> +            params->next = g_malloc0(sizeof(*params));
> +            params = params->next;
> +        }
> +        params->value = g_malloc(sizeof(*params->value));
> +        params->value->kind = i;
> +        params->value->data = g_malloc(sizeof(MigrationParameterInt));
> +        data = (MigrationParameterInt *)params->value->data;
> +        data->value = s->parameters[i];
> +    }
> +
> +    return head;
> +}
> +
>  static void get_xbzrle_cache_stats(MigrationInfo *info)
>  {
>      if (migrate_use_xbzrle()) {
> @@ -294,6 +324,44 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>      }
>  }
>  
> +void qmp_migrate_set_parameters(MigrationParameterStatusList *params,
> +                                Error **errp)
> +{
> +    MigrationState *s = migrate_get_current();
> +    MigrationParameterStatusList *p;
> +    MigrationParameterInt *data;
> +
> +    for (p = params; p; p = p->next) {
> +        switch (p->value->kind) {
> +        case MIGRATION_PARAMETER_COMPRESS_LEVEL:
> +            data = (MigrationParameterInt *)p->value->data;
> +            if (data->value < 0 || data->value > 9) {
> +                error_set(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
> +                          "is invalid, it should be in the range of 0 to 9");
> +                return;
> +            }
> +            break;
> +        case MIGRATION_PARAMETER_COMPRESS_THREADS:
> +        case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
> +            if (s->state == MIG_STATE_ACTIVE || s->state == MIG_STATE_SETUP) {
> +                error_set(errp, QERR_MIGRATION_ACTIVE);
> +                return;
> +            }
> +            data = (MigrationParameterInt *)p->value->data;
> +            if (data->value < 1 || data->value > 255) {
> +                error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> +                          "(de)compress_threads",
> +                          "is invalid, it should be in the range of 1 to 255");
> +                return;
> +            }
> +            break;
> +        default:
> +           return;
> +        }
> +        s->parameters[p->value->kind] = data->value;
> +    }
> +}
> +
>  /* shared migration helpers */
>  
>  static void migrate_set_state(MigrationState *s, int old_state, int new_state)
> @@ -400,9 +468,11 @@ static MigrationState *migrate_init(const MigrationParams *params)
>      int64_t bandwidth_limit = s->bandwidth_limit;
>      bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
>      int64_t xbzrle_cache_size = s->xbzrle_cache_size;
> -    int compress_level = s->compress_level;
> -    int compress_thread_count = s->compress_thread_count;
> -    int decompress_thread_count = s->decompress_thread_count;
> +    int compress_level = s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL];
> +    int compress_thread_count =
> +            s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS];
> +    int decompress_thread_count =
> +            s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS];
>  
>      memcpy(enabled_capabilities, s->enabled_capabilities,
>             sizeof(enabled_capabilities));
> @@ -413,9 +483,11 @@ static MigrationState *migrate_init(const MigrationParams *params)
>             sizeof(enabled_capabilities));
>      s->xbzrle_cache_size = xbzrle_cache_size;
>  
> -    s->compress_level = compress_level;
> -    s->compress_thread_count = compress_thread_count;
> -    s->decompress_thread_count = decompress_thread_count;
> +    s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL] = compress_level;
> +    s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS] =
> +               compress_thread_count;
> +    s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] =
> +               decompress_thread_count;
>      s->bandwidth_limit = bandwidth_limit;
>      s->state = MIG_STATE_SETUP;
>      trace_migrate_set_state(MIG_STATE_SETUP);
> @@ -603,7 +675,7 @@ int migrate_compress_level(void)
>  
>      s = migrate_get_current();
>  
> -    return s->compress_level;
> +    return s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL];
>  }
>  
>  int migrate_compress_threads(void)
> @@ -612,7 +684,7 @@ int migrate_compress_threads(void)
>  
>      s = migrate_get_current();
>  
> -    return s->compress_thread_count;
> +    return s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS];
>  }
>  
>  int migrate_decompress_threads(void)
> @@ -621,7 +693,7 @@ int migrate_decompress_threads(void)
>  
>      s = migrate_get_current();
>  
> -    return s->decompress_thread_count;
> +    return s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS];
>  }
>  
>  int migrate_use_xbzrle(void)
> diff --git a/monitor.c b/monitor.c
> index c3cc060..499ae1c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2873,6 +2873,13 @@ static mon_cmd_t info_cmds[] = {
>          .mhandler.cmd = hmp_info_migrate_capabilities,
>      },
>      {
> +        .name       = "migrate_parameters",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show current migration parameters",
> +        .mhandler.cmd = hmp_info_migrate_parameters,
> +    },
> +    {
>          .name       = "migrate_cache_size",
>          .args_type  = "",
>          .params     = "",
> @@ -4555,6 +4562,24 @@ void migrate_set_capability_completion(ReadLineState *rs, int nb_args,
>      }
>  }
>  
> +void migrate_set_parameter_completion(ReadLineState *rs, int nb_args,
> +                                      const char *str)
> +{
> +    size_t len;
> +
> +    len = strlen(str);
> +    readline_set_completion_index(rs, len);
> +    if (nb_args == 2) {
> +        int i;
> +        for (i = 0; i < MIGRATION_PARAMETER_MAX; i++) {
> +            const char *name = MigrationParameter_lookup[i];
> +            if (!strncmp(str, name, len)) {
> +                readline_add_completion(rs, name);
> +            }
> +        }
> +    }
> +}
> +
>  void host_net_add_completion(ReadLineState *rs, int nb_args, const char *str)
>  {
>      int i;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 0dfc4ce..5bf21fe 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -541,6 +541,92 @@
>  ##
>  { 'command': 'query-migrate-capabilities', 'returns':   ['MigrationCapabilityStatus']}
>  
> +# @MigrationParameter
> +#
> +# Migration parameters enumeration
> +#
> +# @compress-level: Set the compression level to be used in live migration,
> +#          the compression level is an integer between 0 and 9, where 0 means
> +#          no compression, 1 means the best compression speed, and 9 means best
> +#          compression ratio which will consume more CPU.
> +#
> +# @compress-threads: Set compression thread count to be used in live migration,
> +#          the compression thread count is an integer between 1 and 255.
> +#
> +# @decompress-threads: Set decompression thread count to be used in live
> +#          migration, the decompression thread count is an integer between 1
> +#          and 255. Usually, decompression is at least 4 times as fast as
> +#          compression, so set the decompress-threads to the number about 1/4
> +#          of compress-threads is adequate.
> +#
> +# Since: 2.3
> +##
> +{ 'enum': 'MigrationParameter',
> +  'data': ['compress-level', 'compress-threads', 'decompress-threads'] }
> +##
> +# @MigrationParameterBase
> +#
> +# Migration parameter information
> +#
> +# @parameter: the parameter of migration
> +#
> +# Since: 2.3
> +##
> +{ 'type': 'MigrationParameterBase',
> +  'data': {'parameter': 'MigrationParameter'} }
> +##
> +# @MigrationParameterInt
> +#
> +# Migration parameter information
> +#
> +# @value: parameter int
> +#
> +# Since: 2.3
> +##
> +{ 'type': 'MigrationParameterInt',
> +  'data': {'value': 'int'} }
> +##
> +# @MigrationParameterStatus
> +#
> +# Migration parameter information
> +#
> +# @compress-level: compression level
> +#
> +# @compress-threads: compression thread count
> +#
> +# @decompress-threads: decompression thread count
> +#
> +# Since: 2.3
> +##
> +{ 'union': 'MigrationParameterStatus',
> +  'base': 'MigrationParameterBase',
> +  'discriminator': 'parameter',
> +  'data': { 'compress-level': 'MigrationParameterInt',
> +            'compress-threads': 'MigrationParameterInt',
> +            'decompress-threads': 'MigrationParameterInt'} }
> +#
> +# @migrate-set-parameters
> +#
> +# Set the following migration parameters (like compress-level)
> +#
> +# @parameters: json array of parameter modifications to make
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'migrate-set-parameters',
> +  'data': { 'parameters': ['MigrationParameterStatus'] } }
> +##
> +# @query-migrate-parameters
> +#
> +# Returns information about the current migration parameters status
> +#
> +# Returns: @MigrationParametersStatus
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'query-migrate-parameters',
> +  'returns': ['MigrationParameterStatus'] }
> +##
>  ##
>  # @MouseInfo:
>  #
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index a85d847..2c4737b 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3295,6 +3295,72 @@ EQMP
>      },
>  
>  SQMP
> +migrate-set-parameters
> +----------------------
> +
> +Set migration parameters
> +
> +- "compress-level": set compression level during migration
> +- "compress-threads": set compression thread count for migration
> +- "decompress-threads": set decompression thread count for migration
> +
> +Arguments:
> +
> +Example:
> +
> +-> { "execute": "migrate-set-parameters" , "arguments":
> +     { "parameters": [ { "parameter": "compress-level", "value": 1 } ] } }
> +
> +EQMP
> +
> +    {
> +        .name       = "migrate-set-parameters",
> +        .args_type  = "parameters:O",
> +        .params     = "parameter:s,value:O",
> +	.mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters,
> +    },
> +SQMP
> +query-migrate-parameters
> +------------------------
> +
> +Query current migration parameters
> +
> +- "parameters": migration parameters value
> +         - "compress-level" : compression level value (json-int)
> +         - "compress-threads" : compression thread count value (json-int)
> +         - "decompress-threads" : decompression thread count value (json-int)
> +
> +Arguments:
> +
> +Example:
> +
> +-> { "execute": "query-migrate-parameters" }
> +<- {
> +      "return": [
> +         {
> +            "parameter": "compress-level",
> +            "value": 1
> +         },
> +         {
> +            "parameter": "compress-threads",
> +            "value": 8
> +         },
> +         {
> +            "parameter": "decompress-threads",
> +            "value": 2
> +         }
> +      ]
> +   }
> +
> +EQMP
> +
> +    {
> +        .name       = "query-migrate-parameters",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_query_migrate_parameters,
> +    },
> +
> +SQMP
>  query-balloon
>  -------------
>  
> -- 
> 1.9.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Copying Eric for additional QAPI design expertise.

I'm going to review just the QAPI schema changes.

Liang Li <liang.z.li@intel.com> writes:

> Add the qmp and hmp commands to tune and query the parameters used in
> live migration.
>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
[...]
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 0dfc4ce..5bf21fe 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -541,6 +541,92 @@
>  ##
>  { 'command': 'query-migrate-capabilities', 'returns':   ['MigrationCapabilityStatus']}
>  
> +# @MigrationParameter
> +#
> +# Migration parameters enumeration
> +#
> +# @compress-level: Set the compression level to be used in live migration,
> +#          the compression level is an integer between 0 and 9, where 0 means
> +#          no compression, 1 means the best compression speed, and 9 means best
> +#          compression ratio which will consume more CPU.
> +#
> +# @compress-threads: Set compression thread count to be used in live migration,
> +#          the compression thread count is an integer between 1 and 255.
> +#
> +# @decompress-threads: Set decompression thread count to be used in live
> +#          migration, the decompression thread count is an integer between 1
> +#          and 255. Usually, decompression is at least 4 times as fast as
> +#          compression, so set the decompress-threads to the number about 1/4
> +#          of compress-threads is adequate.
> +#
> +# Since: 2.3
> +##
> +{ 'enum': 'MigrationParameter',
> +  'data': ['compress-level', 'compress-threads', 'decompress-threads'] }
> +##
> +# @MigrationParameterBase
> +#
> +# Migration parameter information
> +#
> +# @parameter: the parameter of migration
> +#
> +# Since: 2.3
> +##
> +{ 'type': 'MigrationParameterBase',
> +  'data': {'parameter': 'MigrationParameter'} }
> +##
> +# @MigrationParameterInt
> +#
> +# Migration parameter information
> +#
> +# @value: parameter int
> +#
> +# Since: 2.3
> +##
> +{ 'type': 'MigrationParameterInt',
> +  'data': {'value': 'int'} }
> +##
> +# @MigrationParameterStatus
> +#
> +# Migration parameter information
> +#
> +# @compress-level: compression level
> +#
> +# @compress-threads: compression thread count
> +#
> +# @decompress-threads: decompression thread count
> +#
> +# Since: 2.3
> +##
> +{ 'union': 'MigrationParameterStatus',
> +  'base': 'MigrationParameterBase',
> +  'discriminator': 'parameter',
> +  'data': { 'compress-level': 'MigrationParameterInt',
> +            'compress-threads': 'MigrationParameterInt',
> +            'decompress-threads': 'MigrationParameterInt'} }
> +#
> +# @migrate-set-parameters
> +#
> +# Set the following migration parameters (like compress-level)
> +#
> +# @parameters: json array of parameter modifications to make
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'migrate-set-parameters',
> +  'data': { 'parameters': ['MigrationParameterStatus'] } }

The command takes a list of key-value pairs.  Looks like this (example
stolen from your patch to qmp-commands.hx):

    { "execute": "migrate-set-parameters",
      "arguments": { "parameters":
                     [ { "parameter": "compress-level", "value": 1 } ] } }

Awkward.  I'd very much prefer

    { "execute": "migrate-set-parameters",
      "arguments": { "compress-level", 1 } }

I.e. the command simply takes the parameters as optional arguments.
Simpler, and a natural use of the schema language.

> +##
> +# @query-migrate-parameters
> +#
> +# Returns information about the current migration parameters status
> +#
> +# Returns: @MigrationParametersStatus
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'query-migrate-parameters',
> +  'returns': ['MigrationParameterStatus'] }
> +##
>  ##
>  # @MouseInfo:
>  #

Produces a list of key-value pairs.  Looks like this (stolen from the
same place):

    {
       "return": [
          {
             "parameter": "compress-level",
             "value": 1
          },
          {
             "parameter": "compress-threads",
             "value": 8
          },
          {
             "parameter": "decompress-threads",
             "value": 2
          }
       ]
    }

I'd very much prefer a simple object instead:

    { "return": { "compress-level": 1,
                  "compress-threads": 8,
                  "decompress-threads": 2 } }

[...]

> > +#
> > +# Migration parameter information
> > +#
> > +# @compress-level: compression level
> > +#
> > +# @compress-threads: compression thread count # #
> > +@decompress-threads: decompression thread count # # Since: 2.3 ## {
> > +'union': 'MigrationParameterStatus',
> > +  'base': 'MigrationParameterBase',
> > +  'discriminator': 'parameter',
> > +  'data': { 'compress-level': 'MigrationParameterInt',
> > +            'compress-threads': 'MigrationParameterInt',
> > +            'decompress-threads': 'MigrationParameterInt'} } # #
> > +@migrate-set-parameters # # Set the following migration parameters
> > +(like compress-level) # # @parameters: json array of parameter
> > +modifications to make # # Since: 2.3 ## { 'command':
> > +'migrate-set-parameters',
> > +  'data': { 'parameters': ['MigrationParameterStatus'] } }
> 
> The command takes a list of key-value pairs.  Looks like this (example stolen
> from your patch to qmp-commands.hx):
> 
>     { "execute": "migrate-set-parameters",
>       "arguments": { "parameters":
>                      [ { "parameter": "compress-level", "value": 1 } ] } }
> 
> Awkward.  I'd very much prefer
> 
>     { "execute": "migrate-set-parameters",
>       "arguments": { "compress-level", 1 } }
> 
> I.e. the command simply takes the parameters as optional arguments.
> Simpler, and a natural use of the schema language.

Yes, it seems complicated.  Eric suggested to use this type of interface, because it can 
support different type of parameters if some new parameters will be added.  

> > +##
> > +# @query-migrate-parameters
> > +#
> > +# Returns information about the current migration parameters status #
> > +# Returns: @MigrationParametersStatus # # Since: 2.3 ## { 'command':
> > +'query-migrate-parameters',
> > +  'returns': ['MigrationParameterStatus'] } ##
> >  ##
> >  # @MouseInfo:
> >  #
> 
> Produces a list of key-value pairs.  Looks like this (stolen from the same
> place):
> 
>     {
>        "return": [
>           {
>              "parameter": "compress-level",
>              "value": 1
>           },
>           {
>              "parameter": "compress-threads",
>              "value": 8
>           },
>           {
>              "parameter": "decompress-threads",
>              "value": 2
>           }
>        ]
>     }
> 
> I'd very much prefer a simple object instead:
> 
>     { "return": { "compress-level": 1,
>                   "compress-threads": 8,
>                   "decompress-threads": 2 } }
> 

The same reason.

"Li, Liang Z" <liang.z.li@intel.com> writes:

>> > +#
>> > +# Migration parameter information
>> > +#
>> > +# @compress-level: compression level
>> > +#
>> > +# @compress-threads: compression thread count # #
>> > +@decompress-threads: decompression thread count # # Since: 2.3 ## {
>> > +'union': 'MigrationParameterStatus',
>> > +  'base': 'MigrationParameterBase',
>> > +  'discriminator': 'parameter',
>> > +  'data': { 'compress-level': 'MigrationParameterInt',
>> > +            'compress-threads': 'MigrationParameterInt',
>> > +            'decompress-threads': 'MigrationParameterInt'} } # #
>> > +@migrate-set-parameters # # Set the following migration parameters
>> > +(like compress-level) # # @parameters: json array of parameter
>> > +modifications to make # # Since: 2.3 ## { 'command':
>> > +'migrate-set-parameters',
>> > +  'data': { 'parameters': ['MigrationParameterStatus'] } }
>> 
>> The command takes a list of key-value pairs.  Looks like this (example stolen
>> from your patch to qmp-commands.hx):
>> 
>>     { "execute": "migrate-set-parameters",
>>       "arguments": { "parameters":
>>                      [ { "parameter": "compress-level", "value": 1 } ] } }
>> 
>> Awkward.  I'd very much prefer
>> 
>>     { "execute": "migrate-set-parameters",
>>       "arguments": { "compress-level", 1 } }
>> 
>> I.e. the command simply takes the parameters as optional arguments.
>> Simpler, and a natural use of the schema language.
>
> Yes, it seems complicated.  Eric suggested to use this type of
> interface, because it can
> support different type of parameters if some new parameters will be added.  

I don't understand.

Schema for the three parameters we have now:

{ 'command': 'migrate-set-parameters',
  'data': { '*compression-level': 'int',
            '*compression-threads': 'int,
            '*decompression-threads': 'int' } }

Let's add a parameter with some other type:

{ 'command': 'migrate-set-parameters',
  'data': { '*compression-level': 'int',
            '*compression-threads': 'int,
            '*compression-algorithm': 'CompressionAlgorithm',
            '*decompression-threads': 'int', } }

CompressionAlgorithm could be an enum.

Why wouldn't that work?

>> > +##
>> > +# @query-migrate-parameters
>> > +#
>> > +# Returns information about the current migration parameters status #
>> > +# Returns: @MigrationParametersStatus # # Since: 2.3 ## { 'command':
>> > +'query-migrate-parameters',
>> > +  'returns': ['MigrationParameterStatus'] } ##
>> >  ##
>> >  # @MouseInfo:
>> >  #
>> 
>> Produces a list of key-value pairs.  Looks like this (stolen from the same
>> place):
>> 
>>     {
>>        "return": [
>>           {
>>              "parameter": "compress-level",
>>              "value": 1
>>           },
>>           {
>>              "parameter": "compress-threads",
>>              "value": 8
>>           },
>>           {
>>              "parameter": "decompress-threads",
>>              "value": 2
>>           }
>>        ]
>>     }
>> 
>> I'd very much prefer a simple object instead:
>> 
>>     { "return": { "compress-level": 1,
>>                   "compress-threads": 8,
>>                   "decompress-threads": 2 } }
>> 
>
> The same reason.

{ 'command': 'query-migrate-parameters',
  'returns': 'MigrationParameters' }

{ 'type': 'MigrationParameters',
  'data': { 'compression-level': 'int',
            'compression-threads': 'int,
            'decompression-threads': 'int' } }

Add one as above:

{ 'type': 'MigrationParameters',
  'data': { 'compression-level': 'int',
            'compression-threads': 'int,
            'compression-algorithm': 'CompressionAlgorithm',
            'decompression-threads': 'int', } }

Why wouldn't that work?

> >> The command takes a list of key-value pairs.  Looks like this
> >> (example stolen from your patch to qmp-commands.hx):
> >>
> >>     { "execute": "migrate-set-parameters",
> >>       "arguments": { "parameters":
> >>                      [ { "parameter": "compress-level", "value": 1 }
> >> ] } }
> >>
> >> Awkward.  I'd very much prefer
> >>
> >>     { "execute": "migrate-set-parameters",
> >>       "arguments": { "compress-level", 1 } }
> >>
> >> I.e. the command simply takes the parameters as optional arguments.
> >> Simpler, and a natural use of the schema language.
> >
> > Yes, it seems complicated.  Eric suggested to use this type of
> > interface, because it can support different type of parameters if some
> > new parameters will be added.
> 
> I don't understand.
> 
> Schema for the three parameters we have now:
> 
> { 'command': 'migrate-set-parameters',
>   'data': { '*compression-level': 'int',
>             '*compression-threads': 'int,
>             '*decompression-threads': 'int' } }
> 
> Let's add a parameter with some other type:
> 
> { 'command': 'migrate-set-parameters',
>   'data': { '*compression-level': 'int',
>             '*compression-threads': 'int,
>             '*compression-algorithm': 'CompressionAlgorithm',
>             '*decompression-threads': 'int', } }
> 
> CompressionAlgorithm could be an enum.
> 
> Why wouldn't that work?

To be honest, I know very little about QMP and JSON. What should I do to make the 
schema as you described?

 
> >> > +##
> >> > +# @query-migrate-parameters
> >> > +#
> >> > +# Returns information about the current migration parameters
> >> > +status # # Returns: @MigrationParametersStatus # # Since: 2.3 ##
> { 'command':
> >> > +'query-migrate-parameters',
> >> > +  'returns': ['MigrationParameterStatus'] } ##
> >> >  ##
> >> >  # @MouseInfo:
> >> >  #
> >>
> >> Produces a list of key-value pairs.  Looks like this (stolen from the
> >> same
> >> place):
> >>
> >>     {
> >>        "return": [
> >>           {
> >>              "parameter": "compress-level",
> >>              "value": 1
> >>           },
> >>           {
> >>              "parameter": "compress-threads",
> >>              "value": 8
> >>           },
> >>           {
> >>              "parameter": "decompress-threads",
> >>              "value": 2
> >>           }
> >>        ]
> >>     }
> >>
> >> I'd very much prefer a simple object instead:
> >>
> >>     { "return": { "compress-level": 1,
> >>                   "compress-threads": 8,
> >>                   "decompress-threads": 2 } }
> >>
> >
> > The same reason.
> 
> { 'command': 'query-migrate-parameters',
>   'returns': 'MigrationParameters' }
> 
> { 'type': 'MigrationParameters',
>   'data': { 'compression-level': 'int',
>             'compression-threads': 'int,
>             'decompression-threads': 'int' } }
> 
> Add one as above:
> 
> { 'type': 'MigrationParameters',
>   'data': { 'compression-level': 'int',
>             'compression-threads': 'int,
>             'compression-algorithm': 'CompressionAlgorithm',
>             'decompression-threads': 'int', } }
> 
> Why wouldn't that work?

Could you be kind to point out what changes should I make?

On 03/18/2015 08:30 PM, Li, Liang Z wrote:

>>> +'migrate-set-parameters',
>>> +  'data': { 'parameters': ['MigrationParameterStatus'] } }
>>
>> The command takes a list of key-value pairs.  Looks like this (example stolen
>> from your patch to qmp-commands.hx):
>>
>>     { "execute": "migrate-set-parameters",
>>       "arguments": { "parameters":
>>                      [ { "parameter": "compress-level", "value": 1 } ] } }
>>
>> Awkward.  I'd very much prefer
>>
>>     { "execute": "migrate-set-parameters",
>>       "arguments": { "compress-level", 1 } }
>>
>> I.e. the command simply takes the parameters as optional arguments.
>> Simpler, and a natural use of the schema language.

Indeed, if we are going to add a new command, then we don't need to
worry about making it super-generic, and can avoid the nesting of
complex types.

> 
> Yes, it seems complicated.  Eric suggested to use this type of interface, because it can 
> support different type of parameters if some new parameters will be added.  

When I originally suggested complex nested types, I was suggesting that
we _reuse_ the existing migrate-set-capabilities (by making it set both
boolean and integer capabilities in one go).  If we do that, then we
need the struct complexity.  But since we are proposing a new command
rather than shoe-horning into an existing command, we might as well do
it cleanly to begin with.  So I like Markus' suggestion that we
eliminate some of the complexity and just go with specifying parameters
directly.


>> I'd very much prefer a simple object instead:
>>
>>     { "return": { "compress-level": 1,
>>                   "compress-threads": 8,
>>                   "decompress-threads": 2 } }
>>
> 
> The same reason.

I like the idea as well.  Sorry for misleading you into a more complex
initial implementation when a simpler will do.

> > >> The command takes a list of key-value pairs.  Looks like this
> > >> (example stolen from your patch to qmp-commands.hx):
> > >>
> > >>     { "execute": "migrate-set-parameters",
> > >>       "arguments": { "parameters":
> > >>                      [ { "parameter": "compress-level", "value": 1
> > >> } ] } }
> > >>
> > >> Awkward.  I'd very much prefer
> > >>
> > >>     { "execute": "migrate-set-parameters",
> > >>       "arguments": { "compress-level", 1 } }
> > >>
> > >> I.e. the command simply takes the parameters as optional arguments.
> > >> Simpler, and a natural use of the schema language.
> > >
> > > Yes, it seems complicated.  Eric suggested to use this type of
> > > interface, because it can support different type of parameters if
> > > some new parameters will be added.
> >
> > I don't understand.
> >
> > Schema for the three parameters we have now:
> >
> > { 'command': 'migrate-set-parameters',
> >   'data': { '*compression-level': 'int',
> >             '*compression-threads': 'int,
> >             '*decompression-threads': 'int' } }
> >
> > Let's add a parameter with some other type:
> >
> > { 'command': 'migrate-set-parameters',
> >   'data': { '*compression-level': 'int',
> >             '*compression-threads': 'int,
> >             '*compression-algorithm': 'CompressionAlgorithm',
> >             '*decompression-threads': 'int', } }
> >
> > CompressionAlgorithm could be an enum.
> >
> > Why wouldn't that work?
> 
> To be honest, I know very little about QMP and JSON. What should I do to
> make the schema as you described?
> 

You have answered me, sorry!