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.
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.
Which guess is less bad? I'm not sure. Thoughts?
Note that additionally checking whether qemu_opts_parse() succeeds would
be next to useless, since qemu_opts_parse() accepts pretty much
anything.
[...]
