Re: [PATCH v3 13/20] migration: Use keyval input visitor in HMP set command

[Markus Armbruster <armbru@redhat.com>]

Fabiano Rosas <farosas@suse.de> writes:

> Change the hmp_migrate_set_parameter command to use a keyval input
> visitor.
>
> Currently a string visitor is used and due to limitations of that
> particular visitor's implementation it's necessary to consult the QAPI
> type enum (MigrationParameter_lookup) and call each visit_type_*
> function individually. Which makes using a visitor pointless.
>
> Since there are other visitors implemented properly and generated code
> to iterate the QAPI object, prefer using one of those. The keyval
> input visitor is adequate because HMP provides basically one key and
> one value for each migrate_set_parameter command.
>
> To switch from string_input_visitor to keyval_input_visitor simply put
> the parameter name and value into a dict and invoke
> visit_type_MigrationParameters().
>
> Note that it's not necessary to go through any of the keyval_* code
> because due to the nature of HMP, there's no parsing to do (no '=', no
> ',', etc).
>
> With this the migrate_set_parameters HMP commands will be
> automatically updated anytime a new migration parameter is added.
>
> Note that one parameter, "cpr-exec-command", takes the strList type so
> the HMP string needs to be first converted to a QList.
>
> A small side-effect improvement of this change is that by not using
> the MigrationParameter enum as a means to verify whether the
> user-provided key is a valid parameter name, the message when an
> invalid parameter is given now changes from "invalid value x" to
> "parameter x is unexpected", which is more accurate.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/migration-hmp-cmds.c     | 187 ++++-------------------------
>  tests/qtest/migration/misc-tests.c |   2 +-
>  2 files changed, 26 insertions(+), 163 deletions(-)
>
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index b3422b848a..7491c79a2e 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -25,7 +25,10 @@
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-migration.h"
>  #include "qapi/qapi-visit-migration.h"
> +#include "qapi/qobject-input-visitor.h"
>  #include "qobject/qdict.h"
> +#include "qobject/qlist.h"
> +#include "qobject/qstring.h"
>  #include "qapi/string-input-visitor.h"
>  #include "qapi/string-output-visitor.h"
>  #include "qemu/cutils.h"
> @@ -605,183 +608,43 @@ void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
>  {
>      const char *param = qdict_get_str(qdict, "parameter");
>      const char *valuestr = qdict_get_str(qdict, "value");
> -    Visitor *v = string_input_visitor_new(valuestr);
> -    MigrationParameters *p = g_new0(MigrationParameters, 1);
> -    uint64_t cache_size;
> +    g_autoptr(QDict) input = qdict_new();
> +    g_autoptr(MigrationParameters) p = NULL;
> +    QObject *value;
> +    Visitor *v;
>      Error *err = NULL;
> -    int val;
>  
> -    val = qapi_enum_parse(&MigrationParameter_lookup, param, -1, &err);
> -    if (val < 0) {
> -        goto cleanup;
> -    }
> -
> -    switch (val) {
> -    case MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD:
> -        p->has_throttle_trigger_threshold = true;
> -        visit_type_uint8(v, param, &p->throttle_trigger_threshold, &err);
> -        break;
> -    case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL:
> -        p->has_cpu_throttle_initial = true;
> -        visit_type_uint8(v, param, &p->cpu_throttle_initial, &err);
> -        break;
> -    case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
> -        p->has_cpu_throttle_increment = true;
> -        visit_type_uint8(v, param, &p->cpu_throttle_increment, &err);
> -        break;
> -    case MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW:
> -        p->has_cpu_throttle_tailslow = true;
> -        visit_type_bool(v, param, &p->cpu_throttle_tailslow, &err);
> -        break;
> -    case MIGRATION_PARAMETER_MAX_CPU_THROTTLE:
> -        p->has_max_cpu_throttle = true;
> -        visit_type_uint8(v, param, &p->max_cpu_throttle, &err);
> -        break;
> -    case MIGRATION_PARAMETER_TLS_CREDS:
> -        p->tls_creds = g_new0(StrOrNull, 1);
> -        p->tls_creds->type = QTYPE_QSTRING;
> -        visit_type_str(v, param, &p->tls_creds->u.s, &err);
> -        break;
> -    case MIGRATION_PARAMETER_TLS_HOSTNAME:
> -        p->tls_hostname = g_new0(StrOrNull, 1);
> -        p->tls_hostname->type = QTYPE_QSTRING;
> -        visit_type_str(v, param, &p->tls_hostname->u.s, &err);
> -        break;
> -    case MIGRATION_PARAMETER_TLS_AUTHZ:
> -        p->tls_authz = g_new0(StrOrNull, 1);
> -        p->tls_authz->type = QTYPE_QSTRING;
> -        visit_type_str(v, param, &p->tls_authz->u.s, &err);
> -        break;
> -    case MIGRATION_PARAMETER_MAX_BANDWIDTH:
> -        p->has_max_bandwidth = true;
> -        visit_type_size(v, param, &p->max_bandwidth, &err);
> -        break;
> -    case MIGRATION_PARAMETER_AVAIL_SWITCHOVER_BANDWIDTH:
> -        p->has_avail_switchover_bandwidth = true;
> -        visit_type_size(v, param, &p->avail_switchover_bandwidth, &err);
> -        break;
> -    case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
> -        p->has_downtime_limit = true;
> -        visit_type_size(v, param, &p->downtime_limit, &err);
> -        break;
> -    case MIGRATION_PARAMETER_X_CHECKPOINT_DELAY:
> -        p->has_x_checkpoint_delay = true;
> -        visit_type_uint32(v, param, &p->x_checkpoint_delay, &err);
> -        break;
> -    case MIGRATION_PARAMETER_MULTIFD_CHANNELS:
> -        p->has_multifd_channels = true;
> -        visit_type_uint8(v, param, &p->multifd_channels, &err);
> -        break;
> -    case MIGRATION_PARAMETER_MULTIFD_COMPRESSION:
> -        p->has_multifd_compression = true;
> -        visit_type_MultiFDCompression(v, param, &p->multifd_compression,
> -                                      &err);
> -        break;
> -    case MIGRATION_PARAMETER_MULTIFD_ZLIB_LEVEL:
> -        p->has_multifd_zlib_level = true;
> -        visit_type_uint8(v, param, &p->multifd_zlib_level, &err);
> -        break;
> -    case MIGRATION_PARAMETER_MULTIFD_QATZIP_LEVEL:
> -        p->has_multifd_qatzip_level = true;
> -        visit_type_uint8(v, param, &p->multifd_qatzip_level, &err);
> -        break;
> -    case MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL:
> -        p->has_multifd_zstd_level = true;
> -        visit_type_uint8(v, param, &p->multifd_zstd_level, &err);
> -        break;
> -    case MIGRATION_PARAMETER_ZERO_PAGE_DETECTION:
> -        p->has_zero_page_detection = true;
> -        visit_type_ZeroPageDetection(v, param, &p->zero_page_detection, &err);
> -        break;
> -    case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
> -        p->has_xbzrle_cache_size = true;
> -        if (!visit_type_size(v, param, &cache_size, &err)) {
> -            break;
> -        }
> -        if (cache_size > INT64_MAX || (size_t)cache_size != cache_size) {
> -            error_setg(&err, "Invalid size %s", valuestr);
> -            break;
> -        }
> -        p->xbzrle_cache_size = cache_size;
> -        break;
> -    case MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH:
> -        p->has_max_postcopy_bandwidth = true;
> -        visit_type_size(v, param, &p->max_postcopy_bandwidth, &err);
> -        break;
> -    case MIGRATION_PARAMETER_ANNOUNCE_INITIAL:
> -        p->has_announce_initial = true;
> -        visit_type_size(v, param, &p->announce_initial, &err);
> -        break;
> -    case MIGRATION_PARAMETER_ANNOUNCE_MAX:
> -        p->has_announce_max = true;
> -        visit_type_size(v, param, &p->announce_max, &err);
> -        break;
> -    case MIGRATION_PARAMETER_ANNOUNCE_ROUNDS:
> -        p->has_announce_rounds = true;
> -        visit_type_size(v, param, &p->announce_rounds, &err);
> -        break;
> -    case MIGRATION_PARAMETER_ANNOUNCE_STEP:
> -        p->has_announce_step = true;
> -        visit_type_size(v, param, &p->announce_step, &err);
> -        break;
> -    case MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING:
> -        error_setg(&err, "The block-bitmap-mapping parameter can only be set "
> -                   "through QMP");
> -        break;
> -    case MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD:
> -        p->has_x_vcpu_dirty_limit_period = true;
> -        visit_type_size(v, param, &p->x_vcpu_dirty_limit_period, &err);
> -        break;
> -    case MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT:
> -        p->has_vcpu_dirty_limit = true;
> -        visit_type_size(v, param, &p->vcpu_dirty_limit, &err);
> -        break;
> -    case MIGRATION_PARAMETER_MODE:
> -        p->has_mode = true;
> -        visit_type_MigMode(v, param, &p->mode, &err);
> -        break;
> -    case MIGRATION_PARAMETER_DIRECT_IO:
> -        p->has_direct_io = true;
> -        visit_type_bool(v, param, &p->direct_io, &err);
> -        break;
> -    case MIGRATION_PARAMETER_X_RDMA_CHUNK_SIZE:
> -        p->has_x_rdma_chunk_size = true;
> -        visit_type_size(v, param, &p->x_rdma_chunk_size, &err);
> -        break;
> -    case MIGRATION_PARAMETER_CPR_EXEC_COMMAND: {
> -        /*
> -         * NOTE: g_autofree will only auto g_free() the strv array when
> -         * needed, it will not free the strings within the array. It's
> -         * intentional: when strv is set, the ownership of the strings will
> -         * always be passed to p->cpr_exec_command via QAPI_LIST_APPEND().
> -         */
> +    if (g_str_equal(param, "block-bitmap-mapping")) {
> +        error_setg(&err, "The %s parameter can only be set through QMP", param);
> +        goto err;
> +    } else if (g_str_equal(param, "cpr-exec-command")) {
>          g_autofree char **strv = NULL;

Not this patch's doing: initialization looks superfluous.

>          g_autoptr(GError) gerr = NULL;
> -        strList **tail = &p->cpr_exec_command;
> +        QList *list;
>  
>          if (!g_shell_parse_argv(valuestr, NULL, &strv, &gerr)) {
>              error_setg(&err, "%s", gerr->message);
> -            break;
> +            goto err;
>          }
> +
> +        list = qlist_new();
>          for (int i = 0; strv[i]; i++) {
> -            QAPI_LIST_APPEND(tail, strv[i]);
> +            qlist_append_str(list, strv[i]);
> +            g_free(strv[i]);
>          }
> -        p->has_cpr_exec_command = true;
> -        break;
> -    }
> -    default:
> -        g_assert_not_reached();
> +        value = QOBJECT(list);
> +    } else {
> +        value = QOBJECT(qstring_from_str(valuestr));
>      }
>  
> -    if (err) {
> -        goto cleanup;
> +    qdict_put_obj(input, param, value);
> +    v = qobject_input_visitor_new_keyval(QOBJECT(input));
> +    if (visit_type_MigrationParameters(v, NULL, &p, &err)) {
> +        qmp_migrate_set_parameters(p, &err);
>      }
>  
> -    qmp_migrate_set_parameters(p, &err);
> -
> - cleanup:
> -    qapi_free_MigrationParameters(p);
>      visit_free(v);
> +err:
>      hmp_handle_error(hmp, err);
>  }
>  
> diff --git a/tests/qtest/migration/misc-tests.c b/tests/qtest/migration/misc-tests.c
> index 182a8518a9..49280e5bc9 100644
> --- a/tests/qtest/migration/misc-tests.c
> +++ b/tests/qtest/migration/misc-tests.c
> @@ -148,7 +148,7 @@ static void test_hmp_migration_parameters(char *name, MigrateCommon *args)
>      HMPTestData test_cases[] = {
>          TEST("", "", "migrate_set_parameter: string expected"),
>          TEST("foo", "", "migrate_set_parameter: string expected"),
> -        TEST("foo", "on", "Error: invalid parameter value: foo"),
> +        TEST("foo", "on", "Error: Parameter 'foo' is unexpected"),
>  
>          /* bool */
>          TEST("cpu-throttle-tailslow", "on", "on"),

Reviewed-by: Markus Armbruster <armbru@redhat.com>