On Tue, 31 Jul 2012 10:46:02 +0300
Orit Wasserman <owass...@redhat.com> wrote:
> On 07/30/2012 08:41 PM, Luiz Capitulino wrote:
> > On Sun, 29 Jul 2012 12:42:54 +0300
> > Orit Wasserman <owass...@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 <owass...@redhat.com>
> >> Signed-off-by: Juan Quintela <quint...@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?
> 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
> >> -------------
> >>
> >
>
