qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Laurent Vivier
Subject: Re: [RFC PATCH v3 04/11] qapi: net: add stream and dgram netdevs
Date: Mon, 20 Jun 2022 19:52:01 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 
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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]