Re: [Qemu-devel] [PATCH 02/11] Add migrate_set_parameter and query-migrate-parameters

[Orit Wasserman <owasserm@redhat.com>]

On 07/30/2012 08:41 PM, Luiz Capitulino wrote:
> On Sun, 29 Jul 2012 12:42:54 +0300
> Orit Wasserman <owasserm@redhat.com> wrote:
> 
>> The management can enable/disable a capability for the next migration by using
>> migrate_set_parameter command.
>> The management can query the current migration capabilities using
>> query-migrate-parameters
> 
> In general looks good to me, I have a question and a few nitpick comments
> below.
> 
>>
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hmp-commands.hx  |   16 ++++++++++++
>>  hmp.c            |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  hmp.h            |    2 +
>>  migration.c      |   42 +++++++++++++++++++++++++++++++-
>>  migration.h      |    2 +
>>  monitor.c        |    7 +++++
>>  qapi-schema.json |   32 ++++++++++++++++++++++++
>>  qmp-commands.hx  |   60 ++++++++++++++++++++++++++++++++++++++++++++-
>>  8 files changed, 229 insertions(+), 3 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 8786148..3e15338 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -861,6 +861,20 @@ Set maximum tolerated downtime (in seconds) for migration.
>>  ETEXI
>>  
>>      {
>> +        .name       = "migrate_set_parameter",
>> +        .args_type  = "capability:s,state:b",
>> +        .params     = "capability state",
>> +        .help       = "Enable/Disable the usage of a capability for migration",
>> +        .mhandler.cmd = hmp_migrate_set_parameter,
>> +    },
>> +
>> +STEXI
>> +@item migrate_set_parameter @var{capability} @var{state}
>> +@findex migrate_set_parameter
>> +Enable/Disable the usage of a capability @var{capability} 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",
>> @@ -1419,6 +1433,8 @@ show user network stack connection states
>>  show migration status
>>  @item info migration_capabilities
>>  show migration capabilities
>> +@item info migrate_parameters
>> +show current migration parameters
>>  @item info balloon
>>  show balloon information
>>  @item info qtree
>> diff --git a/hmp.c b/hmp.c
>> index 5c7d0be..f2f63fd 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -131,8 +131,22 @@ void hmp_info_mice(Monitor *mon)
>>  void hmp_info_migrate(Monitor *mon)
>>  {
>>      MigrationInfo *info;
>> +    MigrationCapabilityStatusList *cap;
>> +    MigrationParameters *params;
>>  
>>      info = qmp_query_migrate(NULL);
>> +    params = qmp_query_migrate_parameters(NULL);
>> +
>> +    /* do not display parameters during setup */
>> +    if (info->has_status && params->capabilities) {
>> +        monitor_printf(mon, "capabilities: ");
>> +        for (cap = params->capabilities; cap; cap = cap->next) {
>> +            monitor_printf(mon, "%s: %s ",
>> +                           MigrationCapability_lookup[cap->value->capability],
>> +                           cap->value->state ? "on" : "off");
>> +        }
>> +        monitor_printf(mon, "\n");
>> +    }
>>  
>>      if (info->has_status) {
>>          monitor_printf(mon, "Migration status: %s\n", info->status);
>> @@ -159,6 +173,7 @@ void hmp_info_migrate(Monitor *mon)
>>      }
>>  
>>      qapi_free_MigrationInfo(info);
>> +    qapi_free_MigrationParameters(params);
>>  }
>>  
>>  void hmp_info_migration_capabilities(Monitor *mon)
>> @@ -180,6 +195,27 @@ void hmp_info_migration_capabilities(Monitor *mon)
>>      qapi_free_MigrationCapabilityStatusList(caps_list);
>>  }
>>  
>> +void hmp_info_migrate_parameters(Monitor *mon)
>> +{
>> +
>> +    MigrationCapabilityStatusList *cap;
>> +    MigrationParameters *params;
>> +
>> +    params = qmp_query_migrate_parameters(NULL);
>> +
>> +    if (params->capabilities) {
>> +        monitor_printf(mon, "capabilities: ");
>> +        for (cap = params->capabilities; cap; cap = cap->next) {
>> +            monitor_printf(mon, "%s: %s ",
>> +                           MigrationCapability_lookup[cap->value->capability],
>> +                           cap->value->state ? "on" : "off");
>> +        }
>> +        monitor_printf(mon, "\n");
>> +    }
>> +
>> +    qapi_free_MigrationParameters(params);
>> +}
>> +
>>  void hmp_info_cpus(Monitor *mon)
>>  {
>>      CpuInfoList *cpu_list, *cpu;
>> @@ -754,6 +790,41 @@ void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict)
>>      qmp_migrate_set_speed(value, NULL);
>>  }
>>  
>> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>> +{
>> +    const char *cap = qdict_get_str(qdict, "capability");
>> +    bool state = qdict_get_bool(qdict, "state");
>> +    Error *err = NULL;
>> +    MigrationCapabilityStatusList *params = NULL;
>> +    int i;
>> +
>> +    for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
>> +        if (strcmp(cap, MigrationCapability_lookup[i]) == 0) {
>> +            if (!params) {
>> +                params = g_malloc0(sizeof(*params));
>> +            }
> 
> This if shouldn't be necessary, right?
> 
>> +            params->value = g_malloc0(sizeof(*params->value));
>> +            params->value->capability = i;
>> +            params->value->state = state;
>> +            params->next = NULL;
> 
> nitpick: I'd make the code ready for future capabilities.
> 
>> +            qmp_migrate_set_parameters(params, &err);
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (i == MIGRATION_CAPABILITY_MAX) {
>> +        error_set(&err, QERR_INVALID_PARAMETER, cap);
>> +    }
>> +
>> +    qapi_free_MigrationCapabilityStatusList(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 2fb44ca..ceb2f06 100644
>> --- a/hmp.h
>> +++ b/hmp.h
>> @@ -26,6 +26,7 @@ void hmp_info_chardev(Monitor *mon);
>>  void hmp_info_mice(Monitor *mon);
>>  void hmp_info_migrate(Monitor *mon);
>>  void hmp_info_migration_capabilities(Monitor *mon);
>> +void hmp_info_migrate_parameters(Monitor *mon);
>>  void hmp_info_cpus(Monitor *mon);
>>  void hmp_info_block(Monitor *mon);
>>  void hmp_info_blockstats(Monitor *mon);
>> @@ -52,6 +53,7 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
>>  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_parameter(Monitor *mon, const QDict *qdict);
>>  void hmp_set_password(Monitor *mon, const QDict *qdict);
>>  void hmp_expire_password(Monitor *mon, const QDict *qdict);
>>  void hmp_eject(Monitor *mon, const QDict *qdict);
>> diff --git a/migration.c b/migration.c
>> index 8c27347..e844290 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -113,6 +113,25 @@ uint64_t migrate_max_downtime(void)
>>      return max_downtime;
>>  }
>>  
>> +MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>> +{
>> +    MigrationParameters *params = g_malloc0(sizeof(*params));
>> +    MigrationState *s = migrate_get_current();
>> +    int i;
>> +
>> +    params->capabilities = g_malloc0(sizeof(*params->capabilities));
>> +
>> +    for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
>> +        params->capabilities->value =
>> +            g_malloc(sizeof(*params->capabilities->value));
>> +        params->capabilities->value->capability = i;
>> +        params->capabilities->value->state = s->enabled_capabilities[i];
>> +        params->capabilities->next = NULL;
>> +    }
>> +
>> +    return params;
>> +}
>> +
>>  MigrationInfo *qmp_query_migrate(Error **errp)
>>  {
>>      MigrationInfo *info = g_malloc0(sizeof(*info));
>> @@ -177,6 +196,22 @@ MigrationCapabilityStatusList *qmp_query_migration_capabilities(Error **errp)
>>      return caps_list;
>>  }
>>  
>> +void qmp_migrate_set_parameters(MigrationCapabilityStatusList *params,
>> +                                Error **errp)
>> +{
>> +    MigrationState *s = migrate_get_current();
>> +    MigrationCapabilityStatusList *cap;
>> +
>> +    if (s->state == MIG_STATE_ACTIVE) {
>> +        error_set(errp, QERR_MIGRATION_ACTIVE);
>> +        return;
>> +    }
>> +
>> +    for (cap = params; cap; cap = cap->next) {
>> +        s->enabled_capabilities[cap->value->capability] = cap->value->state;
>> +    }
>> +}
>> +
>>  /* shared migration helpers */
>>  
>>  static int migrate_fd_cleanup(MigrationState *s)
>> @@ -386,12 +421,17 @@ static MigrationState *migrate_init(const MigrationParams *params)
>>  {
>>      MigrationState *s = migrate_get_current();
>>      int64_t bandwidth_limit = s->bandwidth_limit;
>> +    bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
>> +
>> +    memcpy(enabled_capabilities, s->enabled_capabilities,
>> +           sizeof(enabled_capabilities));
> 
> Is s->enabled_capabilities initialized somewhere or are we making xbzrle
> disabled by default?
It is automatically zeroed because it is static, which means all capabilities are
disabled by default

Orit
> 
>>  
>>      memset(s, 0, sizeof(*s));
>>      s->bandwidth_limit = bandwidth_limit;
>>      s->params = *params;
>> +    memcpy(s->enabled_capabilities, enabled_capabilities,
>> +           sizeof(enabled_capabilities));
>>  
>> -    s->bandwidth_limit = bandwidth_limit;
> 
> Unrelated change.
> 
>>      s->state = MIG_STATE_SETUP;
>>      s->total_time = qemu_get_clock_ms(rt_clock);
>>  
>> diff --git a/migration.h b/migration.h
>> index 57572a6..713aae0 100644
>> --- a/migration.h
>> +++ b/migration.h
>> @@ -19,6 +19,7 @@
>>  #include "notify.h"
>>  #include "error.h"
>>  #include "vmstate.h"
>> +#include "qapi-types.h"
>>  
>>  struct MigrationParams {
>>      bool blk;
>> @@ -39,6 +40,7 @@ struct MigrationState
>>      void *opaque;
>>      MigrationParams params;
>>      int64_t total_time;
>> +    bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
>>  };
>>  
>>  void process_incoming_migration(QEMUFile *f);
>> diff --git a/monitor.c b/monitor.c
>> index fd57c5e..e07af97 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -2669,6 +2669,13 @@ static mon_cmd_t info_cmds[] = {
>>          .mhandler.info = hmp_info_migration_capabilities,
>>      },
>>      {
>> +        .name       = "migrate-parameters",
>> +        .args_type  = "",
>> +        .params     = "",
>> +        .help       = "show current migration parameters",
>> +        .mhandler.info = hmp_info_migrate_parameters,
>> +    },
>> +    {
>>          .name       = "balloon",
>>          .args_type  = "",
>>          .params     = "",
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index b4d4dd6..9d6759d 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -345,6 +345,38 @@
>>  { 'command': 'query-migration-capabilities', 'returns': ['MigrationCapabilityStatus'] }
>>  
>>  ##
>> +# @migrate-set-parameters
>> +#
>> +# Enable/Disable the following migration capabilities (like xbzrle)
>> +#
>> +# Since: 1.2
>> +##
>> +{ 'command': 'migrate-set-parameters',
>> +  'data': { 'capabilities': ['MigrationCapabilityStatus'] } }
>> +
>> +##
>> +# @MigrationParameters
>> +#
>> +# @capabilities: @MigrationCapabilityStatus list contain current migration
>> +#                capabilities status
>> +# Since: 1.2
>> +##
>> +{ 'type': 'MigrationParameters',
>> +  'data': {'capabilities': ['MigrationCapabilityStatus']} }
>> +
>> +
>> +##
>> +# @query-migrate-parameters
>> +#
>> +# Returns information about the current migration capabilities status
>> +#
>> +# Returns: @MigrationParameters
>> +#
>> +# Since: 1.2
>> +##
>> +{ 'command': 'query-migrate-parameters', 'returns': 'MigrationParameters' }
>> +
>> +##
>>  # @MouseInfo:
>>  #
>>  # Information about a mouse device.
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index c0ed14c..7f40e2a 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -2081,7 +2081,6 @@ The main json-object contains the following:
>>           - "transferred": amount transferred (json-int)
>>           - "remaining": amount remaining (json-int)
>>           - "total": total (json-int)
>> -
>>  Examples:
>>  
>>  1. Before the first migration
>> @@ -2092,7 +2091,15 @@ Examples:
>>  2. Migration is done and has succeeded
>>  
>>  -> { "execute": "query-migrate" }
>> -<- { "return": { "status": "completed" } }
>> +<- { "return": {
>> +        "status": "completed",
>> +        "ram":{
>> +          "transferred":123,
>> +          "remaining":123,
>> +          "total":246
>> +        }
>> +     }
>> +   }
>>  
>>  3. Migration is done and has failed
>>  
>> @@ -2165,6 +2172,55 @@ EQMP
>>      },
>>  
>>  SQMP
>> +migrate-set-parameters
>> +-------
>> +
>> +Enable/Disable migration capabilities
>> +
>> +- "xbzrle": xbzrle support
>> +
>> +Arguments:
>> +
>> +Example:
>> +
>> +-> { "execute": "migrate-set-parameters" , "arguments":
>> +     { "parameters": [ { "capability": "xbzrle", "state": true } ] } }
>> +
>> +EQMP
>> +
>> +    {
>> +        .name       = "migrate_set_parameters",
>> +        .args_type  = "parameters:O",
>> +        .params     = "capability:s,state:b",
>> +	.mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters,
>> +    },
>> +SQMP
>> +query-migrate-parameters
>> +-------
>> +
>> +Query current migration parameters
>> +
>> +- "capabilities": migration capabilities state
>> +         - "xbzrle" : XBZRLE state (json-bool)
>> +
>> +Arguments:
>> +
>> +Example:
>> +
>> +-> { "execute": "query-migrate-parameters" }
>> +<- { "return": {
>> +        "capabilities" :  [ { "capability" : "xbzrle", "state" : false } ]
>> +     }
>> +   }
>> +EQMP
>> +
>> +    {
>> +        .name       = "query-migrate-parameters",
>> +        .args_type  = "",
>> +        .mhandler.cmd_new = qmp_marshal_input_query_migrate_parameters,
>> +    },
>> +
>> +SQMP
>>  query-balloon
>>  -------------
>>  
>