[Qemu-devel] [PATCH 0/9] chardev: various cleanups and improvements

[Qemu-devel] [PATCH 0/9] chardev: various cleanups and improvements

[Marc-André Lureau]

Hi,

Some chardev-related patches I have accumulated.
Please review, thanks!

Marc-André Lureau (9):
  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
  char: update the mux hanlders in class callback
  char-fe: set_handlers() needs an associted chardev
  terminal3270: do not use backend timer sources
  chardev: add a note about frontend sources and context switch
  char-pty: remove check for connection on write
  char-pty: remove write_lock usage

 include/chardev/char-fe.h  | 84 ++++++++++++++++++--------------------
 include/chardev/char-mux.h |  1 -
 include/chardev/char.h     | 81 +++++++++++++++++++-----------------
 chardev/char-fe.c          | 11 +----
 chardev/char-io.c          | 48 +++-------------------
 chardev/char-mux.c         |  5 ++-
 chardev/char-pty.c         | 56 ++-----------------------
 chardev/char.c             | 32 +++++++++++----
 gdbstub.c                  |  6 ++-
 hw/char/terminal3270.c     | 15 +++----
 hw/char/xen_console.c      |  5 ++-
 net/slirp.c                |  5 ++-
 vl.c                       | 10 ++---
 13 files changed, 145 insertions(+), 214 deletions(-)

-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 1/9] char.h: fix gtk-doc comment style

[Marc-André Lureau]

Use the 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>
---
 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..21071f1fb1 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:
- *
- * Read data to a buffer from the back end.
- *
+ * 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.
+ *
  * 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:
- *
- * Issue a device specific ioctl to a backend.  This function is thread-safe.
- *
+ * 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.
+ *
  * 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.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 2/9] 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>
---
 include/chardev/char.h | 24 ++++++++++++++++++++----
 chardev/char.c         | 32 +++++++++++++++++++++++++-------
 gdbstub.c              |  6 +++++-
 hw/char/xen_console.c  |  5 ++++-
 net/slirp.c            |  5 ++++-
 vl.c                   | 10 +++++-----
 6 files changed, 63 insertions(+), 19 deletions(-)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index 3e4fe6dad0..268daaa90b 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
+ * @with_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 with_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 with_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..c1b89b6638 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 with_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 (!with_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 with_mux_mon)
 {
     const char *p;
     Chardev *chr;
@@ -694,25 +700,27 @@ 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, with_mux_mon);
     if (!opts)
         return NULL;
 
     chr = qemu_chr_new_from_opts(opts, &err);
     if (err) {
         error_report_err(err);
-    }
-    if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
+    } else if (qemu_opt_get_bool(opts, "mux", 0)) {
+        assert(with_mux_mon);
         monitor_init(chr, MONITOR_USE_READLINE);
     }
     qemu_opts_del(opts);
     return chr;
 }
 
-Chardev *qemu_chr_new(const char *label, const char *filename)
+static Chardev *qemu_chr_new_with_mux_mon(const char *label,
+                                          const char *filename,
+                                          bool with_mux_mon)
 {
     Chardev *chr;
-    chr = qemu_chr_new_noreplay(label, filename);
+    chr = qemu_chr_new_noreplay(label, filename, with_mux_mon);
     if (chr) {
         if (replay_mode != REPLAY_MODE_NONE) {
             qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_REPLAY);
@@ -726,6 +734,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_with_mux_mon(label, filename, false);
+}
+
+Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename)
+{
+    return qemu_chr_new_with_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..6a231d487b 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -207,7 +207,10 @@ 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: should it support implicit muxed monitors?
+                          */
+                         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 1e14318b4d..677dc36fe4 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -836,7 +836,10 @@ static int slirp_guestfwd(SlirpState *s, const char *config_str,
         }
     } else {
         Error *err = NULL;
-        Chardev *chr = qemu_chr_new(buf, p);
+        /*
+         * FIXME: should it support implicit muxed monitors?
+         */
+        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 5ba06adf78..b38e49ca43 100644
--- a/vl.c
+++ b/vl.c
@@ -2353,7 +2353,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);
@@ -2425,7 +2425,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);
@@ -2461,7 +2461,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);
@@ -2492,7 +2492,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);
@@ -2508,7 +2508,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.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 3/9] 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>
---
 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.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 4/9] char: update the mux hanlders in class callback

[Marc-André Lureau]

Instead of handling mux chardev in a special way in
qemu_chr_fe_set_handlers(), we may use the chr_update_read_handler
class callback instead.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/chardev/char-mux.h | 1 -
 chardev/char-fe.c          | 4 ----
 chardev/char-mux.c         | 5 +++--
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/include/chardev/char-mux.h b/include/chardev/char-mux.h
index 1e13187767..572cefd517 100644
--- a/include/chardev/char-mux.h
+++ b/include/chardev/char-mux.h
@@ -55,7 +55,6 @@ typedef struct MuxChardev {
 #define CHARDEV_IS_MUX(chr)                             \
     object_dynamic_cast(OBJECT(chr), TYPE_CHARDEV_MUX)
 
-void mux_chr_set_handlers(Chardev *chr, GMainContext *context);
 void mux_set_focus(Chardev *chr, int focus);
 void mux_chr_send_all_event(Chardev *chr, int event);
 
diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index b1f228e8b5..6ed8bff46a 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -288,10 +288,6 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
             qemu_chr_be_event(s, CHR_EVENT_OPENED);
         }
     }
-
-    if (CHARDEV_IS_MUX(s)) {
-        mux_chr_set_handlers(s, context);
-    }
 }
 
 void qemu_chr_fe_take_focus(CharBackend *b)
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 6055e76293..9406eaf08d 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -278,7 +278,7 @@ static void char_mux_finalize(Object *obj)
     qemu_chr_fe_deinit(&d->chr, false);
 }
 
-void mux_chr_set_handlers(Chardev *chr, GMainContext *context)
+static void mux_chr_update_read_handlers(Chardev *chr)
 {
     MuxChardev *d = MUX_CHARDEV(chr);
 
@@ -289,7 +289,7 @@ void mux_chr_set_handlers(Chardev *chr, GMainContext *context)
                              mux_chr_event,
                              NULL,
                              chr,
-                             context, true);
+                             chr->gcontext, true);
 }
 
 void mux_set_focus(Chardev *chr, int focus)
@@ -383,6 +383,7 @@ static void char_mux_class_init(ObjectClass *oc, void *data)
     cc->chr_add_watch = mux_chr_add_watch;
     cc->chr_be_event = mux_chr_be_event;
     cc->chr_machine_done = open_muxes;
+    cc->chr_update_read_handler = mux_chr_update_read_handlers;
 }
 
 static const TypeInfo char_mux_type_info = {
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 5/9] char-fe: set_handlers() needs an associted chardev

[Marc-André Lureau]

It is futile to call qemu_chr_fe_set_handlers() without an associated
chardev, because the function is doing nothing in that case, not even
reporting an error, it would likely be a programming error. Let's not
handle that hypothetical case.

(fwiw, I introduced the check in commit
94a40fc56036b5058b0b194d9e372a22e65ce7be, that was a mistake imho)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/chardev/char-fe.h | 2 --
 chardev/char-fe.c         | 7 +------
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index 21071f1fb1..4677a9e65a 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -82,8 +82,6 @@ bool qemu_chr_fe_backend_open(CharBackend *be);
  *
  * Set the front end char handlers. The front end takes the focus if
  * any of the handler is non-NULL.
- *
- * Without associated Chardev, nothing is changed.
  */
 void qemu_chr_fe_set_handlers(CharBackend *b,
                               IOCanReadHandler *fd_can_read,
diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index 6ed8bff46a..e3b1c54721 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -254,14 +254,9 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
                               GMainContext *context,
                               bool set_open)
 {
-    Chardev *s;
+    Chardev *s = b->chr;
     int fe_open;
 
-    s = b->chr;
-    if (!s) {
-        return;
-    }
-
     if (!opaque && !fd_can_read && !fd_read && !fd_event) {
         fe_open = 0;
         remove_fd_in_watch(s);
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 6/9] terminal3270: do not use backend timer sources

[Marc-André Lureau]

terminal3270 is uses the front-end side of the chardev. It shouldn't
create sources from backend side context. Fwiw, send_timing_mark_cb
calls qemu_chr_fe_write_all() which should be thread safe.

This partially reverts changes from commit
2c716ba1506769c9be2caa02f0f6d6e7c00f4304.

CC: Peter Xu <peterx@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/char/terminal3270.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
index e9c45e55b1..35b079d5c4 100644
--- a/hw/char/terminal3270.c
+++ b/hw/char/terminal3270.c
@@ -31,7 +31,7 @@ typedef struct Terminal3270 {
     uint8_t outv[OUTPUT_BUFFER_SIZE];
     int in_len;
     bool handshake_done;
-    GSource *timer_src;
+    guint timer_tag;
 } Terminal3270;
 
 #define TYPE_TERMINAL_3270 "x-terminal3270"
@@ -47,10 +47,9 @@ static int terminal_can_read(void *opaque)
 
 static void terminal_timer_cancel(Terminal3270 *t)
 {
-    if (t->timer_src) {
-        g_source_destroy(t->timer_src);
-        g_source_unref(t->timer_src);
-        t->timer_src = NULL;
+    if (t->timer_tag) {
+        g_source_remove(t->timer_tag);
+        t->timer_tag = 0;
     }
 }
 
@@ -100,8 +99,7 @@ static void terminal_read(void *opaque, const uint8_t *buf, int size)
     assert(size <= (INPUT_BUFFER_SIZE - t->in_len));
 
     terminal_timer_cancel(t);
-    t->timer_src = qemu_chr_timeout_add_ms(t->chr.chr, 600 * 1000,
-                                           send_timing_mark_cb, t);
+    t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
     memcpy(&t->inv[t->in_len], buf, size);
     t->in_len += size;
     if (t->in_len < 2) {
@@ -160,8 +158,7 @@ static void chr_event(void *opaque, int event)
          * char-socket.c. Once qemu receives the terminal-type of the
          * client, mark handshake done and trigger everything rolling again.
          */
-        t->timer_src = qemu_chr_timeout_add_ms(t->chr.chr, 600 * 1000,
-                                               send_timing_mark_cb, t);
+        t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
         break;
     case CHR_EVENT_CLOSED:
         sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 7/9] chardev: add a note about frontend sources and context switch

[Marc-André Lureau]

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

diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index 4677a9e65a..25fe62e1cf 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -166,6 +166,9 @@ void qemu_chr_fe_printf(CharBackend *be, const char *fmt, ...)
  * is active; return the #GSource's tag.  If it is disconnected,
  * or without associated Chardev, return 0.
  *
+ * Note that you are responsible to update the front-end sources if
+ * you are switching the main context with qemu_chr_fe_set_handlers().
+ *
  * Returns: the source tag
  */
 guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 8/9] char-pty: remove check for connection on write

[Marc-André Lureau]

This doesn't give much compared to the 1 second timer: there is no
reliable / unracy way to have slave connect & master write. However,
we can simplify the code around chr_write() since the write lock is no
longer needed for various other char-pty callbacks (see following
patch).

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

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index 68fd4e20c3..ebd7035c6d 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -134,11 +134,7 @@ static int char_pty_chr_write(Chardev *chr, const uint8_t *buf, int len)
     PtyChardev *s = PTY_CHARDEV(chr);
 
     if (!s->connected) {
-        /* guest sends data, check for (re-)connect */
-        pty_chr_update_read_handler_locked(chr);
-        if (!s->connected) {
-            return len;
-        }
+        return len;
     }
     return io_channel_send(s->ioc, buf, len);
 }
-- 
2.18.0.547.g1d89318c48

[Qemu-devel] [PATCH 9/9] char-pty: remove write_lock usage

[Marc-André Lureau]

The lock usage was described with its introduction in commit
9005b2a7589540a3733b3abdcfbccfe7746cd1a1. It was necessary because PTY
write() shares more state than GIOChannel with other
operations.

This made char-pty a bit different from other chardev, that only lock
around the write operation.  This was apparent in commit
7b3621f47a990c5099c6385728347f69a8d0e55c, which introduced an idle
source to avoid the lock.

By removing the PTY chardev state sharing on write() with previous
patch, we can remove the lock and the idle source.

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

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index ebd7035c6d..b28bf97233 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -40,15 +40,12 @@ typedef struct {
     QIOChannel *ioc;
     int read_bytes;
 
-    /* Protected by the Chardev chr_write_lock.  */
     int connected;
     GSource *timer_src;
-    GSource *open_source;
 } PtyChardev;
 
 #define PTY_CHARDEV(obj) OBJECT_CHECK(PtyChardev, (obj), TYPE_CHARDEV_PTY)
 
-static void pty_chr_update_read_handler_locked(Chardev *chr);
 static void pty_chr_state(Chardev *chr, int connected);
 
 static void pty_chr_timer_cancel(PtyChardev *s)
@@ -60,32 +57,19 @@ static void pty_chr_timer_cancel(PtyChardev *s)
     }
 }
 
-static void pty_chr_open_src_cancel(PtyChardev *s)
-{
-    if (s->open_source) {
-        g_source_destroy(s->open_source);
-        g_source_unref(s->open_source);
-        s->open_source = NULL;
-    }
-}
-
 static gboolean pty_chr_timer(gpointer opaque)
 {
     struct Chardev *chr = CHARDEV(opaque);
     PtyChardev *s = PTY_CHARDEV(opaque);
 
-    qemu_mutex_lock(&chr->chr_write_lock);
     pty_chr_timer_cancel(s);
-    pty_chr_open_src_cancel(s);
     if (!s->connected) {
         /* Next poll ... */
-        pty_chr_update_read_handler_locked(chr);
+        qemu_chr_be_update_read_handlers(chr, chr->gcontext);
     }
-    qemu_mutex_unlock(&chr->chr_write_lock);
     return FALSE;
 }
 
-/* Called with chr_write_lock held.  */
 static void pty_chr_rearm_timer(Chardev *chr, int ms)
 {
     PtyChardev *s = PTY_CHARDEV(chr);
@@ -98,8 +82,7 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms)
     g_free(name);
 }
 
-/* Called with chr_write_lock held.  */
-static void pty_chr_update_read_handler_locked(Chardev *chr)
+static void pty_chr_update_read_handler(Chardev *chr)
 {
     PtyChardev *s = PTY_CHARDEV(chr);
     GPollFD pfd;
@@ -121,14 +104,6 @@ static void pty_chr_update_read_handler_locked(Chardev *chr)
     }
 }
 
-static void pty_chr_update_read_handler(Chardev *chr)
-{
-    qemu_mutex_lock(&chr->chr_write_lock);
-    pty_chr_update_read_handler_locked(chr);
-    qemu_mutex_unlock(&chr->chr_write_lock);
-}
-
-/* Called with chr_write_lock held.  */
 static int char_pty_chr_write(Chardev *chr, const uint8_t *buf, int len)
 {
     PtyChardev *s = PTY_CHARDEV(chr);
@@ -183,23 +158,11 @@ static gboolean pty_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
     return TRUE;
 }
 
-static gboolean qemu_chr_be_generic_open_func(gpointer opaque)
-{
-    Chardev *chr = CHARDEV(opaque);
-    PtyChardev *s = PTY_CHARDEV(opaque);
-
-    s->open_source = NULL;
-    qemu_chr_be_event(chr, CHR_EVENT_OPENED);
-    return FALSE;
-}
-
-/* Called with chr_write_lock held.  */
 static void pty_chr_state(Chardev *chr, int connected)
 {
     PtyChardev *s = PTY_CHARDEV(chr);
 
     if (!connected) {
-        pty_chr_open_src_cancel(s);
         remove_fd_in_watch(chr);
         s->connected = 0;
         /* (re-)connect poll interval for idle guests: once per second.
@@ -209,13 +172,8 @@ static void pty_chr_state(Chardev *chr, int connected)
     } else {
         pty_chr_timer_cancel(s);
         if (!s->connected) {
-            g_assert(s->open_source == NULL);
-            s->open_source = g_idle_source_new();
             s->connected = 1;
-            g_source_set_callback(s->open_source,
-                                  qemu_chr_be_generic_open_func,
-                                  chr, NULL);
-            g_source_attach(s->open_source, chr->gcontext);
+            qemu_chr_be_event(chr, CHR_EVENT_OPENED);
         }
         if (!chr->gsource) {
             chr->gsource = io_add_watch_poll(chr, s->ioc,
@@ -231,11 +189,9 @@ static void char_pty_finalize(Object *obj)
     Chardev *chr = CHARDEV(obj);
     PtyChardev *s = PTY_CHARDEV(obj);
 
-    qemu_mutex_lock(&chr->chr_write_lock);
     pty_chr_state(chr, 0);
     object_unref(OBJECT(s->ioc));
     pty_chr_timer_cancel(s);
-    qemu_mutex_unlock(&chr->chr_write_lock);
     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
 
-- 
2.18.0.547.g1d89318c48

Re: [Qemu-devel] [PATCH 6/9] terminal3270: do not use backend timer sources

[Peter Xu]

On Tue, Aug 28, 2018 at 12:23:19AM +0200, Marc-André Lureau wrote:
> terminal3270 is uses the front-end side of the chardev. It shouldn't
> create sources from backend side context. Fwiw, send_timing_mark_cb
> calls qemu_chr_fe_write_all() which should be thread safe.
> 
> This partially reverts changes from commit
> 2c716ba1506769c9be2caa02f0f6d6e7c00f4304.

I don't fully understand too on why "It shouldn't create sources from
backend side context".  Could you explain a bit?

If you don't want the backend gcontext to be exposed to the frontend,
IMHO the simplest thing is just to use the NULL gcontext in
qemu_chr_timeout_add_ms() which is a one-liner change. I don't see
much point on re-using the GSource tags since IMHO GSource pointer is
always superior to the old tag system.

> 
> CC: Peter Xu <peterx@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/char/terminal3270.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
> index e9c45e55b1..35b079d5c4 100644
> --- a/hw/char/terminal3270.c
> +++ b/hw/char/terminal3270.c
> @@ -31,7 +31,7 @@ typedef struct Terminal3270 {
>      uint8_t outv[OUTPUT_BUFFER_SIZE];
>      int in_len;
>      bool handshake_done;
> -    GSource *timer_src;
> +    guint timer_tag;
>  } Terminal3270;
>  
>  #define TYPE_TERMINAL_3270 "x-terminal3270"
> @@ -47,10 +47,9 @@ static int terminal_can_read(void *opaque)
>  
>  static void terminal_timer_cancel(Terminal3270 *t)
>  {
> -    if (t->timer_src) {
> -        g_source_destroy(t->timer_src);
> -        g_source_unref(t->timer_src);
> -        t->timer_src = NULL;
> +    if (t->timer_tag) {
> +        g_source_remove(t->timer_tag);
> +        t->timer_tag = 0;
>      }
>  }
>  
> @@ -100,8 +99,7 @@ static void terminal_read(void *opaque, const uint8_t *buf, int size)
>      assert(size <= (INPUT_BUFFER_SIZE - t->in_len));
>  
>      terminal_timer_cancel(t);
> -    t->timer_src = qemu_chr_timeout_add_ms(t->chr.chr, 600 * 1000,
> -                                           send_timing_mark_cb, t);
> +    t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
>      memcpy(&t->inv[t->in_len], buf, size);
>      t->in_len += size;
>      if (t->in_len < 2) {
> @@ -160,8 +158,7 @@ static void chr_event(void *opaque, int event)
>           * char-socket.c. Once qemu receives the terminal-type of the
>           * client, mark handshake done and trigger everything rolling again.
>           */
> -        t->timer_src = qemu_chr_timeout_add_ms(t->chr.chr, 600 * 1000,
> -                                               send_timing_mark_cb, t);
> +        t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
>          break;
>      case CHR_EVENT_CLOSED:
>          sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;
> -- 
> 2.18.0.547.g1d89318c48
> 

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 6/9] terminal3270: do not use backend timer sources

[Marc-André Lureau]

Hi

On Tue, Aug 28, 2018 at 5:52 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Aug 28, 2018 at 12:23:19AM +0200, Marc-André Lureau wrote:
> > terminal3270 is uses the front-end side of the chardev. It shouldn't
> > create sources from backend side context. Fwiw, send_timing_mark_cb
> > calls qemu_chr_fe_write_all() which should be thread safe.
> >
> > This partially reverts changes from commit
> > 2c716ba1506769c9be2caa02f0f6d6e7c00f4304.
>
> I don't fully understand too on why "It shouldn't create sources from
> backend side context".  Could you explain a bit?

A frontend shouldn't use a backend function, and doesn't have to use
the backend gmaincontext.

>
> If you don't want the backend gcontext to be exposed to the frontend,
> IMHO the simplest thing is just to use the NULL gcontext in
> qemu_chr_timeout_add_ms() which is a one-liner change. I don't see
> much point on re-using the GSource tags since IMHO GSource pointer is
> always superior to the old tag system.

Using the tag or the GSource * doesn't change much afaik. Not only
this is not a hot path, but glib gmain code does create the tag on
attach anyway. You may be marginally faster on remove/detach by
avoiding one lookup.

(btw, I think both tag and GSource* are as old)

>
> >
> > CC: Peter Xu <peterx@redhat.com>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  hw/char/terminal3270.c | 15 ++++++---------
> >  1 file changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
> > index e9c45e55b1..35b079d5c4 100644
> > --- a/hw/char/terminal3270.c
> > +++ b/hw/char/terminal3270.c
> > @@ -31,7 +31,7 @@ typedef struct Terminal3270 {
> >      uint8_t outv[OUTPUT_BUFFER_SIZE];
> >      int in_len;
> >      bool handshake_done;
> > -    GSource *timer_src;
> > +    guint timer_tag;
> >  } Terminal3270;
> >
> >  #define TYPE_TERMINAL_3270 "x-terminal3270"
> > @@ -47,10 +47,9 @@ static int terminal_can_read(void *opaque)
> >
> >  static void terminal_timer_cancel(Terminal3270 *t)
> >  {
> > -    if (t->timer_src) {
> > -        g_source_destroy(t->timer_src);
> > -        g_source_unref(t->timer_src);
> > -        t->timer_src = NULL;
> > +    if (t->timer_tag) {
> > +        g_source_remove(t->timer_tag);
> > +        t->timer_tag = 0;
> >      }
> >  }
> >
> > @@ -100,8 +99,7 @@ static void terminal_read(void *opaque, const uint8_t *buf, int size)
> >      assert(size <= (INPUT_BUFFER_SIZE - t->in_len));
> >
> >      terminal_timer_cancel(t);
> > -    t->timer_src = qemu_chr_timeout_add_ms(t->chr.chr, 600 * 1000,
> > -                                           send_timing_mark_cb, t);
> > +    t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
> >      memcpy(&t->inv[t->in_len], buf, size);
> >      t->in_len += size;
> >      if (t->in_len < 2) {
> > @@ -160,8 +158,7 @@ static void chr_event(void *opaque, int event)
> >           * char-socket.c. Once qemu receives the terminal-type of the
> >           * client, mark handshake done and trigger everything rolling again.
> >           */
> > -        t->timer_src = qemu_chr_timeout_add_ms(t->chr.chr, 600 * 1000,
> > -                                               send_timing_mark_cb, t);
> > +        t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
> >          break;
> >      case CHR_EVENT_CLOSED:
> >          sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;
> > --
> > 2.18.0.547.g1d89318c48
> >
>
> Regards,
>
> --
> Peter Xu
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 6/9] terminal3270: do not use backend timer sources

[Peter Xu]

On Tue, Aug 28, 2018 at 11:09:28AM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Aug 28, 2018 at 5:52 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Tue, Aug 28, 2018 at 12:23:19AM +0200, Marc-André Lureau wrote:
> > > terminal3270 is uses the front-end side of the chardev. It shouldn't
> > > create sources from backend side context. Fwiw, send_timing_mark_cb
> > > calls qemu_chr_fe_write_all() which should be thread safe.
> > >
> > > This partially reverts changes from commit
> > > 2c716ba1506769c9be2caa02f0f6d6e7c00f4304.
> >
> > I don't fully understand too on why "It shouldn't create sources from
> > backend side context".  Could you explain a bit?
> 
> A frontend shouldn't use a backend function, and doesn't have to use
> the backend gmaincontext.
> 
> >
> > If you don't want the backend gcontext to be exposed to the frontend,
> > IMHO the simplest thing is just to use the NULL gcontext in
> > qemu_chr_timeout_add_ms() which is a one-liner change. I don't see
> > much point on re-using the GSource tags since IMHO GSource pointer is
> > always superior to the old tag system.
> 
> Using the tag or the GSource * doesn't change much afaik.

I did some convertions in the past year to convert tag -> GSource and
that shows some differences when we want non-default gcontexts...
Otherwise I won't bother. :)

> Not only
> this is not a hot path, but glib gmain code does create the tag on
> attach anyway. You may be marginally faster on remove/detach by
> avoiding one lookup.
> 
> (btw, I think both tag and GSource* are as old)

I am not strong on this, but my rule is that if I don't have explicit
reason to change a code I keep it there.  Here I don't see an obvious
reason to switch back to the tag things (not to mention that I still
think GSource is better, always).

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 6/9] terminal3270: do not use backend timer sources

[Marc-André Lureau]

Hi

On Tue, Aug 28, 2018 at 12:20 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Aug 28, 2018 at 11:09:28AM +0200, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Aug 28, 2018 at 5:52 AM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > On Tue, Aug 28, 2018 at 12:23:19AM +0200, Marc-André Lureau wrote:
> > > > terminal3270 is uses the front-end side of the chardev. It shouldn't
> > > > create sources from backend side context. Fwiw, send_timing_mark_cb
> > > > calls qemu_chr_fe_write_all() which should be thread safe.
> > > >
> > > > This partially reverts changes from commit
> > > > 2c716ba1506769c9be2caa02f0f6d6e7c00f4304.
> > >
> > > I don't fully understand too on why "It shouldn't create sources from
> > > backend side context".  Could you explain a bit?
> >
> > A frontend shouldn't use a backend function, and doesn't have to use
> > the backend gmaincontext.
> >
> > >
> > > If you don't want the backend gcontext to be exposed to the frontend,
> > > IMHO the simplest thing is just to use the NULL gcontext in
> > > qemu_chr_timeout_add_ms() which is a one-liner change. I don't see
> > > much point on re-using the GSource tags since IMHO GSource pointer is
> > > always superior to the old tag system.
> >
> > Using the tag or the GSource * doesn't change much afaik.
>
> I did some convertions in the past year to convert tag -> GSource and
> that shows some differences when we want non-default gcontexts...
> Otherwise I won't bother. :)

It doesn't need a specific context here, does it?

>
> > Not only
> > this is not a hot path, but glib gmain code does create the tag on
> > attach anyway. You may be marginally faster on remove/detach by
> > avoiding one lookup.
> >
> > (btw, I think both tag and GSource* are as old)
>
> I am not strong on this, but my rule is that if I don't have explicit
> reason to change a code I keep it there.  Here I don't see an obvious
> reason to switch back to the tag things (not to mention that I still
> think GSource is better, always).

As I explained, you shouldn't use backend fonctions in the frontend,
and there is no reason to use the backend context. I try to keep all
the sources created around chardev under control, and this one is
incorrect imho.

Using g_timeout_add() is convenient, since it does all the work you do
manually by creating a source, setting callback, attaching, unref etc.
And it uses the default context, which is in general the right thing.

I can do all the manual work and keep the GSource* around, but I don't
think that helps in any way here.


>
> Thanks,
>
> --
> Peter Xu



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 1/9] char.h: fix gtk-doc comment style

[Daniel P. Berrangé]

On Tue, Aug 28, 2018 at 12:23:14AM +0200, Marc-André Lureau wrote:
> Use the gtk-doc function comment style, as documented in:
> https://developer.gnome.org/gtk-doc-manual/stable/documenting_symbols.html.en

I don't think we ever agreed what syntax we are supposed to be using
for inline API docs in QEMU.

I vaguely recall a suggestion that we use the same syntax  that the
kernel uses since they had created some tools that to turn them into
a markup format that was appealing to QEMU for docs integration. I
can't remember the details though.

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

Despite the above caveat, I think this change is better than
what is here already, so

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


> 
> diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
> index c67271f1ba..21071f1fb1 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:
> - *
> - * Read data to a buffer from the back end.
> - *
> + * 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.
> + *
>   * 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:
> - *
> - * Issue a device specific ioctl to a backend.  This function is thread-safe.
> - *
> + * 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.
> + *
>   * 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.18.0.547.g1d89318c48
> 

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: [Qemu-devel] [PATCH 3/9] chardev: use a child source for qio input source

[Daniel P. Berrangé]

On Tue, Aug 28, 2018 at 12:23:16AM +0200, Marc-André Lureau wrote:
> 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>
> ---
>  chardev/char-io.c | 48 +++++------------------------------------------
>  1 file changed, 5 insertions(+), 43 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: [Qemu-devel] [PATCH 1/9] char.h: fix gtk-doc comment style

[Markus Armbruster]

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

> Use the gtk-doc function comment style, as documented in:
> https://developer.gnome.org/gtk-doc-manual/stable/documenting_symbols.html.en

I'm no friend of this style, but these headers use it already, except
they get it wrong in places.  Your subject's "fix" expresses that, but
then your commit message body suggests a conversion to gtk-doc style.
Suggest to replace it by

  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>
> ---
>  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..21071f1fb1 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:

Preexisting: fails to document parameters.  Leaving them undocumented is
acceptable for this style-fix patch.  More of the same below.

>   *
>   * 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.

Preexisting: missing Returns:.  Again, leaving it that way is acceptable
for this style-fix patch.

> @@ -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

Colon after @echo, please.

>   *
>   * 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

Likewise.

>   *
>   * 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

Likewise.

>   *
>   * 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

Likewise.

>   *
>   * 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

Likewise.

>   *
>   * 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);
[...]

With the missing colons fixed, and preferably with the commit message
clarified:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 2/9] chardev: mark the calls that allow an implicit mux monitor

[Markus Armbruster]

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

> 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>
> ---
>  include/chardev/char.h | 24 ++++++++++++++++++++----
>  chardev/char.c         | 32 +++++++++++++++++++++++++-------
>  gdbstub.c              |  6 +++++-
>  hw/char/xen_console.c  |  5 ++++-
>  net/slirp.c            |  5 ++++-
>  vl.c                   | 10 +++++-----
>  6 files changed, 63 insertions(+), 19 deletions(-)
>
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 3e4fe6dad0..268daaa90b 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
> + * @with_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 with_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 with_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..c1b89b6638 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 with_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 (!with_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) {

Perhaps @permit_mux_mon would be a better name.  Your choice.

> @@ -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 with_mux_mon)
>  {
>      const char *p;
>      Chardev *chr;
> @@ -694,25 +700,27 @@ 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, with_mux_mon);
>      if (!opts)
>          return NULL;
>  
>      chr = qemu_chr_new_from_opts(opts, &err);
>      if (err) {
>          error_report_err(err);
> -    }
> -    if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
> +    } else if (qemu_opt_get_bool(opts, "mux", 0)) {
> +        assert(with_mux_mon);
>          monitor_init(chr, MONITOR_USE_READLINE);
>      }
>      qemu_opts_del(opts);
>      return chr;
>  }

Took me a second look to understand.  I'd prefer

       chr = qemu_chr_new_from_opts(opts, &err);
       if (!chr) {
           error_report_err(err);
           goto out;
       }
       if (qemu_opt_get_bool(opts, "mux", 0)) {
           assert(with_mux_mon);
           monitor_init(chr, MONITOR_USE_READLINE);
       }

   out:
       qemu_opts_del(opts);
       return chr;

or

       mux = qemu_opt_get_bool(opts, "mux", 0);
       chr = qemu_chr_new_from_opts(opts, &err);
       qemu_opts_del(opts);
       if (!chr) {
           error_report_err(err);
           return NULL;
       }
       if (mux) {
           monitor_init(chr, MONITOR_USE_READLINE);
       }
       return chr;

Your choice, of course.

>  
> -Chardev *qemu_chr_new(const char *label, const char *filename)
> +static Chardev *qemu_chr_new_with_mux_mon(const char *label,
> +                                          const char *filename,
> +                                          bool with_mux_mon)
>  {
>      Chardev *chr;
> -    chr = qemu_chr_new_noreplay(label, filename);
> +    chr = qemu_chr_new_noreplay(label, filename, with_mux_mon);
>      if (chr) {
>          if (replay_mode != REPLAY_MODE_NONE) {
>              qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_REPLAY);
> @@ -726,6 +734,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_with_mux_mon(label, filename, false);
> +}
> +
> +Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename)
> +{
> +    return qemu_chr_new_with_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..6a231d487b 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -207,7 +207,10 @@ 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: should it support implicit muxed monitors?

This sounds like it didn't, but perhaps it should.  Actually, it does,
but perhaps it shouldn't.  Suggest something like "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 1e14318b4d..677dc36fe4 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -836,7 +836,10 @@ static int slirp_guestfwd(SlirpState *s, const char *config_str,
>          }
>      } else {
>          Error *err = NULL;
> -        Chardev *chr = qemu_chr_new(buf, p);
> +        /*
> +         * FIXME: should it support implicit muxed monitors?
> +         */

Likewise.

> +        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 5ba06adf78..b38e49ca43 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2353,7 +2353,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);
> @@ -2425,7 +2425,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);
> @@ -2461,7 +2461,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);
> @@ -2492,7 +2492,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);
> @@ -2508,7 +2508,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);

I have a couple of suggestions, but nothing to withhold my
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 4/9] char: update the mux hanlders in class callback

[Markus Armbruster]

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

> Instead of handling mux chardev in a special way in
> qemu_chr_fe_set_handlers(), we may use the chr_update_read_handler
> class callback instead.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

In your subject, s/hanlders/handlers/.

Re: [Qemu-devel] [PATCH 5/9] char-fe: set_handlers() needs an associted chardev

[Markus Armbruster]

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

> It is futile to call qemu_chr_fe_set_handlers() without an associated
> chardev, because the function is doing nothing in that case, not even
> reporting an error, it would likely be a programming error. Let's not
> handle that hypothetical case.
>
> (fwiw, I introduced the check in commit
> 94a40fc56036b5058b0b194d9e372a22e65ce7be, that was a mistake imho)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

In your subject, s/associted/associated/.

Re: [Qemu-devel] [PATCH 2/9] chardev: mark the calls that allow an implicit mux monitor

[Marc-André Lureau]

Hi
On Thu, Aug 30, 2018 at 4:58 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > 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>
> > ---
> >  include/chardev/char.h | 24 ++++++++++++++++++++----
> >  chardev/char.c         | 32 +++++++++++++++++++++++++-------
> >  gdbstub.c              |  6 +++++-
> >  hw/char/xen_console.c  |  5 ++++-
> >  net/slirp.c            |  5 ++++-
> >  vl.c                   | 10 +++++-----
> >  6 files changed, 63 insertions(+), 19 deletions(-)
> >
> > diff --git a/include/chardev/char.h b/include/chardev/char.h
> > index 3e4fe6dad0..268daaa90b 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
> > + * @with_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 with_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 with_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..c1b89b6638 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 with_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 (!with_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) {
>
> Perhaps @permit_mux_mon would be a better name.  Your choice.

ok

>
> > @@ -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 with_mux_mon)
> >  {
> >      const char *p;
> >      Chardev *chr;
> > @@ -694,25 +700,27 @@ 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, with_mux_mon);
> >      if (!opts)
> >          return NULL;
> >
> >      chr = qemu_chr_new_from_opts(opts, &err);
> >      if (err) {
> >          error_report_err(err);
> > -    }
> > -    if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
> > +    } else if (qemu_opt_get_bool(opts, "mux", 0)) {
> > +        assert(with_mux_mon);
> >          monitor_init(chr, MONITOR_USE_READLINE);
> >      }
> >      qemu_opts_del(opts);
> >      return chr;
> >  }
>
> Took me a second look to understand.  I'd prefer
>
>        chr = qemu_chr_new_from_opts(opts, &err);
>        if (!chr) {
>            error_report_err(err);
>            goto out;
>        }
>        if (qemu_opt_get_bool(opts, "mux", 0)) {
>            assert(with_mux_mon);
>            monitor_init(chr, MONITOR_USE_READLINE);
>        }
>
>    out:
>        qemu_opts_del(opts);
>        return chr;
>
> or
>
>        mux = qemu_opt_get_bool(opts, "mux", 0);
>        chr = qemu_chr_new_from_opts(opts, &err);
>        qemu_opts_del(opts);
>        if (!chr) {
>            error_report_err(err);
>            return NULL;
>        }
>        if (mux) {
>            monitor_init(chr, MONITOR_USE_READLINE);
>        }
>        return chr;
>
> Your choice, of course.

I prefer the first version, ok

>
> >
> > -Chardev *qemu_chr_new(const char *label, const char *filename)
> > +static Chardev *qemu_chr_new_with_mux_mon(const char *label,
> > +                                          const char *filename,
> > +                                          bool with_mux_mon)
> >  {
> >      Chardev *chr;
> > -    chr = qemu_chr_new_noreplay(label, filename);
> > +    chr = qemu_chr_new_noreplay(label, filename, with_mux_mon);
> >      if (chr) {
> >          if (replay_mode != REPLAY_MODE_NONE) {
> >              qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_REPLAY);
> > @@ -726,6 +734,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_with_mux_mon(label, filename, false);
> > +}
> > +
> > +Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename)
> > +{
> > +    return qemu_chr_new_with_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..6a231d487b 100644
> > --- a/hw/char/xen_console.c
> > +++ b/hw/char/xen_console.c
> > @@ -207,7 +207,10 @@ 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: should it support implicit muxed monitors?
>
> This sounds like it didn't, but perhaps it should.  Actually, it does,
> but perhaps it shouldn't.  Suggest something like "FIXME sure we want to
> support implicit muxed monitors here".
>

ok

> > +                          */
> > +                         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 1e14318b4d..677dc36fe4 100644
> > --- a/net/slirp.c
> > +++ b/net/slirp.c
> > @@ -836,7 +836,10 @@ static int slirp_guestfwd(SlirpState *s, const char *config_str,
> >          }
> >      } else {
> >          Error *err = NULL;
> > -        Chardev *chr = qemu_chr_new(buf, p);
> > +        /*
> > +         * FIXME: should it support implicit muxed monitors?
> > +         */
>
> Likewise.
>
> > +        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 5ba06adf78..b38e49ca43 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2353,7 +2353,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);
> > @@ -2425,7 +2425,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);
> > @@ -2461,7 +2461,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);
> > @@ -2492,7 +2492,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);
> > @@ -2508,7 +2508,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);
>
> I have a couple of suggestions, but nothing to withhold my
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>

thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 4/9] char: update the mux hanlders in class callback

[Marc-André Lureau]

Hi

On Thu, Aug 30, 2018 at 4:58 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Instead of handling mux chardev in a special way in
> > qemu_chr_fe_set_handlers(), we may use the chr_update_read_handler
> > class callback instead.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> In your subject, s/hanlders/handlers/.

oops, fixed

does it get you r-b ?
thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 5/9] char-fe: set_handlers() needs an associted chardev

[Marc-André Lureau]

Hi

On Thu, Aug 30, 2018 at 4:58 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > It is futile to call qemu_chr_fe_set_handlers() without an associated
> > chardev, because the function is doing nothing in that case, not even
> > reporting an error, it would likely be a programming error. Let's not
> > handle that hypothetical case.
> >
> > (fwiw, I introduced the check in commit
> > 94a40fc56036b5058b0b194d9e372a22e65ce7be, that was a mistake imho)
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> In your subject, s/associted/associated/.

oops, fixed

same question as previous patch :)

thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 4/9] char: update the mux hanlders in class callback

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Thu, Aug 30, 2018 at 4:58 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > Instead of handling mux chardev in a special way in
>> > qemu_chr_fe_set_handlers(), we may use the chr_update_read_handler
>> > class callback instead.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> In your subject, s/hanlders/handlers/.
>
> oops, fixed
>
> does it get you r-b ?

I haven't looked at the patch, yet :)

Re: [Qemu-devel] [PATCH 3/9] chardev: use a child source for qio input source

[KONRAD Frederic]

Hi guys,

We have some random delays with gdbstub since this commit under Windows.
Basically while debugging the next command can sometimes take more than 5
seconds while it usually tooks 30ms before. It is not target dependent I see
that happening with sparc, ppc, riscv, etc.. but only on Windows host.

Does that ring a bell?

Cheers,
Fred

Le 8/28/18 à 12:23 AM, Marc-André Lureau a écrit :
> 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>
> ---
>   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;
>       }
>   }
> 

Re: [Qemu-devel] [PATCH 3/9] chardev: use a child source for qio input source

[Marc-André Lureau]

Hi

On Thu, Apr 4, 2019 at 5:18 PM KONRAD Frederic
<frederic.konrad@adacore.com> wrote:
>
> Hi guys,
>
> We have some random delays with gdbstub since this commit under Windows.
> Basically while debugging the next command can sometimes take more than 5
> seconds while it usually tooks 30ms before. It is not target dependent I see
> that happening with sparc, ppc, riscv, etc.. but only on Windows host.
>
> Does that ring a bell?

No, is this with a socket chardev or other kind?

Could you check the chardev behaviour with the HMP monitor for ex:
qemu -chardev socket,id=foo,server,host=localhost,port=9999 -monitor chardev:foo

Is this also taking a few seconds to reply?

Have you checked running tests/test-char on windows? Is this also
taking long (>2sec)?

>
> Cheers,
> Fred
>
> Le 8/28/18 à 12:23 AM, Marc-André Lureau a écrit :
> > 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>
> > ---
> >   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;
> >       }
> >   }
> >

Re: [Qemu-devel] [PATCH 3/9] chardev: use a child source for qio input source

[KONRAD Frederic]

Le 4/4/19 à 5:44 PM, Marc-André Lureau a écrit :
> Hi
> 
> On Thu, Apr 4, 2019 at 5:18 PM KONRAD Frederic
> <frederic.konrad@adacore.com> wrote:
>>
>> Hi guys,
>>
>> We have some random delays with gdbstub since this commit under Windows.
>> Basically while debugging the next command can sometimes take more than 5
>> seconds while it usually tooks 30ms before. It is not target dependent I see
>> that happening with sparc, ppc, riscv, etc.. but only on Windows host.
>>
>> Does that ring a bell?
> 
> No, is this with a socket chardev or other kind?

It's connecting through the TCP socket. '-s -S' option.

> 
> Could you check the chardev behaviour with the HMP monitor for ex:
> qemu -chardev socket,id=foo,server,host=localhost,port=9999 -monitor chardev:foo
> 
> Is this also taking a few seconds to reply?
> 
> Have you checked running tests/test-char on windows? Is this also
> taking long (>2sec)?

I'll try to test that.

Thanks!
Fred

> 
>>
>> Cheers,
>> Fred
>>
>> Le 8/28/18 à 12:23 AM, Marc-André Lureau a écrit :
>>> 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>
>>> ---
>>>    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;
>>>        }
>>>    }
>>>

Re: [Qemu-devel] [PATCH 3/9] chardev: use a child source for qio input source

[KONRAD Frederic]

Hi Marc,

I did more experiments.. It seems to be an issue in glib-2.44.1 which is quite
old. I updated and it seems I don't see the issue anymore.

Thanks for your input on this!

Cheers,
Fred

Le 4/4/19 à 5:49 PM, KONRAD Frederic a écrit :
> 
> 
> Le 4/4/19 à 5:44 PM, Marc-André Lureau a écrit :
>> Hi
>>
>> On Thu, Apr 4, 2019 at 5:18 PM KONRAD Frederic
>> <frederic.konrad@adacore.com> wrote:
>>>
>>> Hi guys,
>>>
>>> We have some random delays with gdbstub since this commit under Windows.
>>> Basically while debugging the next command can sometimes take more than 5
>>> seconds while it usually tooks 30ms before. It is not target dependent I see
>>> that happening with sparc, ppc, riscv, etc.. but only on Windows host.
>>>
>>> Does that ring a bell?
>>
>> No, is this with a socket chardev or other kind?
> 
> It's connecting through the TCP socket. '-s -S' option.
> 
>>
>> Could you check the chardev behaviour with the HMP monitor for ex:
>> qemu -chardev socket,id=foo,server,host=localhost,port=9999 -monitor chardev:foo
>>
>> Is this also taking a few seconds to reply?
>>
>> Have you checked running tests/test-char on windows? Is this also
>> taking long (>2sec)?
> 
> I'll try to test that.
> 
> Thanks!
> Fred
> 

Re: [Qemu-devel] [PATCH 3/9] chardev: use a child source for qio input source

[KONRAD Frederic]

Hi Marc,

I did more experiments.. It seems to be an issue in glib-2.44.1 which is quite
old. I updated and it seems I don't see the issue anymore.

Thanks for your input on this!

Cheers,
Fred

Le 4/4/19 à 5:49 PM, KONRAD Frederic a écrit :
> 
> 
> Le 4/4/19 à 5:44 PM, Marc-André Lureau a écrit :
>> Hi
>>
>> On Thu, Apr 4, 2019 at 5:18 PM KONRAD Frederic
>> <frederic.konrad@adacore.com> wrote:
>>>
>>> Hi guys,
>>>
>>> We have some random delays with gdbstub since this commit under Windows.
>>> Basically while debugging the next command can sometimes take more than 5
>>> seconds while it usually tooks 30ms before. It is not target dependent I see
>>> that happening with sparc, ppc, riscv, etc.. but only on Windows host.
>>>
>>> Does that ring a bell?
>>
>> No, is this with a socket chardev or other kind?
> 
> It's connecting through the TCP socket. '-s -S' option.
> 
>>
>> Could you check the chardev behaviour with the HMP monitor for ex:
>> qemu -chardev socket,id=foo,server,host=localhost,port=9999 -monitor chardev:foo
>>
>> Is this also taking a few seconds to reply?
>>
>> Have you checked running tests/test-char on windows? Is this also
>> taking long (>2sec)?
> 
> I'll try to test that.
> 
> Thanks!
> Fred
>