| Message ID | 20260902221547.1812481-14-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | migration: MigrationParameters changes | expand |
On Wed, Sep 02, 2026 at 07:15:41PM -0300, Fabiano Rosas wrote: > 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. > > Note that this was not a for-free improvement, the HMP command format > output was changed incompatibly in a previous patch. It doesn't output > units anymore. IIUC both qlist and qdict are only used in BitmapMigrationNodeAlias only. It would be nice to attach an example of HMP dump before/after the change of these two patches, either in previous patch or here (assuming the layout changed once). In case HMP has some suggestion in general, I suggest copy Dave when repost in case he has something to say. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > migration/migration-hmp-cmds.c | 243 ++++++++++++++------------------- > 1 file changed, 100 insertions(+), 143 deletions(-) > > diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c > index 089c6d4ff46..dff69650a0c 100644 > --- a/migration/migration-hmp-cmds.c > +++ b/migration/migration-hmp-cmds.c > @@ -25,7 +25,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" > @@ -316,170 +321,122 @@ void hmp_info_migrate_capabilities(Monitor *mon, const QDict *qdict) > qapi_free_MigrationCapabilityStatusList(caps); > } > > -static void monitor_print_cpr_exec_command(Monitor *mon, strList *args) > +static QDict *migrate_params_to_dict(MigrationParameters *p, Error **errp) > { > - monitor_printf(mon, "%s:", > - MigrationParameter_str(MIGRATION_PARAMETER_CPR_EXEC_COMMAND)); > + QObject *obj = NULL; > + Visitor *v = qobject_output_visitor_new(&obj); > > - while (args) { > - monitor_printf(mon, " %s", args->value); > - args = args->next; > + if (visit_type_MigrationParameters(v, NULL, &p, errp)) { > + visit_complete(v, &obj); > } > - monitor_printf(mon, "\n"); > + visit_free(v); > + return qobject_to(QDict, obj); > } This exact function also exists in options.c (with the same name and impl). Can reuse. > > -void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) > +static void hmp_migrate_print_qobject(Monitor *mon, const char *label, > + QObject *obj) > { > - MigrationParameters *params; > - MigrationState *s = migrate_get_current(); > + const char *sep; > > - params = qmp_query_migrate_parameters(NULL); > + if (!obj) { > + return; > + } > > - if (params) { > - monitor_printf(mon, "%s: %" PRIu64 "\n", > - MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_INITIAL), > - params->announce_initial); > - monitor_printf(mon, "%s: %" PRIu64 "\n", > - MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_MAX), > - params->announce_max); > - monitor_printf(mon, "%s: %" PRIu64 "\n", > - MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_ROUNDS), > - params->announce_rounds); > - monitor_printf(mon, "%s: %" PRIu64 "\n", > - MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_STEP), > - params->announce_step); > - assert(params->has_throttle_trigger_threshold); > - monitor_printf(mon, "%s: %u\n", > - MigrationParameter_str(MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD), > - params->throttle_trigger_threshold); > - assert(params->has_cpu_throttle_initial); > - monitor_printf(mon, "%s: %u\n", > - MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL), > - params->cpu_throttle_initial); > - assert(params->has_cpu_throttle_increment); > - monitor_printf(mon, "%s: %u\n", > - MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT), > - params->cpu_throttle_increment); > - assert(params->has_cpu_throttle_tailslow); > - monitor_printf(mon, "%s: %s\n", > - MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW), > - params->cpu_throttle_tailslow ? "on" : "off"); > - assert(params->has_max_cpu_throttle); > - monitor_printf(mon, "%s: %u\n", > - MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE), > - params->max_cpu_throttle); > - assert(params->tls_creds); > - monitor_printf(mon, "%s: %s\n", > - MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS), > - params->tls_creds->u.s); > - assert(params->tls_hostname); > - monitor_printf(mon, "%s: %s\n", > - MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME), > - params->tls_hostname->u.s); > - assert(params->tls_authz); > - monitor_printf(mon, "%s: %s\n", > - MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ), > - params->tls_authz->u.s); > - assert(params->has_max_bandwidth); > - monitor_printf(mon, "%s: %" PRIu64 "\n", > - MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH), > - params->max_bandwidth); > - assert(params->has_avail_switchover_bandwidth); > - monitor_printf(mon, "%s: %" PRIu64 "\n", > - MigrationParameter_str(MIGRATION_PARAMETER_AVAIL_SWITCHOVER_BANDWIDTH), > - params->avail_switchover_bandwidth); > - assert(params->has_max_postcopy_bandwidth); > - monitor_printf(mon, "%s: %" PRIu64 "\n", > - MigrationParameter_str(MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH), > - params->max_postcopy_bandwidth); > - assert(params->has_downtime_limit); > - monitor_printf(mon, "%s: %" PRIu64 "\n", > - MigrationParameter_str(MIGRATION_PARAMETER_DOWNTIME_LIMIT), > - params->downtime_limit); > - assert(params->has_x_checkpoint_delay); > - monitor_printf(mon, "%s: %u\n", > - MigrationParameter_str(MIGRATION_PARAMETER_X_CHECKPOINT_DELAY), > - params->x_checkpoint_delay); > - monitor_printf(mon, "%s: %u\n", > - MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_CHANNELS), > - params->multifd_channels); > - monitor_printf(mon, "%s: %s\n", > - MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_COMPRESSION), > - MultiFDCompression_str(params->multifd_compression)); > - assert(params->has_zero_page_detection); > - monitor_printf(mon, "%s: %s\n", > - MigrationParameter_str(MIGRATION_PARAMETER_ZERO_PAGE_DETECTION), > - qapi_enum_lookup(&ZeroPageDetection_lookup, > - params->zero_page_detection)); > - monitor_printf(mon, "%s: %" PRIu64 "\n", > - MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE), > - params->xbzrle_cache_size); > + /* > + * Put a space after labels > + * foo: bar > + * ^ > + */ > + if (label && label[0] && label[strlen(label) - 1] == ':') { > + sep = " "; > + } else { > + sep = ""; > + } > > - if (s->has_block_bitmap_mapping) { > - BitmapMigrationNodeAliasList *nal; > - BitmapMigrationNodeAlias *na; > - BitmapMigrationBitmapAliasList *bal; > - BitmapMigrationBitmapAlias *ba; > - BitmapMigrationBitmapAliasTransform *bat; > + switch (qobject_type(obj)) { > + case QTYPE_NONE: > + g_assert_not_reached(); > + case QTYPE_QNULL: > + break; Shall we assert too if we don't expect it? > + case QTYPE_QNUM: { > + int64_t i64; > > - monitor_printf(mon, "%s:", > - MigrationParameter_str( > - MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING)); > + if (qnum_get_try_int(qobject_to(QNum, obj), &i64)) { > + monitor_printf(mon, "%s%s%" PRId64, label, sep, i64); Should we use qnum_to_string() like what dump_qobject() does? Logically any size/u64 parameter (which may not be a normal use case..) can be set larger than INT64_MAX, so qnum_get_try_int() may return false? Here skipping to print anything is benign if it only happens with a malicious monitor client, but I think we should try to capture all cases. > + } > + break; > + } > + case QTYPE_QSTRING: { > + QString *str = qobject_to(QString, obj); > > - for (nal = params->block_bitmap_mapping; nal; nal = nal->next) > - { > - na = nal->value; > - monitor_printf(mon, " bitmaps:"); > - for (bal = na->bitmaps; bal; bal = bal->next) { > - ba = bal->value; > - bat = ba->transform; > + if (str) { Any chance str==NULL here? > + monitor_printf(mon, "%s%s%s", label, sep, qstring_get_str(str)); > + } > + break; > + } > + case QTYPE_QDICT: { > + QDict *d = qobject_to(QDict, obj); > + const QDictEntry *e; > + int i = 0; > > - monitor_printf(mon, " name: %s", ba->name); > - if (bat && bat->has_persistent) { > - if (bat->persistent) { > - monitor_printf(mon, " persistent: on"); > - } else { > - monitor_printf(mon, " persistent: off"); > - } > - } > - monitor_printf(mon, " alias: %s", ba->alias); > + if (d) { > + for (e = qdict_first(d); e; e = qdict_next(d, e), i++) { > + g_autofree char *l = g_strdup_printf("%s:", qdict_entry_key(e)); > + if (i) { > + monitor_printf(mon, " "); > } > - monitor_printf(mon, " node-name: %s alias: %s", > - na->node_name, na->alias); > + hmp_migrate_print_qobject(mon, l, qdict_entry_value(e)); > } > - > - monitor_printf(mon, "\n"); > } > + break; > + } > + case QTYPE_QLIST: { > + QList *l = qobject_to(QList, obj); > + const QListEntry *e; > > - monitor_printf(mon, "%s: %" PRIu64 "\n", > - MigrationParameter_str(MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD), > - params->x_vcpu_dirty_limit_period); > + if (l) { > + monitor_printf(mon, "%s", label); > > - monitor_printf(mon, "%s: %" PRIu64 "\n", > - MigrationParameter_str(MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT), > - params->vcpu_dirty_limit); > + for (e = qlist_first(l); e; e = qlist_next(e)) { > + /* > + * In the first iteration, this is the space after the > + * colon, otherwise it's the space between list > + * elements. > + */ > + monitor_printf(mon, " "); > + hmp_migrate_print_qobject(mon, "", e->value); > + } > + } > + break; > + } > + case QTYPE_QBOOL: { > + QBool *b = qobject_to(QBool, obj); > + if (b) { Similarly, I'd drop "if" if it will always happen, making qbool_get_bool() assert itself by deref. > + monitor_printf(mon, "%s%s%s", label, sep, > + qbool_get_bool(b) ? "on" : "off"); > + } > + break; > + } > + default: > + g_assert_not_reached(); > + break; > + } > +} > > - assert(params->has_mode); > - monitor_printf(mon, "%s: %s\n", > - MigrationParameter_str(MIGRATION_PARAMETER_MODE), > - qapi_enum_lookup(&MigMode_lookup, params->mode)); > +void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) > +{ > + MigrationParameters *params = qmp_query_migrate_parameters(NULL); > + g_autoptr(QDict) d; > + const QDictEntry *e; > > - if (params->has_direct_io) { > - monitor_printf(mon, "%s: %s\n", > - MigrationParameter_str( > - MIGRATION_PARAMETER_DIRECT_IO), > - params->direct_io ? "on" : "off"); > - } > + assert(params); > > - if (params->has_x_rdma_chunk_size) { > - monitor_printf(mon, "%s: %" PRIu64 "\n", > - MigrationParameter_str( > - MIGRATION_PARAMETER_X_RDMA_CHUNK_SIZE), > - params->x_rdma_chunk_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)); > > - assert(params->has_cpr_exec_command); > - monitor_print_cpr_exec_command(mon, params->cpr_exec_command); > + hmp_migrate_print_qobject(mon, label, qdict_entry_value(e)); > + monitor_printf(mon, "\n"); > } > > qapi_free_MigrationParameters(params); > -- > 2.53.0 >
Peter Xu <peterx@redhat.com> writes: > On Wed, Sep 02, 2026 at 07:15:41PM -0300, Fabiano Rosas wrote: >> 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. >> >> Note that this was not a for-free improvement, the HMP command format >> output was changed incompatibly in a previous patch. It doesn't output >> units anymore. > > IIUC both qlist and qdict are only used in BitmapMigrationNodeAlias only. > It would be nice to attach an example of HMP dump before/after the change > of these two patches, either in previous patch or here (assuming the layout > changed once). In case HMP has some suggestion in general, I suggest copy > Dave when repost in case he has something to say. > Will do. >> >> Signed-off-by: Fabiano Rosas <farosas@suse.de> >> --- >> migration/migration-hmp-cmds.c | 243 ++++++++++++++------------------- >> 1 file changed, 100 insertions(+), 143 deletions(-) >> >> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c >> index 089c6d4ff46..dff69650a0c 100644 >> --- a/migration/migration-hmp-cmds.c >> +++ b/migration/migration-hmp-cmds.c >> @@ -25,7 +25,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" >> @@ -316,170 +321,122 @@ void hmp_info_migrate_capabilities(Monitor *mon, const QDict *qdict) >> qapi_free_MigrationCapabilityStatusList(caps); >> } >> >> -static void monitor_print_cpr_exec_command(Monitor *mon, strList *args) >> +static QDict *migrate_params_to_dict(MigrationParameters *p, Error **errp) >> { >> - monitor_printf(mon, "%s:", >> - MigrationParameter_str(MIGRATION_PARAMETER_CPR_EXEC_COMMAND)); >> + QObject *obj = NULL; >> + Visitor *v = qobject_output_visitor_new(&obj); >> >> - while (args) { >> - monitor_printf(mon, " %s", args->value); >> - args = args->next; >> + if (visit_type_MigrationParameters(v, NULL, &p, errp)) { >> + visit_complete(v, &obj); >> } >> - monitor_printf(mon, "\n"); >> + visit_free(v); >> + return qobject_to(QDict, obj); >> } > > This exact function also exists in options.c (with the same name and > impl). Can reuse. > Of course! I just forgot to do it. Thanks. >> >> -void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) >> +static void hmp_migrate_print_qobject(Monitor *mon, const char *label, >> + QObject *obj) >> { >> - MigrationParameters *params; >> - MigrationState *s = migrate_get_current(); >> + const char *sep; >> >> - params = qmp_query_migrate_parameters(NULL); >> + if (!obj) { >> + return; >> + } >> >> - if (params) { >> - monitor_printf(mon, "%s: %" PRIu64 "\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_INITIAL), >> - params->announce_initial); >> - monitor_printf(mon, "%s: %" PRIu64 "\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_MAX), >> - params->announce_max); >> - monitor_printf(mon, "%s: %" PRIu64 "\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_ROUNDS), >> - params->announce_rounds); >> - monitor_printf(mon, "%s: %" PRIu64 "\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_STEP), >> - params->announce_step); >> - assert(params->has_throttle_trigger_threshold); >> - monitor_printf(mon, "%s: %u\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD), >> - params->throttle_trigger_threshold); >> - assert(params->has_cpu_throttle_initial); >> - monitor_printf(mon, "%s: %u\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL), >> - params->cpu_throttle_initial); >> - assert(params->has_cpu_throttle_increment); >> - monitor_printf(mon, "%s: %u\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT), >> - params->cpu_throttle_increment); >> - assert(params->has_cpu_throttle_tailslow); >> - monitor_printf(mon, "%s: %s\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW), >> - params->cpu_throttle_tailslow ? "on" : "off"); >> - assert(params->has_max_cpu_throttle); >> - monitor_printf(mon, "%s: %u\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE), >> - params->max_cpu_throttle); >> - assert(params->tls_creds); >> - monitor_printf(mon, "%s: %s\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS), >> - params->tls_creds->u.s); >> - assert(params->tls_hostname); >> - monitor_printf(mon, "%s: %s\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME), >> - params->tls_hostname->u.s); >> - assert(params->tls_authz); >> - monitor_printf(mon, "%s: %s\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ), >> - params->tls_authz->u.s); >> - assert(params->has_max_bandwidth); >> - monitor_printf(mon, "%s: %" PRIu64 "\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH), >> - params->max_bandwidth); >> - assert(params->has_avail_switchover_bandwidth); >> - monitor_printf(mon, "%s: %" PRIu64 "\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_AVAIL_SWITCHOVER_BANDWIDTH), >> - params->avail_switchover_bandwidth); >> - assert(params->has_max_postcopy_bandwidth); >> - monitor_printf(mon, "%s: %" PRIu64 "\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH), >> - params->max_postcopy_bandwidth); >> - assert(params->has_downtime_limit); >> - monitor_printf(mon, "%s: %" PRIu64 "\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_DOWNTIME_LIMIT), >> - params->downtime_limit); >> - assert(params->has_x_checkpoint_delay); >> - monitor_printf(mon, "%s: %u\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_X_CHECKPOINT_DELAY), >> - params->x_checkpoint_delay); >> - monitor_printf(mon, "%s: %u\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_CHANNELS), >> - params->multifd_channels); >> - monitor_printf(mon, "%s: %s\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_COMPRESSION), >> - MultiFDCompression_str(params->multifd_compression)); >> - assert(params->has_zero_page_detection); >> - monitor_printf(mon, "%s: %s\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_ZERO_PAGE_DETECTION), >> - qapi_enum_lookup(&ZeroPageDetection_lookup, >> - params->zero_page_detection)); >> - monitor_printf(mon, "%s: %" PRIu64 "\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE), >> - params->xbzrle_cache_size); >> + /* >> + * Put a space after labels >> + * foo: bar >> + * ^ >> + */ >> + if (label && label[0] && label[strlen(label) - 1] == ':') { >> + sep = " "; >> + } else { >> + sep = ""; >> + } >> >> - if (s->has_block_bitmap_mapping) { >> - BitmapMigrationNodeAliasList *nal; >> - BitmapMigrationNodeAlias *na; >> - BitmapMigrationBitmapAliasList *bal; >> - BitmapMigrationBitmapAlias *ba; >> - BitmapMigrationBitmapAliasTransform *bat; >> + switch (qobject_type(obj)) { >> + case QTYPE_NONE: >> + g_assert_not_reached(); >> + case QTYPE_QNULL: >> + break; > > Shall we assert too if we don't expect it? > Could be, I'll check. >> + case QTYPE_QNUM: { >> + int64_t i64; >> >> - monitor_printf(mon, "%s:", >> - MigrationParameter_str( >> - MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING)); >> + if (qnum_get_try_int(qobject_to(QNum, obj), &i64)) { >> + monitor_printf(mon, "%s%s%" PRId64, label, sep, i64); > > Should we use qnum_to_string() like what dump_qobject() does? > I missed that helper, let me see. > Logically any size/u64 parameter (which may not be a normal use case..) can > be set larger than INT64_MAX, so qnum_get_try_int() may return false? Here > skipping to print anything is benign if it only happens with a malicious > monitor client, but I think we should try to capture all cases. > Yeah, makes sense. >> + } >> + break; >> + } >> + case QTYPE_QSTRING: { >> + QString *str = qobject_to(QString, obj); >> >> - for (nal = params->block_bitmap_mapping; nal; nal = nal->next) >> - { >> - na = nal->value; >> - monitor_printf(mon, " bitmaps:"); >> - for (bal = na->bitmaps; bal; bal = bal->next) { >> - ba = bal->value; >> - bat = ba->transform; >> + if (str) { > > Any chance str==NULL here? > Yes. But I get your point, see below. >> + monitor_printf(mon, "%s%s%s", label, sep, qstring_get_str(str)); >> + } >> + break; >> + } >> + case QTYPE_QDICT: { >> + QDict *d = qobject_to(QDict, obj); >> + const QDictEntry *e; >> + int i = 0; >> >> - monitor_printf(mon, " name: %s", ba->name); >> - if (bat && bat->has_persistent) { >> - if (bat->persistent) { >> - monitor_printf(mon, " persistent: on"); >> - } else { >> - monitor_printf(mon, " persistent: off"); >> - } >> - } >> - monitor_printf(mon, " alias: %s", ba->alias); >> + if (d) { >> + for (e = qdict_first(d); e; e = qdict_next(d, e), i++) { >> + g_autofree char *l = g_strdup_printf("%s:", qdict_entry_key(e)); >> + if (i) { >> + monitor_printf(mon, " "); >> } >> - monitor_printf(mon, " node-name: %s alias: %s", >> - na->node_name, na->alias); >> + hmp_migrate_print_qobject(mon, l, qdict_entry_value(e)); >> } >> - >> - monitor_printf(mon, "\n"); >> } >> + break; >> + } >> + case QTYPE_QLIST: { >> + QList *l = qobject_to(QList, obj); >> + const QListEntry *e; >> >> - monitor_printf(mon, "%s: %" PRIu64 "\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD), >> - params->x_vcpu_dirty_limit_period); >> + if (l) { >> + monitor_printf(mon, "%s", label); >> >> - monitor_printf(mon, "%s: %" PRIu64 "\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT), >> - params->vcpu_dirty_limit); >> + for (e = qlist_first(l); e; e = qlist_next(e)) { >> + /* >> + * In the first iteration, this is the space after the >> + * colon, otherwise it's the space between list >> + * elements. >> + */ >> + monitor_printf(mon, " "); >> + hmp_migrate_print_qobject(mon, "", e->value); >> + } >> + } >> + break; >> + } >> + case QTYPE_QBOOL: { >> + QBool *b = qobject_to(QBool, obj); >> + if (b) { > > Similarly, I'd drop "if" if it will always happen, making qbool_get_bool() > assert itself by deref. > I'd rather not have such asserts in user-facing code. Even with testing, it's hard to ensure this 'obj' will reach here in integrity. >> + monitor_printf(mon, "%s%s%s", label, sep, >> + qbool_get_bool(b) ? "on" : "off"); >> + } >> + break; >> + } >> + default: >> + g_assert_not_reached(); >> + break; >> + } >> +} >> >> - assert(params->has_mode); >> - monitor_printf(mon, "%s: %s\n", >> - MigrationParameter_str(MIGRATION_PARAMETER_MODE), >> - qapi_enum_lookup(&MigMode_lookup, params->mode)); >> +void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) >> +{ >> + MigrationParameters *params = qmp_query_migrate_parameters(NULL); >> + g_autoptr(QDict) d; >> + const QDictEntry *e; >> >> - if (params->has_direct_io) { >> - monitor_printf(mon, "%s: %s\n", >> - MigrationParameter_str( >> - MIGRATION_PARAMETER_DIRECT_IO), >> - params->direct_io ? "on" : "off"); >> - } >> + assert(params); >> >> - if (params->has_x_rdma_chunk_size) { >> - monitor_printf(mon, "%s: %" PRIu64 "\n", >> - MigrationParameter_str( >> - MIGRATION_PARAMETER_X_RDMA_CHUNK_SIZE), >> - params->x_rdma_chunk_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)); >> >> - assert(params->has_cpr_exec_command); >> - monitor_print_cpr_exec_command(mon, params->cpr_exec_command); >> + hmp_migrate_print_qobject(mon, label, qdict_entry_value(e)); >> + monitor_printf(mon, "\n"); >> } >> >> qapi_free_MigrationParameters(params); >> -- >> 2.53.0 >>
On Fri, Sep 04, 2026 at 10:41:21AM -0300, Fabiano Rosas wrote: > >> + case QTYPE_QBOOL: { > >> + QBool *b = qobject_to(QBool, obj); > >> + if (b) { > > > > Similarly, I'd drop "if" if it will always happen, making qbool_get_bool() > > assert itself by deref. > > > > I'd rather not have such asserts in user-facing code. Even with testing, > it's hard to ensure this 'obj' will reach here in integrity. Not a big deal here, but just for sake of pure discussion.. IMHO it's not the "user triggerable path" that we are avoiding assert()s, but user input that may affect the result of the assert(). Here if we just checked obj type is QTYPE_QBOOL, I can't see anything that can make this if not true. I still think assert() good guarding programming errors. Say, if something we wanted to print here but skipped, I want it to crash hard, rather than silently ignored.
Peter Xu <peterx@redhat.com> writes: > On Fri, Sep 04, 2026 at 10:41:21AM -0300, Fabiano Rosas wrote: >> >> + case QTYPE_QBOOL: { >> >> + QBool *b = qobject_to(QBool, obj); >> >> + if (b) { >> > >> > Similarly, I'd drop "if" if it will always happen, making qbool_get_bool() >> > assert itself by deref. >> > >> >> I'd rather not have such asserts in user-facing code. Even with testing, >> it's hard to ensure this 'obj' will reach here in integrity. > > Not a big deal here, but just for sake of pure discussion.. > > IMHO it's not the "user triggerable path" that we are avoiding assert()s, > but user input that may affect the result of the assert(). > In this case, there should be none, agreed. > Here if we just checked obj type is QTYPE_QBOOL, I can't see anything that > can make this if not true. > Thanks for saying it explicitly, I wasn't seeing the obvious redundancy there. You're right, I'll remove the checks. > I still think assert() good guarding programming errors. Say, if something > we wanted to print here but skipped, I want it to crash hard, rather than > silently ignored.
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c index 089c6d4ff46..dff69650a0c 100644 --- a/migration/migration-hmp-cmds.c +++ b/migration/migration-hmp-cmds.c @@ -25,7 +25,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" @@ -316,170 +321,122 @@ void hmp_info_migrate_capabilities(Monitor *mon, const QDict *qdict) qapi_free_MigrationCapabilityStatusList(caps); } -static void monitor_print_cpr_exec_command(Monitor *mon, strList *args) +static QDict *migrate_params_to_dict(MigrationParameters *p, Error **errp) { - monitor_printf(mon, "%s:", - MigrationParameter_str(MIGRATION_PARAMETER_CPR_EXEC_COMMAND)); + QObject *obj = NULL; + Visitor *v = qobject_output_visitor_new(&obj); - while (args) { - monitor_printf(mon, " %s", args->value); - args = args->next; + if (visit_type_MigrationParameters(v, NULL, &p, errp)) { + visit_complete(v, &obj); } - monitor_printf(mon, "\n"); + visit_free(v); + return qobject_to(QDict, obj); } -void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) +static void hmp_migrate_print_qobject(Monitor *mon, const char *label, + QObject *obj) { - MigrationParameters *params; - MigrationState *s = migrate_get_current(); + const char *sep; - params = qmp_query_migrate_parameters(NULL); + if (!obj) { + return; + } - if (params) { - monitor_printf(mon, "%s: %" PRIu64 "\n", - MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_INITIAL), - params->announce_initial); - monitor_printf(mon, "%s: %" PRIu64 "\n", - MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_MAX), - params->announce_max); - monitor_printf(mon, "%s: %" PRIu64 "\n", - MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_ROUNDS), - params->announce_rounds); - monitor_printf(mon, "%s: %" PRIu64 "\n", - MigrationParameter_str(MIGRATION_PARAMETER_ANNOUNCE_STEP), - params->announce_step); - assert(params->has_throttle_trigger_threshold); - monitor_printf(mon, "%s: %u\n", - MigrationParameter_str(MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD), - params->throttle_trigger_threshold); - assert(params->has_cpu_throttle_initial); - monitor_printf(mon, "%s: %u\n", - MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL), - params->cpu_throttle_initial); - assert(params->has_cpu_throttle_increment); - monitor_printf(mon, "%s: %u\n", - MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT), - params->cpu_throttle_increment); - assert(params->has_cpu_throttle_tailslow); - monitor_printf(mon, "%s: %s\n", - MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW), - params->cpu_throttle_tailslow ? "on" : "off"); - assert(params->has_max_cpu_throttle); - monitor_printf(mon, "%s: %u\n", - MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE), - params->max_cpu_throttle); - assert(params->tls_creds); - monitor_printf(mon, "%s: %s\n", - MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS), - params->tls_creds->u.s); - assert(params->tls_hostname); - monitor_printf(mon, "%s: %s\n", - MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME), - params->tls_hostname->u.s); - assert(params->tls_authz); - monitor_printf(mon, "%s: %s\n", - MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ), - params->tls_authz->u.s); - assert(params->has_max_bandwidth); - monitor_printf(mon, "%s: %" PRIu64 "\n", - MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH), - params->max_bandwidth); - assert(params->has_avail_switchover_bandwidth); - monitor_printf(mon, "%s: %" PRIu64 "\n", - MigrationParameter_str(MIGRATION_PARAMETER_AVAIL_SWITCHOVER_BANDWIDTH), - params->avail_switchover_bandwidth); - assert(params->has_max_postcopy_bandwidth); - monitor_printf(mon, "%s: %" PRIu64 "\n", - MigrationParameter_str(MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH), - params->max_postcopy_bandwidth); - assert(params->has_downtime_limit); - monitor_printf(mon, "%s: %" PRIu64 "\n", - MigrationParameter_str(MIGRATION_PARAMETER_DOWNTIME_LIMIT), - params->downtime_limit); - assert(params->has_x_checkpoint_delay); - monitor_printf(mon, "%s: %u\n", - MigrationParameter_str(MIGRATION_PARAMETER_X_CHECKPOINT_DELAY), - params->x_checkpoint_delay); - monitor_printf(mon, "%s: %u\n", - MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_CHANNELS), - params->multifd_channels); - monitor_printf(mon, "%s: %s\n", - MigrationParameter_str(MIGRATION_PARAMETER_MULTIFD_COMPRESSION), - MultiFDCompression_str(params->multifd_compression)); - assert(params->has_zero_page_detection); - monitor_printf(mon, "%s: %s\n", - MigrationParameter_str(MIGRATION_PARAMETER_ZERO_PAGE_DETECTION), - qapi_enum_lookup(&ZeroPageDetection_lookup, - params->zero_page_detection)); - monitor_printf(mon, "%s: %" PRIu64 "\n", - MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE), - params->xbzrle_cache_size); + /* + * Put a space after labels + * foo: bar + * ^ + */ + if (label && label[0] && label[strlen(label) - 1] == ':') { + sep = " "; + } else { + sep = ""; + } - if (s->has_block_bitmap_mapping) { - BitmapMigrationNodeAliasList *nal; - BitmapMigrationNodeAlias *na; - BitmapMigrationBitmapAliasList *bal; - BitmapMigrationBitmapAlias *ba; - BitmapMigrationBitmapAliasTransform *bat; + switch (qobject_type(obj)) { + case QTYPE_NONE: + g_assert_not_reached(); + case QTYPE_QNULL: + break; + case QTYPE_QNUM: { + int64_t i64; - monitor_printf(mon, "%s:", - MigrationParameter_str( - MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING)); + if (qnum_get_try_int(qobject_to(QNum, obj), &i64)) { + monitor_printf(mon, "%s%s%" PRId64, label, sep, i64); + } + break; + } + case QTYPE_QSTRING: { + QString *str = qobject_to(QString, obj); - for (nal = params->block_bitmap_mapping; nal; nal = nal->next) - { - na = nal->value; - monitor_printf(mon, " bitmaps:"); - for (bal = na->bitmaps; bal; bal = bal->next) { - ba = bal->value; - bat = ba->transform; + if (str) { + monitor_printf(mon, "%s%s%s", label, sep, qstring_get_str(str)); + } + break; + } + case QTYPE_QDICT: { + QDict *d = qobject_to(QDict, obj); + const QDictEntry *e; + int i = 0; - monitor_printf(mon, " name: %s", ba->name); - if (bat && bat->has_persistent) { - if (bat->persistent) { - monitor_printf(mon, " persistent: on"); - } else { - monitor_printf(mon, " persistent: off"); - } - } - monitor_printf(mon, " alias: %s", ba->alias); + if (d) { + for (e = qdict_first(d); e; e = qdict_next(d, e), i++) { + g_autofree char *l = g_strdup_printf("%s:", qdict_entry_key(e)); + if (i) { + monitor_printf(mon, " "); } - monitor_printf(mon, " node-name: %s alias: %s", - na->node_name, na->alias); + hmp_migrate_print_qobject(mon, l, qdict_entry_value(e)); } - - monitor_printf(mon, "\n"); } + break; + } + case QTYPE_QLIST: { + QList *l = qobject_to(QList, obj); + const QListEntry *e; - monitor_printf(mon, "%s: %" PRIu64 "\n", - MigrationParameter_str(MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD), - params->x_vcpu_dirty_limit_period); + if (l) { + monitor_printf(mon, "%s", label); - monitor_printf(mon, "%s: %" PRIu64 "\n", - MigrationParameter_str(MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT), - params->vcpu_dirty_limit); + for (e = qlist_first(l); e; e = qlist_next(e)) { + /* + * In the first iteration, this is the space after the + * colon, otherwise it's the space between list + * elements. + */ + monitor_printf(mon, " "); + hmp_migrate_print_qobject(mon, "", e->value); + } + } + break; + } + case QTYPE_QBOOL: { + QBool *b = qobject_to(QBool, obj); + if (b) { + monitor_printf(mon, "%s%s%s", label, sep, + qbool_get_bool(b) ? "on" : "off"); + } + break; + } + default: + g_assert_not_reached(); + break; + } +} - assert(params->has_mode); - monitor_printf(mon, "%s: %s\n", - MigrationParameter_str(MIGRATION_PARAMETER_MODE), - qapi_enum_lookup(&MigMode_lookup, params->mode)); +void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) +{ + MigrationParameters *params = qmp_query_migrate_parameters(NULL); + g_autoptr(QDict) d; + const QDictEntry *e; - if (params->has_direct_io) { - monitor_printf(mon, "%s: %s\n", - MigrationParameter_str( - MIGRATION_PARAMETER_DIRECT_IO), - params->direct_io ? "on" : "off"); - } + assert(params); - if (params->has_x_rdma_chunk_size) { - monitor_printf(mon, "%s: %" PRIu64 "\n", - MigrationParameter_str( - MIGRATION_PARAMETER_X_RDMA_CHUNK_SIZE), - params->x_rdma_chunk_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)); - assert(params->has_cpr_exec_command); - monitor_print_cpr_exec_command(mon, params->cpr_exec_command); + hmp_migrate_print_qobject(mon, label, qdict_entry_value(e)); + monitor_printf(mon, "\n"); } qapi_free_MigrationParameters(params);
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. Note that this was not a for-free improvement, the HMP command format output was changed incompatibly in a previous patch. It doesn't output units anymore. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- migration/migration-hmp-cmds.c | 243 ++++++++++++++------------------- 1 file changed, 100 insertions(+), 143 deletions(-)