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