[Qemu-devel] [PULL 0/6] Chardev patches

[Qemu-devel] [PULL 0/6] Chardev patches

[Marc-André Lureau]

The following changes since commit dafd95053611aa14dda40266857608d12ddce658:

  Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2018-10-02 18:27:18 +0100)

are available in the Git repository at:

  https://github.com/elmarco/qemu.git tags/chardev-pull-request

for you to fetch changes up to a7077b8e354d90fec26c2921aa2dea85b90dff90:

  chardev: use a child source for qio input source (2018-10-03 14:45:05 +0400)

----------------------------------------------------------------
chardev patches

----------------------------------------------------------------

Marc-André Lureau (6):
  chardev: avoid crash if no associated address
  chardev: remove qemu_chr_fe_read_all() counter
  chardev: unref if underlying chardev has no parent
  char.h: fix gtk-doc comment style
  chardev: mark the calls that allow an implicit mux monitor
  chardev: use a child source for qio input source

 include/chardev/char-fe.h | 81 ++++++++++++++++++---------------------
 include/chardev/char.h    | 81 +++++++++++++++++++++------------------
 chardev/char-fe.c         | 13 ++++---
 chardev/char-io.c         | 48 +++--------------------
 chardev/char-socket.c     |  8 +++-
 chardev/char.c            | 37 ++++++++++++++----
 gdbstub.c                 |  6 ++-
 hw/char/xen_console.c     |  6 ++-
 net/slirp.c               |  6 ++-
 vl.c                      | 10 ++---
 10 files changed, 149 insertions(+), 147 deletions(-)

-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PULL 1/6] chardev: avoid crash if no associated address

[Marc-André Lureau]

A socket chardev may not have associated address (when adding client
fd manually for example). But on disconnect, updating socket filename
expects an address and may lead to this crash:

  Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
  0x0000555555d8c70c in SocketAddress_to_str (prefix=0x555556043062 "disconnected:", addr=0x0, is_listen=false, is_telnet=false) at /home/elmarco/src/qq/chardev/char-socket.c:388
  388	    switch (addr->type) {
  (gdb) bt
  #0  0x0000555555d8c70c in SocketAddress_to_str (prefix=0x555556043062 "disconnected:", addr=0x0, is_listen=false, is_telnet=false) at /home/elmarco/src/qq/chardev/char-socket.c:388
  #1  0x0000555555d8c8aa in update_disconnected_filename (s=0x555556b1ed00) at /home/elmarco/src/qq/chardev/char-socket.c:419
  #2  0x0000555555d8c959 in tcp_chr_disconnect (chr=0x555556b1ed00) at /home/elmarco/src/qq/chardev/char-socket.c:438
  #3  0x0000555555d8cba1 in tcp_chr_hup (channel=0x555556b75690, cond=G_IO_HUP, opaque=0x555556b1ed00) at /home/elmarco/src/qq/chardev/char-socket.c:482
  #4  0x0000555555da596e in qio_channel_fd_source_dispatch (source=0x555556bb68b0, callback=0x555555d8cb58 <tcp_chr_hup>, user_data=0x555556b1ed00) at /home/elmarco/src/qq/io/channel-watch.c:84

Replace filename with a generic "disconnected:socket" in this case.

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

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 7cd0ae2824..a75b46d9fe 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -423,8 +423,12 @@ static void update_disconnected_filename(SocketChardev *s)
     Chardev *chr = CHARDEV(s);
 
     g_free(chr->filename);
-    chr->filename = SocketAddress_to_str("disconnected:", s->addr,
-                                         s->is_listen, s->is_telnet);
+    if (s->addr) {
+        chr->filename = SocketAddress_to_str("disconnected:", s->addr,
+                                             s->is_listen, s->is_telnet);
+    } else {
+        chr->filename = g_strdup("disconnected:socket");
+    }
 }
 
 /* NB may be called even if tcp_chr_connect has not been
-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PULL 2/6] chardev: remove qemu_chr_fe_read_all() counter

[Marc-André Lureau]

There is no obvious reason to have a loop counter. This limits from
reading several megabytes large buffers in one go, since socket
read/write usually have a limit.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 chardev/char-fe.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index b1f228e8b5..f158f158f8 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -56,7 +56,7 @@ int qemu_chr_fe_write_all(CharBackend *be, const uint8_t *buf, int len)
 int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len)
 {
     Chardev *s = be->chr;
-    int offset = 0, counter = 10;
+    int offset = 0;
     int res;
 
     if (!s || !CHARDEV_GET_CLASS(s)->chr_sync_read) {
@@ -88,10 +88,6 @@ int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len)
         }
 
         offset += res;
-
-        if (!counter--) {
-            break;
-        }
     }
 
     if (qemu_chr_replay(s) && replay_mode == REPLAY_MODE_RECORD) {
-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PULL 3/6] chardev: unref if underlying chardev has no parent

[Marc-André Lureau]

It's possible to write code creating a chardev backend that is not
registered. When it is not user-created, it makes sense to keep it
hidden. Let the associated frontend destroy it also in this case.

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

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index f158f158f8..a8931f7afd 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -235,7 +235,12 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del)
             d->backends[b->tag] = NULL;
         }
         if (del) {
-            object_unparent(OBJECT(b->chr));
+            Object *obj = OBJECT(b->chr);
+            if (obj->parent) {
+                object_unparent(obj);
+            } else {
+                object_unref(obj);
+            }
         }
         b->chr = NULL;
     }
-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PULL 4/6] char.h: fix gtk-doc comment style

[Marc-André Lureau]

Fix up conformance to GTK-Doc function comment style, as documented in
https://developer.gnome.org/gtk-doc-manual/stable/documenting_symbols.html.en

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 include/chardev/char-fe.h | 81 ++++++++++++++++++---------------------
 include/chardev/char.h    | 61 +++++++++++++----------------
 2 files changed, 63 insertions(+), 79 deletions(-)

diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index c67271f1ba..46c997d352 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -20,7 +20,7 @@ struct CharBackend {
 };
 
 /**
- * @qemu_chr_fe_init:
+ * qemu_chr_fe_init:
  *
  * Initializes a front end for the given CharBackend and
  * Chardev. Call qemu_chr_fe_deinit() to remove the association and
@@ -31,7 +31,7 @@ struct CharBackend {
 bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp);
 
 /**
- * @qemu_chr_fe_deinit:
+ * qemu_chr_fe_deinit:
  * @b: a CharBackend
  * @del: if true, delete the chardev backend
 *
@@ -42,9 +42,9 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp);
 void qemu_chr_fe_deinit(CharBackend *b, bool del);
 
 /**
- * @qemu_chr_fe_get_driver:
+ * qemu_chr_fe_get_driver:
  *
- * Returns the driver associated with a CharBackend or NULL if no
+ * Returns: the driver associated with a CharBackend or NULL if no
  * associated Chardev.
  * Note: avoid this function as the driver should never be accessed directly,
  *       especially by the frontends that support chardevice hotswap.
@@ -53,21 +53,21 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del);
 Chardev *qemu_chr_fe_get_driver(CharBackend *be);
 
 /**
- * @qemu_chr_fe_backend_connected:
+ * qemu_chr_fe_backend_connected:
  *
- * Returns true if there is a chardevice associated with @be.
+ * Returns: true if there is a chardevice associated with @be.
  */
 bool qemu_chr_fe_backend_connected(CharBackend *be);
 
 /**
- * @qemu_chr_fe_backend_open:
+ * qemu_chr_fe_backend_open:
  *
- * Returns true if chardevice associated with @be is open.
+ * Returns: true if chardevice associated with @be is open.
  */
 bool qemu_chr_fe_backend_open(CharBackend *be);
 
 /**
- * @qemu_chr_fe_set_handlers:
+ * qemu_chr_fe_set_handlers:
  * @b: a CharBackend
  * @fd_can_read: callback to get the amount of data the frontend may
  *               receive
@@ -95,7 +95,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
                               bool set_open);
 
 /**
- * @qemu_chr_fe_take_focus:
+ * qemu_chr_fe_take_focus:
  *
  * Take the focus (if the front end is muxed).
  *
@@ -104,14 +104,14 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
 void qemu_chr_fe_take_focus(CharBackend *b);
 
 /**
- * @qemu_chr_fe_accept_input:
+ * qemu_chr_fe_accept_input:
  *
  * Notify that the frontend is ready to receive data
  */
 void qemu_chr_fe_accept_input(CharBackend *be);
 
 /**
- * @qemu_chr_fe_disconnect:
+ * qemu_chr_fe_disconnect:
  *
  * Close a fd accepted by character backend.
  * Without associated Chardev, do nothing.
@@ -119,7 +119,7 @@ void qemu_chr_fe_accept_input(CharBackend *be);
 void qemu_chr_fe_disconnect(CharBackend *be);
 
 /**
- * @qemu_chr_fe_wait_connected:
+ * qemu_chr_fe_wait_connected:
  *
  * Wait for characted backend to be connected, return < 0 on error or
  * if no associated Chardev.
@@ -127,19 +127,18 @@ void qemu_chr_fe_disconnect(CharBackend *be);
 int qemu_chr_fe_wait_connected(CharBackend *be, Error **errp);
 
 /**
- * @qemu_chr_fe_set_echo:
+ * qemu_chr_fe_set_echo:
+ * @echo: true to enable echo, false to disable echo
  *
  * Ask the backend to override its normal echo setting.  This only really
  * applies to the stdio backend and is used by the QMP server such that you
  * can see what you type if you try to type QMP commands.
  * Without associated Chardev, do nothing.
- *
- * @echo true to enable echo, false to disable echo
  */
 void qemu_chr_fe_set_echo(CharBackend *be, bool echo);
 
 /**
- * @qemu_chr_fe_set_open:
+ * qemu_chr_fe_set_open:
  *
  * Set character frontend open status.  This is an indication that the
  * front end is ready (or not) to begin doing I/O.
@@ -148,83 +147,77 @@ void qemu_chr_fe_set_echo(CharBackend *be, bool echo);
 void qemu_chr_fe_set_open(CharBackend *be, int fe_open);
 
 /**
- * @qemu_chr_fe_printf:
+ * qemu_chr_fe_printf:
+ * @fmt: see #printf
  *
  * Write to a character backend using a printf style interface.  This
  * function is thread-safe. It does nothing without associated
  * Chardev.
- *
- * @fmt see #printf
  */
 void qemu_chr_fe_printf(CharBackend *be, const char *fmt, ...)
     GCC_FMT_ATTR(2, 3);
 
 /**
- * @qemu_chr_fe_add_watch:
+ * qemu_chr_fe_add_watch:
+ * @cond: the condition to poll for
+ * @func: the function to call when the condition happens
+ * @user_data: the opaque pointer to pass to @func
  *
  * If the backend is connected, create and add a #GSource that fires
  * when the given condition (typically G_IO_OUT|G_IO_HUP or G_IO_HUP)
  * is active; return the #GSource's tag.  If it is disconnected,
  * or without associated Chardev, return 0.
  *
- * @cond the condition to poll for
- * @func the function to call when the condition happens
- * @user_data the opaque pointer to pass to @func
- *
  * Returns: the source tag
  */
 guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
                             GIOFunc func, void *user_data);
 
 /**
- * @qemu_chr_fe_write:
+ * qemu_chr_fe_write:
+ * @buf: the data
+ * @len: the number of bytes to send
  *
  * Write data to a character backend from the front end.  This function
  * will send data from the front end to the back end.  This function
  * is thread-safe.
  *
- * @buf the data
- * @len the number of bytes to send
- *
  * Returns: the number of bytes consumed (0 if no associated Chardev)
  */
 int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len);
 
 /**
- * @qemu_chr_fe_write_all:
+ * qemu_chr_fe_write_all:
+ * @buf: the data
+ * @len: the number of bytes to send
  *
  * Write data to a character backend from the front end.  This function will
  * send data from the front end to the back end.  Unlike @qemu_chr_fe_write,
  * this function will block if the back end cannot consume all of the data
  * attempted to be written.  This function is thread-safe.
  *
- * @buf the data
- * @len the number of bytes to send
- *
  * Returns: the number of bytes consumed (0 if no associated Chardev)
  */
 int qemu_chr_fe_write_all(CharBackend *be, const uint8_t *buf, int len);
 
 /**
- * @qemu_chr_fe_read_all:
+ * qemu_chr_fe_read_all:
+ * @buf: the data buffer
+ * @len: the number of bytes to read
  *
  * Read data to a buffer from the back end.
  *
- * @buf the data buffer
- * @len the number of bytes to read
- *
  * Returns: the number of bytes read (0 if no associated Chardev)
  */
 int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len);
 
 /**
- * @qemu_chr_fe_ioctl:
+ * qemu_chr_fe_ioctl:
+ * @cmd: see CHR_IOCTL_*
+ * @arg: the data associated with @cmd
  *
  * Issue a device specific ioctl to a backend.  This function is thread-safe.
  *
- * @cmd see CHR_IOCTL_*
- * @arg the data associated with @cmd
- *
  * Returns: if @cmd is not supported by the backend or there is no
  *          associated Chardev, -ENOTSUP, otherwise the return
  *          value depends on the semantics of @cmd
@@ -232,7 +225,7 @@ int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len);
 int qemu_chr_fe_ioctl(CharBackend *be, int cmd, void *arg);
 
 /**
- * @qemu_chr_fe_get_msgfd:
+ * qemu_chr_fe_get_msgfd:
  *
  * For backends capable of fd passing, return the latest file descriptor passed
  * by a client.
@@ -245,7 +238,7 @@ int qemu_chr_fe_ioctl(CharBackend *be, int cmd, void *arg);
 int qemu_chr_fe_get_msgfd(CharBackend *be);
 
 /**
- * @qemu_chr_fe_get_msgfds:
+ * qemu_chr_fe_get_msgfds:
  *
  * For backends capable of fd passing, return the number of file received
  * descriptors and fills the fds array up to num elements
@@ -258,7 +251,7 @@ int qemu_chr_fe_get_msgfd(CharBackend *be);
 int qemu_chr_fe_get_msgfds(CharBackend *be, int *fds, int num);
 
 /**
- * @qemu_chr_fe_set_msgfds:
+ * qemu_chr_fe_set_msgfds:
  *
  * For backends capable of fd passing, set an array of fds to be passed with
  * the next send operation.
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 6f0576e214..3e4fe6dad0 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -68,12 +68,11 @@ struct Chardev {
 };
 
 /**
- * @qemu_chr_new_from_opts:
+ * qemu_chr_new_from_opts:
+ * @opts: see qemu-config.c for a list of valid options
  *
  * Create a new character backend from a QemuOpts list.
  *
- * @opts see qemu-config.c for a list of valid options
- *
  * Returns: on success: a new character backend
  *          otherwise:  NULL; @errp specifies the error
  *                            or left untouched in case of help option
@@ -82,17 +81,16 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
                                 Error **errp);
 
 /**
- * @qemu_chr_parse_common:
+ * qemu_chr_parse_common:
+ * @opts: the options that still need parsing
+ * @backend: a new backend
  *
  * Parse the common options available to all character backends.
- *
- * @opts the options that still need parsing
- * @backend a new backend
  */
 void qemu_chr_parse_common(QemuOpts *opts, ChardevCommon *backend);
 
 /**
- * @qemu_chr_parse_opts:
+ * qemu_chr_parse_opts:
  *
  * Parse the options to the ChardevBackend struct.
  *
@@ -102,49 +100,46 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
                                     Error **errp);
 
 /**
- * @qemu_chr_new:
+ * qemu_chr_new:
+ * @label: the name of the backend
+ * @filename: the URI
  *
  * Create a new character backend from a URI.
  *
- * @label the name of the backend
- * @filename the URI
- *
  * Returns: a new character backend
  */
 Chardev *qemu_chr_new(const char *label, const char *filename);
 
 /**
- * @qemu_chr_change:
+ * qemu_chr_change:
+ * @opts: the new backend options
  *
  * Change an existing character backend
- *
- * @opts the new backend options
  */
 void qemu_chr_change(QemuOpts *opts, Error **errp);
 
 /**
- * @qemu_chr_cleanup:
+ * qemu_chr_cleanup:
  *
  * Delete all chardevs (when leaving qemu)
  */
 void qemu_chr_cleanup(void);
 
 /**
- * @qemu_chr_new_noreplay:
+ * qemu_chr_new_noreplay:
+ * @label: the name of the backend
+ * @filename: the URI
  *
  * Create a new character backend from a URI.
  * Character device communications are not written
  * into the replay log.
  *
- * @label the name of the backend
- * @filename the URI
- *
  * Returns: a new character backend
  */
 Chardev *qemu_chr_new_noreplay(const char *label, const char *filename);
 
 /**
- * @qemu_chr_be_can_write:
+ * qemu_chr_be_can_write:
  *
  * Determine how much data the front end can currently accept.  This function
  * returns the number of bytes the front end can accept.  If it returns 0, the
@@ -156,43 +151,39 @@ Chardev *qemu_chr_new_noreplay(const char *label, const char *filename);
 int qemu_chr_be_can_write(Chardev *s);
 
 /**
- * @qemu_chr_be_write:
+ * qemu_chr_be_write:
+ * @buf: a buffer to receive data from the front end
+ * @len: the number of bytes to receive from the front end
  *
  * Write data from the back end to the front end.  Before issuing this call,
  * the caller should call @qemu_chr_be_can_write to determine how much data
  * the front end can currently accept.
- *
- * @buf a buffer to receive data from the front end
- * @len the number of bytes to receive from the front end
  */
 void qemu_chr_be_write(Chardev *s, uint8_t *buf, int len);
 
 /**
- * @qemu_chr_be_write_impl:
+ * qemu_chr_be_write_impl:
+ * @buf: a buffer to receive data from the front end
+ * @len: the number of bytes to receive from the front end
  *
  * Implementation of back end writing. Used by replay module.
- *
- * @buf a buffer to receive data from the front end
- * @len the number of bytes to receive from the front end
  */
 void qemu_chr_be_write_impl(Chardev *s, uint8_t *buf, int len);
 
 /**
- * @qemu_chr_be_update_read_handlers:
+ * qemu_chr_be_update_read_handlers:
+ * @context: the gcontext that will be used to attach the watch sources
  *
  * Invoked when frontend read handlers are setup
- *
- * @context the gcontext that will be used to attach the watch sources
  */
 void qemu_chr_be_update_read_handlers(Chardev *s,
                                       GMainContext *context);
 
 /**
- * @qemu_chr_be_event:
+ * qemu_chr_be_event:
+ * @event: the event to send
  *
  * Send an event from the back end to the front end.
- *
- * @event the event to send
  */
 void qemu_chr_be_event(Chardev *s, int event);
 
-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PULL 5/6] chardev: mark the calls that allow an implicit mux monitor

[Marc-André Lureau]

This is mostly for readability of the code. Let's make it clear which
callers can create an implicit monitor when the chardev is muxed.

This will also enforce a safer behaviour, as we don't really support
creating monitor anywhere/anytime at the moment. Add an assert() to
make sure the programmer explicitely wanted that behaviour.

There are documented cases, such as: -serial/-parallel/-virtioconsole
and to less extent -debugcon.

Less obvious and questionable ones are -gdb, SLIRP -guestfwd and Xen
console. Add a FIXME note for those, but keep the support for now.

Other qemu_chr_new() callers either have a fixed parameter/filename
string or do not need it, such as -qtest:

* qtest.c: qtest_init()
  Afaik, only used by tests/libqtest.c, without mux. I don't think we
  support it outside of qemu testing: drop support for implicit mux
  monitor (qemu_chr_new() call: no implicit mux now).

* hw/
  All with literal @filename argument that doesn't enable mux monitor.

* tests/
  All with @filename argument that doesn't enable mux monitor.

On a related note, the list of monitor creation places:

- the chardev creators listed above: all from command line (except
  perhaps Xen console?)

- -gdb & hmp gdbserver will create a "GDB monitor command" chardev
  that is wired to an HMP monitor.

- -mon command line option

>From this short study, I would like to think that a monitor may only
be created in the main thread today, though I remain skeptical :)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 include/chardev/char.h | 24 ++++++++++++++++++++----
 chardev/char.c         | 37 ++++++++++++++++++++++++++++++-------
 gdbstub.c              |  6 +++++-
 hw/char/xen_console.c  |  6 +++++-
 net/slirp.c            |  6 +++++-
 vl.c                   | 10 +++++-----
 6 files changed, 70 insertions(+), 19 deletions(-)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index 3e4fe6dad0..7becd8c80c 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -105,14 +105,27 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
  * @filename: the URI
  *
  * Create a new character backend from a URI.
+ * Do not implicitly initialize a monitor if the chardev is muxed.
  *
  * Returns: a new character backend
  */
 Chardev *qemu_chr_new(const char *label, const char *filename);
 
 /**
- * qemu_chr_change:
- * @opts: the new backend options
+ * qemu_chr_new_mux_mon:
+ * @label: the name of the backend
+ * @filename: the URI
+ *
+ * Create a new character backend from a URI.
+ * Implicitly initialize a monitor if the chardev is muxed.
+ *
+ * Returns: a new character backend
+ */
+Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename);
+
+/**
+* qemu_chr_change:
+* @opts: the new backend options
  *
  * Change an existing character backend
  */
@@ -129,6 +142,7 @@ void qemu_chr_cleanup(void);
  * qemu_chr_new_noreplay:
  * @label: the name of the backend
  * @filename: the URI
+ * @permit_mux_mon: if chardev is muxed, initialize a monitor
  *
  * Create a new character backend from a URI.
  * Character device communications are not written
@@ -136,7 +150,8 @@ void qemu_chr_cleanup(void);
  *
  * Returns: a new character backend
  */
-Chardev *qemu_chr_new_noreplay(const char *label, const char *filename);
+Chardev *qemu_chr_new_noreplay(const char *label, const char *filename,
+                               bool permit_mux_mon);
 
 /**
  * qemu_chr_be_can_write:
@@ -194,7 +209,8 @@ bool qemu_chr_has_feature(Chardev *chr,
                           ChardevFeature feature);
 void qemu_chr_set_feature(Chardev *chr,
                           ChardevFeature feature);
-QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
+QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
+                                bool permit_mux_mon);
 int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all);
 #define qemu_chr_write_all(s, buf, len) qemu_chr_write(s, buf, len, true)
 int qemu_chr_wait_connected(Chardev *chr, Error **errp);
diff --git a/chardev/char.c b/chardev/char.c
index 76d866e6fe..e115166995 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -329,7 +329,8 @@ int qemu_chr_wait_connected(Chardev *chr, Error **errp)
     return 0;
 }
 
-QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
+QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
+                                bool permit_mux_mon)
 {
     char host[65], port[33], width[8], height[8];
     int pos;
@@ -344,6 +345,10 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
     }
 
     if (strstart(filename, "mon:", &p)) {
+        if (!permit_mux_mon) {
+            error_report("mon: isn't supported in this context");
+            return NULL;
+        }
         filename = p;
         qemu_opt_set(opts, "mux", "on", &error_abort);
         if (strcmp(filename, "stdio") == 0) {
@@ -683,7 +688,8 @@ out:
     return chr;
 }
 
-Chardev *qemu_chr_new_noreplay(const char *label, const char *filename)
+Chardev *qemu_chr_new_noreplay(const char *label, const char *filename,
+                               bool permit_mux_mon)
 {
     const char *p;
     Chardev *chr;
@@ -694,25 +700,32 @@ Chardev *qemu_chr_new_noreplay(const char *label, const char *filename)
         return qemu_chr_find(p);
     }
 
-    opts = qemu_chr_parse_compat(label, filename);
+    opts = qemu_chr_parse_compat(label, filename, permit_mux_mon);
     if (!opts)
         return NULL;
 
     chr = qemu_chr_new_from_opts(opts, &err);
-    if (err) {
+    if (!chr) {
         error_report_err(err);
+        goto out;
     }
-    if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
+
+    if (qemu_opt_get_bool(opts, "mux", 0)) {
+        assert(permit_mux_mon);
         monitor_init(chr, MONITOR_USE_READLINE);
     }
+
+out:
     qemu_opts_del(opts);
     return chr;
 }
 
-Chardev *qemu_chr_new(const char *label, const char *filename)
+static Chardev *qemu_chr_new_permit_mux_mon(const char *label,
+                                          const char *filename,
+                                          bool permit_mux_mon)
 {
     Chardev *chr;
-    chr = qemu_chr_new_noreplay(label, filename);
+    chr = qemu_chr_new_noreplay(label, filename, permit_mux_mon);
     if (chr) {
         if (replay_mode != REPLAY_MODE_NONE) {
             qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_REPLAY);
@@ -726,6 +739,16 @@ Chardev *qemu_chr_new(const char *label, const char *filename)
     return chr;
 }
 
+Chardev *qemu_chr_new(const char *label, const char *filename)
+{
+    return qemu_chr_new_permit_mux_mon(label, filename, false);
+}
+
+Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename)
+{
+    return qemu_chr_new_permit_mux_mon(label, filename, true);
+}
+
 static int qmp_query_chardev_foreach(Object *obj, void *data)
 {
     Chardev *chr = CHARDEV(obj);
diff --git a/gdbstub.c b/gdbstub.c
index d6ab95006c..c8478de8f5 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2038,7 +2038,11 @@ int gdbserver_start(const char *device)
             sigaction(SIGINT, &act, NULL);
         }
 #endif
-        chr = qemu_chr_new_noreplay("gdb", device);
+        /*
+         * FIXME: it's a bit weird to allow using a mux chardev here
+         * and implicitly setup a monitor. We may want to break this.
+         */
+        chr = qemu_chr_new_noreplay("gdb", device, true);
         if (!chr)
             return -1;
     }
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index 8b4b4bf523..44f7236382 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -207,7 +207,11 @@ static int con_init(struct XenDevice *xendev)
     } else {
         snprintf(label, sizeof(label), "xencons%d", con->xendev.dev);
         qemu_chr_fe_init(&con->chr,
-                         qemu_chr_new(label, output), &error_abort);
+                         /*
+                          * FIXME: sure we want to support implicit
+                          * muxed monitors here?
+                          */
+                         qemu_chr_new_mux_mon(label, output), &error_abort);
     }
 
     xenstore_store_pv_console_info(con->xendev.dev,
diff --git a/net/slirp.c b/net/slirp.c
index c93b64dd91..99884de204 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -764,7 +764,11 @@ static int slirp_guestfwd(SlirpState *s, const char *config_str, Error **errp)
         }
     } else {
         Error *err = NULL;
-        Chardev *chr = qemu_chr_new(buf, p);
+        /*
+         * FIXME: sure we want to support implicit
+         * muxed monitors here?
+         */
+        Chardev *chr = qemu_chr_new_mux_mon(buf, p);
 
         if (!chr) {
             error_setg(errp, "Could not open guest forwarding device '%s'",
diff --git a/vl.c b/vl.c
index 0388852deb..a867c9c4d9 100644
--- a/vl.c
+++ b/vl.c
@@ -2310,7 +2310,7 @@ static void monitor_parse(const char *optarg, const char *mode, bool pretty)
     } else {
         snprintf(label, sizeof(label), "compat_monitor%d",
                  monitor_device_index);
-        opts = qemu_chr_parse_compat(label, optarg);
+        opts = qemu_chr_parse_compat(label, optarg, true);
         if (!opts) {
             error_report("parse error: %s", optarg);
             exit(1);
@@ -2382,7 +2382,7 @@ static int serial_parse(const char *devname)
     snprintf(label, sizeof(label), "serial%d", index);
     serial_hds = g_renew(Chardev *, serial_hds, index + 1);
 
-    serial_hds[index] = qemu_chr_new(label, devname);
+    serial_hds[index] = qemu_chr_new_mux_mon(label, devname);
     if (!serial_hds[index]) {
         error_report("could not connect serial device"
                      " to character backend '%s'", devname);
@@ -2418,7 +2418,7 @@ static int parallel_parse(const char *devname)
         exit(1);
     }
     snprintf(label, sizeof(label), "parallel%d", index);
-    parallel_hds[index] = qemu_chr_new(label, devname);
+    parallel_hds[index] = qemu_chr_new_mux_mon(label, devname);
     if (!parallel_hds[index]) {
         error_report("could not connect parallel device"
                      " to character backend '%s'", devname);
@@ -2449,7 +2449,7 @@ static int virtcon_parse(const char *devname)
     qemu_opt_set(dev_opts, "driver", "virtconsole", &error_abort);
 
     snprintf(label, sizeof(label), "virtcon%d", index);
-    virtcon_hds[index] = qemu_chr_new(label, devname);
+    virtcon_hds[index] = qemu_chr_new_mux_mon(label, devname);
     if (!virtcon_hds[index]) {
         error_report("could not connect virtio console"
                      " to character backend '%s'", devname);
@@ -2465,7 +2465,7 @@ static int debugcon_parse(const char *devname)
 {
     QemuOpts *opts;
 
-    if (!qemu_chr_new("debugcon", devname)) {
+    if (!qemu_chr_new_mux_mon("debugcon", devname)) {
         exit(1);
     }
     opts = qemu_opts_create(qemu_find_opts("device"), "debugcon", 1, NULL);
-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PULL 6/6] chardev: use a child source for qio input source

[Marc-André Lureau]

GLib child source were added with version 2.28. We can use them now
that we bumped our requirement to 2.40.

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

diff --git a/chardev/char-io.c b/chardev/char-io.c
index f81052481a..8ced184160 100644
--- a/chardev/char-io.c
+++ b/chardev/char-io.c
@@ -33,7 +33,6 @@ typedef struct IOWatchPoll {
     IOCanReadHandler *fd_can_read;
     GSourceFunc fd_read;
     void *opaque;
-    GMainContext *context;
 } IOWatchPoll;
 
 static IOWatchPoll *io_watch_poll_from_source(GSource *source)
@@ -55,47 +54,24 @@ static gboolean io_watch_poll_prepare(GSource *source,
         iwp->src = qio_channel_create_watch(
             iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
         g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL);
-        g_source_attach(iwp->src, iwp->context);
-    } else {
-        g_source_destroy(iwp->src);
+        g_source_add_child_source(source, iwp->src);
         g_source_unref(iwp->src);
+    } else {
+        g_source_remove_child_source(source, iwp->src);
         iwp->src = NULL;
     }
     return FALSE;
 }
 
-static gboolean io_watch_poll_check(GSource *source)
-{
-    return FALSE;
-}
-
 static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback,
                                        gpointer user_data)
 {
-    abort();
-}
-
-static void io_watch_poll_finalize(GSource *source)
-{
-    /* Due to a glib bug, removing the last reference to a source
-     * inside a finalize callback causes recursive locking (and a
-     * deadlock).  This is not a problem inside other callbacks,
-     * including dispatch callbacks, so we call io_remove_watch_poll
-     * to remove this source.  At this point, iwp->src must
-     * be NULL, or we would leak it.
-     *
-     * This would be solved much more elegantly by child sources,
-     * but we support older glib versions that do not have them.
-     */
-    IOWatchPoll *iwp = io_watch_poll_from_source(source);
-    assert(iwp->src == NULL);
+    return G_SOURCE_CONTINUE;
 }
 
 static GSourceFuncs io_watch_poll_funcs = {
     .prepare = io_watch_poll_prepare,
-    .check = io_watch_poll_check,
     .dispatch = io_watch_poll_dispatch,
-    .finalize = io_watch_poll_finalize,
 };
 
 GSource *io_add_watch_poll(Chardev *chr,
@@ -115,7 +91,6 @@ GSource *io_add_watch_poll(Chardev *chr,
     iwp->ioc = ioc;
     iwp->fd_read = (GSourceFunc) fd_read;
     iwp->src = NULL;
-    iwp->context = context;
 
     name = g_strdup_printf("chardev-iowatch-%s", chr->label);
     g_source_set_name((GSource *)iwp, name);
@@ -126,23 +101,10 @@ GSource *io_add_watch_poll(Chardev *chr,
     return (GSource *)iwp;
 }
 
-static void io_remove_watch_poll(GSource *source)
-{
-    IOWatchPoll *iwp;
-
-    iwp = io_watch_poll_from_source(source);
-    if (iwp->src) {
-        g_source_destroy(iwp->src);
-        g_source_unref(iwp->src);
-        iwp->src = NULL;
-    }
-    g_source_destroy(&iwp->parent);
-}
-
 void remove_fd_in_watch(Chardev *chr)
 {
     if (chr->gsource) {
-        io_remove_watch_poll(chr->gsource);
+        g_source_destroy(chr->gsource);
         chr->gsource = NULL;
     }
 }
-- 
2.19.0.271.gfe8321ec05

Re: [Qemu-devel] [PULL 0/6] Chardev patches

[Peter Maydell]

On 3 October 2018 at 11:57, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> The following changes since commit dafd95053611aa14dda40266857608d12ddce658:
>
>   Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2018-10-02 18:27:18 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/elmarco/qemu.git tags/chardev-pull-request
>
> for you to fetch changes up to a7077b8e354d90fec26c2921aa2dea85b90dff90:
>
>   chardev: use a child source for qio input source (2018-10-03 14:45:05 +0400)
>
> ----------------------------------------------------------------
> chardev patches
>


Applied, thanks.

-- PMM