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

[Markus Armbruster]

Laurent Vivier <lvivier@redhat.com> writes:

> On 21/06/2022 10:49, Markus Armbruster wrote:
>> 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.
>
> 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?