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

[Markus Armbruster <armbru@redhat.com>]

Fabiano Rosas <farosas@suse.de> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> 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.
>>
>> 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.

Should the commit message make this argument?

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

This is due to separation of concerns in the design of the visitor
pipelines.  Example:

         JSON         input
        parser       visitor
    JSON ---> QObject ---> QAPI-generated C type

         JSON         output
       formatter     visitor
    JSON <--- QObject <--- QAPI-generated C type

Parser/formatter deal with JSON.  All they know about QAPI is QObject.

Visitors deal with QAPI-generated C types.  All they know about JSON is
QObject.

Keyval came much later, and we had to accept a weaker separation for it.
In JSON, the type of scalars is explicit in the syntax.  In keyval, it
is not.  So scalar values are always strings in the parser's output, and
the QObject keyval visitor needs to parse these strings.  This leads to
restrictions discussed in keyval.c's big comment.

But this applies *only* to scalars!  The separation still holds for
objects and arrays.

> ... although it wouldn't help with cpr-exec-command because of the
> "shell parsing" semantics. But that's another issue.

Yes.

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

We use g_shell_parse_argv() for cpr-exec-command and nothing else, which
effectively makes it custom syntax.  At least it's custom syntax we
didn't design and implement, but still.  Too.  Much.  Syntax.  Too.
Many.  Parsers.

>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>

[...]

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

hmp_migrate_set_parameter() takes a parameter argument that names a
member of MigrationParameters.  Trick: it parses it as a member of
related type MigrationParameter, with qapi_enum_parse().  Since the
members of MigrationParameter are *values*, qapi_enum_parse() reasonably
reports "invalid parameter value".

Trickery breed bad error messages, film at eleven.

Suggest to mention the improvement in the commit message.

>>>  
>>>      /* bool */
>>>      TEST("cpu-throttle-tailslow", "on", "on"),