Fabiano Rosas <[email protected]> writes:
> The hmp_info_migrate_parameters function currently open-codes the
> mon_printf calls for each migration parameter. As with the set command
> in the last patch, this should not be necessary as the QAPI
> infrastructure already has generated code that takes type and struct
> member names into account, including converting _ from C into the '-'
> character as part of parameter names strings.
>
> The current code is also quite painful to rebase if a series has been
> carried for a long time while parameters have been added in master.
>
> Replace all of this with a conversion from MigrationParameters to
> QDict using an output visitor and a loop over the QDict that prints
> per-QAPI-type formatted strings.
>
> Modelled after block/qapi.c:dump_qobject, but with some changes to
> keep the migration command output formatting.
>
> Signed-off-by: Fabiano Rosas <[email protected]>
Old output of info migrate_parameters for me:
announce-initial: 50
announce-max: 550
announce-rounds: 5
announce-step: 100
throttle-trigger-threshold: 50
cpu-throttle-initial: 20
cpu-throttle-increment: 10
cpu-throttle-tailslow: off
max-cpu-throttle: 99
tls-creds:
tls-hostname:
tls-authz:
max-bandwidth: 134217728
avail-switchover-bandwidth: 0
max-postcopy-bandwidth: 0
downtime-limit: 300
x-checkpoint-delay: 20000
multifd-channels: 2
multifd-compression: none
zero-page-detection: multifd
xbzrle-cache-size: 67108864
x-vcpu-dirty-limit-period: 1000
vcpu-dirty-limit: 1
mode: normal
direct-io: off
x-rdma-chunk-size: 1048576
cpr-exec-command:
New output:
cpu-throttle-tailslow: off
xbzrle-cache-size: 67108864
cpu-throttle-initial: 20
announce-max: 550
direct-io: off
avail-switchover-bandwidth: 0
local: off
zero-page-detection: multifd
multifd-qatzip-level: 1
x-rdma-chunk-size: 1048576
multifd-channels: 2
mode: normal
multifd-zstd-level: 1
announce-initial: 50
downtime-limit: 300
tls-authz:
cpr-exec-command:
vcpu-dirty-limit: 1
multifd-compression: none
announce-rounds: 5
announce-step: 100
tls-creds:
x-vcpu-dirty-limit-period: 1000
multifd-zlib-level: 1
max-cpu-throttle: 99
max-postcopy-bandwidth: 0
tls-hostname:
throttle-trigger-threshold: 50
max-bandwidth: 134217728
x-checkpoint-delay: 20000
cpu-throttle-increment: 10
New output has an unwanted blank line after "cpr-exec-command:".
New output adds @local, @multifd-qatzip-level, @multifd-zlib-level,
@multifd-zstd-level. Commit message should point that out.
Order of parameters changes, and not for the better. Hmm.
> ---
> migration/migration-hmp-cmds.c | 234 +++++++++++----------------------
> migration/options.c | 2 +-
> migration/options.h | 1 +
> 3 files changed, 81 insertions(+), 156 deletions(-)
>
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index 7491c79a2e..00d2d310af 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -26,8 +26,12 @@
> #include "qapi/qapi-commands-migration.h"
> #include "qapi/qapi-visit-migration.h"
> #include "qapi/qobject-input-visitor.h"
> +#include "qapi/qobject-output-visitor.h"
> +#include "qobject/qbool.h"
> #include "qobject/qdict.h"
> +#include "qobject/qjson.h"
> #include "qobject/qlist.h"
> +#include "qobject/qnum.h"
> #include "qobject/qstring.h"
> #include "qapi/string-input-visitor.h"
> #include "qapi/string-output-visitor.h"
> @@ -318,171 +322,91 @@ void hmp_info_migrate_capabilities(MonitorHMP *hmp,
> const QDict *qdict)
> qapi_free_MigrationCapabilityStatusList(caps);
> }
>
> -static void monitor_print_cpr_exec_command(MonitorHMP *hmp, strList *args)
> +static void hmp_migrate_print_qobject(MonitorHMP *hmp, const char *label,
> + QObject *obj)
> {
> - monitor_hmp_printf(hmp, "%s:",
> - MigrationParameter_str(MIGRATION_PARAMETER_CPR_EXEC_COMMAND));
> + const char *sep;
>
> - while (args) {
> - monitor_hmp_printf(hmp, " %s", args->value);
> - args = args->next;
> + if (!obj) {
> + return;
> + }
> +
> + /*
> + * Put a space after labels
> + * foo: bar
> + * ^
> + */
> + if (label && label[0] && label[strlen(label) - 1] == ':') {
> + sep = " ";
> + } else {
> + sep = "";
> + }
Awkward.
Could simply we pass "foo: " instead of "foo"? Hmm, see case
QTYPE_QLIST below.
> +
> + switch (qobject_type(obj)) {
> + case QTYPE_QNUM: {
> + g_autofree char *str = qnum_to_string(qobject_to(QNum, obj));
> +
> + monitor_hmp_printf(hmp, "%s%s%s", label, sep, str);
> + break;
> + }
> + case QTYPE_QSTRING:
> + monitor_hmp_printf(hmp, "%s%s%s", label, sep,
> + qstring_get_str(qobject_to(QString, obj)));
> + break;
> + case QTYPE_QDICT: {
> + QDict *d = qobject_to(QDict, obj);
> + const QDictEntry *e;
> + int i = 0;
> +
> + monitor_hmp_printf(hmp, "%s%s", label, sep);
> +
> + for (e = qdict_first(d); e; e = qdict_next(d, e), i++) {
Printing members in alphabetic order would be much better.
> + g_autofree char *l = g_strdup_printf("%s:", qdict_entry_key(e));
> + if (i) {
> + monitor_hmp_printf(hmp, " ");
> + }
> + hmp_migrate_print_qobject(hmp, l, qdict_entry_value(e));
> + }
> + break;
> + }
dump_qobject() prints nested stuff indented. This function doesn't. I
fear the output will be confusing. Can you show us some example output
with a non-empty QDict?
> + case QTYPE_QLIST: {
> + const QListEntry *e;
> +
> + monitor_hmp_printf(hmp, "%s\n", label);
Here we print @label and not @sep. Why? Can you show us some example
output with a non-empty QList?
> + for (e = qlist_first(qobject_to(QList, obj)); e; e = qlist_next(e)) {
> + hmp_migrate_print_qobject(hmp, " ", e->value);
> + monitor_hmp_printf(hmp, "\n");
> + }
> +
Recommend to scratch the blank line.
> + break;
This case's output ends with a newline, unlike the other cases.
hmp_info_migrate_parameters() adds another newline. That's where the
unwanted blank line mentioned above comes from.
> + }
> + case QTYPE_QBOOL:
> + monitor_hmp_printf(hmp, "%s%s%s", label, sep,
> + qbool_get_bool(qobject_to(QBool, obj)) ?
> + "on" : "off");
> + break;
> + case QTYPE_NONE:
> + case QTYPE_QNULL:
I'd scratch these two lines. Matter of taste.
> + default:
> + g_assert_not_reached();
> + break;
> }
> - monitor_hmp_printf(hmp, "\n");
> }
>
> void hmp_info_migrate_parameters(MonitorHMP *hmp, const QDict *qdict)
> {
> - MigrationParameters *params;
> - MigrationState *s = migrate_get_current();
> + MigrationParameters *params = qmp_query_migrate_parameters(NULL);
> + g_autoptr(QDict) d;
> + const QDictEntry *e;
>
> - params = qmp_query_migrate_parameters(NULL);
> + assert(params);
>
> - if (params) {
> - monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_INITIAL),
> - params->announce_initial);
> - monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_MAX),
> - params->announce_max);
> - monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_ROUNDS),
> - params->announce_rounds);
> - monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_STEP),
> - params->announce_step);
> - assert(params->has_throttle_trigger_threshold);
> - monitor_hmp_printf(hmp, "%s: %u\n",
> -
> MigrationParameter_str(MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD),
> - params->throttle_trigger_threshold);
> - assert(params->has_cpu_throttle_initial);
> - monitor_hmp_printf(hmp, "%s: %u\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL),
> - params->cpu_throttle_initial);
> - assert(params->has_cpu_throttle_increment);
> - monitor_hmp_printf(hmp, "%s: %u\n",
> -
> MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT),
> - params->cpu_throttle_increment);
> - assert(params->has_cpu_throttle_tailslow);
> - monitor_hmp_printf(hmp, "%s: %s\n",
> -
> MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW),
> - params->cpu_throttle_tailslow ? "on" : "off");
> - assert(params->has_max_cpu_throttle);
> - monitor_hmp_printf(hmp, "%s: %u\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE),
> - params->max_cpu_throttle);
> - assert(params->tls_creds);
> - monitor_hmp_printf(hmp, "%s: %s\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS),
> - params->tls_creds->u.s);
> - assert(params->tls_hostname);
> - monitor_hmp_printf(hmp, "%s: %s\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
> - params->tls_hostname->u.s);
> - assert(params->tls_authz);
> - monitor_hmp_printf(hmp, "%s: %s\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
> - params->tls_authz->u.s);
> - assert(params->has_max_bandwidth);
> - monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH),
> - params->max_bandwidth);
> - assert(params->has_avail_switchover_bandwidth);
> - monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
> -
> MigrationParameter_str(MIGRATION_PARAMETER_AVAIL_SWITCHOVER_BANDWIDTH),
> - params->avail_switchover_bandwidth);
> - assert(params->has_max_postcopy_bandwidth);
> - monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
> -
> MigrationParameter_str(MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH),
> - params->max_postcopy_bandwidth);
> - assert(params->has_downtime_limit);
> - monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_DOWNTIME_LIMIT),
> - params->downtime_limit);
> - assert(params->has_x_checkpoint_delay);
> - monitor_hmp_printf(hmp, "%s: %u\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_X_CHECKPOINT_DELAY),
> - params->x_checkpoint_delay);
> - monitor_hmp_printf(hmp, "%s: %u\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_CHANNELS),
> - params->multifd_channels);
> - monitor_hmp_printf(hmp, "%s: %s\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_COMPRESSION),
> - MultiFDCompression_str(params->multifd_compression));
> - assert(params->has_zero_page_detection);
> - monitor_hmp_printf(hmp, "%s: %s\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_ZERO_PAGE_DETECTION),
> - qapi_enum_lookup(&ZeroPageDetection_lookup,
> - params->zero_page_detection));
> - monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
> - params->xbzrle_cache_size);
> + d = migrate_params_to_dict(params, NULL);
> + for (e = qdict_first(d); e; e = qdict_next(d, e)) {
> + g_autofree char *label = g_strdup_printf("%s:", qdict_entry_key(e));
>
> - if (s->has_block_bitmap_mapping) {
> - BitmapMigrationNodeAliasList *nal;
> - BitmapMigrationNodeAlias *na;
> - BitmapMigrationBitmapAliasList *bal;
> - BitmapMigrationBitmapAlias *ba;
> - BitmapMigrationBitmapAliasTransform *bat;
> -
> - monitor_hmp_printf(hmp, "%s:\n",
> - MigrationParameter_str(
> - MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING));
> -
> - for (nal = params->block_bitmap_mapping; nal; nal = nal->next)
> - {
> - na = nal->value;
> - monitor_hmp_printf(hmp, " bitmaps:\n");
> - for (bal = na->bitmaps; bal; bal = bal->next) {
> - ba = bal->value;
> - bat = ba->transform;
> -
> - monitor_hmp_printf(hmp, " name: %s", ba->name);
> - if (bat && bat->has_persistent) {
> - monitor_hmp_printf(hmp, " transform:");
> - if (bat->persistent) {
> - monitor_hmp_printf(hmp, " persistent: on");
> - } else {
> - monitor_hmp_printf(hmp, " persistent: off");
> - }
> - }
> - monitor_hmp_printf(hmp, " alias: %s\n", ba->alias);
> - }
> - monitor_hmp_printf(hmp, " node-name: %s alias: %s\n",
> - na->node_name, na->alias);
> - }
> -
> - monitor_hmp_printf(hmp, "\n");
> - }
> -
> - monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
> -
> MigrationParameter_str(MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD),
> - params->x_vcpu_dirty_limit_period);
> -
> - monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT),
> - params->vcpu_dirty_limit);
> -
> - assert(params->has_mode);
> - monitor_hmp_printf(hmp, "%s: %s\n",
> - MigrationParameter_str(MIGRATION_PARAMETER_MODE),
> - qapi_enum_lookup(&MigMode_lookup, params->mode));
> -
> - if (params->has_direct_io) {
> - monitor_hmp_printf(hmp, "%s: %s\n",
> - MigrationParameter_str(
> - MIGRATION_PARAMETER_DIRECT_IO),
> - params->direct_io ? "on" : "off");
> - }
> -
> - if (params->has_x_rdma_chunk_size) {
> - monitor_hmp_printf(hmp, "%s: %" PRIu64 "\n",
> - MigrationParameter_str(
> - MIGRATION_PARAMETER_X_RDMA_CHUNK_SIZE),
> - params->x_rdma_chunk_size);
> - }
> -
> - assert(params->has_cpr_exec_command);
> - monitor_print_cpr_exec_command(hmp, params->cpr_exec_command);
> + hmp_migrate_print_qobject(hmp, label, qdict_entry_value(e));
> + monitor_hmp_printf(hmp, "\n");
> }
Why not something like
hmp_migrate_print_qobject(hmp, NULL, d)
?
>
> qapi_free_MigrationParameters(params);
> diff --git a/migration/options.c b/migration/options.c
> index 46d040fc3f..29b96f48c1 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -1097,7 +1097,7 @@ static void tls_opt_to_str(StrOrNull *opt)
> opt->u.s = g_strdup("");
> }
>
> -static QDict *migrate_params_to_dict(MigrationParameters *p, Error **errp)
> +QDict *migrate_params_to_dict(MigrationParameters *p, Error **errp)
> {
> QObject *obj = NULL;
> Visitor *v = qobject_output_visitor_new(&obj);
> diff --git a/migration/options.h b/migration/options.h
> index c7da2d0b5b..0d9f88c11e 100644
> --- a/migration/options.h
> +++ b/migration/options.h
> @@ -94,4 +94,5 @@ uint64_t migrate_rdma_chunk_size(void);
> bool migrate_params_check(MigrationParameters *params, Error **errp);
> void migrate_params_init(MigrationParameters *params);
> bool migrate_params_free(MigrationParameters *params, Error **errp);
> +QDict *migrate_params_to_dict(MigrationParameters *p, Error **errp);
> #endif
