Laurent Vivier <lviv...@redhat.com> writes:
> On 21/06/2022 10:49, Markus Armbruster wrote:
>> Laurent Vivier <lviv...@redhat.com> writes:
>>
>>> On 20/06/2022 17:21, Markus Armbruster wrote:
>>>> Laurent Vivier <lviv...@redhat.com> writes:
>>>>
>>>>> Copied from socket netdev file and modified to use SocketAddress
>>>>> to be able to introduce new features like unix socket.
>>>>>
>>>>> "udp" and "mcast" are squashed into dgram netdev, multicast is detected
>>>>> according to the IP address type.
>>>>> "listen" and "connect" modes are managed by stream netdev. An optional
>>>>> parameter "server" defines the mode (server by default)
>>>>>
>>>>> Signed-off-by: Laurent Vivier <lviv...@redhat.com>
>>>>> Reviewed-by: Stefano Brivio <sbri...@redhat.com>
>>>>> ---
>>
>> [...]
>>
>>>>> diff --git a/net/net.c b/net/net.c
>>>>> index c337d3d753fe..440957b272ee 100644
>>>>> --- a/net/net.c
>>>>> +++ b/net/net.c
>>> ...
>>>>> @@ -1612,7 +1617,19 @@ void net_init_clients(void)
>>>>> */
>>>>> static bool netdev_is_modern(const char *optarg)
>>>>> {
>>>>> - return false;
>>>>> + QDict *args;
>>>>> + const char *type;
>>>>> + bool is_modern;
>>>>> +
>>>>> + args = keyval_parse(optarg, "type", NULL, NULL);
>>>>> + if (!args) {
>>>>> + return false;
>>>>> + }
>>>>> + type = qdict_get_try_str(args, "type");
>>>>> + is_modern = !g_strcmp0(type, "stream") || !g_strcmp0(type, "dgram");
>>>>> + qobject_unref(args);
>>>>> +
>>>>> + return is_modern;
>>>>> }
>>>>
>>>> You could use g_autoptr here:
>>>>
>>>> g_autoptr(QDict) args = NULL;
>>>> const char *type;
>>>> bool is_modern;
>>>>
>>>> args = keyval_parse(optarg, "type", NULL, NULL);
>>>> if (!args) {
>>>> return false;
>>>> }
>>>> type = qdict_get_try_str(args, "type");
>>>> return !g_strcmp0(type, "stream") || !g_strcmp0(type, "dgram");
>>>>
>>>> Matter of taste; you decide.
>>>
>>> Looks good. We already had some series to convert existing code to
>>> g_autoptr(), so it
>>> seems the way to do.
>>>
>>>>
>>>> Now recall how this function is used: it decides whether to parse the
>>>> modern way (with qobject_input_visitor_new_str()) or the traditional way
>>>> (with qemu_opts_parse_noisily()).
>>>>
>>>> qemu_opts_parse_noisily() parses into a QemuOpts, for later use with the
>>>> opts visitor.
>>>>
>>>> qobject_input_visitor_new_str() supports both dotted keys and JSON. The
>>>> former is parsed with keyval_parse(), the latter with
>>>> qobject_from_json(). It returns the resulting parse tree wrapped in a
>>>> suitable QAPI input visitor.
>>>>
>>>> Issue 1: since we get there only when keyval_parse() succeeds, JSON is
>>>> unreachable. Reproducer:
>>>>
>>>> $ qemu-system-x86_64 -netdev '{"id":"foo"}'
>>>> upstream-qemu: -netdev {"id":"foo"}: Parameter 'id' is missing
>>>>
>>>> This is parsed with qemu_opts_parse_noisily(), resulting in a QemuOpts
>>>> with a single option 'type' with value '{"id":"foo"}'. The error
>>>> message comes from the opts visitor.
>>>>
>>>> To fix this, make netdev_is_modern() return true when optarg[0] == '{'.
>>>> This matches how qobject_input_visitor_new_str() recognizes JSON.
>>>
>>> OK
>>>
>>>>
>>>> Issue 2: when keyval_parse() detects an error, we throw it away and fall
>>>> back to QemuOpts. This is commonly what we want. But not always. For
>>>> instance:
>>>>
>>>> $ qemu-system-x86_64 -netdev
>>>> 'type=stream,id=foo,addr.type=inet,addr.host=localhost,addr.port=1234,addr.ipv4-off'
>>>>
>>>> Note the typo "ipv4-off" instead of ipv4=off. The error reporting is crap:
>>>>
>>>> qemu-system-x86_64: -netdev
>>>> type=stream,id=foo,addr.type=inet,addr.host=localhost,addr.port=1234,addr.ipv4-off:
>>>> warning: short-form boolean option 'addr.ipv4-off' deprecated
>>>> Please use addr.ipv4-off=on instead
>>>> qemu-system-x86_64: -netdev
>>>> type=stream,id=foo,addr.type=inet,addr.host=localhost,addr.port=1234,addr.ipv4-off:
>>>> Parameter 'type' is missing
>>>>
>>>> We get this because netdev_is_modern() guesses wrongly: keyval_parse()
>>>> fails with the perfectly reasonable error message "Expected '=' after
>>>> parameter 'addr.ipv4-off'", but netdev_is_modern() ignores the error,
>>>> and fails. We fall back to QemuOpts, and confusion ensues.
>>>>
>>>> I'm not sure we can do much better with reasonable effort. If we decide
>>>> to accept this behavior, it should be documented at least in the source
>>>> code.
>>>
>>> What about using modern syntax by default?
>>>
>>> args = keyval_parse(optarg, "type", NULL, NULL);
>>> if (!args) {
>>> /* cannot detect the syntax, use new style syntax */
>>> return true;
>>> }
>>
>> As is, netdev_is_modern() has three cases:
>>
>> 1. keyval_parse() fails
>>
>> 2. keyval_parse() succeeds, but value of @type is not modern
>>
>> 3. keyval_parse() succeeds, and value of @type is modern
>>
>> In case 3. we're sure, because even if qemu_opts_parse_noisily() also
>> succeeded, it would result in the same value of @type.
>>
>> In case 2, assuming traditional seems reasonable. The assumption can be
>> wrong when the user intends modern, but fat-fingers the type=T part.
>>
>> In case 1, we know nothing.
>>
>> Guessing modern is wrong when the user intends traditional. This
>> happens when a meant-to-be-traditional @optarg also parses as modern.
>> Quite possible.
>
> I don't see why keyval_parse() fails in this case. Any example?
Brain cramp on my part, I'm afraid %-} Let me start over.
Guessing modern is wrong when the user intends traditional. Two
sub-cases then:
* @optarg parses fine as traditional. For instance,
$ qemu-system-x86_64 -netdev type=user,id=foo,ipv4
parses with a warning:
option 'ipv4' deprecated
Please use ipv4=on instead
This is how current master behaves.
Guessing modern makes this fail instead:
qemu-system-x86_64: -netdev type=user,id=foo,ipv4: Expected '=' after
parameter 'ipv4'
Regression.
* @optarg fails to parse traditional, too. The error reporting is for
modern even though the user intends traditional. Can be misleading.
Example:
$ qemu-system-x86_64 -netdev type=user,id=_,ipv4
Current master:
qemu-system-x86_64: -netdev type=user,id=_,ipv4: Parameter 'id' expects an
identifier
Identifiers consist of letters, digits, '-', '.', '_', starting with a
letter.
Guessing modern instead:
qemu-system-x86_64: -netdev type=user,id=_,ipv4: Expected '=' after
parameter 'ipv4'
This should be rare in practice, as traditional parsing detects very
few errors.
>> Guessing traditional is wrong when the user intends modern. This
>> happens when a meant-to-be-modern @optarg fails to parse as modern,
>> i.e. whenever the user screws up modern syntax.
>
> This one is the example you gave (ipv4-off)
Two sub-cases then:
* @optarg parses fine as traditional. The parse result is unlikely to
make sense, though. For instance,
$ qemu-system-x86_64 -netdev type=stream,id=foo,server
parses with a warning:
qemu-system-x86_64: -netdev type=stream,id=foo,server: warning: short-form
boolean option 'server' deprecated
Please use server=on instead
But the result fails in the opts visitor:
qemu-system-x86_64: -netdev type=stream,id=foo,server: Parameter 'type' is
missing
In this case, we're better off with guessing modern:
qemu-system-x86_64: -netdev type=stream,id=foo,server: Expected '=' after
parameter
* @optarg fails to parse traditional, too. The error reporting is for
traditional even though the user intends modern. Can be misleading.
This is my ipv4-off example.
Can't win. Parsers simply don't compose that way.
>> Which guess is less bad? I'm not sure. Thoughts?
>
> Perhaps we can simply fail if keyval_parse() fails?
>
> something like:
>
> args = keyval_parse(optarg, "type", NULL, &error_fatal);
> type = qdict_get_try_str(args, "type");
> return !g_strcmp0(type, "stream") || !g_strcmp0(type, "dgram");
This rejects working option arguments that don't also parse as modern,
such as "-netdev type=user,id=foo,ipv4".
Guessing traditional seems to be the least bad solution so far.
Supporting both traditional and modern syntax in an option argument is a
swamp. Can we bypass it somehow?
-object uses traditional QemuOpts parsing for key=value,..., and modern
parsing for JSON. Sticking to traditional sidesteps compatibility
issues. But you have to use JSON for things traditional can't express.
Thoughts?
