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

[Luiz Capitulino]

On Tue, 31 Jul 2012 10:46:02 +0300
Orit Wasserman <address@hidden> wrote:

> On 07/30/2012 08:41 PM, Luiz Capitulino wrote:
> > On Sun, 29 Jul 2012 12:42:54 +0300
> > Orit Wasserman <address@hidden> 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 <address@hidden>
> >> Signed-off-by: Juan Quintela <address@hidden>
> >> ---
> >>  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
> >> address@hidden migrate_set_parameter @var{capability} @var{state}
> >> address@hidden 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
> >> address@hidden 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?
> You are right I can always allocate it.
> > 
> >> +            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.
> Could you give me a bit more details ?

This is not a proper list, it will break when we add a new capability.

But that comment doesn't apply here, I meant to add that command to
qmp_query_migrate_parameters()'s hunk.

> 
> Orit
> > 
> >> +            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?
> > 
> >>  
> >>      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
> >>  -------------
> >>  
> > 
>