[PATCH 00/13] char: QAPIfy the command line parsing

[PATCH 00/13] char: QAPIfy the command line parsing

[Kevin Wolf]

The character device subsystem is special in that it uses QAPI objects
(ChardevBackend) internally to configure the backends, but for mapping
the command line to it, instead of making use of QAPI visitors, it uses
a hand-crafted parser. At the same time, the QMP command chardev-add
obviously does use the QAPI parser.

This is nasty because there is nothing that will make sure that the two
parsers provide access to the same feature set or that the syntax to
access features is consistent. We have also seen patches that changed
things for one path, but forgot that the other one needed to be changed,
too, resulting in regressions.

In order to solve this problem and make consistency between both paths
automatic, this series removes the hand-crafted parser that goes from
QemuOpts to ChardevBackend. Instead, QAPI aliases and only a few manual
tweaks of the input are used to make a keyval parser and QAPI visitors
suitable for parsing command lines while retaining backwards
compatibility.

As a welcome side effect, this also makes features available on the
command line that were previously restricted to QMP.

Another welcome side effect is the diffstat. :-)

QemuOpts is not entirely removed from the chardev interfaces yet. This
is mostly because -readconfig still uses it and changing vl.c to work
with a mix of QAPI object and QemuOpts would be harder than just keeping
some QemuOpts based interfaces that just internally translate. This can
be addressed after -readconfig has been reworked (probably resulting in
another nice diffstat).

A git tag is available that contains this series and its prerequisites:

    https://repo.or.cz/qemu/kevin.git qapi-alias-chardev-v1

Based-on: 20201111130834.33985-1-kwolf@redhat.com
('[PATCH 0/2] char: Deprecate backend aliases')

Based-on: 20201112172850.401925-1-kwolf@redhat.com
('[PATCH 0/6] qapi: Add support for aliases')

Kevin Wolf (13):
  char: Factor out qemu_chr_print_types()
  char: Add ChardevOptions and qemu_chr_new_cli()
  char: Some QAPI aliases for CLI compatibility
  char: Add qemu_chr_translate_legacy_options()
  char-socket: Implement compat code for CLI QAPIfication
  char-udp: Implement compat code for CLI QAPIfication
  char: Add qemu_chr_parse_cli_dict/str()
  char: Add mux option to ChardevOptions
  qemu-storage-daemon: QAPIfy --chardev
  char: Implement qemu_chr_new_from_opts() in terms of QAPI
  hmp/char: Use qemu_chr_parse_cli_str() for chardev-change
  char: Remove qemu_chr_parse_opts()
  char: Remove ChardevClass.parse

 qapi/char.json                       |  42 ++++-
 qapi/sockets.json                    |   6 +-
 include/chardev/char.h               |  56 ++++--
 chardev/char-file.c                  |  20 --
 chardev/char-mux.c                   |  17 --
 chardev/char-parallel.c              |  17 --
 chardev/char-pipe.c                  |  17 --
 chardev/char-ringbuf.c               |  18 --
 chardev/char-serial.c                |  17 --
 chardev/char-socket.c                | 121 +++++--------
 chardev/char-stdio.c                 |  13 --
 chardev/char-udp.c                   |  79 +++-----
 chardev/char.c                       | 262 ++++++++++++++++-----------
 chardev/spice.c                      |  34 ----
 monitor/hmp-cmds.c                   |  27 +--
 storage-daemon/qemu-storage-daemon.c |  17 +-
 ui/console.c                         |  35 ----
 ui/gtk.c                             |   1 -
 ui/spice-app.c                       |   1 -
 19 files changed, 321 insertions(+), 479 deletions(-)

-- 
2.28.0

[PATCH 01/13] char: Factor out qemu_chr_print_types()

[Kevin Wolf]

We'll want to call the same from a non-QemuOpts code path.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 include/chardev/char.h |  1 +
 chardev/char.c         | 16 ++++++++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index db42f0a8c6..3b91645081 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -212,6 +212,7 @@ void qemu_chr_be_update_read_handlers(Chardev *s,
  */
 void qemu_chr_be_event(Chardev *s, QEMUChrEvent event);
 
+void qemu_chr_print_types(void);
 int qemu_chr_add_client(Chardev *s, int fd);
 Chardev *qemu_chr_find(const char *name);
 
diff --git a/chardev/char.c b/chardev/char.c
index f9e297185d..de39e2d79b 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -644,6 +644,15 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts, Error **errp)
     return backend;
 }
 
+void qemu_chr_print_types(void)
+{
+    g_autoptr(GString) str = g_string_new("");
+
+    chardev_name_foreach(help_string_append, str);
+
+    qemu_printf("Available chardev backend types: %s\n", str->str);
+}
+
 Chardev *qemu_chr_new_from_opts(QemuOpts *opts, GMainContext *context,
                                 Error **errp)
 {
@@ -655,12 +664,7 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts, GMainContext *context,
     char *bid = NULL;
 
     if (name && is_help_option(name)) {
-        GString *str = g_string_new("");
-
-        chardev_name_foreach(help_string_append, str);
-
-        qemu_printf("Available chardev backend types: %s\n", str->str);
-        g_string_free(str, true);
+        qemu_chr_print_types();
         return NULL;
     }
 
-- 
2.28.0

[PATCH 02/13] char: Add ChardevOptions and qemu_chr_new_cli()

[Kevin Wolf]

This provides an interface to create a chardev from a QAPI
representation of the command line.

At this point, the only difference between it and QMP chardev-add is
that it allows 'backend' to be flattened and returns a Chardev pointer.
We'll add support for mux=on and more compatibility glue to support
legacy command line syntax later.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/char.json         | 15 +++++++++++++++
 include/chardev/char.h |  9 +++++++++
 chardev/char.c         | 24 ++++++++++++++++++------
 3 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/qapi/char.json b/qapi/char.json
index 43486d1daa..14ee06a52d 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -435,6 +435,21 @@
 { 'struct' : 'ChardevReturn',
   'data': { '*pty': 'str' } }
 
+##
+# @ChardevOptions:
+#
+# Command line options for creating a character device backend
+#
+# @id: the chardev's ID, must be unique
+# @backend: backend type and parameters
+#
+# Since: 6.0
+##
+{ 'struct': 'ChardevOptions',
+  'data': { 'id': 'str',
+            'backend': 'ChardevBackend' },
+  'aliases': [ { 'source': ['backend'] } ] }
+
 ##
 # @chardev-add:
 #
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 3b91645081..54fa2ed8e2 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -85,6 +85,15 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
                                 GMainContext *context,
                                 Error **errp);
 
+/**
+ * qemu_chr_new_cli:
+ * @options: Character device creation options as defined in QAPI
+ *
+ * Returns: on success: a new character backend
+ *          otherwise:  NULL; @errp specifies the error
+ */
+Chardev *qemu_chr_new_cli(ChardevOptions *options, Error **errp);
+
 /**
  * qemu_chr_parse_common:
  * @opts: the options that still need parsing
diff --git a/chardev/char.c b/chardev/char.c
index de39e2d79b..9f00e475d4 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -1037,20 +1037,27 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,
     return chardev_new(id, typename, backend, gcontext, errp);
 }
 
-ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
-                               Error **errp)
+static Chardev *chardev_new_qapi(const char *id, ChardevBackend *backend,
+                                 Error **errp)
 {
     const ChardevClass *cc;
-    ChardevReturn *ret;
-    Chardev *chr;
 
     cc = char_get_class(ChardevBackendKind_str(backend->type), errp);
     if (!cc) {
         return NULL;
     }
 
-    chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
-                      backend, NULL, errp);
+    return chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
+                       backend, NULL, errp);
+}
+
+ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
+                               Error **errp)
+{
+    ChardevReturn *ret;
+    Chardev *chr;
+
+    chr = chardev_new_qapi(id, backend, errp);
     if (!chr) {
         return NULL;
     }
@@ -1064,6 +1071,11 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
     return ret;
 }
 
+Chardev *qemu_chr_new_cli(ChardevOptions *options, Error **errp)
+{
+    return chardev_new_qapi(options->id, options->backend, errp);
+}
+
 ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
                                   Error **errp)
 {
-- 
2.28.0

[PATCH 03/13] char: Some QAPI aliases for CLI compatibility

[Kevin Wolf]

For all chardev backend types where this is enough to achieve
compatibility with the old QemuOpts based command line parser, add
aliases to the QAPI schema.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/char.json | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/qapi/char.json b/qapi/char.json
index 14ee06a52d..91c0dbfa1e 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -227,7 +227,8 @@
   'data': { '*in': 'str',
             'out': 'str',
             '*append': 'bool' },
-  'base': 'ChardevCommon' }
+  'base': 'ChardevCommon',
+  'aliases': [ {'alias': 'path', 'source': ['out'] } ] }
 
 ##
 # @ChardevHostdev:
@@ -241,7 +242,8 @@
 ##
 { 'struct': 'ChardevHostdev',
   'data': { 'device': 'str' },
-  'base': 'ChardevCommon' }
+  'base': 'ChardevCommon',
+  'aliases': [ {'alias': 'path', 'source': ['device'] } ] }
 
 ##
 # @ChardevSocket:
@@ -342,7 +344,8 @@
 { 'struct': 'ChardevSpiceChannel',
   'data': { 'type': 'str' },
   'base': 'ChardevCommon',
-  'if': 'defined(CONFIG_SPICE)' }
+  'if': 'defined(CONFIG_SPICE)',
+  'aliases': [ {'alias': 'name', 'source': ['type'] } ] }
 
 ##
 # @ChardevSpicePort:
@@ -356,7 +359,8 @@
 { 'struct': 'ChardevSpicePort',
   'data': { 'fqdn': 'str' },
   'base': 'ChardevCommon',
-  'if': 'defined(CONFIG_SPICE)' }
+  'if': 'defined(CONFIG_SPICE)',
+  'aliases': [ {'alias': 'name', 'source': ['fqdn'] } ] }
 
 ##
 # @ChardevVC:
@@ -420,7 +424,8 @@
             'vc': 'ChardevVC',
             'ringbuf': 'ChardevRingbuf',
             # next one is just for compatibility
-            'memory': 'ChardevRingbuf' } }
+            'memory': 'ChardevRingbuf' },
+  'aliases': [ { 'source': ['data'] } ] }
 
 ##
 # @ChardevReturn:
-- 
2.28.0

[PATCH 04/13] char: Add qemu_chr_translate_legacy_options()

[Kevin Wolf]

This translates legacy command line options that can't be made
compatible with QAPI just by using aliases from the traditional command
line structure into the structure of ChardevOptions.

As a first step, add support for backend name aliases if 'backend' is
given instead of 'type'.

Also add a todo comment for everything that is still incompatible
between the QemuOpts based -chardev and chardev creation by going
through a keyval parser, qemu_chr_translate_legacy_options() and
qemu_chr_new_cli().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/chardev/char.h | 13 +++++++++++++
 chardev/char.c         | 23 +++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index 54fa2ed8e2..7795e17ca5 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -94,6 +94,19 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
  */
 Chardev *qemu_chr_new_cli(ChardevOptions *options, Error **errp);
 
+/**
+ * qemu_chr_translate_legacy_options:
+ * @args: Character device creation options as returned by the keyval parser
+ *
+ * Change @args so that the legacy command line options in it are translated
+ * and @args can be used as the input for a ChardevOptions visitor.
+ *
+ * If @args was not a valid legacy command line, translation may be partially
+ * skipped and the visitor may return an error if @args was not already
+ * suitable for QAPI parsing.
+ */
+void qemu_chr_translate_legacy_options(QDict *args);
+
 /**
  * qemu_chr_parse_common:
  * @opts: the options that still need parsing
diff --git a/chardev/char.c b/chardev/char.c
index 9f00e475d4..40c3f02ec9 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -32,6 +32,7 @@
 #include "chardev/char.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-char.h"
+#include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
 #include "sysemu/replay.h"
 #include "qemu/help_option.h"
@@ -717,6 +718,28 @@ out:
     return chr;
 }
 
+void qemu_chr_translate_legacy_options(QDict *args)
+{
+    const char *name;
+
+    /* "backend" instead of "type" enables legacy CLI compatibility */
+    name = qdict_get_try_str(args, "backend");
+    if (!name || qdict_haskey(args, "type")) {
+        return;
+    }
+
+    name = chardev_alias_translate(name);
+    qdict_put_str(args, "type", name);
+    qdict_del(args, "backend");
+
+    /*
+     * TODO:
+     * All backend types: "mux"
+     * socket: "addr.type", "delay", "server", "wait", "fd"
+     * udp: defaults for "host"/"localaddr"/"localport"
+     */
+}
+
 Chardev *qemu_chr_new_noreplay(const char *label, const char *filename,
                                bool permit_mux_mon, GMainContext *context)
 {
-- 
2.28.0

[PATCH 05/13] char-socket: Implement compat code for CLI QAPIfication

[Kevin Wolf]

Socket backends have a few differences between CLI and QMP. This adds
QAPI aliases and a .translate_legacy_options() implementation that
converts CLI inputs to a form that's usable for a QAPIfied --chardev.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/char.json         |  3 ++-
 qapi/sockets.json      |  6 ++++-
 include/chardev/char.h |  1 +
 chardev/char-socket.c  | 53 ++++++++++++++++++++++++++++++++++++++++++
 chardev/char.c         | 10 +++++++-
 5 files changed, 70 insertions(+), 3 deletions(-)

diff --git a/qapi/char.json b/qapi/char.json
index 91c0dbfa1e..1930e90e95 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -287,7 +287,8 @@
             '*tn3270': 'bool',
             '*websocket': 'bool',
             '*reconnect': 'int' },
-  'base': 'ChardevCommon' }
+  'base': 'ChardevCommon',
+  'aliases': [ { 'source': ['addr'] } ] }
 
 ##
 # @ChardevUdp:
diff --git a/qapi/sockets.json b/qapi/sockets.json
index 2e83452797..8c61787311 100644
--- a/qapi/sockets.json
+++ b/qapi/sockets.json
@@ -125,7 +125,11 @@
     'inet': 'InetSocketAddress',
     'unix': 'UnixSocketAddress',
     'vsock': 'VsockSocketAddress',
-    'fd': 'String' } }
+    'fd': 'String' },
+  'aliases': [
+    {'source': ['data']},
+    {'alias': 'fd', 'source': ['data', 'str']}
+  ]}
 
 ##
 # @SocketAddressType:
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 7795e17ca5..c0944f5828 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -275,6 +275,7 @@ struct ChardevClass {
 
     bool internal; /* TODO: eventually use TYPE_USER_CREATABLE */
     void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
+    void (*translate_legacy_options)(QDict *args);
 
     void (*open)(Chardev *chr, ChardevBackend *backend,
                  bool *be_opened, Error **errp);
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 213a4c8dd0..6bf916a3e4 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -34,6 +34,7 @@
 #include "qapi/error.h"
 #include "qapi/clone-visitor.h"
 #include "qapi/qapi-visit-sockets.h"
+#include "qapi/qmp/qdict.h"
 
 #include "chardev/char-io.h"
 #include "qom/object.h"
@@ -1484,6 +1485,57 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
     sock->addr = addr;
 }
 
+static void qemu_chr_translate_socket(QDict *args)
+{
+    const char *path = qdict_get_try_str(args, "path");
+    const char *host = qdict_get_try_str(args, "host");
+    const char *fd = qdict_get_try_str(args, "fd");
+    const char *delay = qdict_get_try_str(args, "delay");
+    const char *server = qdict_get_try_str(args, "server");
+    const char *wait = qdict_get_try_str(args, "wait");
+    QDict *addr;
+
+    if ((!!path + !!fd + !!host) != 1) {
+        return;
+    }
+
+    /* If "addr" is not present, automatically set the type */
+    if (!qdict_haskey(args, "addr")) {
+        addr = qdict_new();
+        qdict_put(args, "addr", addr);
+
+        if (path) {
+            qdict_put_str(addr, "type", "unix");
+        } else if (host) {
+            qdict_put_str(addr, "type", "inet");
+        } else if (fd) {
+            qdict_put_str(addr, "type", "fd");
+        }
+    }
+
+    /* "delay" is translated into "nodelay" */
+    if (delay && !qdict_haskey(args, "nodelay")) {
+        if (!strcmp(delay, "on")) {
+            qdict_put_str(args, "nodelay", "off");
+            qdict_del(args, "delay");
+        } else if (!strcmp(delay, "off")) {
+            qdict_put_str(args, "nodelay", "on");
+            qdict_del(args, "delay");
+        }
+    }
+
+    /* "server=off" is the CLI default */
+    if (!server) {
+        server = "off";
+        qdict_put_str(args, "server", server);
+    }
+
+    /* "wait=on" is the default if "server=on" */
+    if (!wait && !strcmp(server, "on")) {
+        qdict_put_str(args, "wait", "on");
+    }
+}
+
 static void
 char_socket_get_addr(Object *obj, Visitor *v, const char *name,
                      void *opaque, Error **errp)
@@ -1506,6 +1558,7 @@ static void char_socket_class_init(ObjectClass *oc, void *data)
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
     cc->parse = qemu_chr_parse_socket;
+    cc->translate_legacy_options = qemu_chr_translate_socket;
     cc->open = qmp_chardev_open_socket;
     cc->chr_wait_connected = tcp_chr_wait_connected;
     cc->chr_write = tcp_chr_write;
diff --git a/chardev/char.c b/chardev/char.c
index 40c3f02ec9..91b44e53b6 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -720,6 +720,7 @@ out:
 
 void qemu_chr_translate_legacy_options(QDict *args)
 {
+    const ChardevClass *cc;
     const char *name;
 
     /* "backend" instead of "type" enables legacy CLI compatibility */
@@ -730,12 +731,19 @@ void qemu_chr_translate_legacy_options(QDict *args)
 
     name = chardev_alias_translate(name);
     qdict_put_str(args, "type", name);
+
+    cc = char_get_class(name, NULL);
+    if (cc != NULL && cc->translate_legacy_options) {
+        QDict *backend_data = qdict_get_qdict(args, "data") ?: args;
+        cc->translate_legacy_options(backend_data);
+    }
+
+    /* name may refer to a QDict entry, so delete it only now */
     qdict_del(args, "backend");
 
     /*
      * TODO:
      * All backend types: "mux"
-     * socket: "addr.type", "delay", "server", "wait", "fd"
      * udp: defaults for "host"/"localaddr"/"localport"
      */
 }
-- 
2.28.0

[PATCH 06/13] char-udp: Implement compat code for CLI QAPIfication

[Kevin Wolf]

UDP backends have a few differences between CLI and QMP. This adds
QAPI aliases and a .translate_legacy_options() implementation that
converts CLI inputs to a form that's usable for a QAPIfied --chardev.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/char.json     |  7 ++++++-
 chardev/char-udp.c | 33 +++++++++++++++++++++++++++++++++
 chardev/char.c     |  1 -
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/qapi/char.json b/qapi/char.json
index 1930e90e95..e1f9347044 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -303,7 +303,12 @@
 { 'struct': 'ChardevUdp',
   'data': { 'remote': 'SocketAddressLegacy',
             '*local': 'SocketAddressLegacy' },
-  'base': 'ChardevCommon' }
+  'base': 'ChardevCommon',
+  'aliases': [
+    { 'source': ['remote'] },
+    { 'alias': 'localaddr', 'source': ['local', 'host'] },
+    { 'alias': 'localport', 'source': ['local', 'port'] }
+  ]}
 
 ##
 # @ChardevMux:
diff --git a/chardev/char-udp.c b/chardev/char-udp.c
index 16b5dbce58..61752b1c51 100644
--- a/chardev/char-udp.c
+++ b/chardev/char-udp.c
@@ -23,9 +23,11 @@
  */
 
 #include "qemu/osdep.h"
+#include "block/qdict.h"
 #include "chardev/char.h"
 #include "io/channel-socket.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
 
@@ -190,6 +192,36 @@ static void qemu_chr_parse_udp(QemuOpts *opts, ChardevBackend *backend,
     }
 }
 
+static void qemu_chr_translate_udp(QDict *args)
+{
+    QDict *remote;
+    QDict *local;
+
+    /*
+     * If "local" or "remote" are given, it's not a legacy command line.
+     * Not translating in this case saves us checking whether an alias is
+     * already given before applying defaults.
+     */
+    if (qdict_haskey(args, "local") || qdict_haskey(args, "remote")) {
+        return;
+    }
+
+    remote = qdict_new();
+    qdict_put_str(remote, "type", "inet");
+    qdict_put(args, "remote", remote);
+
+    qdict_set_default_str(args, "host", "localhost");
+
+    if (qdict_haskey(args, "localaddr") || qdict_haskey(args, "localport")) {
+        local = qdict_new();
+        qdict_put_str(local, "type", "inet");
+        qdict_put(args, "local", local);
+
+        qdict_set_default_str(args, "localaddr", "");
+        qdict_set_default_str(args, "localport", "0");
+    }
+}
+
 static void qmp_chardev_open_udp(Chardev *chr,
                                  ChardevBackend *backend,
                                  bool *be_opened,
@@ -225,6 +257,7 @@ static void char_udp_class_init(ObjectClass *oc, void *data)
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
     cc->parse = qemu_chr_parse_udp;
+    cc->translate_legacy_options = qemu_chr_translate_udp;
     cc->open = qmp_chardev_open_udp;
     cc->chr_write = udp_chr_write;
     cc->chr_update_read_handler = udp_chr_update_read_handler;
diff --git a/chardev/char.c b/chardev/char.c
index 91b44e53b6..99feaae275 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -744,7 +744,6 @@ void qemu_chr_translate_legacy_options(QDict *args)
     /*
      * TODO:
      * All backend types: "mux"
-     * udp: defaults for "host"/"localaddr"/"localport"
      */
 }
 
-- 
2.28.0

[PATCH 07/13] char: Add qemu_chr_parse_cli_dict/str()

[Kevin Wolf]

This adds a function that parses a command line definition of a
character device into ChardevOptions, which can then be passed to
qemu_chr_new_cli().

You can start both from a string (for actual CLI) or from a QDict, which
is not only the intermediate representation after calling the keyval
parser, but also what HMP handlers receive.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/chardev/char.h | 30 ++++++++++++++++++++++++++++
 chardev/char.c         | 45 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index c0944f5828..5cd46207f6 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -94,6 +94,36 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
  */
 Chardev *qemu_chr_new_cli(ChardevOptions *options, Error **errp);
 
+/**
+ * qemu_chr_parse_cli_dict:
+ * @args: Options defining a new character device
+ * @help: true if help should be printed instead of returning ChardevOptions
+ *
+ * Parses the given command line option QDict into ChardevOptions, using
+ * qemu_chr_translate_legacy_options() to maintain compatibility with
+ * legacy command line syntax.
+ *
+ * Returns: On successful conversion, a ChardevOptions object containing the
+ * requested options. NULL and @errp is unchanged if help was requested and
+ * printed. NULL and @errp is set in error cases.
+ */
+ChardevOptions *qemu_chr_parse_cli_dict(QDict *args, bool help,
+                                        Error **errp);
+
+/**
+ * qemu_chr_parse_cli_str:
+ * @optarg: Command line argument defining a new character device
+ *
+ * Parses the given command line option into ChardevOptions, using
+ * qemu_chr_translate_legacy_options() to maintain compatibility with
+ * legacy command line syntax.
+ *
+ * Returns: On successful conversion, a ChardevOptions object containing the
+ * requested options. NULL and @errp is unchanged if help was requested and
+ * printed. NULL and @errp is set in error cases.
+ */
+ChardevOptions *qemu_chr_parse_cli_str(const char *optarg, Error **errp);
+
 /**
  * qemu_chr_translate_legacy_options:
  * @args: Character device creation options as returned by the keyval parser
diff --git a/chardev/char.c b/chardev/char.c
index 99feaae275..a5d6be9dc8 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -32,8 +32,10 @@
 #include "chardev/char.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-char.h"
+#include "qapi/qapi-visit-char.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qobject-input-visitor.h"
 #include "sysemu/replay.h"
 #include "qemu/help_option.h"
 #include "qemu/module.h"
@@ -1106,6 +1108,49 @@ Chardev *qemu_chr_new_cli(ChardevOptions *options, Error **errp)
     return chardev_new_qapi(options->id, options->backend, errp);
 }
 
+ChardevOptions *qemu_chr_parse_cli_dict(QDict *args, bool help,
+                                        Error **errp)
+{
+    Visitor *v;
+    ChardevOptions *chr_options;
+
+    qemu_chr_translate_legacy_options(args);
+
+    if (help) {
+        if (qdict_haskey(args, "type")) {
+            /* TODO Print help based on the QAPI schema */
+            qemu_opts_print_help(&qemu_chardev_opts, true);
+        } else {
+            qemu_chr_print_types();
+        }
+        return NULL;
+    }
+
+    v = qobject_input_visitor_new_keyval(QOBJECT(args));
+    visit_type_ChardevOptions(v, NULL, &chr_options, errp);
+    visit_free(v);
+
+    return chr_options;
+}
+
+ChardevOptions *qemu_chr_parse_cli_str(const char *optarg, Error **errp)
+{
+    ERRP_GUARD();
+    QDict *args;
+    ChardevOptions *chr_options;
+    bool help;
+
+    args = keyval_parse(optarg, "backend", &help, errp);
+    if (!args) {
+        return NULL;
+    }
+
+    chr_options = qemu_chr_parse_cli_dict(args, help, errp);
+    qobject_unref(args);
+
+    return chr_options;
+}
+
 ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
                                   Error **errp)
 {
-- 
2.28.0

[PATCH 08/13] char: Add mux option to ChardevOptions

[Kevin Wolf]

The final missing piece to achieve compatibility between
qemu_chr_parse_cli_str()/qemu_chr_new_cli() and the legacy command line
is support for the 'mux' option. Implement it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/char.json |  4 +++-
 chardev/char.c | 41 +++++++++++++++++++++++++++++++++++------
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/qapi/char.json b/qapi/char.json
index e1f9347044..d6733a5473 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -453,12 +453,14 @@
 #
 # @id: the chardev's ID, must be unique
 # @backend: backend type and parameters
+# @mux: enable multiplexing mode (default: false)
 #
 # Since: 6.0
 ##
 { 'struct': 'ChardevOptions',
   'data': { 'id': 'str',
-            'backend': 'ChardevBackend' },
+            'backend': 'ChardevBackend',
+            '*mux': 'bool' },
   'aliases': [ { 'source': ['backend'] } ] }
 
 ##
diff --git a/chardev/char.c b/chardev/char.c
index a5d6be9dc8..3bb6a743f7 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -742,11 +742,6 @@ void qemu_chr_translate_legacy_options(QDict *args)
 
     /* name may refer to a QDict entry, so delete it only now */
     qdict_del(args, "backend");
-
-    /*
-     * TODO:
-     * All backend types: "mux"
-     */
 }
 
 Chardev *qemu_chr_new_noreplay(const char *label, const char *filename,
@@ -1105,7 +1100,41 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
 
 Chardev *qemu_chr_new_cli(ChardevOptions *options, Error **errp)
 {
-    return chardev_new_qapi(options->id, options->backend, errp);
+    Chardev *chr;
+    char *bid = NULL;
+
+    if (options->mux) {
+        bid = g_strdup_printf("%s-base", options->id);
+    }
+
+    chr = chardev_new_qapi(bid ?: options->id, options->backend, errp);
+    if (!chr) {
+        goto out;
+    }
+
+    if (options->mux) {
+        Chardev *mux;
+        ChardevMux mux_data = {
+            .chardev = bid,
+        };
+        ChardevBackend backend = {
+            .type = CHARDEV_BACKEND_KIND_MUX,
+            .u.mux.data = &mux_data,
+        };
+
+        mux = qemu_chardev_new(options->id, TYPE_CHARDEV_MUX, &backend, NULL,
+                               errp);
+        if (mux == NULL) {
+            object_unparent(OBJECT(chr));
+            chr = NULL;
+            goto out;
+        }
+        chr = mux;
+    }
+
+out:
+    g_free(bid);
+    return chr;
 }
 
 ChardevOptions *qemu_chr_parse_cli_dict(QDict *args, bool help,
-- 
2.28.0

[PATCH 09/13] qemu-storage-daemon: QAPIfy --chardev

[Kevin Wolf]

Make use of the QAPIfied command line interface of the chardev
subsystem. With this, --chardev supports QMP-like syntax (i.e.
chardev-add mapped to the command line) as well as the legacy
syntax that it already supported and which is shared with the
system emulator.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 storage-daemon/qemu-storage-daemon.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index e419ba9f19..149d08ad6d 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -128,8 +128,6 @@ enum {
     OPTION_OBJECT,
 };
 
-extern QemuOptsList qemu_chardev_opts;
-
 static QemuOptsList qemu_object_opts = {
     .name = "object",
     .implied_opt_name = "qom-type",
@@ -207,18 +205,15 @@ static void process_options(int argc, char *argv[])
             }
         case OPTION_CHARDEV:
             {
-                /* TODO This interface is not stable until we QAPIfy it */
-                QemuOpts *opts = qemu_opts_parse_noisily(&qemu_chardev_opts,
-                                                         optarg, true);
-                if (opts == NULL) {
-                    exit(EXIT_FAILURE);
-                }
+                ChardevOptions *options;
 
-                if (!qemu_chr_new_from_opts(opts, NULL, &error_fatal)) {
-                    /* No error, but NULL returned means help was printed */
+                options = qemu_chr_parse_cli_str(optarg, &error_fatal);
+                if (!options) {
+                    /* Help was printed */
                     exit(EXIT_SUCCESS);
                 }
-                qemu_opts_del(opts);
+                qemu_chr_new_cli(options, &error_fatal);
+                qapi_free_ChardevOptions(options);
                 break;
             }
         case OPTION_EXPORT:
-- 
2.28.0

[PATCH 10/13] char: Implement qemu_chr_new_from_opts() in terms of QAPI

[Kevin Wolf]

Instead of having a second parser, qemu_chr_new_from_opts() uses
qemu_chr_translate_legacy_options() and qemu_chr_new_cli() now.

This switches -chardev of the system emulator to use the QAPI generated
parser rather than the hand-written QemuOpts based parser. All existing
command line options should keep working, but it gains support for
anything that was previously only supported in QMP (e.g. vsock socket
addresses).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 chardev/char.c | 116 +++++++++++++++++++------------------------------
 1 file changed, 45 insertions(+), 71 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 3bb6a743f7..4a444a0353 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -656,70 +656,6 @@ void qemu_chr_print_types(void)
     qemu_printf("Available chardev backend types: %s\n", str->str);
 }
 
-Chardev *qemu_chr_new_from_opts(QemuOpts *opts, GMainContext *context,
-                                Error **errp)
-{
-    const ChardevClass *cc;
-    Chardev *chr = NULL;
-    ChardevBackend *backend = NULL;
-    const char *name = chardev_alias_translate(qemu_opt_get(opts, "backend"));
-    const char *id = qemu_opts_id(opts);
-    char *bid = NULL;
-
-    if (name && is_help_option(name)) {
-        qemu_chr_print_types();
-        return NULL;
-    }
-
-    if (id == NULL) {
-        error_setg(errp, "chardev: no id specified");
-        return NULL;
-    }
-
-    backend = qemu_chr_parse_opts(opts, errp);
-    if (backend == NULL) {
-        return NULL;
-    }
-
-    cc = char_get_class(name, errp);
-    if (cc == NULL) {
-        goto out;
-    }
-
-    if (qemu_opt_get_bool(opts, "mux", 0)) {
-        bid = g_strdup_printf("%s-base", id);
-    }
-
-    chr = qemu_chardev_new(bid ? bid : id,
-                           object_class_get_name(OBJECT_CLASS(cc)),
-                           backend, context, errp);
-
-    if (chr == NULL) {
-        goto out;
-    }
-
-    if (bid) {
-        Chardev *mux;
-        qapi_free_ChardevBackend(backend);
-        backend = g_new0(ChardevBackend, 1);
-        backend->type = CHARDEV_BACKEND_KIND_MUX;
-        backend->u.mux.data = g_new0(ChardevMux, 1);
-        backend->u.mux.data->chardev = g_strdup(bid);
-        mux = qemu_chardev_new(id, TYPE_CHARDEV_MUX, backend, context, errp);
-        if (mux == NULL) {
-            object_unparent(OBJECT(chr));
-            chr = NULL;
-            goto out;
-        }
-        chr = mux;
-    }
-
-out:
-    qapi_free_ChardevBackend(backend);
-    g_free(bid);
-    return chr;
-}
-
 void qemu_chr_translate_legacy_options(QDict *args)
 {
     const ChardevClass *cc;
@@ -1065,7 +1001,7 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,
 }
 
 static Chardev *chardev_new_qapi(const char *id, ChardevBackend *backend,
-                                 Error **errp)
+                                 GMainContext *context, Error **errp)
 {
     const ChardevClass *cc;
 
@@ -1075,7 +1011,7 @@ static Chardev *chardev_new_qapi(const char *id, ChardevBackend *backend,
     }
 
     return chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
-                       backend, NULL, errp);
+                       backend, context, errp);
 }
 
 ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
@@ -1084,7 +1020,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
     ChardevReturn *ret;
     Chardev *chr;
 
-    chr = chardev_new_qapi(id, backend, errp);
+    chr = chardev_new_qapi(id, backend, NULL, errp);
     if (!chr) {
         return NULL;
     }
@@ -1098,7 +1034,9 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
     return ret;
 }
 
-Chardev *qemu_chr_new_cli(ChardevOptions *options, Error **errp)
+static Chardev *qemu_chr_new_cli_gcontext(ChardevOptions *options,
+                                          GMainContext *context,
+                                          Error **errp)
 {
     Chardev *chr;
     char *bid = NULL;
@@ -1107,7 +1045,7 @@ Chardev *qemu_chr_new_cli(ChardevOptions *options, Error **errp)
         bid = g_strdup_printf("%s-base", options->id);
     }
 
-    chr = chardev_new_qapi(bid ?: options->id, options->backend, errp);
+    chr = chardev_new_qapi(bid ?: options->id, options->backend, context, errp);
     if (!chr) {
         goto out;
     }
@@ -1122,8 +1060,8 @@ Chardev *qemu_chr_new_cli(ChardevOptions *options, Error **errp)
             .u.mux.data = &mux_data,
         };
 
-        mux = qemu_chardev_new(options->id, TYPE_CHARDEV_MUX, &backend, NULL,
-                               errp);
+        mux = qemu_chardev_new(options->id, TYPE_CHARDEV_MUX, &backend,
+                               context, errp);
         if (mux == NULL) {
             object_unparent(OBJECT(chr));
             chr = NULL;
@@ -1137,6 +1075,11 @@ out:
     return chr;
 }
 
+Chardev *qemu_chr_new_cli(ChardevOptions *options, Error **errp)
+{
+    return qemu_chr_new_cli_gcontext(options, NULL, errp);
+}
+
 ChardevOptions *qemu_chr_parse_cli_dict(QDict *args, bool help,
                                         Error **errp)
 {
@@ -1180,6 +1123,37 @@ ChardevOptions *qemu_chr_parse_cli_str(const char *optarg, Error **errp)
     return chr_options;
 }
 
+Chardev *qemu_chr_new_from_opts(QemuOpts *opts, GMainContext *context,
+                                Error **errp)
+{
+    ChardevOptions *chr_options;
+    Chardev *chr;
+    QDict *args;
+    const char *name = qemu_opt_get(opts, "backend");
+    bool help;
+
+    args = qemu_opts_to_qdict(opts, NULL);
+
+    if (name && is_help_option(name)) {
+        qdict_del(args, "backend");
+        qdict_del(args, "type");
+        help = true;
+    } else {
+        help = qemu_opt_has_help_opt(opts);
+    }
+
+    chr_options = qemu_chr_parse_cli_dict(args, help, errp);
+    qobject_unref(args);
+
+    if (!chr_options) {
+        return NULL;
+    }
+
+    chr = qemu_chr_new_cli_gcontext(chr_options, context, errp);
+    qapi_free_ChardevOptions(chr_options);
+    return chr;
+}
+
 ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
                                   Error **errp)
 {
-- 
2.28.0

[PATCH 11/13] hmp/char: Use qemu_chr_parse_cli_str() for chardev-change

[Kevin Wolf]

Instead of going through the QemuOpts-based parser, go directly from the
given option string to ChardevOptions. This doesn't only avoid legacy
code, but it also simplifies the implementation.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 monitor/hmp-cmds.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index a6a6684df1..0244068de8 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1793,34 +1793,25 @@ void hmp_chardev_add(Monitor *mon, const QDict *qdict)
 void hmp_chardev_change(Monitor *mon, const QDict *qdict)
 {
     const char *args = qdict_get_str(qdict, "args");
-    const char *id;
+    const char *id = qdict_get_str(qdict, "id");
+    char *optstr;
     Error *err = NULL;
-    ChardevBackend *backend = NULL;
+    ChardevOptions *options = NULL;
     ChardevReturn *ret = NULL;
-    QemuOpts *opts = qemu_opts_parse_noisily(qemu_find_opts("chardev"), args,
-                                             true);
-    if (!opts) {
-        error_setg(&err, "Parsing chardev args failed");
-        goto end;
-    }
 
-    id = qdict_get_str(qdict, "id");
-    if (qemu_opts_id(opts)) {
-        error_setg(&err, "Unexpected 'id' parameter");
-        goto end;
-    }
+    optstr = g_strdup_printf("%s,id=%s", args, id);
 
-    backend = qemu_chr_parse_opts(opts, &err);
-    if (!backend) {
+    options = qemu_chr_parse_cli_str(optstr, &err);
+    if (!options) {
         goto end;
     }
 
-    ret = qmp_chardev_change(id, backend, &err);
+    ret = qmp_chardev_change(options->id, options->backend, &err);
 
 end:
+    g_free(optstr);
     qapi_free_ChardevReturn(ret);
-    qapi_free_ChardevBackend(backend);
-    qemu_opts_del(opts);
+    qapi_free_ChardevOptions(options);
     hmp_handle_error(mon, err);
 }
 
-- 
2.28.0

[PATCH 12/13] char: Remove qemu_chr_parse_opts()

[Kevin Wolf]

The function is unused now, remove it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/chardev/char.h | 10 ----------
 chardev/char.c         | 37 -------------------------------------
 2 files changed, 47 deletions(-)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index 5cd46207f6..761c521bd8 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -146,16 +146,6 @@ void qemu_chr_translate_legacy_options(QDict *args);
  */
 void qemu_chr_parse_common(QemuOpts *opts, ChardevCommon *backend);
 
-/**
- * qemu_chr_parse_opts:
- *
- * Parse the options to the ChardevBackend struct.
- *
- * Returns: a new backend or NULL on error
- */
-ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
-                                    Error **errp);
-
 /**
  * qemu_chr_new:
  * @label: the name of the backend
diff --git a/chardev/char.c b/chardev/char.c
index 4a444a0353..c3cfd473b4 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -610,43 +610,6 @@ static const char *chardev_alias_translate(const char *name)
     return name;
 }
 
-ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts, Error **errp)
-{
-    Error *local_err = NULL;
-    const ChardevClass *cc;
-    ChardevBackend *backend = NULL;
-    const char *name = chardev_alias_translate(qemu_opt_get(opts, "backend"));
-
-    if (name == NULL) {
-        error_setg(errp, "chardev: \"%s\" missing backend",
-                   qemu_opts_id(opts));
-        return NULL;
-    }
-
-    cc = char_get_class(name, errp);
-    if (cc == NULL) {
-        return NULL;
-    }
-
-    backend = g_new0(ChardevBackend, 1);
-    backend->type = CHARDEV_BACKEND_KIND_NULL;
-
-    if (cc->parse) {
-        cc->parse(opts, backend, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            qapi_free_ChardevBackend(backend);
-            return NULL;
-        }
-    } else {
-        ChardevCommon *ccom = g_new0(ChardevCommon, 1);
-        qemu_chr_parse_common(opts, ccom);
-        backend->u.null.data = ccom; /* Any ChardevCommon member would work */
-    }
-
-    return backend;
-}
-
 void qemu_chr_print_types(void)
 {
     g_autoptr(GString) str = g_string_new("");
-- 
2.28.0

[PATCH 13/13] char: Remove ChardevClass.parse

[Kevin Wolf]

The QemuOpts based ChardevClass.parse has been replaced by the QAPI
parser and is unused now, remove it.

After removing all .parse implementations, qemu_chr_parse_common() is
unused, too, so remove that one as well.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/chardev/char.h  | 10 -----
 chardev/char-file.c     | 20 ---------
 chardev/char-mux.c      | 17 --------
 chardev/char-parallel.c | 17 --------
 chardev/char-pipe.c     | 17 --------
 chardev/char-ringbuf.c  | 18 --------
 chardev/char-serial.c   | 17 --------
 chardev/char-socket.c   | 92 -----------------------------------------
 chardev/char-stdio.c    | 13 ------
 chardev/char-udp.c      | 60 ---------------------------
 chardev/char.c          | 11 -----
 chardev/spice.c         | 34 ---------------
 ui/console.c            | 35 ----------------
 ui/gtk.c                |  1 -
 ui/spice-app.c          |  1 -
 15 files changed, 363 deletions(-)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index 761c521bd8..ea095dd998 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -137,15 +137,6 @@ ChardevOptions *qemu_chr_parse_cli_str(const char *optarg, Error **errp);
  */
 void qemu_chr_translate_legacy_options(QDict *args);
 
-/**
- * qemu_chr_parse_common:
- * @opts: the options that still need parsing
- * @backend: a new backend
- *
- * Parse the common options available to all character backends.
- */
-void qemu_chr_parse_common(QemuOpts *opts, ChardevCommon *backend);
-
 /**
  * qemu_chr_new:
  * @label: the name of the backend
@@ -294,7 +285,6 @@ struct ChardevClass {
     ObjectClass parent_class;
 
     bool internal; /* TODO: eventually use TYPE_USER_CREATABLE */
-    void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
     void (*translate_legacy_options)(QDict *args);
 
     void (*open)(Chardev *chr, ChardevBackend *backend,
diff --git a/chardev/char-file.c b/chardev/char-file.c
index 2fd80707e5..5684bfb9b1 100644
--- a/chardev/char-file.c
+++ b/chardev/char-file.c
@@ -96,30 +96,10 @@ static void qmp_chardev_open_file(Chardev *chr,
 #endif
 }
 
-static void qemu_chr_parse_file_out(QemuOpts *opts, ChardevBackend *backend,
-                                    Error **errp)
-{
-    const char *path = qemu_opt_get(opts, "path");
-    ChardevFile *file;
-
-    backend->type = CHARDEV_BACKEND_KIND_FILE;
-    if (path == NULL) {
-        error_setg(errp, "chardev: file: no filename given");
-        return;
-    }
-    file = backend->u.file.data = g_new0(ChardevFile, 1);
-    qemu_chr_parse_common(opts, qapi_ChardevFile_base(file));
-    file->out = g_strdup(path);
-
-    file->has_append = true;
-    file->append = qemu_opt_get_bool(opts, "append", false);
-}
-
 static void char_file_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
-    cc->parse = qemu_chr_parse_file_out;
     cc->open = qmp_chardev_open_file;
 }
 
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 6f980bb836..91542bb3e3 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -332,22 +332,6 @@ static void qemu_chr_open_mux(Chardev *chr,
     qemu_chr_fe_init(&d->chr, drv, errp);
 }
 
-static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend *backend,
-                               Error **errp)
-{
-    const char *chardev = qemu_opt_get(opts, "chardev");
-    ChardevMux *mux;
-
-    if (chardev == NULL) {
-        error_setg(errp, "chardev: mux: no chardev given");
-        return;
-    }
-    backend->type = CHARDEV_BACKEND_KIND_MUX;
-    mux = backend->u.mux.data = g_new0(ChardevMux, 1);
-    qemu_chr_parse_common(opts, qapi_ChardevMux_base(mux));
-    mux->chardev = g_strdup(chardev);
-}
-
 /**
  * Called after processing of default and command-line-specified
  * chardevs to deliver CHR_EVENT_OPENED events to any FEs attached
@@ -377,7 +361,6 @@ static void char_mux_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
-    cc->parse = qemu_chr_parse_mux;
     cc->open = qemu_chr_open_mux;
     cc->chr_write = mux_chr_write;
     cc->chr_accept_input = mux_chr_accept_input;
diff --git a/chardev/char-parallel.c b/chardev/char-parallel.c
index 05e7efbd6c..066f1fc7ba 100644
--- a/chardev/char-parallel.c
+++ b/chardev/char-parallel.c
@@ -254,27 +254,10 @@ static void qmp_chardev_open_parallel(Chardev *chr,
     qemu_chr_open_pp_fd(chr, fd, be_opened, errp);
 }
 
-static void qemu_chr_parse_parallel(QemuOpts *opts, ChardevBackend *backend,
-                                    Error **errp)
-{
-    const char *device = qemu_opt_get(opts, "path");
-    ChardevHostdev *parallel;
-
-    if (device == NULL) {
-        error_setg(errp, "chardev: parallel: no device path given");
-        return;
-    }
-    backend->type = CHARDEV_BACKEND_KIND_PARALLEL;
-    parallel = backend->u.parallel.data = g_new0(ChardevHostdev, 1);
-    qemu_chr_parse_common(opts, qapi_ChardevHostdev_base(parallel));
-    parallel->device = g_strdup(device);
-}
-
 static void char_parallel_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
-    cc->parse = qemu_chr_parse_parallel;
     cc->open = qmp_chardev_open_parallel;
 #if defined(__linux__)
     cc->chr_ioctl = pp_ioctl;
diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
index 7eca5d9a56..97143c67fc 100644
--- a/chardev/char-pipe.c
+++ b/chardev/char-pipe.c
@@ -154,27 +154,10 @@ static void qemu_chr_open_pipe(Chardev *chr,
 
 #endif /* !_WIN32 */
 
-static void qemu_chr_parse_pipe(QemuOpts *opts, ChardevBackend *backend,
-                                Error **errp)
-{
-    const char *device = qemu_opt_get(opts, "path");
-    ChardevHostdev *dev;
-
-    if (device == NULL) {
-        error_setg(errp, "chardev: pipe: no device path given");
-        return;
-    }
-    backend->type = CHARDEV_BACKEND_KIND_PIPE;
-    dev = backend->u.pipe.data = g_new0(ChardevHostdev, 1);
-    qemu_chr_parse_common(opts, qapi_ChardevHostdev_base(dev));
-    dev->device = g_strdup(device);
-}
-
 static void char_pipe_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
-    cc->parse = qemu_chr_parse_pipe;
     cc->open = qemu_chr_open_pipe;
 }
 
diff --git a/chardev/char-ringbuf.c b/chardev/char-ringbuf.c
index d40d21d3cf..16eb46e971 100644
--- a/chardev/char-ringbuf.c
+++ b/chardev/char-ringbuf.c
@@ -206,28 +206,10 @@ char *qmp_ringbuf_read(const char *device, int64_t size,
     return data;
 }
 
-static void qemu_chr_parse_ringbuf(QemuOpts *opts, ChardevBackend *backend,
-                                   Error **errp)
-{
-    int val;
-    ChardevRingbuf *ringbuf;
-
-    backend->type = CHARDEV_BACKEND_KIND_RINGBUF;
-    ringbuf = backend->u.ringbuf.data = g_new0(ChardevRingbuf, 1);
-    qemu_chr_parse_common(opts, qapi_ChardevRingbuf_base(ringbuf));
-
-    val = qemu_opt_get_size(opts, "size", 0);
-    if (val != 0) {
-        ringbuf->has_size = true;
-        ringbuf->size = val;
-    }
-}
-
 static void char_ringbuf_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
-    cc->parse = qemu_chr_parse_ringbuf;
     cc->open = qemu_chr_open_ringbuf;
     cc->chr_write = ringbuf_chr_write;
 }
diff --git a/chardev/char-serial.c b/chardev/char-serial.c
index 7c3d84ae24..7e207339dc 100644
--- a/chardev/char-serial.c
+++ b/chardev/char-serial.c
@@ -279,27 +279,10 @@ static void qmp_chardev_open_serial(Chardev *chr,
 #endif /* __linux__ || __sun__ */
 
 #ifdef HAVE_CHARDEV_SERIAL
-static void qemu_chr_parse_serial(QemuOpts *opts, ChardevBackend *backend,
-                                  Error **errp)
-{
-    const char *device = qemu_opt_get(opts, "path");
-    ChardevHostdev *serial;
-
-    if (device == NULL) {
-        error_setg(errp, "chardev: serial/tty: no device path given");
-        return;
-    }
-    backend->type = CHARDEV_BACKEND_KIND_SERIAL;
-    serial = backend->u.serial.data = g_new0(ChardevHostdev, 1);
-    qemu_chr_parse_common(opts, qapi_ChardevHostdev_base(serial));
-    serial->device = g_strdup(device);
-}
-
 static void char_serial_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
-    cc->parse = qemu_chr_parse_serial;
     cc->open = qmp_chardev_open_serial;
 #ifndef _WIN32
     cc->chr_ioctl = tty_serial_ioctl;
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 6bf916a3e4..a38b9d939f 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1394,97 +1394,6 @@ static void qmp_chardev_open_socket(Chardev *chr,
     }
 }
 
-static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
-                                  Error **errp)
-{
-    const char *path = qemu_opt_get(opts, "path");
-    const char *host = qemu_opt_get(opts, "host");
-    const char *port = qemu_opt_get(opts, "port");
-    const char *fd = qemu_opt_get(opts, "fd");
-#ifdef CONFIG_LINUX
-    bool tight = qemu_opt_get_bool(opts, "tight", true);
-    bool abstract = qemu_opt_get_bool(opts, "abstract", false);
-#endif
-    SocketAddressLegacy *addr;
-    ChardevSocket *sock;
-
-    if ((!!path + !!fd + !!host) != 1) {
-        error_setg(errp,
-                   "Exactly one of 'path', 'fd' or 'host' required");
-        return;
-    }
-
-    if (host && !port) {
-        error_setg(errp, "chardev: socket: no port given");
-        return;
-    }
-
-    backend->type = CHARDEV_BACKEND_KIND_SOCKET;
-    sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
-    qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock));
-
-    sock->has_nodelay = qemu_opt_get(opts, "delay");
-    sock->nodelay = !qemu_opt_get_bool(opts, "delay", true);
-    /*
-     * We have different default to QMP for 'server', hence
-     * we can't just check for existence of 'server'
-     */
-    sock->has_server = true;
-    sock->server = qemu_opt_get_bool(opts, "server", false);
-    sock->has_telnet = qemu_opt_get(opts, "telnet");
-    sock->telnet = qemu_opt_get_bool(opts, "telnet", false);
-    sock->has_tn3270 = qemu_opt_get(opts, "tn3270");
-    sock->tn3270 = qemu_opt_get_bool(opts, "tn3270", false);
-    sock->has_websocket = qemu_opt_get(opts, "websocket");
-    sock->websocket = qemu_opt_get_bool(opts, "websocket", false);
-    /*
-     * We have different default to QMP for 'wait' when 'server'
-     * is set, hence we can't just check for existence of 'wait'
-     */
-    sock->has_wait = qemu_opt_find(opts, "wait") || sock->server;
-    sock->wait = qemu_opt_get_bool(opts, "wait", true);
-    sock->has_reconnect = qemu_opt_find(opts, "reconnect");
-    sock->reconnect = qemu_opt_get_number(opts, "reconnect", 0);
-    sock->has_tls_creds = qemu_opt_get(opts, "tls-creds");
-    sock->tls_creds = g_strdup(qemu_opt_get(opts, "tls-creds"));
-    sock->has_tls_authz = qemu_opt_get(opts, "tls-authz");
-    sock->tls_authz = g_strdup(qemu_opt_get(opts, "tls-authz"));
-
-    addr = g_new0(SocketAddressLegacy, 1);
-    if (path) {
-        UnixSocketAddress *q_unix;
-        addr->type = SOCKET_ADDRESS_LEGACY_KIND_UNIX;
-        q_unix = addr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
-        q_unix->path = g_strdup(path);
-#ifdef CONFIG_LINUX
-        q_unix->has_tight = true;
-        q_unix->tight = tight;
-        q_unix->has_abstract = true;
-        q_unix->abstract = abstract;
-#endif
-    } else if (host) {
-        addr->type = SOCKET_ADDRESS_LEGACY_KIND_INET;
-        addr->u.inet.data = g_new(InetSocketAddress, 1);
-        *addr->u.inet.data = (InetSocketAddress) {
-            .host = g_strdup(host),
-            .port = g_strdup(port),
-            .has_to = qemu_opt_get(opts, "to"),
-            .to = qemu_opt_get_number(opts, "to", 0),
-            .has_ipv4 = qemu_opt_get(opts, "ipv4"),
-            .ipv4 = qemu_opt_get_bool(opts, "ipv4", 0),
-            .has_ipv6 = qemu_opt_get(opts, "ipv6"),
-            .ipv6 = qemu_opt_get_bool(opts, "ipv6", 0),
-        };
-    } else if (fd) {
-        addr->type = SOCKET_ADDRESS_LEGACY_KIND_FD;
-        addr->u.fd.data = g_new(String, 1);
-        addr->u.fd.data->str = g_strdup(fd);
-    } else {
-        g_assert_not_reached();
-    }
-    sock->addr = addr;
-}
-
 static void qemu_chr_translate_socket(QDict *args)
 {
     const char *path = qdict_get_try_str(args, "path");
@@ -1557,7 +1466,6 @@ static void char_socket_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
-    cc->parse = qemu_chr_parse_socket;
     cc->translate_legacy_options = qemu_chr_translate_socket;
     cc->open = qmp_chardev_open_socket;
     cc->chr_wait_connected = tcp_chr_wait_connected;
diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c
index 403da308c9..74e2b25e9b 100644
--- a/chardev/char-stdio.c
+++ b/chardev/char-stdio.c
@@ -117,23 +117,10 @@ static void qemu_chr_open_stdio(Chardev *chr,
 }
 #endif
 
-static void qemu_chr_parse_stdio(QemuOpts *opts, ChardevBackend *backend,
-                                 Error **errp)
-{
-    ChardevStdio *stdio;
-
-    backend->type = CHARDEV_BACKEND_KIND_STDIO;
-    stdio = backend->u.stdio.data = g_new0(ChardevStdio, 1);
-    qemu_chr_parse_common(opts, qapi_ChardevStdio_base(stdio));
-    stdio->has_signal = true;
-    stdio->signal = qemu_opt_get_bool(opts, "signal", true);
-}
-
 static void char_stdio_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
-    cc->parse = qemu_chr_parse_stdio;
 #ifndef _WIN32
     cc->open = qemu_chr_open_stdio;
     cc->chr_set_echo = qemu_chr_set_echo_stdio;
diff --git a/chardev/char-udp.c b/chardev/char-udp.c
index 61752b1c51..1543fbca0c 100644
--- a/chardev/char-udp.c
+++ b/chardev/char-udp.c
@@ -133,65 +133,6 @@ static void char_udp_finalize(Object *obj)
     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
 
-static void qemu_chr_parse_udp(QemuOpts *opts, ChardevBackend *backend,
-                               Error **errp)
-{
-    const char *host = qemu_opt_get(opts, "host");
-    const char *port = qemu_opt_get(opts, "port");
-    const char *localaddr = qemu_opt_get(opts, "localaddr");
-    const char *localport = qemu_opt_get(opts, "localport");
-    bool has_local = false;
-    SocketAddressLegacy *addr;
-    ChardevUdp *udp;
-
-    backend->type = CHARDEV_BACKEND_KIND_UDP;
-    if (host == NULL || strlen(host) == 0) {
-        host = "localhost";
-    }
-    if (port == NULL || strlen(port) == 0) {
-        error_setg(errp, "chardev: udp: remote port not specified");
-        return;
-    }
-    if (localport == NULL || strlen(localport) == 0) {
-        localport = "0";
-    } else {
-        has_local = true;
-    }
-    if (localaddr == NULL || strlen(localaddr) == 0) {
-        localaddr = "";
-    } else {
-        has_local = true;
-    }
-
-    udp = backend->u.udp.data = g_new0(ChardevUdp, 1);
-    qemu_chr_parse_common(opts, qapi_ChardevUdp_base(udp));
-
-    addr = g_new0(SocketAddressLegacy, 1);
-    addr->type = SOCKET_ADDRESS_LEGACY_KIND_INET;
-    addr->u.inet.data = g_new(InetSocketAddress, 1);
-    *addr->u.inet.data = (InetSocketAddress) {
-        .host = g_strdup(host),
-        .port = g_strdup(port),
-        .has_ipv4 = qemu_opt_get(opts, "ipv4"),
-        .ipv4 = qemu_opt_get_bool(opts, "ipv4", 0),
-        .has_ipv6 = qemu_opt_get(opts, "ipv6"),
-        .ipv6 = qemu_opt_get_bool(opts, "ipv6", 0),
-    };
-    udp->remote = addr;
-
-    if (has_local) {
-        udp->has_local = true;
-        addr = g_new0(SocketAddressLegacy, 1);
-        addr->type = SOCKET_ADDRESS_LEGACY_KIND_INET;
-        addr->u.inet.data = g_new(InetSocketAddress, 1);
-        *addr->u.inet.data = (InetSocketAddress) {
-            .host = g_strdup(localaddr),
-            .port = g_strdup(localport),
-        };
-        udp->local = addr;
-    }
-}
-
 static void qemu_chr_translate_udp(QDict *args)
 {
     QDict *remote;
@@ -256,7 +197,6 @@ static void char_udp_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
-    cc->parse = qemu_chr_parse_udp;
     cc->translate_legacy_options = qemu_chr_translate_udp;
     cc->open = qmp_chardev_open_udp;
     cc->chr_write = udp_chr_write;
diff --git a/chardev/char.c b/chardev/char.c
index c3cfd473b4..a26fe5555e 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -497,17 +497,6 @@ fail:
     return NULL;
 }
 
-void qemu_chr_parse_common(QemuOpts *opts, ChardevCommon *backend)
-{
-    const char *logfile = qemu_opt_get(opts, "logfile");
-
-    backend->has_logfile = logfile != NULL;
-    backend->logfile = g_strdup(logfile);
-
-    backend->has_logappend = true;
-    backend->logappend = qemu_opt_get_bool(opts, "logappend", false);
-}
-
 static const ChardevClass *char_get_class(const char *driver, Error **errp)
 {
     ObjectClass *oc;
diff --git a/chardev/spice.c b/chardev/spice.c
index 1104426e3a..c87bd7e18e 100644
--- a/chardev/spice.c
+++ b/chardev/spice.c
@@ -317,38 +317,6 @@ static void qemu_chr_open_spice_port(Chardev *chr,
     vmc_register_interface(s);
 }
 
-static void qemu_chr_parse_spice_vmc(QemuOpts *opts, ChardevBackend *backend,
-                                     Error **errp)
-{
-    const char *name = qemu_opt_get(opts, "name");
-    ChardevSpiceChannel *spicevmc;
-
-    if (name == NULL) {
-        error_setg(errp, "chardev: spice channel: no name given");
-        return;
-    }
-    backend->type = CHARDEV_BACKEND_KIND_SPICEVMC;
-    spicevmc = backend->u.spicevmc.data = g_new0(ChardevSpiceChannel, 1);
-    qemu_chr_parse_common(opts, qapi_ChardevSpiceChannel_base(spicevmc));
-    spicevmc->type = g_strdup(name);
-}
-
-static void qemu_chr_parse_spice_port(QemuOpts *opts, ChardevBackend *backend,
-                                      Error **errp)
-{
-    const char *name = qemu_opt_get(opts, "name");
-    ChardevSpicePort *spiceport;
-
-    if (name == NULL) {
-        error_setg(errp, "chardev: spice port: no name given");
-        return;
-    }
-    backend->type = CHARDEV_BACKEND_KIND_SPICEPORT;
-    spiceport = backend->u.spiceport.data = g_new0(ChardevSpicePort, 1);
-    qemu_chr_parse_common(opts, qapi_ChardevSpicePort_base(spiceport));
-    spiceport->fqdn = g_strdup(name);
-}
-
 static void char_spice_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
@@ -371,7 +339,6 @@ static void char_spicevmc_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
-    cc->parse = qemu_chr_parse_spice_vmc;
     cc->open = qemu_chr_open_spice_vmc;
     cc->chr_set_fe_open = spice_vmc_set_fe_open;
 }
@@ -386,7 +353,6 @@ static void char_spiceport_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
-    cc->parse = qemu_chr_parse_spice_port;
     cc->open = qemu_chr_open_spice_port;
     cc->chr_set_fe_open = spice_port_set_fe_open;
 }
diff --git a/ui/console.c b/ui/console.c
index e8e59707d3..7d8c1d1306 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2386,40 +2386,6 @@ void qemu_display_help(void)
     }
 }
 
-void qemu_chr_parse_vc(QemuOpts *opts, ChardevBackend *backend, Error **errp)
-{
-    int val;
-    ChardevVC *vc;
-
-    backend->type = CHARDEV_BACKEND_KIND_VC;
-    vc = backend->u.vc.data = g_new0(ChardevVC, 1);
-    qemu_chr_parse_common(opts, qapi_ChardevVC_base(vc));
-
-    val = qemu_opt_get_number(opts, "width", 0);
-    if (val != 0) {
-        vc->has_width = true;
-        vc->width = val;
-    }
-
-    val = qemu_opt_get_number(opts, "height", 0);
-    if (val != 0) {
-        vc->has_height = true;
-        vc->height = val;
-    }
-
-    val = qemu_opt_get_number(opts, "cols", 0);
-    if (val != 0) {
-        vc->has_cols = true;
-        vc->cols = val;
-    }
-
-    val = qemu_opt_get_number(opts, "rows", 0);
-    if (val != 0) {
-        vc->has_rows = true;
-        vc->rows = val;
-    }
-}
-
 static const TypeInfo qemu_console_info = {
     .name = TYPE_QEMU_CONSOLE,
     .parent = TYPE_OBJECT,
@@ -2431,7 +2397,6 @@ static void char_vc_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
-    cc->parse = qemu_chr_parse_vc;
     cc->open = vc_chr_open;
     cc->chr_write = vc_chr_write;
     cc->chr_set_echo = vc_chr_set_echo;
diff --git a/ui/gtk.c b/ui/gtk.c
index a752aa22be..7a5702c28b 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -1738,7 +1738,6 @@ static void char_gd_vc_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
-    cc->parse = qemu_chr_parse_vc;
     cc->open = gd_vc_open;
     cc->chr_write = gd_vc_chr_write;
     cc->chr_set_echo = gd_vc_chr_set_echo;
diff --git a/ui/spice-app.c b/ui/spice-app.c
index 026124ef56..f3418111fb 100644
--- a/ui/spice-app.c
+++ b/ui/spice-app.c
@@ -101,7 +101,6 @@ static void char_vc_class_init(ObjectClass *oc, void *data)
 
     vc->parent_open = cc->open;
 
-    cc->parse = qemu_chr_parse_vc;
     cc->open = vc_chr_open;
     cc->chr_set_echo = vc_chr_set_echo;
 }
-- 
2.28.0

Re: [PATCH 08/13] char: Add mux option to ChardevOptions

[Paolo Bonzini]

On 12/11/20 18:59, Kevin Wolf wrote:
> The final missing piece to achieve compatibility between
> qemu_chr_parse_cli_str()/qemu_chr_new_cli() and the legacy command line
> is support for the 'mux' option. Implement it.
> 
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> ---
>   qapi/char.json |  4 +++-
>   chardev/char.c | 41 +++++++++++++++++++++++++++++++++++------
>   2 files changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/qapi/char.json b/qapi/char.json
> index e1f9347044..d6733a5473 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -453,12 +453,14 @@
>   #
>   # @id: the chardev's ID, must be unique
>   # @backend: backend type and parameters
> +# @mux: enable multiplexing mode (default: false)
>   #
>   # Since: 6.0
>   ##
>   { 'struct': 'ChardevOptions',
>     'data': { 'id': 'str',
> -            'backend': 'ChardevBackend' },
> +            'backend': 'ChardevBackend',
> +            '*mux': 'bool' },
>     'aliases': [ { 'source': ['backend'] } ] }
>   

I think this shows that -chardev and chardev-add are both kind of 
unrepairable.  The right way to do a mux with a serial and monitor on 
top should be explicit:

             stdio
                ↑
          mux-controller
           ↑        ↑
          mux      mux
           ↑        ↑
        serial   monitor

	-object chardev-stdio,id=stdio
	-object chardev-mux-controller,id=mux,backend=stdio
	-object chardev-mux,id=serial-chardev,controller=mux
	-object chardev-mux,id=mon-chardev,controller=mux
	-serial chardev:serial-chardev
	-monitor chardev:mon-chardev

Adding automagic "mux" creation to -chardev is wrong.  I don't entirely 
object to the series since it's quite nice, but I see it as more of a 
cleanup than the final stage.  It hinges on what Markus thinks of 
aliases, I guess.

Paolo

Re: [PATCH 08/13] char: Add mux option to ChardevOptions

[Kevin Wolf]

Am 13.11.2020 um 12:50 hat Paolo Bonzini geschrieben:
> On 12/11/20 18:59, Kevin Wolf wrote:
> > The final missing piece to achieve compatibility between
> > qemu_chr_parse_cli_str()/qemu_chr_new_cli() and the legacy command line
> > is support for the 'mux' option. Implement it.
> > 
> > Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> > ---
> >   qapi/char.json |  4 +++-
> >   chardev/char.c | 41 +++++++++++++++++++++++++++++++++++------
> >   2 files changed, 38 insertions(+), 7 deletions(-)
> > 
> > diff --git a/qapi/char.json b/qapi/char.json
> > index e1f9347044..d6733a5473 100644
> > --- a/qapi/char.json
> > +++ b/qapi/char.json
> > @@ -453,12 +453,14 @@
> >   #
> >   # @id: the chardev's ID, must be unique
> >   # @backend: backend type and parameters
> > +# @mux: enable multiplexing mode (default: false)
> >   #
> >   # Since: 6.0
> >   ##
> >   { 'struct': 'ChardevOptions',
> >     'data': { 'id': 'str',
> > -            'backend': 'ChardevBackend' },
> > +            'backend': 'ChardevBackend',
> > +            '*mux': 'bool' },
> >     'aliases': [ { 'source': ['backend'] } ] }
> 
> I think this shows that -chardev and chardev-add are both kind of
> unrepairable.  The right way to do a mux with a serial and monitor on
> top should be explicit:
> 
>             stdio
>                ↑
>          mux-controller
>           ↑        ↑
>          mux      mux
>           ↑        ↑
>        serial   monitor
> 
> 	-object chardev-stdio,id=stdio
> 	-object chardev-mux-controller,id=mux,backend=stdio
> 	-object chardev-mux,id=serial-chardev,controller=mux
> 	-object chardev-mux,id=mon-chardev,controller=mux
> 	-serial chardev:serial-chardev
> 	-monitor chardev:mon-chardev

I don't think these "mux" chardevs plugged to a "mux-controller"
actually exist, at least today. You can directly plug serial and monitor
to a mux chardev that sits on top of stdio.

This is the current syntax for explicitly configuring things:

    -chardev stdio,id=stdio
    -chardev mux,chardev=stdio,id=mux
    -mon chardev=mux
    -serial chardev:mux

And of course this is still possible after my series, and it is what
management tools should be using.

> Adding automagic "mux" creation to -chardev is wrong.

I'm not really adding it, it's already there.

While the code is temporarily duplicated and it looks like an addition
here, at the end of the series it's effectively just some preexisting
code moved (and QAPIfied) from qemu_chr_new_from_opts().

Of course, we can deprecate it, but I don't think it's really in the way
because it just desugars to two normal chardev definitions. On the other
hand, I've never used it (apart from testing this patch), so I don't
really care in practice if it exists or not.

> I don't entirely object to the series since it's quite nice, but I see
> it as more of a cleanup than the final stage.  It hinges on what
> Markus thinks of aliases, I guess.

Yes, I completely agree that this is mostly a cleanup. Most QAPIfication
actually is, because QemuOpts and hand written parsers do work and we
have been successfully using them for years. They just work in less
consistent ways and take a bit more code.

I never said it has to be the final stage, but I think whatever the
final stage is, having external interfaces that are consistent and use
the QAPI schema as the one source of truth will be helpful.

This is basically what I meant when I said your QOM conversion and my
QAPIfication work aren't conflicting (conceptually), but addressing
separate problems.

Kevin

Re: [PATCH 08/13] char: Add mux option to ChardevOptions

[Paolo Bonzini]

On 13/11/20 14:20, Kevin Wolf wrote:
>> 	-object chardev-stdio,id=stdio
>> 	-object chardev-mux-controller,id=mux,backend=stdio
>> 	-object chardev-mux,id=serial-chardev,controller=mux
>> 	-object chardev-mux,id=mon-chardev,controller=mux
>> 	-serial chardev:serial-chardev
>> 	-monitor chardev:mon-chardev
> I don't think these "mux" chardevs plugged to a "mux-controller"
> actually exist, at least today. You can directly plug serial and monitor
> to a mux chardev that sits on top of stdio.

Yes, you're right.  There's 2 CharBackends plugged into a single mux 
Chardev.

> This is basically what I meant when I said your QOM conversion and my
> QAPIfication work aren't conflicting (conceptually), but addressing
> separate problems.

Fair enough, I think I understand what you mean now.  And I can't really 
argue with the diffstat and the usage of aliases to clean up dash vs. 
underscores.

Paolo

Re: [PATCH 11/13] hmp/char: Use qemu_chr_parse_cli_str() for chardev-change

[Dr. David Alan Gilbert]

* Kevin Wolf (kwolf@redhat.com) wrote:
> Instead of going through the QemuOpts-based parser, go directly from the
> given option string to ChardevOptions. This doesn't only avoid legacy
> code, but it also simplifies the implementation.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

OK, from HMP I think

Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

I'm assuming there's no change in the escaping from you extracting it
from the qdict and then printfing it back to go throguh the parser?

Dave

> ---
>  monitor/hmp-cmds.c | 27 +++++++++------------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index a6a6684df1..0244068de8 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1793,34 +1793,25 @@ void hmp_chardev_add(Monitor *mon, const QDict *qdict)
>  void hmp_chardev_change(Monitor *mon, const QDict *qdict)
>  {
>      const char *args = qdict_get_str(qdict, "args");
> -    const char *id;
> +    const char *id = qdict_get_str(qdict, "id");
> +    char *optstr;
>      Error *err = NULL;
> -    ChardevBackend *backend = NULL;
> +    ChardevOptions *options = NULL;
>      ChardevReturn *ret = NULL;
> -    QemuOpts *opts = qemu_opts_parse_noisily(qemu_find_opts("chardev"), args,
> -                                             true);
> -    if (!opts) {
> -        error_setg(&err, "Parsing chardev args failed");
> -        goto end;
> -    }
>  
> -    id = qdict_get_str(qdict, "id");
> -    if (qemu_opts_id(opts)) {
> -        error_setg(&err, "Unexpected 'id' parameter");
> -        goto end;
> -    }
> +    optstr = g_strdup_printf("%s,id=%s", args, id);
>  
> -    backend = qemu_chr_parse_opts(opts, &err);
> -    if (!backend) {
> +    options = qemu_chr_parse_cli_str(optstr, &err);
> +    if (!options) {
>          goto end;
>      }
>  
> -    ret = qmp_chardev_change(id, backend, &err);
> +    ret = qmp_chardev_change(options->id, options->backend, &err);
>  
>  end:
> +    g_free(optstr);
>      qapi_free_ChardevReturn(ret);
> -    qapi_free_ChardevBackend(backend);
> -    qemu_opts_del(opts);
> +    qapi_free_ChardevOptions(options);
>      hmp_handle_error(mon, err);
>  }
>  
> -- 
> 2.28.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK