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

[Markus Armbruster]

Laurent Vivier <lvivier@redhat.com> writes:

> On 22/06/2022 13:47, Markus Armbruster wrote:
>> 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?
>> 
>
> I don't want to force user to switch to JSON to ease move from "-netdev 
> socket" to 
> "-netdev stream" and "-netdev dgram".
> But I need to be able to parse SocketAddress to specify unix and inet socket 
> address.

That's fair.

> As we want to keep the same behavior on error cases (it's what I understand 
> from your 
> analysis),

We must not break working usage.

We may change how erroneous usage fails, but we should try to avoid bad
error reporting.  Common errors are more important here.  Whether they
are in existing or in new options doesn't matter.

>            perhaps we can rely on traditional mechanism to detect the type: 
> qemu_opts_parse()?
>
> bool netdev_is_modern(const char *optarg)
> {
>      QemuOpts *opts;
>      bool is_modern;
>      const char *type;
>      static QemuOptsList dummy_opts = {
>          .name = "netdev",
>          .implied_opt_name = "type",
>          .head = QTAILQ_HEAD_INITIALIZER(dummy_opts.head),
>          .desc = { { } },
>      };
>
>      if (optarg[0] == '{') {
>          return true;
>      }
>
>      opts = qemu_opts_parse(&dummy_opts, optarg, true, &error_fatal);
>      type = qemu_opt_get(opts, "type");
>      is_modern = !g_strcmp0(type, "stream") || !g_strcmp0(type, "dgram");
>
>      qemu_opts_reset(&dummy_opts);
>
>      return is_modern;
> }
>
> The errors are directly reported by qemu_opts_parse(..., &error_fatal), and 
> are the ones 
> expected in the traditional case.
>
> But the error reported for the modern case are not correct anymore.
>
> I really don't think there is a good solution to our problem.
>
> We must accept an incorrect error report in one of these cases.
>
> To not break existing error report seems to be the way to go 
> (qemu_opt_parse()).

I'm not sure about passing &error_fatal to qemu_opts_parse().  Let's
examine the possible errors.

qemu_opts_parse() calls opts_parse().

opts_parse() extracts the value of parameter "id" with opts_parse_id().

It then creates a QemuOpts with qemu_opts_create().  Errors:

* "Invalid parameter 'id'" is impossible since !dummy_opts.merge_lists.

* "Parameter "id" expects an identifier" when value of "id" is not
  wellformed.

* "Duplicate ID 'FOO' for netdev" is impossible since @dummy_opts is
  always empty.

It then parses for real with opts_do_parse().  Errors can only come from
opt_validate():

* "Invalid parameter 'FOO'" is impossible since
  opts_accepts_any(&dummy_opts)

* a bunch from qemu_opt_parse(), all impossible since !dummy_opt.desc.

Conclusion: can only error out when value of "id" is not wellformed.

Detecting this error is counter-productive here, because it masks the
value of "type", which is what we're after.

Better, I think: use opts_do_parse() directly.  Something like

    opts = qemu_opts_create(&dummy_opts, NULL, false, &error_abort);
    qemu_opts_do_parse(opts, optarg, dummy_opts.implied_opt_name,
                       &error_abort);
    type = qemu_opt_get(opts, "type");

Note this cannot fail.  It may be unable to extract the value of type if
the user messes up badly enough.  We then assume traditional syntax.
Least bad option so far, I think.

For once, QemuOpts having basically no syntax checks comes it handy.
I'm surprised.