[Qemu-devel] [PATCH] RFC: 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.

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

Less obvious and questionnable ones are -gdb 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 preliminary checks on the string (such as slirp), or do
not need it, such as -qtest.

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 <address@hidden>
---
 include/chardev/char.h | 18 +++++++++++++++++-
 chardev/char.c         | 21 +++++++++++++++++----
 gdbstub.c              |  6 +++++-
 hw/char/xen_console.c  |  5 ++++-
 vl.c                   |  8 ++++----
 5 files changed, 47 insertions(+), 11 deletions(-)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index 6f0576e214..be2b9b817e 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -105,6 +105,7 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
  * @qemu_chr_new:
  *
  * Create a new character backend from a URI.
+ * Do not implicitely initialize a monitor if the chardev is muxed.
  *
  * @label the name of the backend
  * @filename the URI
@@ -113,6 +114,19 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
  */
 Chardev *qemu_chr_new(const char *label, const char *filename);
 
+/**
+ * @qemu_chr_new_mux_mon:
+ *
+ * Create a new character backend from a URI.
+ * Implicitely initialize a monitor if the chardev is muxed.
+ *
+ * @label the name of the backend
+ * @filename the URI
+ *
+ * Returns: a new character backend
+ */
+Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename);
+
 /**
  * @qemu_chr_change:
  *
@@ -138,10 +152,12 @@ void qemu_chr_cleanup(void);
  *
  * @label the name of the backend
  * @filename the URI
+ * @with_mux_mon if chardev is muxed, initialize a monitor
  *
  * 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:
diff --git a/chardev/char.c b/chardev/char.c
index 76d866e6fe..2c295ad676 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -683,7 +683,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;
@@ -702,17 +703,19 @@ Chardev *qemu_chr_new_noreplay(const char *label, const 
char *filename)
     if (err) {
         error_report_err(err);
     }
-    if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
+    if (with_mux_mon && chr && qemu_opt_get_bool(opts, "mux", 0)) {
         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 +729,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..963531ea90 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 setup implicitely 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/vl.c b/vl.c
index 16b913f9d5..20b5f321ec 100644
--- a/vl.c
+++ b/vl.c
@@ -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