Re: [RFC PATCH v3 04/11] qapi: net: add stream and dgram netdevs

[Markus Armbruster <armbru@redhat.com>]

Laurent Vivier <lvivier@redhat.com> writes:

> On 20/06/2022 17:21, Markus Armbruster wrote:
>> Laurent Vivier <lvivier@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 <lvivier@redhat.com>
>>> Reviewed-by: Stefano Brivio <sbrivio@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.

[...]