[PATCH v2 00/11] chardev related fixes

[PATCH v2 00/11] chardev related fixes

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

Those patches have been sent earlier. I compiled them here, as I intend to send
a pull request for 6.1. We may want to drop the last 2/3 patches since we are
in freeze though.

Marc-André Lureau (11):
  util: fix abstract socket path copy
  chardev/socket: print a more correct command-line address
  chardev: remove needless class method
  chardev: add some comments about the class methods
  chardev: mark explicitly first argument as poisoned
  chardev: fix fd_chr_add_watch() when in != out
  chardev: fix qemu_chr_open_fd() being called with fd=-1
  chardev: fix qemu_chr_open_fd() with fd_in==fd_out
  chardev: give some context on chardev-add error
  chardev: report a simpler error about duplicated id
  chardev: reuse qmp_chardev_new()

 include/chardev/char-fe.h |   8 ++-
 include/chardev/char.h    |  34 ++++++++++-
 chardev/char-fd.c         | 119 ++++++++++++++++++++++++++++++++++----
 chardev/char-fe.c         |   2 +-
 chardev/char-mux.c        |   6 +-
 chardev/char-socket.c     |   4 +-
 chardev/char.c            |  33 ++++++-----
 hw/char/cadence_uart.c    |   2 +-
 hw/char/cmsdk-apb-uart.c  |   2 +-
 hw/char/ibex_uart.c       |   2 +-
 hw/char/nrf51_uart.c      |   2 +-
 hw/char/serial.c          |   2 +-
 hw/char/virtio-console.c  |   2 +-
 hw/usb/redirect.c         |   2 +-
 hw/virtio/vhost-user.c    |   2 +-
 monitor/monitor.c         |   2 +-
 net/vhost-user.c          |   4 +-
 util/qemu-sockets.c       |   5 +-
 18 files changed, 184 insertions(+), 49 deletions(-)

-- 
2.32.0.264.g75ae10bc75

[PATCH v2 01/11] util: fix abstract socket path copy

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
support" neglected to update socket_sockaddr_to_address_unix() and
copied the whole sun_path without taking "salen" into account.

Later, commit 3b14b4ec49 "sockets: Fix socket_sockaddr_to_address_unix()
for abstract sockets" handled the abstract UNIX path, by stripping the
leading \0 character and fixing address details, but didn't use salen
either.

Not taking "salen" into account may result in incorrect "path" being
returned in monitors commands, as we read past the address which is not
necessarily \0-terminated.

Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
Fixes: 3b14b4ec49a801067da19d6b8469eb1c1911c020
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: xiaoqiang zhao <zxq_yx_007@163.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 util/qemu-sockets.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 080a240b74..f2f3676d1f 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1345,13 +1345,16 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
     SocketAddress *addr;
     struct sockaddr_un *su = (struct sockaddr_un *)sa;
 
+    assert(salen >= sizeof(su->sun_family) + 1 &&
+           salen <= sizeof(struct sockaddr_un));
+
     addr = g_new0(SocketAddress, 1);
     addr->type = SOCKET_ADDRESS_TYPE_UNIX;
 #ifdef CONFIG_LINUX
     if (!su->sun_path[0]) {
         /* Linux abstract socket */
         addr->u.q_unix.path = g_strndup(su->sun_path + 1,
-                                        sizeof(su->sun_path) - 1);
+                                        salen - sizeof(su->sun_family) - 1);
         addr->u.q_unix.has_abstract = true;
         addr->u.q_unix.abstract = true;
         addr->u.q_unix.has_tight = true;
-- 
2.32.0.264.g75ae10bc75

[PATCH v2 02/11] chardev/socket: print a more correct command-line address

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Better reflect the command line version of the socket address arguments,
following the now recommended long-form opt=on syntax.

Complement/fixes commit 9d902d51 "chardev: do not use short form boolean
options in non-QemuOpts character device descriptions".

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 chardev/char-socket.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index d0fb545963..c43668cc15 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -468,9 +468,9 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix)
 
 #ifdef CONFIG_LINUX
         if (sa->has_abstract && sa->abstract) {
-            abstract = ",abstract";
+            abstract = ",abstract=on";
             if (sa->has_tight && sa->tight) {
-                tight = ",tight";
+                tight = ",tight=on";
             }
         }
 #endif
-- 
2.32.0.264.g75ae10bc75

[PATCH v2 03/11] chardev: remove needless class method

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

"chr_option_parsed" is only implemented by the "mux" chardev, we can
specialize the code there to avoid the needless generic class method.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/chardev/char.h | 1 -
 chardev/char-mux.c     | 6 ++----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index 7c0444f90d..589e7fe46d 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -273,7 +273,6 @@ struct ChardevClass {
     void (*chr_set_echo)(Chardev *chr, bool echo);
     void (*chr_set_fe_open)(Chardev *chr, int fe_open);
     void (*chr_be_event)(Chardev *s, QEMUChrEvent event);
-    void (*chr_options_parsed)(Chardev *chr);
 };
 
 Chardev *qemu_chardev_new(const char *id, const char *typename,
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 5baf419010..ada0c6866f 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -386,10 +386,9 @@ void suspend_mux_open(void)
 static int chardev_options_parsed_cb(Object *child, void *opaque)
 {
     Chardev *chr = (Chardev *)child;
-    ChardevClass *class = CHARDEV_GET_CLASS(chr);
 
-    if (!chr->be_open && class->chr_options_parsed) {
-        class->chr_options_parsed(chr);
+    if (!chr->be_open && CHARDEV_IS_MUX(chr)) {
+        open_muxes(chr);
     }
 
     return 0;
@@ -412,7 +411,6 @@ static void char_mux_class_init(ObjectClass *oc, void *data)
     cc->chr_accept_input = mux_chr_accept_input;
     cc->chr_add_watch = mux_chr_add_watch;
     cc->chr_be_event = mux_chr_be_event;
-    cc->chr_options_parsed = open_muxes;
     cc->chr_update_read_handler = mux_chr_update_read_handlers;
 }
 
-- 
2.32.0.264.g75ae10bc75

[PATCH v2 04/11] chardev: add some comments about the class methods

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/chardev/char.h | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index 589e7fe46d..a319b5fdff 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -254,24 +254,57 @@ struct ChardevClass {
 
     bool internal; /* TODO: eventually use TYPE_USER_CREATABLE */
     bool supports_yank;
+
+    /* parse command line options and populate QAPI @backend */
     void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
 
+    /* called after construction, open/starts the backend */
     void (*open)(Chardev *chr, ChardevBackend *backend,
                  bool *be_opened, Error **errp);
 
+    /* write buf to the backend */
     int (*chr_write)(Chardev *s, const uint8_t *buf, int len);
+
+    /*
+     * Read from the backend (blocking). A typical front-end will instead rely
+     * on chr_can_read/chr_read being called when polling/looping.
+     */
     int (*chr_sync_read)(Chardev *s, const uint8_t *buf, int len);
+
+    /* create a watch on the backend */
     GSource *(*chr_add_watch)(Chardev *s, GIOCondition cond);
+
+    /* update the backend internal sources */
     void (*chr_update_read_handler)(Chardev *s);
+
+    /* send an ioctl to the backend */
     int (*chr_ioctl)(Chardev *s, int cmd, void *arg);
+
+    /* get ancillary-received fds during last read */
     int (*get_msgfds)(Chardev *s, int* fds, int num);
+
+    /* set ancillary fds to be sent with next write */
     int (*set_msgfds)(Chardev *s, int *fds, int num);
+
+    /* accept the given fd */
     int (*chr_add_client)(Chardev *chr, int fd);
+
+    /* wait for a connection */
     int (*chr_wait_connected)(Chardev *chr, Error **errp);
+
+    /* disconnect a connection */
     void (*chr_disconnect)(Chardev *chr);
+
+    /* called by frontend when it can read */
     void (*chr_accept_input)(Chardev *chr);
+
+    /* set terminal echo */
     void (*chr_set_echo)(Chardev *chr, bool echo);
+
+    /* notify the backend of frontend open state */
     void (*chr_set_fe_open)(Chardev *chr, int fe_open);
+
+    /* handle various events */
     void (*chr_be_event)(Chardev *s, QEMUChrEvent event);
 };
 
-- 
2.32.0.264.g75ae10bc75

[PATCH v2 05/11] chardev: mark explicitly first argument as poisoned

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Since commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2 "char: convert
from GIOChannel to QIOChannel", the first argument to the watch callback
can actually be a QIOChannel, which is not a GIOChannel (but a QEMU
Object).

Even though we never used that pointer, change the callback type to warn
the users. Possibly a better fix later, we may want to store the
callback and call it from intermediary functions.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/chardev/char-fe.h | 8 +++++++-
 chardev/char-fe.c         | 2 +-
 hw/char/cadence_uart.c    | 2 +-
 hw/char/cmsdk-apb-uart.c  | 2 +-
 hw/char/ibex_uart.c       | 2 +-
 hw/char/nrf51_uart.c      | 2 +-
 hw/char/serial.c          | 2 +-
 hw/char/virtio-console.c  | 2 +-
 hw/usb/redirect.c         | 2 +-
 hw/virtio/vhost-user.c    | 2 +-
 monitor/monitor.c         | 2 +-
 net/vhost-user.c          | 4 ++--
 12 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index a553843364..867ef1b3b2 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -174,6 +174,9 @@ void qemu_chr_fe_set_open(CharBackend *be, int fe_open);
 void qemu_chr_fe_printf(CharBackend *be, const char *fmt, ...)
     GCC_FMT_ATTR(2, 3);
 
+
+typedef gboolean (*FEWatchFunc)(void *do_not_use, GIOCondition condition, void *data);
+
 /**
  * qemu_chr_fe_add_watch:
  * @cond: the condition to poll for
@@ -188,10 +191,13 @@ void qemu_chr_fe_printf(CharBackend *be, const char *fmt, ...)
  * Note that you are responsible to update the front-end sources if
  * you are switching the main context with qemu_chr_fe_set_handlers().
  *
+ * Warning: DO NOT use the first callback argument (it may be either
+ * a GIOChannel or a QIOChannel, depending on the underlying chardev)
+ *
  * Returns: the source tag
  */
 guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
-                            GIOFunc func, void *user_data);
+                            FEWatchFunc func, void *user_data);
 
 /**
  * qemu_chr_fe_write:
diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index 474715c5a9..7789f7be9c 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -354,7 +354,7 @@ void qemu_chr_fe_set_open(CharBackend *be, int fe_open)
 }
 
 guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
-                            GIOFunc func, void *user_data)
+                            FEWatchFunc func, void *user_data)
 {
     Chardev *s = be->chr;
     GSource *src;
diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index ceb677bc5a..8ee6f74b8c 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -288,7 +288,7 @@ static void uart_write_rx_fifo(void *opaque, const uint8_t *buf, int size)
     uart_update_status(s);
 }
 
-static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
+static gboolean cadence_uart_xmit(GIOChannel *do_not_use, GIOCondition cond,
                                   void *opaque)
 {
     CadenceUARTState *s = opaque;
diff --git a/hw/char/cmsdk-apb-uart.c b/hw/char/cmsdk-apb-uart.c
index ba2cbbee3d..b07a9dee4f 100644
--- a/hw/char/cmsdk-apb-uart.c
+++ b/hw/char/cmsdk-apb-uart.c
@@ -191,7 +191,7 @@ static uint64_t uart_read(void *opaque, hwaddr offset, unsigned size)
 /* Try to send tx data, and arrange to be called back later if
  * we can't (ie the char backend is busy/blocking).
  */
-static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void *opaque)
+static gboolean uart_transmit(GIOChannel *do_not_use, GIOCondition cond, void *opaque)
 {
     CMSDKAPBUART *s = CMSDK_APB_UART(opaque);
     int ret;
diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
index 6b0c9330bf..e493ea08c0 100644
--- a/hw/char/ibex_uart.c
+++ b/hw/char/ibex_uart.c
@@ -135,7 +135,7 @@ static void ibex_uart_receive(void *opaque, const uint8_t *buf, int size)
     ibex_uart_update_irqs(s);
 }
 
-static gboolean ibex_uart_xmit(GIOChannel *chan, GIOCondition cond,
+static gboolean ibex_uart_xmit(GIOChannel *do_not_use, GIOCondition cond,
                                void *opaque)
 {
     IbexUartState *s = opaque;
diff --git a/hw/char/nrf51_uart.c b/hw/char/nrf51_uart.c
index 045ca5fa40..0b89b0eae4 100644
--- a/hw/char/nrf51_uart.c
+++ b/hw/char/nrf51_uart.c
@@ -75,7 +75,7 @@ static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
     return r;
 }
 
-static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void *opaque)
+static gboolean uart_transmit(GIOChannel *do_not_use, GIOCondition cond, void *opaque)
 {
     NRF51UARTState *s = NRF51_UART(opaque);
     int r;
diff --git a/hw/char/serial.c b/hw/char/serial.c
index bc2e322970..7061aacbce 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -220,7 +220,7 @@ static void serial_update_msl(SerialState *s)
     }
 }
 
-static gboolean serial_watch_cb(GIOChannel *chan, GIOCondition cond,
+static gboolean serial_watch_cb(void *do_not_use, GIOCondition cond,
                                 void *opaque)
 {
     SerialState *s = opaque;
diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
index 6b132caa29..dd5a02e339 100644
--- a/hw/char/virtio-console.c
+++ b/hw/char/virtio-console.c
@@ -38,7 +38,7 @@ struct VirtConsole {
  * Callback function that's called from chardevs when backend becomes
  * writable.
  */
-static gboolean chr_write_unblocked(GIOChannel *chan, GIOCondition cond,
+static gboolean chr_write_unblocked(void *do_not_use, GIOCondition cond,
                                     void *opaque)
 {
     VirtConsole *vcon = opaque;
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 1ec909a63a..5f0ef9cb3b 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -270,7 +270,7 @@ static int usbredir_read(void *priv, uint8_t *data, int count)
     return count;
 }
 
-static gboolean usbredir_write_unblocked(GIOChannel *chan, GIOCondition cond,
+static gboolean usbredir_write_unblocked(void *do_not_use, GIOCondition cond,
                                          void *opaque)
 {
     USBRedirDevice *dev = opaque;
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 29ea2b4fce..aec6cc1990 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -303,7 +303,7 @@ struct vhost_user_read_cb_data {
     int ret;
 };
 
-static gboolean vhost_user_read_cb(GIOChannel *source, GIOCondition condition,
+static gboolean vhost_user_read_cb(void *do_not_use, GIOCondition condition,
                                    gpointer opaque)
 {
     struct vhost_user_read_cb_data *data = opaque;
diff --git a/monitor/monitor.c b/monitor/monitor.c
index b90c0f4051..46a171bca6 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -156,7 +156,7 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
 
 static void monitor_flush_locked(Monitor *mon);
 
-static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
+static gboolean monitor_unblocked(void *do_not_use, GIOCondition cond,
                                   void *opaque)
 {
     Monitor *mon = opaque;
diff --git a/net/vhost-user.c b/net/vhost-user.c
index ffbd94d944..6adfcd623a 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -208,8 +208,8 @@ static NetClientInfo net_vhost_user_info = {
         .set_vnet_le = vhost_user_set_vnet_endianness,
 };
 
-static gboolean net_vhost_user_watch(GIOChannel *chan, GIOCondition cond,
-                                           void *opaque)
+static gboolean net_vhost_user_watch(void *do_not_use, GIOCondition cond,
+                                     void *opaque)
 {
     NetVhostUserState *s = opaque;
 
-- 
2.32.0.264.g75ae10bc75

[PATCH v2 06/11] chardev: fix fd_chr_add_watch() when in != out

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Create child sources for the different streams, and dispatch on the
parent source with the synthesized conditions.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-fd.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 1 deletion(-)

diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index 1cd62f2779..743d3989b4 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -28,6 +28,7 @@
 #include "qemu/sockets.h"
 #include "qapi/error.h"
 #include "chardev/char.h"
+#include "chardev/char-fe.h"
 #include "io/channel-file.h"
 
 #include "chardev/char-fd.h"
@@ -80,10 +81,85 @@ static int fd_chr_read_poll(void *opaque)
     return s->max_size;
 }
 
+typedef struct FDSource {
+    GSource parent;
+
+    GIOCondition cond;
+} FDSource;
+
+static gboolean
+fd_source_prepare(GSource *source,
+                  gint *timeout_)
+{
+    FDSource *src = (FDSource *)source;
+
+    return src->cond != 0;
+}
+
+static gboolean
+fd_source_check(GSource *source)
+{
+    FDSource *src = (FDSource *)source;
+
+    return src->cond != 0;
+}
+
+static gboolean
+fd_source_dispatch(GSource *source, GSourceFunc callback,
+                   gpointer user_data)
+{
+    FDSource *src = (FDSource *)source;
+    FEWatchFunc func = (FEWatchFunc)callback;
+    gboolean ret = G_SOURCE_CONTINUE;
+
+    if (src->cond) {
+        ret = func(NULL, src->cond, user_data);
+        src->cond = 0;
+    }
+
+    return ret;
+}
+
+static GSourceFuncs fd_source_funcs = {
+  fd_source_prepare,
+  fd_source_check,
+  fd_source_dispatch,
+  NULL, NULL, NULL
+};
+
+static GSource *fd_source_new(FDChardev *chr)
+{
+    return g_source_new(&fd_source_funcs, sizeof(FDSource));
+}
+
+static gboolean child_func(GIOChannel *source,
+                           GIOCondition condition,
+                           gpointer data)
+{
+    FDSource *parent = data;
+
+    parent->cond |= condition;
+
+    return G_SOURCE_CONTINUE;
+}
+
 static GSource *fd_chr_add_watch(Chardev *chr, GIOCondition cond)
 {
     FDChardev *s = FD_CHARDEV(chr);
-    return qio_channel_create_watch(s->ioc_out, cond);
+    g_autoptr(GSource) source = fd_source_new(s);
+
+    if (s->ioc_out) {
+        g_autoptr(GSource) child = qio_channel_create_watch(s->ioc_out, cond & ~G_IO_IN);
+        g_source_set_callback(child, (GSourceFunc)child_func, source, NULL);
+        g_source_add_child_source(source, child);
+    }
+    if (s->ioc_in) {
+        g_autoptr(GSource) child = qio_channel_create_watch(s->ioc_in, cond & ~G_IO_OUT);
+        g_source_set_callback(child, (GSourceFunc)child_func, source, NULL);
+        g_source_add_child_source(source, child);
+    }
+
+    return g_steal_pointer(&source);
 }
 
 static void fd_chr_update_read_handler(Chardev *chr)
-- 
2.32.0.264.g75ae10bc75

[PATCH v2 07/11] chardev: fix qemu_chr_open_fd() being called with fd=-1

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The "file" chardev may call qemu_chr_open_fd() with fd_in=-1. This may
cause invalid system calls, as the QIOChannel is assumed to be properly
initialized later on.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-fd.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index 743d3989b4..c11b1037f9 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -39,6 +39,10 @@ static int fd_chr_write(Chardev *chr, const uint8_t *buf, int len)
 {
     FDChardev *s = FD_CHARDEV(chr);
 
+    if (!s->ioc_out) {
+        return -1;
+    }
+
     return io_channel_send(s->ioc_out, buf, len);
 }
 
@@ -209,15 +213,19 @@ void qemu_chr_open_fd(Chardev *chr,
     FDChardev *s = FD_CHARDEV(chr);
     char *name;
 
-    s->ioc_in = QIO_CHANNEL(qio_channel_file_new_fd(fd_in));
-    name = g_strdup_printf("chardev-file-in-%s", chr->label);
-    qio_channel_set_name(QIO_CHANNEL(s->ioc_in), name);
-    g_free(name);
-    s->ioc_out = QIO_CHANNEL(qio_channel_file_new_fd(fd_out));
-    name = g_strdup_printf("chardev-file-out-%s", chr->label);
-    qio_channel_set_name(QIO_CHANNEL(s->ioc_out), name);
-    g_free(name);
-    qemu_set_nonblock(fd_out);
+    if (fd_in >= 0) {
+        s->ioc_in = QIO_CHANNEL(qio_channel_file_new_fd(fd_in));
+        name = g_strdup_printf("chardev-file-in-%s", chr->label);
+        qio_channel_set_name(QIO_CHANNEL(s->ioc_in), name);
+        g_free(name);
+    }
+    if (fd_out >= 0) {
+        s->ioc_out = QIO_CHANNEL(qio_channel_file_new_fd(fd_out));
+        name = g_strdup_printf("chardev-file-out-%s", chr->label);
+        qio_channel_set_name(QIO_CHANNEL(s->ioc_out), name);
+        g_free(name);
+        qemu_set_nonblock(fd_out);
+    }
 }
 
 static void char_fd_class_init(ObjectClass *oc, void *data)
-- 
2.32.0.264.g75ae10bc75

[PATCH v2 08/11] chardev: fix qemu_chr_open_fd() with fd_in==fd_out

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The "serial" chardev calls qemu_chr_open_fd() with the same fd. This
may lead to double-close as each QIOChannel owns the fd.

Instead, share the reference to the same QIOChannel.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-fd.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index c11b1037f9..93c56913b4 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -211,20 +211,31 @@ void qemu_chr_open_fd(Chardev *chr,
                       int fd_in, int fd_out)
 {
     FDChardev *s = FD_CHARDEV(chr);
-    char *name;
+    g_autofree char *name = NULL;
+
+    if (fd_out >= 0) {
+        qemu_set_nonblock(fd_out);
+    }
+
+    if (fd_out == fd_in && fd_in >= 0) {
+        s->ioc_in = QIO_CHANNEL(qio_channel_file_new_fd(fd_in));
+        name = g_strdup_printf("chardev-file-%s", chr->label);
+        qio_channel_set_name(QIO_CHANNEL(s->ioc_in), name);
+        s->ioc_out = QIO_CHANNEL(object_ref(s->ioc_in));
+        return;
+    }
 
     if (fd_in >= 0) {
         s->ioc_in = QIO_CHANNEL(qio_channel_file_new_fd(fd_in));
         name = g_strdup_printf("chardev-file-in-%s", chr->label);
         qio_channel_set_name(QIO_CHANNEL(s->ioc_in), name);
-        g_free(name);
     }
+
     if (fd_out >= 0) {
         s->ioc_out = QIO_CHANNEL(qio_channel_file_new_fd(fd_out));
+        g_free(name);
         name = g_strdup_printf("chardev-file-out-%s", chr->label);
         qio_channel_set_name(QIO_CHANNEL(s->ioc_out), name);
-        g_free(name);
-        qemu_set_nonblock(fd_out);
     }
 }
 
-- 
2.32.0.264.g75ae10bc75

[PATCH v2 09/11] chardev: give some context on chardev-add error

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Description from Daniel P. Berrangé:
> The original code reported:
>
>  "attempt to add duplicate property 'char2' to object (type 'container')"
>
> Since adding yank support, the current code reports
>
>  "duplicate yank instance"
>
> With this patch applied it now reports:
>
>  "Failed to add chardev 'char2': duplicate yank instance"
>
> This is marginally better, but still not great, not that the original
> error was great either.
>
> It would be nice if we could report
>
>   "chardev with id 'char2' already exists"

Related to:
https://bugzilla.redhat.com/show_bug.cgi?id=1984721

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index d959eec522..f59a61774b 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -1031,27 +1031,26 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,
 ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
                                Error **errp)
 {
+    ERRP_GUARD();
     const ChardevClass *cc;
     ChardevReturn *ret;
-    Chardev *chr;
+    g_autoptr(Chardev) chr = NULL;
 
     cc = char_get_class(ChardevBackendKind_str(backend->type), errp);
     if (!cc) {
-        return NULL;
+        goto err;
     }
 
     chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
                       backend, NULL, false, errp);
     if (!chr) {
-        return NULL;
+        goto err;
     }
 
     if (!object_property_try_add_child(get_chardevs_root(), id, OBJECT(chr),
                                        errp)) {
-        object_unref(OBJECT(chr));
-        return NULL;
+        goto err;
     }
-    object_unref(OBJECT(chr));
 
     ret = g_new0(ChardevReturn, 1);
     if (CHARDEV_IS_PTY(chr)) {
@@ -1060,6 +1059,10 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
     }
 
     return ret;
+
+err:
+    error_prepend(errp, "Failed to add chardev '%s': ", id);
+    return NULL;
 }
 
 ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
-- 
2.32.0.264.g75ae10bc75

[PATCH v2 10/11] chardev: report a simpler error about duplicated id

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Report:
  "Chardev with id 'char2' already exists"
Rather than:
  "Failed to add chardev 'char2': duplicate yank instance"

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/chardev/char.c b/chardev/char.c
index f59a61774b..4595a8d430 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -1036,6 +1036,11 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
     ChardevReturn *ret;
     g_autoptr(Chardev) chr = NULL;
 
+    if (qemu_chr_find(id)) {
+        error_setg(errp, "Chardev with id '%s' already exists", id);
+        return NULL;
+    }
+
     cc = char_get_class(ChardevBackendKind_str(backend->type), errp);
     if (!cc) {
         goto err;
-- 
2.32.0.264.g75ae10bc75

[PATCH v2 11/11] chardev: reuse qmp_chardev_new()

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

qemu_chardev_new() and qmp_chardev_add() are similar, let's factorize
the code a bit.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 4595a8d430..3def40c914 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -1005,7 +1005,7 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,
                           GMainContext *gcontext,
                           Error **errp)
 {
-    Chardev *chr;
+    g_autoptr(Chardev) chr = NULL;
     g_autofree char *genid = NULL;
 
     if (!id) {
@@ -1013,6 +1013,11 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,
         id = genid;
     }
 
+    if (qemu_chr_find(id)) {
+        error_setg(errp, "Chardev with id '%s' already exists", id);
+        return NULL;
+    }
+
     chr = chardev_new(id, typename, backend, gcontext, false, errp);
     if (!chr) {
         return NULL;
@@ -1020,12 +1025,10 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,
 
     if (!object_property_try_add_child(get_chardevs_root(), id, OBJECT(chr),
                                        errp)) {
-        object_unref(OBJECT(chr));
         return NULL;
     }
-    object_unref(OBJECT(chr));
 
-    return chr;
+    return chr; /* returns a shared/unowned reference */
 }
 
 ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
@@ -1034,29 +1037,19 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
     ERRP_GUARD();
     const ChardevClass *cc;
     ChardevReturn *ret;
-    g_autoptr(Chardev) chr = NULL;
-
-    if (qemu_chr_find(id)) {
-        error_setg(errp, "Chardev with id '%s' already exists", id);
-        return NULL;
-    }
+    Chardev *chr = NULL;
 
     cc = char_get_class(ChardevBackendKind_str(backend->type), errp);
     if (!cc) {
         goto err;
     }
 
-    chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
-                      backend, NULL, false, errp);
+    chr = qemu_chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
+                           backend, NULL, errp);
     if (!chr) {
         goto err;
     }
 
-    if (!object_property_try_add_child(get_chardevs_root(), id, OBJECT(chr),
-                                       errp)) {
-        goto err;
-    }
-
     ret = g_new0(ChardevReturn, 1);
     if (CHARDEV_IS_PTY(chr)) {
         ret->pty = g_strdup(chr->filename + 4);
-- 
2.32.0.264.g75ae10bc75

Re: [PATCH v2 02/11] chardev/socket: print a more correct command-line address

[Daniel P. Berrangé]

On Wed, Aug 04, 2021 at 07:48:39PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Better reflect the command line version of the socket address arguments,
> following the now recommended long-form opt=on syntax.
> 
> Complement/fixes commit 9d902d51 "chardev: do not use short form boolean
> options in non-QemuOpts character device descriptions".
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  chardev/char-socket.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 05/11] chardev: mark explicitly first argument as poisoned

[Daniel P. Berrangé]

On Wed, Aug 04, 2021 at 07:48:42PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Since commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2 "char: convert
> from GIOChannel to QIOChannel", the first argument to the watch callback
> can actually be a QIOChannel, which is not a GIOChannel (but a QEMU
> Object).
> 
> Even though we never used that pointer, change the callback type to warn
> the users. Possibly a better fix later, we may want to store the
> callback and call it from intermediary functions.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/chardev/char-fe.h | 8 +++++++-
>  chardev/char-fe.c         | 2 +-
>  hw/char/cadence_uart.c    | 2 +-
>  hw/char/cmsdk-apb-uart.c  | 2 +-
>  hw/char/ibex_uart.c       | 2 +-
>  hw/char/nrf51_uart.c      | 2 +-
>  hw/char/serial.c          | 2 +-
>  hw/char/virtio-console.c  | 2 +-
>  hw/usb/redirect.c         | 2 +-
>  hw/virtio/vhost-user.c    | 2 +-
>  monitor/monitor.c         | 2 +-
>  net/vhost-user.c          | 4 ++--
>  12 files changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
> index a553843364..867ef1b3b2 100644
> --- a/include/chardev/char-fe.h
> +++ b/include/chardev/char-fe.h
> @@ -174,6 +174,9 @@ void qemu_chr_fe_set_open(CharBackend *be, int fe_open);
>  void qemu_chr_fe_printf(CharBackend *be, const char *fmt, ...)
>      GCC_FMT_ATTR(2, 3);
>  
> +
> +typedef gboolean (*FEWatchFunc)(void *do_not_use, GIOCondition condition, void *data);
> +
>  /**
>   * qemu_chr_fe_add_watch:
>   * @cond: the condition to poll for
> @@ -188,10 +191,13 @@ void qemu_chr_fe_printf(CharBackend *be, const char *fmt, ...)
>   * Note that you are responsible to update the front-end sources if
>   * you are switching the main context with qemu_chr_fe_set_handlers().
>   *
> + * Warning: DO NOT use the first callback argument (it may be either
> + * a GIOChannel or a QIOChannel, depending on the underlying chardev)
> + *
>   * Returns: the source tag
>   */
>  guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
> -                            GIOFunc func, void *user_data);
> +                            FEWatchFunc func, void *user_data);
>  
>  /**
>   * qemu_chr_fe_write:
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index 474715c5a9..7789f7be9c 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -354,7 +354,7 @@ void qemu_chr_fe_set_open(CharBackend *be, int fe_open)
>  }
>  
>  guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
> -                            GIOFunc func, void *user_data)
> +                            FEWatchFunc func, void *user_data)
>  {
>      Chardev *s = be->chr;
>      GSource *src;
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index ceb677bc5a..8ee6f74b8c 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -288,7 +288,7 @@ static void uart_write_rx_fifo(void *opaque, const uint8_t *buf, int size)
>      uart_update_status(s);
>  }
>  
> -static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
> +static gboolean cadence_uart_xmit(GIOChannel *do_not_use, GIOCondition cond,
>                                    void *opaque)

Why is this (and the next 3) left as GIOCondition, when you change others
later on to be void ?

>  {
>      CadenceUARTState *s = opaque;
> diff --git a/hw/char/cmsdk-apb-uart.c b/hw/char/cmsdk-apb-uart.c
> index ba2cbbee3d..b07a9dee4f 100644
> --- a/hw/char/cmsdk-apb-uart.c
> +++ b/hw/char/cmsdk-apb-uart.c
> @@ -191,7 +191,7 @@ static uint64_t uart_read(void *opaque, hwaddr offset, unsigned size)
>  /* Try to send tx data, and arrange to be called back later if
>   * we can't (ie the char backend is busy/blocking).
>   */
> -static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void *opaque)
> +static gboolean uart_transmit(GIOChannel *do_not_use, GIOCondition cond, void *opaque)
>  {
>      CMSDKAPBUART *s = CMSDK_APB_UART(opaque);
>      int ret;
> diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
> index 6b0c9330bf..e493ea08c0 100644
> --- a/hw/char/ibex_uart.c
> +++ b/hw/char/ibex_uart.c
> @@ -135,7 +135,7 @@ static void ibex_uart_receive(void *opaque, const uint8_t *buf, int size)
>      ibex_uart_update_irqs(s);
>  }
>  
> -static gboolean ibex_uart_xmit(GIOChannel *chan, GIOCondition cond,
> +static gboolean ibex_uart_xmit(GIOChannel *do_not_use, GIOCondition cond,
>                                 void *opaque)
>  {
>      IbexUartState *s = opaque;
> diff --git a/hw/char/nrf51_uart.c b/hw/char/nrf51_uart.c
> index 045ca5fa40..0b89b0eae4 100644
> --- a/hw/char/nrf51_uart.c
> +++ b/hw/char/nrf51_uart.c
> @@ -75,7 +75,7 @@ static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
>      return r;
>  }
>  
> -static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void *opaque)
> +static gboolean uart_transmit(GIOChannel *do_not_use, GIOCondition cond, void *opaque)
>  {
>      NRF51UARTState *s = NRF51_UART(opaque);
>      int r;
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index bc2e322970..7061aacbce 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -220,7 +220,7 @@ static void serial_update_msl(SerialState *s)
>      }
>  }
>  
> -static gboolean serial_watch_cb(GIOChannel *chan, GIOCondition cond,
> +static gboolean serial_watch_cb(void *do_not_use, GIOCondition cond,
>                                  void *opaque)
>  {
>      SerialState *s = opaque;
> diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
> index 6b132caa29..dd5a02e339 100644
> --- a/hw/char/virtio-console.c
> +++ b/hw/char/virtio-console.c
> @@ -38,7 +38,7 @@ struct VirtConsole {
>   * Callback function that's called from chardevs when backend becomes
>   * writable.
>   */
> -static gboolean chr_write_unblocked(GIOChannel *chan, GIOCondition cond,
> +static gboolean chr_write_unblocked(void *do_not_use, GIOCondition cond,
>                                      void *opaque)
>  {
>      VirtConsole *vcon = opaque;
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 1ec909a63a..5f0ef9cb3b 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -270,7 +270,7 @@ static int usbredir_read(void *priv, uint8_t *data, int count)
>      return count;
>  }
>  
> -static gboolean usbredir_write_unblocked(GIOChannel *chan, GIOCondition cond,
> +static gboolean usbredir_write_unblocked(void *do_not_use, GIOCondition cond,
>                                           void *opaque)
>  {
>      USBRedirDevice *dev = opaque;
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 29ea2b4fce..aec6cc1990 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -303,7 +303,7 @@ struct vhost_user_read_cb_data {
>      int ret;
>  };
>  
> -static gboolean vhost_user_read_cb(GIOChannel *source, GIOCondition condition,
> +static gboolean vhost_user_read_cb(void *do_not_use, GIOCondition condition,
>                                     gpointer opaque)
>  {
>      struct vhost_user_read_cb_data *data = opaque;
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index b90c0f4051..46a171bca6 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -156,7 +156,7 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
>  
>  static void monitor_flush_locked(Monitor *mon);
>  
> -static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> +static gboolean monitor_unblocked(void *do_not_use, GIOCondition cond,
>                                    void *opaque)
>  {
>      Monitor *mon = opaque;
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index ffbd94d944..6adfcd623a 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -208,8 +208,8 @@ static NetClientInfo net_vhost_user_info = {
>          .set_vnet_le = vhost_user_set_vnet_endianness,
>  };
>  
> -static gboolean net_vhost_user_watch(GIOChannel *chan, GIOCondition cond,
> -                                           void *opaque)
> +static gboolean net_vhost_user_watch(void *do_not_use, GIOCondition cond,
> +                                     void *opaque)
>  {
>      NetVhostUserState *s = opaque;
>  
> -- 
> 2.32.0.264.g75ae10bc75
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 06/11] chardev: fix fd_chr_add_watch() when in != out

[Daniel P. Berrangé]

On Wed, Aug 04, 2021 at 07:48:43PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Create child sources for the different streams, and dispatch on the
> parent source with the synthesized conditions.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  chardev/char-fd.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 77 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 07/11] chardev: fix qemu_chr_open_fd() being called with fd=-1

[Daniel P. Berrangé]

On Wed, Aug 04, 2021 at 07:48:44PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The "file" chardev may call qemu_chr_open_fd() with fd_in=-1. This may
> cause invalid system calls, as the QIOChannel is assumed to be properly
> initialized later on.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  chardev/char-fd.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 05/11] chardev: mark explicitly first argument as poisoned

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 9497 bytes --]

Hi

On Thu, Aug 5, 2021 at 2:37 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Wed, Aug 04, 2021 at 07:48:42PM +0400, marcandre.lureau@redhat.com
> wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Since commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2 "char: convert
> > from GIOChannel to QIOChannel", the first argument to the watch callback
> > can actually be a QIOChannel, which is not a GIOChannel (but a QEMU
> > Object).
> >
> > Even though we never used that pointer, change the callback type to warn
> > the users. Possibly a better fix later, we may want to store the
> > callback and call it from intermediary functions.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  include/chardev/char-fe.h | 8 +++++++-
> >  chardev/char-fe.c         | 2 +-
> >  hw/char/cadence_uart.c    | 2 +-
> >  hw/char/cmsdk-apb-uart.c  | 2 +-
> >  hw/char/ibex_uart.c       | 2 +-
> >  hw/char/nrf51_uart.c      | 2 +-
> >  hw/char/serial.c          | 2 +-
> >  hw/char/virtio-console.c  | 2 +-
> >  hw/usb/redirect.c         | 2 +-
> >  hw/virtio/vhost-user.c    | 2 +-
> >  monitor/monitor.c         | 2 +-
> >  net/vhost-user.c          | 4 ++--
> >  12 files changed, 19 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
> > index a553843364..867ef1b3b2 100644
> > --- a/include/chardev/char-fe.h
> > +++ b/include/chardev/char-fe.h
> > @@ -174,6 +174,9 @@ void qemu_chr_fe_set_open(CharBackend *be, int
> fe_open);
> >  void qemu_chr_fe_printf(CharBackend *be, const char *fmt, ...)
> >      GCC_FMT_ATTR(2, 3);
> >
> > +
> > +typedef gboolean (*FEWatchFunc)(void *do_not_use, GIOCondition
> condition, void *data);
> > +
> >  /**
> >   * qemu_chr_fe_add_watch:
> >   * @cond: the condition to poll for
> > @@ -188,10 +191,13 @@ void qemu_chr_fe_printf(CharBackend *be, const
> char *fmt, ...)
> >   * Note that you are responsible to update the front-end sources if
> >   * you are switching the main context with qemu_chr_fe_set_handlers().
> >   *
> > + * Warning: DO NOT use the first callback argument (it may be either
> > + * a GIOChannel or a QIOChannel, depending on the underlying chardev)
> > + *
> >   * Returns: the source tag
> >   */
> >  guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
> > -                            GIOFunc func, void *user_data);
> > +                            FEWatchFunc func, void *user_data);
> >
> >  /**
> >   * qemu_chr_fe_write:
> > diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> > index 474715c5a9..7789f7be9c 100644
> > --- a/chardev/char-fe.c
> > +++ b/chardev/char-fe.c
> > @@ -354,7 +354,7 @@ void qemu_chr_fe_set_open(CharBackend *be, int
> fe_open)
> >  }
> >
> >  guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
> > -                            GIOFunc func, void *user_data)
> > +                            FEWatchFunc func, void *user_data)
> >  {
> >      Chardev *s = be->chr;
> >      GSource *src;
> > diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> > index ceb677bc5a..8ee6f74b8c 100644
> > --- a/hw/char/cadence_uart.c
> > +++ b/hw/char/cadence_uart.c
> > @@ -288,7 +288,7 @@ static void uart_write_rx_fifo(void *opaque, const
> uint8_t *buf, int size)
> >      uart_update_status(s);
> >  }
> >
> > -static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
> > +static gboolean cadence_uart_xmit(GIOChannel *do_not_use, GIOCondition
> cond,
> >                                    void *opaque)
>
> Why is this (and the next 3) left as GIOCondition, when you change others
> later on to be void ?
>

Good catch. It's a leftover. It is fixed in my branch
https://gitlab.com/marcandre.lureau/qemu/-/tree/chardev

thanks


> >  {
> >      CadenceUARTState *s = opaque;
> > diff --git a/hw/char/cmsdk-apb-uart.c b/hw/char/cmsdk-apb-uart.c
> > index ba2cbbee3d..b07a9dee4f 100644
> > --- a/hw/char/cmsdk-apb-uart.c
> > +++ b/hw/char/cmsdk-apb-uart.c
> > @@ -191,7 +191,7 @@ static uint64_t uart_read(void *opaque, hwaddr
> offset, unsigned size)
> >  /* Try to send tx data, and arrange to be called back later if
> >   * we can't (ie the char backend is busy/blocking).
> >   */
> > -static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void
> *opaque)
> > +static gboolean uart_transmit(GIOChannel *do_not_use, GIOCondition
> cond, void *opaque)
> >  {
> >      CMSDKAPBUART *s = CMSDK_APB_UART(opaque);
> >      int ret;
> > diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
> > index 6b0c9330bf..e493ea08c0 100644
> > --- a/hw/char/ibex_uart.c
> > +++ b/hw/char/ibex_uart.c
> > @@ -135,7 +135,7 @@ static void ibex_uart_receive(void *opaque, const
> uint8_t *buf, int size)
> >      ibex_uart_update_irqs(s);
> >  }
> >
> > -static gboolean ibex_uart_xmit(GIOChannel *chan, GIOCondition cond,
> > +static gboolean ibex_uart_xmit(GIOChannel *do_not_use, GIOCondition
> cond,
> >                                 void *opaque)
> >  {
> >      IbexUartState *s = opaque;
> > diff --git a/hw/char/nrf51_uart.c b/hw/char/nrf51_uart.c
> > index 045ca5fa40..0b89b0eae4 100644
> > --- a/hw/char/nrf51_uart.c
> > +++ b/hw/char/nrf51_uart.c
> > @@ -75,7 +75,7 @@ static uint64_t uart_read(void *opaque, hwaddr addr,
> unsigned int size)
> >      return r;
> >  }
> >
> > -static gboolean uart_transmit(GIOChannel *chan, GIOCondition cond, void
> *opaque)
> > +static gboolean uart_transmit(GIOChannel *do_not_use, GIOCondition
> cond, void *opaque)
> >  {
> >      NRF51UARTState *s = NRF51_UART(opaque);
> >      int r;
> > diff --git a/hw/char/serial.c b/hw/char/serial.c
> > index bc2e322970..7061aacbce 100644
> > --- a/hw/char/serial.c
> > +++ b/hw/char/serial.c
> > @@ -220,7 +220,7 @@ static void serial_update_msl(SerialState *s)
> >      }
> >  }
> >
> > -static gboolean serial_watch_cb(GIOChannel *chan, GIOCondition cond,
> > +static gboolean serial_watch_cb(void *do_not_use, GIOCondition cond,
> >                                  void *opaque)
> >  {
> >      SerialState *s = opaque;
> > diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
> > index 6b132caa29..dd5a02e339 100644
> > --- a/hw/char/virtio-console.c
> > +++ b/hw/char/virtio-console.c
> > @@ -38,7 +38,7 @@ struct VirtConsole {
> >   * Callback function that's called from chardevs when backend becomes
> >   * writable.
> >   */
> > -static gboolean chr_write_unblocked(GIOChannel *chan, GIOCondition cond,
> > +static gboolean chr_write_unblocked(void *do_not_use, GIOCondition cond,
> >                                      void *opaque)
> >  {
> >      VirtConsole *vcon = opaque;
> > diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> > index 1ec909a63a..5f0ef9cb3b 100644
> > --- a/hw/usb/redirect.c
> > +++ b/hw/usb/redirect.c
> > @@ -270,7 +270,7 @@ static int usbredir_read(void *priv, uint8_t *data,
> int count)
> >      return count;
> >  }
> >
> > -static gboolean usbredir_write_unblocked(GIOChannel *chan, GIOCondition
> cond,
> > +static gboolean usbredir_write_unblocked(void *do_not_use, GIOCondition
> cond,
> >                                           void *opaque)
> >  {
> >      USBRedirDevice *dev = opaque;
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 29ea2b4fce..aec6cc1990 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -303,7 +303,7 @@ struct vhost_user_read_cb_data {
> >      int ret;
> >  };
> >
> > -static gboolean vhost_user_read_cb(GIOChannel *source, GIOCondition
> condition,
> > +static gboolean vhost_user_read_cb(void *do_not_use, GIOCondition
> condition,
> >                                     gpointer opaque)
> >  {
> >      struct vhost_user_read_cb_data *data = opaque;
> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index b90c0f4051..46a171bca6 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -156,7 +156,7 @@ static inline bool
> monitor_is_hmp_non_interactive(const Monitor *mon)
> >
> >  static void monitor_flush_locked(Monitor *mon);
> >
> > -static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> > +static gboolean monitor_unblocked(void *do_not_use, GIOCondition cond,
> >                                    void *opaque)
> >  {
> >      Monitor *mon = opaque;
> > diff --git a/net/vhost-user.c b/net/vhost-user.c
> > index ffbd94d944..6adfcd623a 100644
> > --- a/net/vhost-user.c
> > +++ b/net/vhost-user.c
> > @@ -208,8 +208,8 @@ static NetClientInfo net_vhost_user_info = {
> >          .set_vnet_le = vhost_user_set_vnet_endianness,
> >  };
> >
> > -static gboolean net_vhost_user_watch(GIOChannel *chan, GIOCondition
> cond,
> > -                                           void *opaque)
> > +static gboolean net_vhost_user_watch(void *do_not_use, GIOCondition
> cond,
> > +                                     void *opaque)
> >  {
> >      NetVhostUserState *s = opaque;
> >
> > --
> > 2.32.0.264.g75ae10bc75
> >
>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 12262 bytes --]

Re: [PATCH v2 08/11] chardev: fix qemu_chr_open_fd() with fd_in==fd_out

[Daniel P. Berrangé]

On Wed, Aug 04, 2021 at 07:48:45PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The "serial" chardev calls qemu_chr_open_fd() with the same fd. This
> may lead to double-close as each QIOChannel owns the fd.
> 
> Instead, share the reference to the same QIOChannel.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  chardev/char-fd.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 10/11] chardev: report a simpler error about duplicated id

[Daniel P. Berrangé]

On Wed, Aug 04, 2021 at 07:48:47PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Report:
>   "Chardev with id 'char2' already exists"
> Rather than:
>   "Failed to add chardev 'char2': duplicate yank instance"
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  chardev/char.c | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 11/11] chardev: reuse qmp_chardev_new()

[Daniel P. Berrangé]

On Wed, Aug 04, 2021 at 07:48:48PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> qemu_chardev_new() and qmp_chardev_add() are similar, let's factorize
> the code a bit.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  chardev/char.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 05/11] chardev: mark explicitly first argument as poisoned

[Daniel P. Berrangé]

On Thu, Aug 05, 2021 at 02:39:28PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Aug 5, 2021 at 2:37 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Wed, Aug 04, 2021 at 07:48:42PM +0400, marcandre.lureau@redhat.com
> > wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Since commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2 "char: convert
> > > from GIOChannel to QIOChannel", the first argument to the watch callback
> > > can actually be a QIOChannel, which is not a GIOChannel (but a QEMU
> > > Object).
> > >
> > > Even though we never used that pointer, change the callback type to warn
> > > the users. Possibly a better fix later, we may want to store the
> > > callback and call it from intermediary functions.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  include/chardev/char-fe.h | 8 +++++++-
> > >  chardev/char-fe.c         | 2 +-
> > >  hw/char/cadence_uart.c    | 2 +-
> > >  hw/char/cmsdk-apb-uart.c  | 2 +-
> > >  hw/char/ibex_uart.c       | 2 +-
> > >  hw/char/nrf51_uart.c      | 2 +-
> > >  hw/char/serial.c          | 2 +-
> > >  hw/char/virtio-console.c  | 2 +-
> > >  hw/usb/redirect.c         | 2 +-
> > >  hw/virtio/vhost-user.c    | 2 +-
> > >  monitor/monitor.c         | 2 +-
> > >  net/vhost-user.c          | 4 ++--
> > >  12 files changed, 19 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
> > > index a553843364..867ef1b3b2 100644
> > > --- a/include/chardev/char-fe.h
> > > +++ b/include/chardev/char-fe.h
> > > @@ -174,6 +174,9 @@ void qemu_chr_fe_set_open(CharBackend *be, int
> > fe_open);
> > >  void qemu_chr_fe_printf(CharBackend *be, const char *fmt, ...)
> > >      GCC_FMT_ATTR(2, 3);
> > >
> > > +
> > > +typedef gboolean (*FEWatchFunc)(void *do_not_use, GIOCondition
> > condition, void *data);
> > > +
> > >  /**
> > >   * qemu_chr_fe_add_watch:
> > >   * @cond: the condition to poll for
> > > @@ -188,10 +191,13 @@ void qemu_chr_fe_printf(CharBackend *be, const
> > char *fmt, ...)
> > >   * Note that you are responsible to update the front-end sources if
> > >   * you are switching the main context with qemu_chr_fe_set_handlers().
> > >   *
> > > + * Warning: DO NOT use the first callback argument (it may be either
> > > + * a GIOChannel or a QIOChannel, depending on the underlying chardev)
> > > + *
> > >   * Returns: the source tag
> > >   */
> > >  guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
> > > -                            GIOFunc func, void *user_data);
> > > +                            FEWatchFunc func, void *user_data);
> > >
> > >  /**
> > >   * qemu_chr_fe_write:
> > > diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> > > index 474715c5a9..7789f7be9c 100644
> > > --- a/chardev/char-fe.c
> > > +++ b/chardev/char-fe.c
> > > @@ -354,7 +354,7 @@ void qemu_chr_fe_set_open(CharBackend *be, int
> > fe_open)
> > >  }
> > >
> > >  guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
> > > -                            GIOFunc func, void *user_data)
> > > +                            FEWatchFunc func, void *user_data)
> > >  {
> > >      Chardev *s = be->chr;
> > >      GSource *src;
> > > diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> > > index ceb677bc5a..8ee6f74b8c 100644
> > > --- a/hw/char/cadence_uart.c
> > > +++ b/hw/char/cadence_uart.c
> > > @@ -288,7 +288,7 @@ static void uart_write_rx_fifo(void *opaque, const
> > uint8_t *buf, int size)
> > >      uart_update_status(s);
> > >  }
> > >
> > > -static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
> > > +static gboolean cadence_uart_xmit(GIOChannel *do_not_use, GIOCondition
> > cond,
> > >                                    void *opaque)
> >
> > Why is this (and the next 3) left as GIOCondition, when you change others
> > later on to be void ?
> >
> 
> Good catch. It's a leftover. It is fixed in my branch
> https://gitlab.com/marcandre.lureau/qemu/-/tree/chardev

Ok, with that

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|