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

[Laurent Vivier <lvivier@redhat.com>]

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>
>> ---
>>   hmp-commands.hx |   2 +-
>>   net/clients.h   |   6 +
>>   net/dgram.c     | 630 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   net/hub.c       |   2 +
>>   net/meson.build |   2 +
>>   net/net.c       |  19 +-
>>   net/stream.c    | 425 ++++++++++++++++++++++++++++++++
>>   qapi/net.json   |  40 ++-
>>   qemu-options.hx |  12 +
>>   9 files changed, 1133 insertions(+), 5 deletions(-)
>>   create mode 100644 net/dgram.c
>>   create mode 100644 net/stream.c
>>
...
>> diff --git a/net/dgram.c b/net/dgram.c
>> new file mode 100644
>> index 000000000000..aa4240501ed0
>> --- /dev/null
>> +++ b/net/dgram.c
>> @@ -0,0 +1,630 @@
>> +/*
>> + * QEMU System Emulator
>> + *
>> + * Copyright (c) 2003-2008 Fabrice Bellard
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
> 
> Blank line here, please.

Done

...

>> 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;
     }

>>   
>>   /*
>> diff --git a/net/stream.c b/net/stream.c
>> new file mode 100644
>> index 000000000000..fdc97ee43a56
>> --- /dev/null
>> +++ b/net/stream.c
>> @@ -0,0 +1,425 @@
>> +/*
>> + * QEMU System Emulator
>> + *
>> + * Copyright (c) 2003-2008 Fabrice Bellard
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
> 
> Blank line here, please.

Done

...
>> diff --git a/qapi/net.json b/qapi/net.json
>> index d6f7cfd4d656..5f72995b1d24 100644
>> --- a/qapi/net.json
>> +++ b/qapi/net.json
...
>> @@ -566,6 +567,37 @@
>>       '*isolated':  'bool' },
>>     'if': 'CONFIG_VMNET' }
>>   
>> +##
>> +# @NetdevStreamOptions:
>> +#
>> +# Configuration info for stream socket netdev
>> +#
>> +# @addr: socket address to listen on (server=true)
>> +#        or connect to (server=false)
>> +# @server: create server socket (default: true)
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'struct': 'NetdevStreamOptions',
>> +  'data': {
>> +    'addr':   'SocketAddress',
>> +    '*server': 'bool' } }
>> +
>> +##
>> +# @NetdevDgramOptions:
>> +#
>> +# Configuration info for datagram socket netdev.
>> +#
>> +# @remote: remote address
>> +# @local: local address
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'struct': 'NetdevDgramOptions',
>> +  'data': {
>> +    '*local':  'SocketAddress',
>> +    '*remote': 'SocketAddress' } }
> 
> In review of v2, I inquired about behavior when members are absent.
> You wrote:
> 
>      The code checks there is at least one of these options and reports an error if not.
> 
>      if remote address is present and it's a multicast address, local address is optional.
> 
>      otherwise local address is required and remote address is optional.
> 
> Please work that into the doc comment.

OK

> 
>> +
>>   ##
>>   # @NetClientDriver:
>>   #
>> @@ -579,9 +611,9 @@
>>   #        @vmnet-bridged since 7.1
>>   ##
>>   { 'enum': 'NetClientDriver',
>> -  'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde',
>> -            'bridge', 'hubport', 'netmap', 'vhost-user', 'vhost-vdpa',
>> -            { 'name': 'vmnet-host', 'if': 'CONFIG_VMNET' },
>> +  'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'stream',
>> +            'dgram', 'vde', 'bridge', 'hubport', 'netmap', 'vhost-user',
>> +            'vhost-vdpa', { 'name': 'vmnet-host', 'if': 'CONFIG_VMNET' },
>>               { 'name': 'vmnet-shared', 'if': 'CONFIG_VMNET' },
>>               { 'name': 'vmnet-bridged', 'if': 'CONFIG_VMNET' }] }
> 
> Opportunity to put vmnet-host on its own line for readability.
> 

OK

Thanks,
Laurent