Fabiano Rosas <[email protected]> writes:
> Markus Armbruster <[email protected]> writes:
>
>> Fabiano Rosas <[email protected]> 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.
>>
>> Also, the less the string visitors are used, the happier I am.
>>
>>> 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().
>>
>> This works for scalar types: the value is a QString, and the QObject
>> keyval input visitor automatically converts to the C type the visitor
>> expects. It doesn't work for non-scalar types; see cpr-exec-command
>> below.
>>
>>> 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).
>>
>> migrate_set_parameter syntax isn't keyval, only val is, i.e. its value
>> argument is in keyval value syntax (more or less).
>>
>> "More or less" is my hedge against differences between the string input
>> visitor and the QObject keyval input visitor. Did you check?
>>
>
> They ultimately use the same functions to do the conversion
> (qemu_strtou64, qemu_strtod_finite, etc). The differences are in
> slightly different wording of error messages and the fact that the
> string input visitor has a different message for -ERANGE while the
> qobject input visitor has a TODO for that case.
>
> I think those differences are of no consequence. The error messages that
> we really care about are the ones resulting from the migration code
> validation (because they're informative to the user). The API level
> messages are too generic anyway.
>
>> Aside: we could create migrate_set_parameters with keyval syntax if we
>> cared.
>>
>>> With this the migrate_set_parameters HMP commands will be
>>> automatically updated anytime a new migration parameter is added.
>>>
>>> One parameter, "cpr-exec-command", takes the strList type which needs
>>> to be built manually. This moves to a "legacy" suffixed function.
>>
>> When visit_type_MigrationParameters() visits "cpr-exec-command", it
>> calls visit_type_strList(). With the QObject keyval input visitor, this
>> expects a QList, not a QString.
>>
>> Thus, hmp_migrate_set_parameter() needs to parse the value argument into
>> a list at least. That's why it needs to be a special case.
>>
>
> I'm having some difficulty understanding why isn't that the job of the
> visit function.
>
> I think what you're saying is that we can convert QAPI's strList into
> QObject's QList of QString, but cannot convert the HMP string directly
> into QList even though we know that string represents a strList. I feel
> like there's code missing somewhere...
>
> ... although it wouldn't help with cpr-exec-command because of the
> "shell parsing" semantics. But that's another issue.
>
>> It parses it with g_shell_parse_argv(). GLib docs "specify" this to
>> parse "a command line [...] in much the same way the shell would, but
>> without many of the expansions the shell would perform (variable
>> expansion, globs, operators, filename expansion, etc. are not
>> supported)." Ugh! But I digress.
>>
>
> One side-effect of having code that handles parameters genericly is that
> there is less room for inventing custom parsing when new parameters are
> introduced.
>
>>> Signed-off-by: Fabiano Rosas <[email protected]>
>>> ---
>>> migration/migration-hmp-cmds.c | 195 +++++++----------------------
>>> tests/qtest/migration/misc-tests.c | 2 +-
>>> 2 files changed, 49 insertions(+), 148 deletions(-)
>>>
>>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>>> index 5d754414c4..0d93d38b05 100644
>>> --- a/migration/migration-hmp-cmds.c
>>> +++ b/migration/migration-hmp-cmds.c
>>> @@ -25,7 +25,9 @@
>>> #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/qstring.h"
>>> #include "qapi/string-input-visitor.h"
>>> #include "qapi/string-output-visitor.h"
>>> #include "qemu/cutils.h"
>>> @@ -600,154 +602,14 @@ end:
>>> hmp_handle_error(hmp, err);
>>> }
>>>
>>> -void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
>>> +static void hmp_migrate_set_parameter_legacy(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;
>>> 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: {
>>> + if (g_str_equal(param, "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
>>> @@ -760,15 +622,14 @@ void hmp_migrate_set_parameter(MonitorHMP *hmp, const
>>> QDict *qdict)
>>>
>>> if (!g_shell_parse_argv(valuestr, NULL, &strv, &gerr)) {
>>> error_setg(&err, "%s", gerr->message);
>>> - break;
>>> + goto cleanup;
>>> }
>>> for (int i = 0; strv[i]; i++) {
>>> QAPI_LIST_APPEND(tail, strv[i]);
>>> }
>>> p->has_cpr_exec_command = true;
>>> - break;
>>> - }
>>> - default:
>>> +
>>> + } else {
>>> g_assert_not_reached();
>>> }
>>>
>>> @@ -778,12 +639,52 @@ void hmp_migrate_set_parameter(MonitorHMP *hmp, const
>>> QDict *qdict)
>>>
>>> qmp_migrate_set_parameters(p, &err);
>>>
>>> - cleanup:
>>> +cleanup:
>>> qapi_free_MigrationParameters(p);
>>> + hmp_handle_error(hmp, err);
>>> +}
>>> +
>>> +static void hmp_migrate_set_parameter_qapi(MonitorHMP *hmp, const QDict
>>> *qdict)
>>> +{
>>> + const char *param = qdict_get_str(qdict, "parameter");
>>> + const char *valuestr = qdict_get_str(qdict, "value");
>>> + g_autoptr(QDict) input = qdict_new();
>>> + g_autoptr(MigrationParameters) p = NULL;
>>> + Visitor *v;
>>> + Error *err = NULL;
>>> +
>>> + /* the same as keyval_parse(), but here there's no need to parse */
>>> + qdict_put_obj(input, param, QOBJECT(qstring_from_str(valuestr)));
>>> +
>>> + v = qobject_input_visitor_new_keyval(QOBJECT(input));
>>> + if (visit_type_MigrationParameters(v, NULL, &p, &err)) {
>>> + qmp_migrate_set_parameters(p, &err);
>>> + }
>>> +
>>> visit_free(v);
>>> hmp_handle_error(hmp, err);
>>> }
>>>
>>> +void hmp_migrate_set_parameter(MonitorHMP *hmp, const QDict *qdict)
>>> +{
>>> + const char *param = qdict_get_str(qdict, "parameter");
>>> +
>>> + if (g_str_equal(param, "block-bitmap-mapping")) {
>>> + Error *err = NULL;
>>> +
>>> + error_setg(&err, "The %s parameter can only be set through QMP",
>>> param);
>>> + hmp_handle_error(hmp, err);
>>
>> I think a hmp_report_error(MonitorHMP *hmp, const char *fmt, ...) would
>> be nice to have.
>>
>
> I'll look into it.
>
I see that hmp_handle_error() takes an hmp argument that's unused and
also that callers often do the redundant:
if (err) {
hmp_handle_error(err);
}
But aside from those I don't see other improvements to be made, many
callsites of hmp_handle_error() re-use the Error for other calls,
e.g. block/monitor/block-hmp-cmds.c:
Error *err = NULL;
...
if (!qdict_get_try_str(qdict, "node-name")) {
qobject_unref(qdict);
error_setg(&err, "'node-name' needs to be specified");
goto out;
}
BlockDriverState *bs = bds_tree_init(qdict, &err);
if (!bs) {
goto out;
}
bdrv_set_monitor_owned(bs);
out:
qemu_opts_del(opts);
hmp_handle_error(err);
So having another helper I think would just create confusion as
error_setg() is already a standard way of creating an error.
>>> + return;
>>> + }
>>> +
>>> + /* this has a non-standard setter */
>>> + if (g_str_equal(param, "cpr-exec-command")) {
>>
>> We check for "cpr-exec-command" in two places, here and in
>> hmp_migrate_set_parameter(). A bit awkward.
>>
>>> + return hmp_migrate_set_parameter_legacy(hmp, qdict);
>>> + }
>>> +
>>> + hmp_migrate_set_parameter_qapi(hmp, qdict);
>>> +}
>>
>> I think I'd do this differently.
>>
>> Recall how we normally parse text into a C object:
>>
>> 1. Parse the string into a QObject.
>>
>> 2. Wrap it in the appropriate visitor.
>>
>> 3. Convert it into a C object with visit_type_TYPE().
>>
>> For QMP, step 1 uses json_message_parser_init() & friends, and step 2
>> is qobject_input_visitor_new().
>>
>> For the command line, step 1 and 2 are commonly
>> qobject_input_visitor_new_str(), which either uses qobject_from_json()
>> and qobject_input_visitor_new(), or keyval_parse() and
>> qobject_input_visitor_new_keyval().
>>
>> hmp_migrate_set_parameter() could do step 1 and 2 like this:
>>
>> // step 1
>> if param == "block-bitmap-mapping"
>> report error and return
>> if param == "cpr-exec-command"
>> value = string parsed into QList
>> else // scalars
>> value = string
>> input = {param: value}
>> // step 2
>> v = qobject_input_visitor_new_keyval(input)
>>
>
> Makes sense, let me try it.
>
Works like a charm, thanks!
>>> +
>>> void hmp_migrate_start_postcopy(MonitorHMP *hmp, const QDict *qdict)
>>> {
>>> Error *err = NULL;
>>> diff --git a/tests/qtest/migration/misc-tests.c
>>> b/tests/qtest/migration/misc-tests.c
>>> index 2261ae7c89..4ac2f42a5a 100644
>>> --- a/tests/qtest/migration/misc-tests.c
>>> +++ b/tests/qtest/migration/misc-tests.c
>>> @@ -50,7 +50,7 @@ typedef struct HMPTestData {
>>> 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"),
>>
>> This appears to be an improvement. Where does it come from?
>>
>
> Old code went through the enum lookup whereas now the QObject visitor's
> qobject_input_check_struct flags the struct member name.
>
>>>
>>> /* bool */
>>> TEST("cpu-throttle-tailslow", "on", "on"),
