[Qemu-devel] [PATCH 0/4] Convert remaining legacy chardevs to parse/kind

[Qemu-devel] [PATCH 0/4] Convert remaining legacy chardevs to parse/kind

[Peter Maydell]

This patchset converts the two remaining legacy chardevs
('socket' and 'udp') to use the new-style parse/kind
mechanisms, and removes all the no-longer-required
legacy machinery.

Patch 1 was posted to the list back in June
(https://patches.linaro.org/32298/). I've fixed the obvious
bug picked up in code review, and as far as I can tell from
the thread we decided that the blocking/non-blocking
difference between QAPI and legacy wasn't a problem.

Patch 2 fixes a hole in the functionality of QAPI-described
UDP chardevs, to avoid regressing the commandline functionality
when we convert the UDP backend in patch 3.

Patch 4 may be easier to review as an ignore-whitespaces
diff (the de-indentation makes the diff a bit awkward).

Peter Maydell (4):
  qemu-char: Convert socket char backend to parse/kind
  util/qemu-sockets.c: Support specifying IPv4 or IPv6 in socket_dgram()
  qemu-char: Convert udp char backend to parse/kind
  qemu-char: Remove register_char_driver() machinery

 include/sysemu/char.h |   1 -
 qemu-char.c           | 309 ++++++++++++++++++++++++--------------------------
 util/qemu-sockets.c   |   3 +-
 3 files changed, 151 insertions(+), 162 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/4] qemu-char: Convert socket char backend to parse/kind

[Peter Maydell]

Convert the socket char backend to the new style QAPI framework;
this allows it to return an Error ** to callers who might not
want it to print directly about socket failures.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 qemu-char.c | 114 +++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 58 insertions(+), 56 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 6964a2d..cac7edb 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2983,61 +2983,6 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
     return chr;
 }
 
-static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
-{
-    CharDriverState *chr = NULL;
-    Error *local_err = NULL;
-    int fd = -1;
-
-    bool is_listen      = qemu_opt_get_bool(opts, "server", false);
-    bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true);
-    bool is_telnet      = qemu_opt_get_bool(opts, "telnet", false);
-    bool do_nodelay     = !qemu_opt_get_bool(opts, "delay", true);
-    bool is_unix        = qemu_opt_get(opts, "path") != NULL;
-
-    if (is_unix) {
-        if (is_listen) {
-            fd = unix_listen_opts(opts, &local_err);
-        } else {
-            fd = unix_connect_opts(opts, &local_err, NULL, NULL);
-        }
-    } else {
-        if (is_listen) {
-            fd = inet_listen_opts(opts, 0, &local_err);
-        } else {
-            fd = inet_connect_opts(opts, &local_err, NULL, NULL);
-        }
-    }
-    if (fd < 0) {
-        goto fail;
-    }
-
-    if (!is_waitconnect)
-        qemu_set_nonblock(fd);
-
-    chr = qemu_chr_open_socket_fd(fd, do_nodelay, is_listen, is_telnet,
-                                  is_waitconnect, &local_err);
-    if (local_err) {
-        goto fail;
-    }
-    return chr;
-
-
- fail:
-    if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-    }
-    if (fd >= 0) {
-        closesocket(fd);
-    }
-    if (chr) {
-        g_free(chr->opaque);
-        g_free(chr);
-    }
-    return NULL;
-}
-
 /*********************************************************/
 /* Ring buffer chardev */
 
@@ -3448,6 +3393,62 @@ static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend *backend,
     backend->mux->chardev = g_strdup(chardev);
 }
 
+static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
+                                  Error **errp)
+{
+    bool is_listen      = qemu_opt_get_bool(opts, "server", false);
+    bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true);
+    bool is_telnet      = qemu_opt_get_bool(opts, "telnet", false);
+    bool do_nodelay     = !qemu_opt_get_bool(opts, "delay", true);
+    const char *path = qemu_opt_get(opts, "path");
+    const char *host = qemu_opt_get(opts, "host");
+    const char *port = qemu_opt_get(opts, "port");
+    SocketAddress *addr;
+
+    if (!path) {
+        if (!host) {
+            error_setg(errp, "chardev: socket: no host given");
+            return;
+        }
+        if (!port) {
+            error_setg(errp, "chardev: socket: no port given");
+            return;
+        }
+    }
+
+    backend->socket = g_new0(ChardevSocket, 1);
+
+    backend->socket->has_nodelay = true;
+    backend->socket->nodelay = do_nodelay;
+    backend->socket->has_server = true;
+    backend->socket->server = is_listen;
+    backend->socket->has_telnet = true;
+    backend->socket->telnet = is_telnet;
+    backend->socket->has_wait = true;
+    backend->socket->wait = is_waitconnect;
+
+    addr = g_new0(SocketAddress, 1);
+    if (path) {
+        addr->kind = SOCKET_ADDRESS_KIND_UNIX;
+        addr->q_unix = g_new0(UnixSocketAddress, 1);
+        addr->q_unix->path = g_strdup(path);
+    } else {
+        addr->kind = SOCKET_ADDRESS_KIND_INET;
+        addr->inet = g_new0(InetSocketAddress, 1);
+        addr->inet->host = g_strdup(host);
+        addr->inet->port = g_strdup(port);
+        addr->inet->to = qemu_opt_get_number(opts, "to", 0);
+        addr->inet->has_to = true;
+        if (qemu_opt_get_bool(opts, "ipv4", 0)) {
+            addr->inet->has_ipv4 = addr->inet->ipv4 = true;
+        }
+        if (qemu_opt_get_bool(opts, "ipv6", 0)) {
+            addr->inet->has_ipv6 = addr->inet->ipv6 = true;
+        }
+    }
+    backend->socket->addr = addr;
+}
+
 typedef struct CharDriver {
     const char *name;
     /* old, pre qapi */
@@ -4136,7 +4137,8 @@ void qmp_chardev_remove(const char *id, Error **errp)
 static void register_types(void)
 {
     register_char_driver_qapi("null", CHARDEV_BACKEND_KIND_NULL, NULL);
-    register_char_driver("socket", qemu_chr_open_socket);
+    register_char_driver_qapi("socket", CHARDEV_BACKEND_KIND_SOCKET,
+                              qemu_chr_parse_socket);
     register_char_driver("udp", qemu_chr_open_udp);
     register_char_driver_qapi("ringbuf", CHARDEV_BACKEND_KIND_RINGBUF,
                               qemu_chr_parse_ringbuf);
-- 
1.9.1

[Qemu-devel] [PATCH 2/4] util/qemu-sockets.c: Support specifying IPv4 or IPv6 in socket_dgram()

[Peter Maydell]

Currently you can specify whether you want a UDP chardev backend
to be IPv4 or IPv6 using the ipv4 or ipv6 options if you use the
QemuOpts parsing code in inet_dgram_opts(). However the QMP struct
parsing code in socket_dgram() doesn't provide this flexibility
(which in turn prevents us from converting the UDP backend handling
to the new style QAPI framework).

Use the existing inet_addr_to_opts() function to convert the
remote->inet address to option strings; this handles ipv4 and
ipv6 flags as well as host and port. (It will also convert any
'to' specification, which is harmless as it is ignored in this
context.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 util/qemu-sockets.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 74cf078..25e5fd1 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -966,8 +966,7 @@ int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
     opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
     switch (remote->kind) {
     case SOCKET_ADDRESS_KIND_INET:
-        qemu_opt_set(opts, "host", remote->inet->host);
-        qemu_opt_set(opts, "port", remote->inet->port);
+        inet_addr_to_opts(opts, remote->inet);
         if (local) {
             qemu_opt_set(opts, "localaddr", local->inet->host);
             qemu_opt_set(opts, "localport", local->inet->port);
-- 
1.9.1

[Qemu-devel] [PATCH 3/4] qemu-char: Convert udp char backend to parse/kind

[Peter Maydell]

Convert the udp char backend to the new style QAPI framework.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 qemu-char.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 54 insertions(+), 15 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index cac7edb..a01ccdc 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2383,20 +2383,6 @@ static CharDriverState *qemu_chr_open_udp_fd(int fd)
     return chr;
 }
 
-static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
-{
-    Error *local_err = NULL;
-    int fd = -1;
-
-    fd = inet_dgram_opts(opts, &local_err);
-    if (fd < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        return NULL;
-    }
-    return qemu_chr_open_udp_fd(fd);
-}
-
 /***********************************************************/
 /* TCP Net console */
 
@@ -3449,6 +3435,58 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
     backend->socket->addr = addr;
 }
 
+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");
+    bool ipv4 = qemu_opt_get_bool(opts, "ipv4", 0);
+    bool ipv6 = qemu_opt_get_bool(opts, "ipv6", 0);
+    const char *localaddr = qemu_opt_get(opts, "localaddr");
+    const char *localport = qemu_opt_get(opts, "localport");
+    bool has_local = false;
+    SocketAddress *addr;
+
+    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;
+    }
+
+    backend->udp = g_new0(ChardevUdp, 1);
+
+    addr = g_new0(SocketAddress, 1);
+    addr->kind = SOCKET_ADDRESS_KIND_INET;
+    addr->inet = g_new0(InetSocketAddress, 1);
+    addr->inet->host = g_strdup(host);
+    addr->inet->port = g_strdup(port);
+    addr->inet->has_ipv4 = addr->inet->ipv4 = ipv4;
+    addr->inet->has_ipv6 = addr->inet->ipv6 = ipv6;
+    backend->udp->remote = addr;
+
+    if (has_local) {
+        backend->udp->has_local = true;
+        addr = g_new0(SocketAddress, 1);
+        addr->kind = SOCKET_ADDRESS_KIND_INET;
+        addr->inet = g_new0(InetSocketAddress, 1);
+        addr->inet->host = g_strdup(localaddr);
+        addr->inet->port = g_strdup(localport);
+        backend->udp->local = addr;
+    }
+}
+
 typedef struct CharDriver {
     const char *name;
     /* old, pre qapi */
@@ -4139,7 +4177,8 @@ static void register_types(void)
     register_char_driver_qapi("null", CHARDEV_BACKEND_KIND_NULL, NULL);
     register_char_driver_qapi("socket", CHARDEV_BACKEND_KIND_SOCKET,
                               qemu_chr_parse_socket);
-    register_char_driver("udp", qemu_chr_open_udp);
+    register_char_driver_qapi("udp", CHARDEV_BACKEND_KIND_UDP,
+                              qemu_chr_parse_udp);
     register_char_driver_qapi("ringbuf", CHARDEV_BACKEND_KIND_RINGBUF,
                               qemu_chr_parse_ringbuf);
     register_char_driver_qapi("file", CHARDEV_BACKEND_KIND_FILE,
-- 
1.9.1

[Qemu-devel] [PATCH 4/4] qemu-char: Remove register_char_driver() machinery

[Peter Maydell]

Now that all the char backends have been converted to the QAPI
framework we can remove the machinery for handling old style
backends.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/sysemu/char.h |   1 -
 qemu-char.c           | 126 +++++++++++++++-----------------------------------
 2 files changed, 38 insertions(+), 89 deletions(-)

diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 98cd4c9..dbfd4e6 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -344,7 +344,6 @@ bool chr_is_ringbuf(const CharDriverState *chr);
 
 QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
 
-void register_char_driver(const char *name, CharDriverState *(*open)(QemuOpts *));
 void register_char_driver_qapi(const char *name, ChardevBackendKind kind,
         void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp));
 
diff --git a/qemu-char.c b/qemu-char.c
index a01ccdc..2ffab1a 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3489,26 +3489,12 @@ static void qemu_chr_parse_udp(QemuOpts *opts, ChardevBackend *backend,
 
 typedef struct CharDriver {
     const char *name;
-    /* old, pre qapi */
-    CharDriverState *(*open)(QemuOpts *opts);
-    /* new, qapi-based */
     ChardevBackendKind kind;
     void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
 } CharDriver;
 
 static GSList *backends;
 
-void register_char_driver(const char *name, CharDriverState *(*open)(QemuOpts *))
-{
-    CharDriver *s;
-
-    s = g_malloc0(sizeof(*s));
-    s->name = g_strdup(name);
-    s->open = open;
-
-    backends = g_slist_append(backends, s);
-}
-
 void register_char_driver_qapi(const char *name, ChardevBackendKind kind,
         void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp))
 {
@@ -3530,8 +3516,12 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
     CharDriver *cd;
     CharDriverState *chr;
     GSList *i;
+    ChardevReturn *ret = NULL;
+    ChardevBackend *backend;
+    const char *id = qemu_opts_id(opts);
+    char *bid = NULL;
 
-    if (qemu_opts_id(opts) == NULL) {
+    if (id == NULL) {
         error_setg(errp, "chardev: no id specified");
         goto err;
     }
@@ -3554,89 +3544,49 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
         goto err;
     }
 
-    if (!cd->open) {
-        /* using new, qapi init */
-        ChardevBackend *backend = g_new0(ChardevBackend, 1);
-        ChardevReturn *ret = NULL;
-        const char *id = qemu_opts_id(opts);
-        char *bid = NULL;
+    backend = g_new0(ChardevBackend, 1);
 
-        if (qemu_opt_get_bool(opts, "mux", 0)) {
-            bid = g_strdup_printf("%s-base", id);
-        }
+    if (qemu_opt_get_bool(opts, "mux", 0)) {
+        bid = g_strdup_printf("%s-base", id);
+    }
 
-        chr = NULL;
-        backend->kind = cd->kind;
-        if (cd->parse) {
-            cd->parse(opts, backend, &local_err);
-            if (local_err) {
-                error_propagate(errp, local_err);
-                goto qapi_out;
-            }
-        }
-        ret = qmp_chardev_add(bid ? bid : id, backend, errp);
-        if (!ret) {
+    chr = NULL;
+    backend->kind = cd->kind;
+    if (cd->parse) {
+        cd->parse(opts, backend, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
             goto qapi_out;
         }
-
-        if (bid) {
-            qapi_free_ChardevBackend(backend);
-            qapi_free_ChardevReturn(ret);
-            backend = g_new0(ChardevBackend, 1);
-            backend->mux = g_new0(ChardevMux, 1);
-            backend->kind = CHARDEV_BACKEND_KIND_MUX;
-            backend->mux->chardev = g_strdup(bid);
-            ret = qmp_chardev_add(id, backend, errp);
-            if (!ret) {
-                chr = qemu_chr_find(bid);
-                qemu_chr_delete(chr);
-                chr = NULL;
-                goto qapi_out;
-            }
-        }
-
-        chr = qemu_chr_find(id);
-        chr->opts = opts;
-
-    qapi_out:
-        qapi_free_ChardevBackend(backend);
-        qapi_free_ChardevReturn(ret);
-        g_free(bid);
-        return chr;
     }
-
-    chr = cd->open(opts);
-    if (!chr) {
-        error_setg(errp, "chardev: opening backend \"%s\" failed",
-                   qemu_opt_get(opts, "backend"));
-        goto err;
+    ret = qmp_chardev_add(bid ? bid : id, backend, errp);
+    if (!ret) {
+        goto qapi_out;
     }
 
-    if (!chr->filename)
-        chr->filename = g_strdup(qemu_opt_get(opts, "backend"));
-    chr->init = init;
-    /* if we didn't create the chardev via qmp_chardev_add, we
-     * need to send the OPENED event here
-     */
-    if (!chr->explicit_be_open) {
-        qemu_chr_be_event(chr, CHR_EVENT_OPENED);
+    if (bid) {
+        qapi_free_ChardevBackend(backend);
+        qapi_free_ChardevReturn(ret);
+        backend = g_new0(ChardevBackend, 1);
+        backend->mux = g_new0(ChardevMux, 1);
+        backend->kind = CHARDEV_BACKEND_KIND_MUX;
+        backend->mux->chardev = g_strdup(bid);
+        ret = qmp_chardev_add(id, backend, errp);
+        if (!ret) {
+            chr = qemu_chr_find(bid);
+            qemu_chr_delete(chr);
+            chr = NULL;
+            goto qapi_out;
+        }
     }
-    QTAILQ_INSERT_TAIL(&chardevs, chr, next);
 
-    if (qemu_opt_get_bool(opts, "mux", 0)) {
-        CharDriverState *base = chr;
-        int len = strlen(qemu_opts_id(opts)) + 6;
-        base->label = g_malloc(len);
-        snprintf(base->label, len, "%s-base", qemu_opts_id(opts));
-        chr = qemu_chr_open_mux(base);
-        chr->filename = base->filename;
-        chr->avail_connections = MAX_MUX;
-        QTAILQ_INSERT_TAIL(&chardevs, chr, next);
-    } else {
-        chr->avail_connections = 1;
-    }
-    chr->label = g_strdup(qemu_opts_id(opts));
+    chr = qemu_chr_find(id);
     chr->opts = opts;
+
+qapi_out:
+    qapi_free_ChardevBackend(backend);
+    qapi_free_ChardevReturn(ret);
+    g_free(bid);
     return chr;
 
 err:
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/4] qemu-char: Convert socket char backend to parse/kind

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> Convert the socket char backend to the new style QAPI framework;
> this allows it to return an Error ** to callers who might not
> want it to print directly about socket failures.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  qemu-char.c | 114 +++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 58 insertions(+), 56 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 6964a2d..cac7edb 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2983,61 +2983,6 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
>      return chr;
>  }
>  
> -static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
> -{
> -    CharDriverState *chr = NULL;
> -    Error *local_err = NULL;
> -    int fd = -1;
> -
> -    bool is_listen      = qemu_opt_get_bool(opts, "server", false);
> -    bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true);
> -    bool is_telnet      = qemu_opt_get_bool(opts, "telnet", false);
> -    bool do_nodelay     = !qemu_opt_get_bool(opts, "delay", true);
> -    bool is_unix        = qemu_opt_get(opts, "path") != NULL;
> -
> -    if (is_unix) {
> -        if (is_listen) {
> -            fd = unix_listen_opts(opts, &local_err);
> -        } else {
> -            fd = unix_connect_opts(opts, &local_err, NULL, NULL);
> -        }
> -    } else {
> -        if (is_listen) {
> -            fd = inet_listen_opts(opts, 0, &local_err);
> -        } else {
> -            fd = inet_connect_opts(opts, &local_err, NULL, NULL);
> -        }
> -    }
> -    if (fd < 0) {
> -        goto fail;
> -    }
> -
> -    if (!is_waitconnect)
> -        qemu_set_nonblock(fd);
> -
> -    chr = qemu_chr_open_socket_fd(fd, do_nodelay, is_listen, is_telnet,
> -                                  is_waitconnect, &local_err);
> -    if (local_err) {
> -        goto fail;
> -    }
> -    return chr;
> -
> -
> - fail:
> -    if (local_err) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> -    }
> -    if (fd >= 0) {
> -        closesocket(fd);
> -    }
> -    if (chr) {
> -        g_free(chr->opaque);
> -        g_free(chr);
> -    }
> -    return NULL;
> -}
> -
>  /*********************************************************/
>  /* Ring buffer chardev */
>  
> @@ -3448,6 +3393,62 @@ static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend *backend,
>      backend->mux->chardev = g_strdup(chardev);
>  }
>  
> +static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
> +                                  Error **errp)
> +{
> +    bool is_listen      = qemu_opt_get_bool(opts, "server", false);
> +    bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true);
> +    bool is_telnet      = qemu_opt_get_bool(opts, "telnet", false);
> +    bool do_nodelay     = !qemu_opt_get_bool(opts, "delay", true);
> +    const char *path = qemu_opt_get(opts, "path");
> +    const char *host = qemu_opt_get(opts, "host");
> +    const char *port = qemu_opt_get(opts, "port");
> +    SocketAddress *addr;
> +
> +    if (!path) {
> +        if (!host) {
> +            error_setg(errp, "chardev: socket: no host given");
> +            return;
> +        }
> +        if (!port) {
> +            error_setg(errp, "chardev: socket: no port given");
> +            return;
> +        }
> +    }
> +
> +    backend->socket = g_new0(ChardevSocket, 1);
> +
> +    backend->socket->has_nodelay = true;
> +    backend->socket->nodelay = do_nodelay;
> +    backend->socket->has_server = true;
> +    backend->socket->server = is_listen;
> +    backend->socket->has_telnet = true;
> +    backend->socket->telnet = is_telnet;
> +    backend->socket->has_wait = true;
> +    backend->socket->wait = is_waitconnect;
> +
> +    addr = g_new0(SocketAddress, 1);
> +    if (path) {
> +        addr->kind = SOCKET_ADDRESS_KIND_UNIX;
> +        addr->q_unix = g_new0(UnixSocketAddress, 1);
> +        addr->q_unix->path = g_strdup(path);
> +    } else {
> +        addr->kind = SOCKET_ADDRESS_KIND_INET;
> +        addr->inet = g_new0(InetSocketAddress, 1);
> +        addr->inet->host = g_strdup(host);
> +        addr->inet->port = g_strdup(port);
> +        addr->inet->to = qemu_opt_get_number(opts, "to", 0);
> +        addr->inet->has_to = true;

This turns "to not specified" into "to=0".  Works because the eventual
consumer inet_listen_opts() treats them the same.  Still, setting
->has_to = qemu_opt_get(opts, "to") would be more obviously correct.

> +        if (qemu_opt_get_bool(opts, "ipv4", 0)) {
> +            addr->inet->has_ipv4 = addr->inet->ipv4 = true;
> +        }

Likewise.

> +        if (qemu_opt_get_bool(opts, "ipv6", 0)) {
> +            addr->inet->has_ipv6 = addr->inet->ipv6 = true;
> +        }

Likewise.

> +    }
> +    backend->socket->addr = addr;
> +}
> +
>  typedef struct CharDriver {
>      const char *name;
>      /* old, pre qapi */
> @@ -4136,7 +4137,8 @@ void qmp_chardev_remove(const char *id, Error **errp)
>  static void register_types(void)
>  {
>      register_char_driver_qapi("null", CHARDEV_BACKEND_KIND_NULL, NULL);
> -    register_char_driver("socket", qemu_chr_open_socket);
> +    register_char_driver_qapi("socket", CHARDEV_BACKEND_KIND_SOCKET,
> +                              qemu_chr_parse_socket);
>      register_char_driver("udp", qemu_chr_open_udp);
>      register_char_driver_qapi("ringbuf", CHARDEV_BACKEND_KIND_RINGBUF,
>                                qemu_chr_parse_ringbuf);

Re: [Qemu-devel] [PATCH 1/4] qemu-char: Convert socket char backend to parse/kind

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> Convert the socket char backend to the new style QAPI framework;
> this allows it to return an Error ** to callers who might not
> want it to print directly about socket failures.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Forgot to mention: if you should respin, consider changing the summary
to

    qemu-char: Convert socket backend to QAPI

closer to the previous conversions' summaries.

Re: [Qemu-devel] [PATCH 3/4] qemu-char: Convert udp char backend to parse/kind

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> Convert the udp char backend to the new style QAPI framework.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  qemu-char.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 54 insertions(+), 15 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index cac7edb..a01ccdc 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2383,20 +2383,6 @@ static CharDriverState *qemu_chr_open_udp_fd(int fd)
>      return chr;
>  }
>  
> -static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
> -{
> -    Error *local_err = NULL;
> -    int fd = -1;
> -
> -    fd = inet_dgram_opts(opts, &local_err);
> -    if (fd < 0) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> -        return NULL;
> -    }
> -    return qemu_chr_open_udp_fd(fd);
> -}
> -
>  /***********************************************************/
>  /* TCP Net console */
>  
> @@ -3449,6 +3435,58 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>      backend->socket->addr = addr;
>  }
>  
> +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");
> +    bool ipv4 = qemu_opt_get_bool(opts, "ipv4", 0);
> +    bool ipv6 = qemu_opt_get_bool(opts, "ipv6", 0);
> +    const char *localaddr = qemu_opt_get(opts, "localaddr");
> +    const char *localport = qemu_opt_get(opts, "localport");
> +    bool has_local = false;
> +    SocketAddress *addr;
> +
> +    if (host == NULL || strlen(host) == 0) {
> +        host = "localhost";
> +    }

We have to make up *some* value, because the host is mandatory for
InetSocketAddress in the schema.

The value We make up matches inet_dgram_opts().  Duplicating its choice
here isn't so hot.  At least it's an obvious choice.

Perhaps host = "" would do.  Your choice.

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

I neither localaddr not localport are specified, has_local remains
false.  Good.

If one of them is specified, we need to make up a value for the other,
because they're mandatory for InetSocketAddress in the schema.  We make
up values the eventual consumer inet_dgram_opts() treates the same as
"not specified".  Not nice, but it works.

> +
> +    backend->udp = g_new0(ChardevUdp, 1);
> +
> +    addr = g_new0(SocketAddress, 1);
> +    addr->kind = SOCKET_ADDRESS_KIND_INET;
> +    addr->inet = g_new0(InetSocketAddress, 1);
> +    addr->inet->host = g_strdup(host);
> +    addr->inet->port = g_strdup(port);
> +    addr->inet->has_ipv4 = addr->inet->ipv4 = ipv4;

This turns "ipv4=off" into "ipv4 not specified".  Works because the
eventual consumer inet_dgram_opts() treats them the same.  Still,
setting ->has_to = qemu_opt_get(opts, "ipv4") would be more obviously
correct.

> +    addr->inet->has_ipv6 = addr->inet->ipv6 = ipv6;

Likewise.

> +    backend->udp->remote = addr;
> +
> +    if (has_local) {
> +        backend->udp->has_local = true;
> +        addr = g_new0(SocketAddress, 1);
> +        addr->kind = SOCKET_ADDRESS_KIND_INET;
> +        addr->inet = g_new0(InetSocketAddress, 1);
> +        addr->inet->host = g_strdup(localaddr);
> +        addr->inet->port = g_strdup(localport);
> +        backend->udp->local = addr;
> +    }
> +}
> +

Like for backend=socket [PATCH 1], we're converting from QemuOpts to
QAPI-generated data structures, only to convert it right back, because
the internal interfaces haven't been qapified.  Ugly, but out of scope
for this series.

>  typedef struct CharDriver {
>      const char *name;
>      /* old, pre qapi */
> @@ -4139,7 +4177,8 @@ static void register_types(void)
>      register_char_driver_qapi("null", CHARDEV_BACKEND_KIND_NULL, NULL);
>      register_char_driver_qapi("socket", CHARDEV_BACKEND_KIND_SOCKET,
>                                qemu_chr_parse_socket);
> -    register_char_driver("udp", qemu_chr_open_udp);
> +    register_char_driver_qapi("udp", CHARDEV_BACKEND_KIND_UDP,
> +                              qemu_chr_parse_udp);
>      register_char_driver_qapi("ringbuf", CHARDEV_BACKEND_KIND_RINGBUF,
>                                qemu_chr_parse_ringbuf);
>      register_char_driver_qapi("file", CHARDEV_BACKEND_KIND_FILE,

Re: [Qemu-devel] [PATCH 4/4] qemu-char: Remove register_char_driver() machinery

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> Now that all the char backends have been converted to the QAPI
> framework we can remove the machinery for handling old style
> backends.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/sysemu/char.h |   1 -
>  qemu-char.c           | 126 +++++++++++++++-----------------------------------
>  2 files changed, 38 insertions(+), 89 deletions(-)
>
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 98cd4c9..dbfd4e6 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -344,7 +344,6 @@ bool chr_is_ringbuf(const CharDriverState *chr);
>  
>  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
>  
> -void register_char_driver(const char *name, CharDriverState *(*open)(QemuOpts *));
>  void register_char_driver_qapi(const char *name, ChardevBackendKind kind,
>          void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp));

Recommend to rename register_char_driver_qapi() to register_char_driver().

>  
> diff --git a/qemu-char.c b/qemu-char.c
> index a01ccdc..2ffab1a 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -3489,26 +3489,12 @@ static void qemu_chr_parse_udp(QemuOpts *opts, ChardevBackend *backend,
>  
>  typedef struct CharDriver {
>      const char *name;
> -    /* old, pre qapi */
> -    CharDriverState *(*open)(QemuOpts *opts);
> -    /* new, qapi-based */
>      ChardevBackendKind kind;
>      void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
>  } CharDriver;
>  
>  static GSList *backends;
>  
> -void register_char_driver(const char *name, CharDriverState *(*open)(QemuOpts *))
> -{
> -    CharDriver *s;
> -
> -    s = g_malloc0(sizeof(*s));
> -    s->name = g_strdup(name);
> -    s->open = open;
> -
> -    backends = g_slist_append(backends, s);
> -}
> -
>  void register_char_driver_qapi(const char *name, ChardevBackendKind kind,
>          void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp))
>  {

The remaining hunks are much easier to review with diff -w:

  @@ -3530,8 +3516,12 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
       CharDriver *cd;
       CharDriverState *chr;
       GSList *i;
  +    ChardevReturn *ret = NULL;
  +    ChardevBackend *backend;
  +    const char *id = qemu_opts_id(opts);
  +    char *bid = NULL;

  -    if (qemu_opts_id(opts) == NULL) {
  +    if (id == NULL) {
           error_setg(errp, "chardev: no id specified");
           goto err;
       }
  @@ -3554,12 +3544,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
           goto err;
       }

  -    if (!cd->open) {
  -        /* using new, qapi init */
  -        ChardevBackend *backend = g_new0(ChardevBackend, 1);
  -        ChardevReturn *ret = NULL;
  -        const char *id = qemu_opts_id(opts);
  -        char *bid = NULL;
  +    backend = g_new0(ChardevBackend, 1);

       if (qemu_opt_get_bool(opts, "mux", 0)) {
           bid = g_strdup_printf("%s-base", id);
  @@ -3603,41 +3588,6 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
       qapi_free_ChardevReturn(ret);
       g_free(bid);
       return chr;
  -    }
  -
  -    chr = cd->open(opts);
  -    if (!chr) {
  -        error_setg(errp, "chardev: opening backend \"%s\" failed",
  -                   qemu_opt_get(opts, "backend"));
  -        goto err;
  -    }
  -
  -    if (!chr->filename)
  -        chr->filename = g_strdup(qemu_opt_get(opts, "backend"));
  -    chr->init = init;
  -    /* if we didn't create the chardev via qmp_chardev_add, we
  -     * need to send the OPENED event here
  -     */
  -    if (!chr->explicit_be_open) {
  -        qemu_chr_be_event(chr, CHR_EVENT_OPENED);
  -    }
  -    QTAILQ_INSERT_TAIL(&chardevs, chr, next);
  -
  -    if (qemu_opt_get_bool(opts, "mux", 0)) {
  -        CharDriverState *base = chr;
  -        int len = strlen(qemu_opts_id(opts)) + 6;
  -        base->label = g_malloc(len);
  -        snprintf(base->label, len, "%s-base", qemu_opts_id(opts));
  -        chr = qemu_chr_open_mux(base);
  -        chr->filename = base->filename;
  -        chr->avail_connections = MAX_MUX;
  -        QTAILQ_INSERT_TAIL(&chardevs, chr, next);
  -    } else {
  -        chr->avail_connections = 1;
  -    }
  -    chr->label = g_strdup(qemu_opts_id(opts));
  -    chr->opts = opts;
  -    return chr;

   err:
       qemu_opts_del(opts);

Looks good.

Re: [Qemu-devel] [PATCH 0/4] Convert remaining legacy chardevs to parse/kind

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> This patchset converts the two remaining legacy chardevs
> ('socket' and 'udp') to use the new-style parse/kind
> mechanisms, and removes all the no-longer-required
> legacy machinery.
>
> Patch 1 was posted to the list back in June
> (https://patches.linaro.org/32298/). I've fixed the obvious
> bug picked up in code review, and as far as I can tell from
> the thread we decided that the blocking/non-blocking
> difference between QAPI and legacy wasn't a problem.
>
> Patch 2 fixes a hole in the functionality of QAPI-described
> UDP chardevs, to avoid regressing the commandline functionality
> when we convert the UDP backend in patch 3.
>
> Patch 4 may be easier to review as an ignore-whitespaces
> diff (the de-indentation makes the diff a bit awkward).

Thanks a lot for finishing this job.

I had a few remarks, and you might want to address them.  But even in
its current state, the series is an improvement, therefore:

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 3/4] qemu-char: Convert udp char backend to parse/kind

[Peter Maydell]

On 19 August 2014 14:16, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> Convert the udp char backend to the new style QAPI framework.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  qemu-char.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 54 insertions(+), 15 deletions(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index cac7edb..a01ccdc 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2383,20 +2383,6 @@ static CharDriverState *qemu_chr_open_udp_fd(int fd)
>>      return chr;
>>  }
>>
>> -static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
>> -{
>> -    Error *local_err = NULL;
>> -    int fd = -1;
>> -
>> -    fd = inet_dgram_opts(opts, &local_err);
>> -    if (fd < 0) {
>> -        qerror_report_err(local_err);
>> -        error_free(local_err);
>> -        return NULL;
>> -    }
>> -    return qemu_chr_open_udp_fd(fd);
>> -}
>> -
>>  /***********************************************************/
>>  /* TCP Net console */
>>
>> @@ -3449,6 +3435,58 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>>      backend->socket->addr = addr;
>>  }
>>
>> +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");
>> +    bool ipv4 = qemu_opt_get_bool(opts, "ipv4", 0);
>> +    bool ipv6 = qemu_opt_get_bool(opts, "ipv6", 0);
>> +    const char *localaddr = qemu_opt_get(opts, "localaddr");
>> +    const char *localport = qemu_opt_get(opts, "localport");
>> +    bool has_local = false;
>> +    SocketAddress *addr;
>> +
>> +    if (host == NULL || strlen(host) == 0) {
>> +        host = "localhost";
>> +    }
>
> We have to make up *some* value, because the host is mandatory for
> InetSocketAddress in the schema.
>
> The value We make up matches inet_dgram_opts().  Duplicating its choice
> here isn't so hot.  At least it's an obvious choice.
>
> Perhaps host = "" would do.  Your choice.

I think I'll leave this as is, then. (Here seemed the
obvious point to handle the required semantics for
command lines with an unspecified value. Eventually
the QemuOpts parsing code in inet_dgram_opts()
will disappear, hopefully.)

>> +    backend->udp = g_new0(ChardevUdp, 1);
>> +
>> +    addr = g_new0(SocketAddress, 1);
>> +    addr->kind = SOCKET_ADDRESS_KIND_INET;
>> +    addr->inet = g_new0(InetSocketAddress, 1);
>> +    addr->inet->host = g_strdup(host);
>> +    addr->inet->port = g_strdup(port);
>> +    addr->inet->has_ipv4 = addr->inet->ipv4 = ipv4;
>
> This turns "ipv4=off" into "ipv4 not specified".  Works because the
> eventual consumer inet_dgram_opts() treats them the same.  Still,
> setting ->has_to = qemu_opt_get(opts, "ipv4") would be more obviously
> correct.

Agreed (and ditto the similar remarks you made on patch 1).

>
>> +    addr->inet->has_ipv6 = addr->inet->ipv6 = ipv6;
>
> Likewise.
>
>> +    backend->udp->remote = addr;
>> +
>> +    if (has_local) {
>> +        backend->udp->has_local = true;
>> +        addr = g_new0(SocketAddress, 1);
>> +        addr->kind = SOCKET_ADDRESS_KIND_INET;
>> +        addr->inet = g_new0(InetSocketAddress, 1);
>> +        addr->inet->host = g_strdup(localaddr);
>> +        addr->inet->port = g_strdup(localport);
>> +        backend->udp->local = addr;
>> +    }
>> +}
>> +
>
> Like for backend=socket [PATCH 1], we're converting from QemuOpts to
> QAPI-generated data structures, only to convert it right back, because
> the internal interfaces haven't been qapified.  Ugly, but out of scope
> for this series.

Yep, the obvious next cleanup is to fold the
inet_dgram_opts() code into socket_dgram() and
avoid the double-conversion. As you say, separate
series.

-- PMM