cur_mon really needs to be coroutine-local as soon as we move monitor
command handlers to coroutines and let them yield. As a first step, just
remove all direct accesses to cur_mon so that we can implement this in
the getter function later.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/monitor/monitor.h  |  3 ++-
 audio/wavcapture.c         |  8 ++++----
 dump/dump.c                |  2 +-
 hw/scsi/vhost-scsi.c       |  2 +-
 hw/virtio/vhost-vsock.c    |  2 +-
 migration/fd.c             |  4 ++--
 monitor/hmp.c              | 10 +++++-----
 monitor/misc.c             | 14 +++++++++-----
 monitor/monitor.c          | 15 ++++++++++++++-
 monitor/qmp-cmds-control.c |  2 ++
 monitor/qmp-cmds.c         |  2 +-
 monitor/qmp.c              |  6 +++---
 net/socket.c               |  2 +-
 net/tap.c                  |  6 +++---
 stubs/monitor-core.c       |  5 ++++-
 tests/test-util-sockets.c  | 22 +++++++++++-----------
 trace/control.c            |  2 +-
 util/qemu-error.c          |  4 ++--
 util/qemu-print.c          |  3 ++-
 util/qemu-sockets.c        |  1 +
 20 files changed, 70 insertions(+), 45 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 0dcaefd4f9..43cc746078 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -5,7 +5,6 @@
 #include "qapi/qapi-types-misc.h"
 #include "qemu/readline.h"
 
-extern __thread Monitor *cur_mon;
 typedef struct MonitorHMP MonitorHMP;
 typedef struct MonitorOptions MonitorOptions;
 
@@ -13,6 +12,8 @@ typedef struct MonitorOptions MonitorOptions;
 
 extern QemuOptsList qemu_mon_opts;
 
+Monitor *monitor_cur(void);
+void monitor_set_cur(Monitor *mon);
 bool monitor_cur_is_qmp(void);
 
 void monitor_init_globals(void);
diff --git a/audio/wavcapture.c b/audio/wavcapture.c
index 8d7ce2eda1..e7dc97d16e 100644
--- a/audio/wavcapture.c
+++ b/audio/wavcapture.c
@@ -1,5 +1,5 @@
 #include "qemu/osdep.h"
-#include "monitor/monitor.h"
+#include "qemu/qemu-print.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "audio.h"
@@ -94,9 +94,9 @@ static void wav_capture_info (void *opaque)
     WAVState *wav = opaque;
     char *path = wav->path;
 
-    monitor_printf (cur_mon, "Capturing audio(%d,%d,%d) to %s: %d bytes\n",
-                    wav->freq, wav->bits, wav->nchannels,
-                    path ? path : "<not available>", wav->bytes);
+    qemu_printf("Capturing audio(%d,%d,%d) to %s: %d bytes\n",
+                wav->freq, wav->bits, wav->nchannels,
+                path ? path : "<not available>", wav->bytes);
 }
 
 static struct capture_ops wav_capture_ops = {
diff --git a/dump/dump.c b/dump/dump.c
index 248ea06370..36d26159a0 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1989,7 +1989,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
 
 #if !defined(WIN32)
     if (strstart(file, "fd:", &p)) {
-        fd = monitor_get_fd(cur_mon, p, errp);
+        fd = monitor_get_fd(monitor_cur(), p, errp);
         if (fd == -1) {
             return;
         }
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index c1b012aea4..3920825bd6 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -177,7 +177,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
     }
 
     if (vs->conf.vhostfd) {
-        vhostfd = monitor_fd_param(cur_mon, vs->conf.vhostfd, errp);
+        vhostfd = monitor_fd_param(monitor_cur(), vs->conf.vhostfd, errp);
         if (vhostfd == -1) {
             error_prepend(errp, "vhost-scsi: unable to parse vhostfd: ");
             return;
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 4a228f5168..e72c9005b4 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -317,7 +317,7 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
     }
 
     if (vsock->conf.vhostfd) {
-        vhostfd = monitor_fd_param(cur_mon, vsock->conf.vhostfd, errp);
+        vhostfd = monitor_fd_param(monitor_cur(), vsock->conf.vhostfd, errp);
         if (vhostfd == -1) {
             error_prepend(errp, "vhost-vsock: unable to parse vhostfd: ");
             return;
diff --git a/migration/fd.c b/migration/fd.c
index 0a29ecdebf..6f2f50475f 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -26,7 +26,7 @@
 void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
 {
     QIOChannel *ioc;
-    int fd = monitor_get_fd(cur_mon, fdname, errp);
+    int fd = monitor_get_fd(monitor_cur(), fdname, errp);
     if (fd == -1) {
         return;
     }
@@ -55,7 +55,7 @@ static gboolean fd_accept_incoming_migration(QIOChannel *ioc,
 void fd_start_incoming_migration(const char *fdname, Error **errp)
 {
     QIOChannel *ioc;
-    int fd = monitor_fd_param(cur_mon, fdname, errp);
+    int fd = monitor_fd_param(monitor_cur(), fdname, errp);
     if (fd == -1) {
         return;
     }
diff --git a/monitor/hmp.c b/monitor/hmp.c
index d598dd02bb..f609fcf75b 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1301,11 +1301,11 @@ cleanup:
 static void monitor_read(void *opaque, const uint8_t *buf, int size)
 {
     MonitorHMP *mon;
-    Monitor *old_mon = cur_mon;
+    Monitor *old_mon = monitor_cur();
     int i;
 
-    cur_mon = opaque;
-    mon = container_of(cur_mon, MonitorHMP, common);
+    monitor_set_cur(opaque);
+    mon = container_of(monitor_cur(), MonitorHMP, common);
 
     if (mon->rs) {
         for (i = 0; i < size; i++) {
@@ -1313,13 +1313,13 @@ static void monitor_read(void *opaque, const uint8_t *buf, int size)
         }
     } else {
         if (size == 0 || buf[size - 1] != 0) {
-            monitor_printf(cur_mon, "corrupted command\n");
+            monitor_printf(&mon->common, "corrupted command\n");
         } else {
             handle_hmp_command(mon, (char *)buf);
         }
     }
 
-    cur_mon = old_mon;
+    monitor_set_cur(old_mon);
 }
 
 static void monitor_event(void *opaque, QEMUChrEvent event)
diff --git a/monitor/misc.c b/monitor/misc.c
index bdf49e49e5..6cf7f60872 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -126,13 +126,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
 
     monitor_data_init(&hmp.common, false, true, false);
 
-    old_mon = cur_mon;
-    cur_mon = &hmp.common;
+    old_mon = monitor_cur();
+    monitor_set_cur(&hmp.common);
 
     if (has_cpu_index) {
         int ret = monitor_set_cpu(&hmp.common, cpu_index);
         if (ret < 0) {
-            cur_mon = old_mon;
+            monitor_set_cur(old_mon);
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
                        "a CPU number");
             goto out;
@@ -140,7 +140,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
     }
 
     handle_hmp_command(&hmp, command_line);
-    cur_mon = old_mon;
+    monitor_set_cur(old_mon);
 
     qemu_mutex_lock(&hmp.common.mon_lock);
     if (qstring_get_length(hmp.common.outbuf) > 0) {
@@ -258,6 +258,7 @@ static void monitor_init_qmp_commands(void)
 /* Set the current CPU defined by the user. Callers must hold BQL. */
 int monitor_set_cpu(Monitor *mon, int cpu_index)
 {
+    Monitor *cur_mon = monitor_cur();
     CPUState *cpu;
 
     cpu = qemu_get_cpu(cpu_index);
@@ -272,6 +273,7 @@ int monitor_set_cpu(Monitor *mon, int cpu_index)
 /* Callers must hold BQL. */
 static CPUState *mon_get_cpu_sync(bool synchronize)
 {
+    Monitor *cur_mon = monitor_cur();
     CPUState *cpu = NULL;
 
     if (cur_mon->mon_cpu_path) {
@@ -1232,6 +1234,7 @@ static void hmp_acl_remove(Monitor *mon, const QDict *qdict)
 
 void qmp_getfd(const char *fdname, Error **errp)
 {
+    Monitor *cur_mon = monitor_cur();
     mon_fd_t *monfd;
     int fd, tmp_fd;
 
@@ -1272,6 +1275,7 @@ void qmp_getfd(const char *fdname, Error **errp)
 
 void qmp_closefd(const char *fdname, Error **errp)
 {
+    Monitor *cur_mon = monitor_cur();
     mon_fd_t *monfd;
     int tmp_fd;
 
@@ -1361,7 +1365,7 @@ AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque,
                       const char *opaque, Error **errp)
 {
     int fd;
-    Monitor *mon = cur_mon;
+    Monitor *mon = monitor_cur();
     AddfdInfo *fdinfo;
 
     fd = qemu_chr_fe_get_msgfd(&mon->chr);
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 125494410a..182ba136b4 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -66,13 +66,24 @@ MonitorList mon_list;
 int mon_refcount;
 static bool monitor_destroyed;
 
-__thread Monitor *cur_mon;
+static __thread Monitor *cur_monitor;
+
+Monitor *monitor_cur(void)
+{
+    return cur_monitor;
+}
+
+void monitor_set_cur(Monitor *mon)
+{
+    cur_monitor = mon;
+}
 
 /**
  * Is the current monitor, if any, a QMP monitor?
  */
 bool monitor_cur_is_qmp(void)
 {
+    Monitor *cur_mon = monitor_cur();
     return cur_mon && monitor_is_qmp(cur_mon);
 }
 
@@ -209,6 +220,7 @@ int monitor_printf(Monitor *mon, const char *fmt, ...)
  */
 int error_vprintf(const char *fmt, va_list ap)
 {
+    Monitor *cur_mon = monitor_cur();
     if (cur_mon && !monitor_cur_is_qmp()) {
         return monitor_vprintf(cur_mon, fmt, ap);
     }
@@ -217,6 +229,7 @@ int error_vprintf(const char *fmt, va_list ap)
 
 int error_vprintf_unless_qmp(const char *fmt, va_list ap)
 {
+    Monitor *cur_mon = monitor_cur();
     if (!cur_mon) {
         return vfprintf(stderr, fmt, ap);
     }
diff --git a/monitor/qmp-cmds-control.c b/monitor/qmp-cmds-control.c
index 8f04cfa6e6..a456762f6a 100644
--- a/monitor/qmp-cmds-control.c
+++ b/monitor/qmp-cmds-control.c
@@ -69,6 +69,7 @@ static bool qmp_caps_accept(MonitorQMP *mon, QMPCapabilityList *list,
 void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
                           Error **errp)
 {
+    Monitor *cur_mon = monitor_cur();
     MonitorQMP *mon;
 
     assert(monitor_is_qmp(cur_mon));
@@ -119,6 +120,7 @@ static void query_commands_cb(const QmpCommand *cmd, void *opaque)
 CommandInfoList *qmp_query_commands(Error **errp)
 {
     CommandInfoList *list = NULL;
+    Monitor *cur_mon = monitor_cur();
     MonitorQMP *mon;
 
     assert(monitor_is_qmp(cur_mon));
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 864cbfa32e..c7bf6bb4dc 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -327,7 +327,7 @@ void qmp_add_client(const char *protocol, const char *fdname,
     Chardev *s;
     int fd;
 
-    fd = monitor_get_fd(cur_mon, fdname, errp);
+    fd = monitor_get_fd(monitor_cur(), fdname, errp);
     if (fd < 0) {
         return;
     }
diff --git a/monitor/qmp.c b/monitor/qmp.c
index d433ceae5b..5e9abd4711 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -139,12 +139,12 @@ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
     QDict *rsp;
     QDict *error;
 
-    old_mon = cur_mon;
-    cur_mon = &mon->common;
+    old_mon = monitor_cur();
+    monitor_set_cur(&mon->common);
 
     rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon));
 
-    cur_mon = old_mon;
+    monitor_set_cur(old_mon);
 
     if (mon->commands == &qmp_cap_negotiation_commands) {
         error = qdict_get_qdict(rsp, "error");
diff --git a/net/socket.c b/net/socket.c
index c92354049b..93bee968a8 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -727,7 +727,7 @@ int net_init_socket(const Netdev *netdev, const char *name,
     if (sock->has_fd) {
         int fd;
 
-        fd = monitor_fd_param(cur_mon, sock->fd, errp);
+        fd = monitor_fd_param(monitor_cur(), sock->fd, errp);
         if (fd == -1) {
             return -1;
         }
diff --git a/net/tap.c b/net/tap.c
index 6207f61f84..fd7d15936d 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -689,7 +689,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
         }
 
         if (vhostfdname) {
-            vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
+            vhostfd = monitor_fd_param(monitor_cur(), vhostfdname, &err);
             if (vhostfd == -1) {
                 if (tap->has_vhostforce && tap->vhostforce) {
                     error_propagate(errp, err);
@@ -789,7 +789,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
             return -1;
         }
 
-        fd = monitor_fd_param(cur_mon, tap->fd, &err);
+        fd = monitor_fd_param(monitor_cur(), tap->fd, &err);
         if (fd == -1) {
             error_propagate(errp, err);
             return -1;
@@ -836,7 +836,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
         }
 
         for (i = 0; i < nfds; i++) {
-            fd = monitor_fd_param(cur_mon, fds[i], &err);
+            fd = monitor_fd_param(monitor_cur(), fds[i], &err);
             if (fd == -1) {
                 error_propagate(errp, err);
                 ret = -1;
diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
index 6cff1c4e1d..0cd2d864b2 100644
--- a/stubs/monitor-core.c
+++ b/stubs/monitor-core.c
@@ -3,7 +3,10 @@
 #include "qemu-common.h"
 #include "qapi/qapi-emit-events.h"
 
-__thread Monitor *cur_mon;
+Monitor *monitor_cur(void)
+{
+    return NULL;
+}
 
 void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
 {
diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index 2ca1e99f17..36fabb5e46 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -53,27 +53,27 @@ static void test_fd_is_socket_good(void)
 static int mon_fd = -1;
 static const char *mon_fdname;
 
-int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
-{
-    g_assert(cur_mon);
-    g_assert(mon == cur_mon);
-    if (mon_fd == -1 || !g_str_equal(mon_fdname, fdname)) {
-        error_setg(errp, "No fd named %s", fdname);
-        return -1;
-    }
-    return dup(mon_fd);
-}
-
 /* Syms in libqemustub.a are discarded at .o file granularity.
  * To replace monitor_get_fd() we must ensure everything in
  * stubs/monitor.c is defined, to make sure monitor.o is discarded
  * otherwise we get duplicate syms at link time.
  */
 __thread Monitor *cur_mon;
+Monitor *monitor_cur(void) { return cur_mon; }
 int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) { abort(); }
 void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp) {}
 void monitor_init_hmp(Chardev *chr, bool use_readline, Error **errp) {}
 
+int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
+{
+    g_assert(cur_mon);
+    g_assert(mon == cur_mon);
+    if (mon_fd == -1 || !g_str_equal(mon_fdname, fdname)) {
+        error_setg(errp, "No fd named %s", fdname);
+        return -1;
+    }
+    return dup(mon_fd);
+}
 
 static void test_socket_fd_pass_name_good(void)
 {
diff --git a/trace/control.c b/trace/control.c
index 2ffe000818..62993daf64 100644
--- a/trace/control.c
+++ b/trace/control.c
@@ -176,7 +176,7 @@ void trace_enable_events(const char *line_buf)
 {
     if (is_help_option(line_buf)) {
         trace_list_events();
-        if (cur_mon == NULL) {
+        if (monitor_cur() == NULL) {
             exit(0);
         }
     } else {
diff --git a/util/qemu-error.c b/util/qemu-error.c
index dac7c7dc50..8d4ed723f5 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -169,7 +169,7 @@ static void print_loc(void)
     int i;
     const char *const *argp;
 
-    if (!cur_mon && progname) {
+    if (!monitor_cur() && progname) {
         fprintf(stderr, "%s:", progname);
         sep = " ";
     }
@@ -206,7 +206,7 @@ static void vreport(report_type type, const char *fmt, va_list ap)
     GTimeVal tv;
     gchar *timestr;
 
-    if (error_with_timestamp && !cur_mon) {
+    if (error_with_timestamp && !monitor_cur()) {
         g_get_current_time(&tv);
         timestr = g_time_val_to_iso8601(&tv);
         error_printf("%s ", timestr);
diff --git a/util/qemu-print.c b/util/qemu-print.c
index e79d6b8396..69ba612f56 100644
--- a/util/qemu-print.c
+++ b/util/qemu-print.c
@@ -20,6 +20,7 @@
  */
 int qemu_vprintf(const char *fmt, va_list ap)
 {
+    Monitor *cur_mon = monitor_cur();
     if (cur_mon) {
         return monitor_vprintf(cur_mon, fmt, ap);
     }
@@ -48,7 +49,7 @@ int qemu_printf(const char *fmt, ...)
 int qemu_vfprintf(FILE *stream, const char *fmt, va_list ap)
 {
     if (!stream) {
-        return monitor_vprintf(cur_mon, fmt, ap);
+        return monitor_vprintf(monitor_cur(), fmt, ap);
     }
     return vfprintf(stream, fmt, ap);
 }
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index b37d288866..40c18ba142 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1092,6 +1092,7 @@ fail:
 
 static int socket_get_fd(const char *fdstr, int num, Error **errp)
 {
+    Monitor *cur_mon = monitor_cur();
     int fd;
     if (num != 1) {
         error_setg_errno(errp, EINVAL, "socket_get_fd: too many connections");
-- 
2.25.4

On 5/28/20 10:37 AM, Kevin Wolf wrote:
> cur_mon really needs to be coroutine-local as soon as we move monitor
> command handlers to coroutines and let them yield. As a first step, just
> remove all direct accesses to cur_mon so that we can implement this in
> the getter function later.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---


> +++ b/audio/wavcapture.c
> @@ -1,5 +1,5 @@
>   #include "qemu/osdep.h"
> -#include "monitor/monitor.h"
> +#include "qemu/qemu-print.h"
>   #include "qapi/error.h"
>   #include "qemu/error-report.h"
>   #include "audio.h"
> @@ -94,9 +94,9 @@ static void wav_capture_info (void *opaque)
>       WAVState *wav = opaque;
>       char *path = wav->path;
>   
> -    monitor_printf (cur_mon, "Capturing audio(%d,%d,%d) to %s: %d bytes\n",
> -                    wav->freq, wav->bits, wav->nchannels,
> -                    path ? path : "<not available>", wav->bytes);
> +    qemu_printf("Capturing audio(%d,%d,%d) to %s: %d bytes\n",
> +                wav->freq, wav->bits, wav->nchannels,
> +                path ? path : "<not available>", wav->bytes);

Why the change to qemu_printf() here instead of a call to monitor_cur() 
as is done in the rest of the patch?

> +++ b/stubs/monitor-core.c
> @@ -3,7 +3,10 @@
>   #include "qemu-common.h"
>   #include "qapi/qapi-emit-events.h"
>   
> -__thread Monitor *cur_mon;
> +Monitor *monitor_cur(void)
> +{
> +    return NULL;
> +}
>   

monitor_set_cur() didn't need a stub?  Odd, but I guess it's okay.

Otherwise the patch looks fine to me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Am 28.05.2020 um 20:31 hat Eric Blake geschrieben:
> On 5/28/20 10:37 AM, Kevin Wolf wrote:
> > cur_mon really needs to be coroutine-local as soon as we move monitor
> > command handlers to coroutines and let them yield. As a first step, just
> > remove all direct accesses to cur_mon so that we can implement this in
> > the getter function later.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> 
> 
> > +++ b/audio/wavcapture.c
> > @@ -1,5 +1,5 @@
> >   #include "qemu/osdep.h"
> > -#include "monitor/monitor.h"
> > +#include "qemu/qemu-print.h"
> >   #include "qapi/error.h"
> >   #include "qemu/error-report.h"
> >   #include "audio.h"
> > @@ -94,9 +94,9 @@ static void wav_capture_info (void *opaque)
> >       WAVState *wav = opaque;
> >       char *path = wav->path;
> > -    monitor_printf (cur_mon, "Capturing audio(%d,%d,%d) to %s: %d bytes\n",
> > -                    wav->freq, wav->bits, wav->nchannels,
> > -                    path ? path : "<not available>", wav->bytes);
> > +    qemu_printf("Capturing audio(%d,%d,%d) to %s: %d bytes\n",
> > +                wav->freq, wav->bits, wav->nchannels,
> > +                path ? path : "<not available>", wav->bytes);
> 
> Why the change to qemu_printf() here instead of a call to
> monitor_cur() as is done in the rest of the patch?

There was actually only one other monitor_printf() call for cur_mon, and
that was in the monitor core, so I think there's no reason to switch
that one.

If you prefer, I can call monitor_cur() directly here, but I thought not
calling it from everywhere when there is already a wrapper like
qemu_printf() would be a good thing.

Maybe wav_capture_info() should actually be passed a Monitor object so
we don't need either.

> > +++ b/stubs/monitor-core.c
> > @@ -3,7 +3,10 @@
> >   #include "qemu-common.h"
> >   #include "qapi/qapi-emit-events.h"
> > -__thread Monitor *cur_mon;
> > +Monitor *monitor_cur(void)
> > +{
> > +    return NULL;
> > +}
> 
> monitor_set_cur() didn't need a stub?  Odd, but I guess it's okay.

Not that odd, only the code to handle monitor commands ever sets the
current monitor. But if you use the stubs, you don't have a monitor.

Kevin

On 5/28/20 10:37 AM, Kevin Wolf wrote:
> cur_mon really needs to be coroutine-local as soon as we move monitor
> command handlers to coroutines and let them yield. As a first step, just
> remove all direct accesses to cur_mon so that we can implement this in
> the getter function later.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

> +++ b/monitor/misc.c

> @@ -258,6 +258,7 @@ static void monitor_init_qmp_commands(void)
>   /* Set the current CPU defined by the user. Callers must hold BQL. */
>   int monitor_set_cpu(Monitor *mon, int cpu_index)
>   {
> +    Monitor *cur_mon = monitor_cur();
>       CPUState *cpu;
>   
>       cpu = qemu_get_cpu(cpu_index);

Bogus hunk, after patch 1/12.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Kevin Wolf <kwolf@redhat.com> writes:

> cur_mon really needs to be coroutine-local as soon as we move monitor
> command handlers to coroutines and let them yield. As a first step, just
> remove all direct accesses to cur_mon so that we can implement this in
> the getter function later.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/monitor/monitor.h  |  3 ++-
>  audio/wavcapture.c         |  8 ++++----
>  dump/dump.c                |  2 +-
>  hw/scsi/vhost-scsi.c       |  2 +-
>  hw/virtio/vhost-vsock.c    |  2 +-
>  migration/fd.c             |  4 ++--
>  monitor/hmp.c              | 10 +++++-----
>  monitor/misc.c             | 14 +++++++++-----
>  monitor/monitor.c          | 15 ++++++++++++++-
>  monitor/qmp-cmds-control.c |  2 ++
>  monitor/qmp-cmds.c         |  2 +-
>  monitor/qmp.c              |  6 +++---
>  net/socket.c               |  2 +-
>  net/tap.c                  |  6 +++---
>  stubs/monitor-core.c       |  5 ++++-
>  tests/test-util-sockets.c  | 22 +++++++++++-----------
>  trace/control.c            |  2 +-
>  util/qemu-error.c          |  4 ++--
>  util/qemu-print.c          |  3 ++-
>  util/qemu-sockets.c        |  1 +
>  20 files changed, 70 insertions(+), 45 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 0dcaefd4f9..43cc746078 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -5,7 +5,6 @@
>  #include "qapi/qapi-types-misc.h"
>  #include "qemu/readline.h"
>  
> -extern __thread Monitor *cur_mon;
>  typedef struct MonitorHMP MonitorHMP;
>  typedef struct MonitorOptions MonitorOptions;
>  
> @@ -13,6 +12,8 @@ typedef struct MonitorOptions MonitorOptions;
>  
>  extern QemuOptsList qemu_mon_opts;
>  
> +Monitor *monitor_cur(void);
> +void monitor_set_cur(Monitor *mon);
>  bool monitor_cur_is_qmp(void);
>  
>  void monitor_init_globals(void);
> diff --git a/audio/wavcapture.c b/audio/wavcapture.c
> index 8d7ce2eda1..e7dc97d16e 100644
> --- a/audio/wavcapture.c
> +++ b/audio/wavcapture.c
> @@ -1,5 +1,5 @@
>  #include "qemu/osdep.h"
> -#include "monitor/monitor.h"
> +#include "qemu/qemu-print.h"
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "audio.h"
> @@ -94,9 +94,9 @@ static void wav_capture_info (void *opaque)
>      WAVState *wav = opaque;
>      char *path = wav->path;
>  
> -    monitor_printf (cur_mon, "Capturing audio(%d,%d,%d) to %s: %d bytes\n",
> -                    wav->freq, wav->bits, wav->nchannels,
> -                    path ? path : "<not available>", wav->bytes);
> +    qemu_printf("Capturing audio(%d,%d,%d) to %s: %d bytes\n",
> +                wav->freq, wav->bits, wav->nchannels,
> +                path ? path : "<not available>", wav->bytes);
>  }
>  

A bit more than purely mechanical transformation.  Fine with me.

>  static struct capture_ops wav_capture_ops = {
> diff --git a/dump/dump.c b/dump/dump.c
> index 248ea06370..36d26159a0 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1989,7 +1989,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>  
>  #if !defined(WIN32)
>      if (strstart(file, "fd:", &p)) {
> -        fd = monitor_get_fd(cur_mon, p, errp);
> +        fd = monitor_get_fd(monitor_cur(), p, errp);

All callers pass cur_mon.  Perhaps we'd be better off without the
parameter.  Observation, not demand.

>          if (fd == -1) {
>              return;
>          }
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index c1b012aea4..3920825bd6 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -177,7 +177,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>      }
>  
>      if (vs->conf.vhostfd) {
> -        vhostfd = monitor_fd_param(cur_mon, vs->conf.vhostfd, errp);
> +        vhostfd = monitor_fd_param(monitor_cur(), vs->conf.vhostfd, errp);

Likewise.

>          if (vhostfd == -1) {
>              error_prepend(errp, "vhost-scsi: unable to parse vhostfd: ");
>              return;
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index 4a228f5168..e72c9005b4 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -317,7 +317,7 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
>      }
>  
>      if (vsock->conf.vhostfd) {
> -        vhostfd = monitor_fd_param(cur_mon, vsock->conf.vhostfd, errp);
> +        vhostfd = monitor_fd_param(monitor_cur(), vsock->conf.vhostfd, errp);
>          if (vhostfd == -1) {
>              error_prepend(errp, "vhost-vsock: unable to parse vhostfd: ");
>              return;
> diff --git a/migration/fd.c b/migration/fd.c
> index 0a29ecdebf..6f2f50475f 100644
> --- a/migration/fd.c
> +++ b/migration/fd.c
> @@ -26,7 +26,7 @@
>  void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
>  {
>      QIOChannel *ioc;
> -    int fd = monitor_get_fd(cur_mon, fdname, errp);
> +    int fd = monitor_get_fd(monitor_cur(), fdname, errp);
>      if (fd == -1) {
>          return;
>      }
> @@ -55,7 +55,7 @@ static gboolean fd_accept_incoming_migration(QIOChannel *ioc,
>  void fd_start_incoming_migration(const char *fdname, Error **errp)
>  {
>      QIOChannel *ioc;
> -    int fd = monitor_fd_param(cur_mon, fdname, errp);
> +    int fd = monitor_fd_param(monitor_cur(), fdname, errp);
>      if (fd == -1) {
>          return;
>      }
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index d598dd02bb..f609fcf75b 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -1301,11 +1301,11 @@ cleanup:
>  static void monitor_read(void *opaque, const uint8_t *buf, int size)
>  {
>      MonitorHMP *mon;
> -    Monitor *old_mon = cur_mon;
> +    Monitor *old_mon = monitor_cur();
>      int i;
>  
> -    cur_mon = opaque;
> -    mon = container_of(cur_mon, MonitorHMP, common);
> +    monitor_set_cur(opaque);
> +    mon = container_of(monitor_cur(), MonitorHMP, common);

Simpler:

       MonitorHMP *mon = container_of(opaque, MonitorHMP, common);

>  
>      if (mon->rs) {
>          for (i = 0; i < size; i++) {
> @@ -1313,13 +1313,13 @@ static void monitor_read(void *opaque, const uint8_t *buf, int size)
>          }
>      } else {
>          if (size == 0 || buf[size - 1] != 0) {
> -            monitor_printf(cur_mon, "corrupted command\n");
> +            monitor_printf(&mon->common, "corrupted command\n");
>          } else {
>              handle_hmp_command(mon, (char *)buf);
>          }
>      }
>  
> -    cur_mon = old_mon;
> +    monitor_set_cur(old_mon);
>  }
>  
>  static void monitor_event(void *opaque, QEMUChrEvent event)
> diff --git a/monitor/misc.c b/monitor/misc.c
> index bdf49e49e5..6cf7f60872 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -126,13 +126,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
>  
>      monitor_data_init(&hmp.common, false, true, false);
>  
> -    old_mon = cur_mon;
> -    cur_mon = &hmp.common;
> +    old_mon = monitor_cur();
> +    monitor_set_cur(&hmp.common);
>  
>      if (has_cpu_index) {
>          int ret = monitor_set_cpu(&hmp.common, cpu_index);
>          if (ret < 0) {
> -            cur_mon = old_mon;
> +            monitor_set_cur(old_mon);
>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
>                         "a CPU number");
>              goto out;
> @@ -140,7 +140,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
>      }
>  
>      handle_hmp_command(&hmp, command_line);
> -    cur_mon = old_mon;
> +    monitor_set_cur(old_mon);
>  
>      qemu_mutex_lock(&hmp.common.mon_lock);
>      if (qstring_get_length(hmp.common.outbuf) > 0) {
> @@ -258,6 +258,7 @@ static void monitor_init_qmp_commands(void)
>  /* Set the current CPU defined by the user. Callers must hold BQL. */
>  int monitor_set_cpu(Monitor *mon, int cpu_index)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      CPUState *cpu;
>  
>      cpu = qemu_get_cpu(cpu_index);

As Eric observed, this hunk adds dead code.

> @@ -272,6 +273,7 @@ int monitor_set_cpu(Monitor *mon, int cpu_index)
>  /* Callers must hold BQL. */
>  static CPUState *mon_get_cpu_sync(bool synchronize)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      CPUState *cpu = NULL;
>  
>      if (cur_mon->mon_cpu_path) {
> @@ -1232,6 +1234,7 @@ static void hmp_acl_remove(Monitor *mon, const QDict *qdict)
>  
>  void qmp_getfd(const char *fdname, Error **errp)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      mon_fd_t *monfd;
>      int fd, tmp_fd;
>  
> @@ -1272,6 +1275,7 @@ void qmp_getfd(const char *fdname, Error **errp)
>  
>  void qmp_closefd(const char *fdname, Error **errp)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      mon_fd_t *monfd;
>      int tmp_fd;
>  
> @@ -1361,7 +1365,7 @@ AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque,
>                        const char *opaque, Error **errp)
>  {
>      int fd;
> -    Monitor *mon = cur_mon;
> +    Monitor *mon = monitor_cur();
>      AddfdInfo *fdinfo;
>  
>      fd = qemu_chr_fe_get_msgfd(&mon->chr);
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 125494410a..182ba136b4 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -66,13 +66,24 @@ MonitorList mon_list;
>  int mon_refcount;
>  static bool monitor_destroyed;
>  
> -__thread Monitor *cur_mon;
> +static __thread Monitor *cur_monitor;
> +
> +Monitor *monitor_cur(void)
> +{
> +    return cur_monitor;
> +}
> +
> +void monitor_set_cur(Monitor *mon)
> +{
> +    cur_monitor = mon;
> +}

All uses of monitor_set_cur() look like this:

    old_mon = monitor_cur();
    monitor_set_cur(new_mon);
    ...
    monitor_set_cur(old_mon);

If we let monitor_set_cur() return the old value, this becomes

    old_mon = monitor_set_cur(new_mon);
    ...
    monitor_set_cur(old_mon);

I like this better.

>  
>  /**
>   * Is the current monitor, if any, a QMP monitor?
>   */
>  bool monitor_cur_is_qmp(void)
>  {
> +    Monitor *cur_mon = monitor_cur();

Blank line between declarations and statements, please.  More of the
same below.

>      return cur_mon && monitor_is_qmp(cur_mon);
>  }
>  
> @@ -209,6 +220,7 @@ int monitor_printf(Monitor *mon, const char *fmt, ...)
>   */
>  int error_vprintf(const char *fmt, va_list ap)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      if (cur_mon && !monitor_cur_is_qmp()) {
>          return monitor_vprintf(cur_mon, fmt, ap);
>      }
> @@ -217,6 +229,7 @@ int error_vprintf(const char *fmt, va_list ap)
>  
>  int error_vprintf_unless_qmp(const char *fmt, va_list ap)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      if (!cur_mon) {
>          return vfprintf(stderr, fmt, ap);
>      }
> diff --git a/monitor/qmp-cmds-control.c b/monitor/qmp-cmds-control.c
> index 8f04cfa6e6..a456762f6a 100644
> --- a/monitor/qmp-cmds-control.c
> +++ b/monitor/qmp-cmds-control.c
> @@ -69,6 +69,7 @@ static bool qmp_caps_accept(MonitorQMP *mon, QMPCapabilityList *list,
>  void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
>                            Error **errp)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      MonitorQMP *mon;
>  
>      assert(monitor_is_qmp(cur_mon));
> @@ -119,6 +120,7 @@ static void query_commands_cb(const QmpCommand *cmd, void *opaque)
>  CommandInfoList *qmp_query_commands(Error **errp)
>  {
>      CommandInfoList *list = NULL;
> +    Monitor *cur_mon = monitor_cur();
>      MonitorQMP *mon;
>  
>      assert(monitor_is_qmp(cur_mon));
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 864cbfa32e..c7bf6bb4dc 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -327,7 +327,7 @@ void qmp_add_client(const char *protocol, const char *fdname,
>      Chardev *s;
>      int fd;
>  
> -    fd = monitor_get_fd(cur_mon, fdname, errp);
> +    fd = monitor_get_fd(monitor_cur(), fdname, errp);
>      if (fd < 0) {
>          return;
>      }
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index d433ceae5b..5e9abd4711 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -139,12 +139,12 @@ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
>      QDict *rsp;
>      QDict *error;
>  
> -    old_mon = cur_mon;
> -    cur_mon = &mon->common;
> +    old_mon = monitor_cur();
> +    monitor_set_cur(&mon->common);
>  
>      rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon));
>  
> -    cur_mon = old_mon;
> +    monitor_set_cur(old_mon);
>  
>      if (mon->commands == &qmp_cap_negotiation_commands) {
>          error = qdict_get_qdict(rsp, "error");
> diff --git a/net/socket.c b/net/socket.c
> index c92354049b..93bee968a8 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -727,7 +727,7 @@ int net_init_socket(const Netdev *netdev, const char *name,
>      if (sock->has_fd) {
>          int fd;
>  
> -        fd = monitor_fd_param(cur_mon, sock->fd, errp);
> +        fd = monitor_fd_param(monitor_cur(), sock->fd, errp);
>          if (fd == -1) {
>              return -1;
>          }
> diff --git a/net/tap.c b/net/tap.c
> index 6207f61f84..fd7d15936d 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -689,7 +689,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>          }
>  
>          if (vhostfdname) {
> -            vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
> +            vhostfd = monitor_fd_param(monitor_cur(), vhostfdname, &err);
>              if (vhostfd == -1) {
>                  if (tap->has_vhostforce && tap->vhostforce) {
>                      error_propagate(errp, err);
> @@ -789,7 +789,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>              return -1;
>          }
>  
> -        fd = monitor_fd_param(cur_mon, tap->fd, &err);
> +        fd = monitor_fd_param(monitor_cur(), tap->fd, &err);
>          if (fd == -1) {
>              error_propagate(errp, err);
>              return -1;
> @@ -836,7 +836,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>          }
>  
>          for (i = 0; i < nfds; i++) {
> -            fd = monitor_fd_param(cur_mon, fds[i], &err);
> +            fd = monitor_fd_param(monitor_cur(), fds[i], &err);
>              if (fd == -1) {
>                  error_propagate(errp, err);
>                  ret = -1;
> diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
> index 6cff1c4e1d..0cd2d864b2 100644
> --- a/stubs/monitor-core.c
> +++ b/stubs/monitor-core.c
> @@ -3,7 +3,10 @@
>  #include "qemu-common.h"
>  #include "qapi/qapi-emit-events.h"
>  
> -__thread Monitor *cur_mon;
> +Monitor *monitor_cur(void)
> +{
> +    return NULL;
> +}

Is this meant to be called?  If not, abort().

>  
>  void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
>  {
> diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
> index 2ca1e99f17..36fabb5e46 100644
> --- a/tests/test-util-sockets.c
> +++ b/tests/test-util-sockets.c
> @@ -53,27 +53,27 @@ static void test_fd_is_socket_good(void)
>  static int mon_fd = -1;
>  static const char *mon_fdname;
>  
> -int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
> -{
> -    g_assert(cur_mon);
> -    g_assert(mon == cur_mon);
> -    if (mon_fd == -1 || !g_str_equal(mon_fdname, fdname)) {
> -        error_setg(errp, "No fd named %s", fdname);
> -        return -1;
> -    }
> -    return dup(mon_fd);
> -}
> -
>  /* Syms in libqemustub.a are discarded at .o file granularity.
>   * To replace monitor_get_fd() we must ensure everything in
>   * stubs/monitor.c is defined, to make sure monitor.o is discarded
>   * otherwise we get duplicate syms at link time.
>   */
>  __thread Monitor *cur_mon;

Hmm.  Since monitor.o's @cur_mon now has internal linkage, the comment
doesn't apply to @cur_mon anymore.  Easy to fix: move the variable
before the comment.  Bonus: you don't have to move monitor_get_fd()
then.

Hmm^2, the comment is stale:

* "libqemustub.a"

  Gone since Commit ebedb37c8d "Makefile: Remove libqemustub.a".  Almost
  three years.  git-grep finds three more occurences, all bogus.

* "stubs/monitor.c"

  Commit 6ede81d576 "stubs: Update monitor stubs for
  qemu-storage-daemon" moved stuff from stubs/monitor.c to
  monitor-core.c.

* "we must ensure everything in stubs/monitor.c is defined"

  We don't.

Mind to clean that up beforehand?

> +Monitor *monitor_cur(void) { return cur_mon; }

Is this meant to be called?  If not, abort().

>  int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) { abort(); }
>  void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp) {}
>  void monitor_init_hmp(Chardev *chr, bool use_readline, Error **errp) {}
>  
> +int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
> +{
> +    g_assert(cur_mon);
> +    g_assert(mon == cur_mon);
> +    if (mon_fd == -1 || !g_str_equal(mon_fdname, fdname)) {
> +        error_setg(errp, "No fd named %s", fdname);
> +        return -1;
> +    }
> +    return dup(mon_fd);
> +}
>  
>  static void test_socket_fd_pass_name_good(void)
>  {
> diff --git a/trace/control.c b/trace/control.c
> index 2ffe000818..62993daf64 100644
> --- a/trace/control.c
> +++ b/trace/control.c
> @@ -176,7 +176,7 @@ void trace_enable_events(const char *line_buf)
>  {
>      if (is_help_option(line_buf)) {
>          trace_list_events();
> -        if (cur_mon == NULL) {
> +        if (monitor_cur() == NULL) {
>              exit(0);
>          }
>      } else {
> diff --git a/util/qemu-error.c b/util/qemu-error.c
> index dac7c7dc50..8d4ed723f5 100644
> --- a/util/qemu-error.c
> +++ b/util/qemu-error.c
> @@ -169,7 +169,7 @@ static void print_loc(void)
>      int i;
>      const char *const *argp;
>  
> -    if (!cur_mon && progname) {
> +    if (!monitor_cur() && progname) {
>          fprintf(stderr, "%s:", progname);
>          sep = " ";
>      }
> @@ -206,7 +206,7 @@ static void vreport(report_type type, const char *fmt, va_list ap)
>      GTimeVal tv;
>      gchar *timestr;
>  
> -    if (error_with_timestamp && !cur_mon) {
> +    if (error_with_timestamp && !monitor_cur()) {
>          g_get_current_time(&tv);
>          timestr = g_time_val_to_iso8601(&tv);
>          error_printf("%s ", timestr);
> diff --git a/util/qemu-print.c b/util/qemu-print.c
> index e79d6b8396..69ba612f56 100644
> --- a/util/qemu-print.c
> +++ b/util/qemu-print.c
> @@ -20,6 +20,7 @@
>   */
>  int qemu_vprintf(const char *fmt, va_list ap)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      if (cur_mon) {
>          return monitor_vprintf(cur_mon, fmt, ap);
>      }
> @@ -48,7 +49,7 @@ int qemu_printf(const char *fmt, ...)
>  int qemu_vfprintf(FILE *stream, const char *fmt, va_list ap)
>  {
>      if (!stream) {
> -        return monitor_vprintf(cur_mon, fmt, ap);
> +        return monitor_vprintf(monitor_cur(), fmt, ap);
>      }
>      return vfprintf(stream, fmt, ap);
>  }
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index b37d288866..40c18ba142 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1092,6 +1092,7 @@ fail:
>  
>  static int socket_get_fd(const char *fdstr, int num, Error **errp)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      int fd;
>      if (num != 1) {
>          error_setg_errno(errp, EINVAL, "socket_get_fd: too many connections");

Am 04.08.2020 um 14:46 hat Markus Armbruster geschrieben:
> > diff --git a/monitor/hmp.c b/monitor/hmp.c
> > index d598dd02bb..f609fcf75b 100644
> > --- a/monitor/hmp.c
> > +++ b/monitor/hmp.c
> > @@ -1301,11 +1301,11 @@ cleanup:
> >  static void monitor_read(void *opaque, const uint8_t *buf, int size)
> >  {
> >      MonitorHMP *mon;
> > -    Monitor *old_mon = cur_mon;
> > +    Monitor *old_mon = monitor_cur();
> >      int i;
> >  
> > -    cur_mon = opaque;
> > -    mon = container_of(cur_mon, MonitorHMP, common);
> > +    monitor_set_cur(opaque);
> > +    mon = container_of(monitor_cur(), MonitorHMP, common);
> 
> Simpler:
> 
>        MonitorHMP *mon = container_of(opaque, MonitorHMP, common);

opaque is void*, so it doesn't have a field 'common'.

> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index 125494410a..182ba136b4 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -66,13 +66,24 @@ MonitorList mon_list;
> >  int mon_refcount;
> >  static bool monitor_destroyed;
> >  
> > -__thread Monitor *cur_mon;
> > +static __thread Monitor *cur_monitor;
> > +
> > +Monitor *monitor_cur(void)
> > +{
> > +    return cur_monitor;
> > +}
> > +
> > +void monitor_set_cur(Monitor *mon)
> > +{
> > +    cur_monitor = mon;
> > +}
> 
> All uses of monitor_set_cur() look like this:
> 
>     old_mon = monitor_cur();
>     monitor_set_cur(new_mon);
>     ...
>     monitor_set_cur(old_mon);
> 
> If we let monitor_set_cur() return the old value, this becomes
> 
>     old_mon = monitor_set_cur(new_mon);
>     ...
>     monitor_set_cur(old_mon);
> 
> I like this better.

Fine with me.

> > diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
> > index 6cff1c4e1d..0cd2d864b2 100644
> > --- a/stubs/monitor-core.c
> > +++ b/stubs/monitor-core.c
> > @@ -3,7 +3,10 @@
> >  #include "qemu-common.h"
> >  #include "qapi/qapi-emit-events.h"
> >  
> > -__thread Monitor *cur_mon;
> > +Monitor *monitor_cur(void)
> > +{
> > +    return NULL;
> > +}
> 
> Is this meant to be called?  If not, abort().

error_report() and friends are supposed to be called pretty much
everywhere, so I'd say yes.

Kevin

Kevin Wolf <kwolf@redhat.com> writes:

> Am 04.08.2020 um 14:46 hat Markus Armbruster geschrieben:
>> > diff --git a/monitor/hmp.c b/monitor/hmp.c
>> > index d598dd02bb..f609fcf75b 100644
>> > --- a/monitor/hmp.c
>> > +++ b/monitor/hmp.c
>> > @@ -1301,11 +1301,11 @@ cleanup:
>> >  static void monitor_read(void *opaque, const uint8_t *buf, int size)
>> >  {
>> >      MonitorHMP *mon;
>> > -    Monitor *old_mon = cur_mon;
>> > +    Monitor *old_mon = monitor_cur();
>> >      int i;
>> >  
>> > -    cur_mon = opaque;
>> > -    mon = container_of(cur_mon, MonitorHMP, common);
>> > +    monitor_set_cur(opaque);
>> > +    mon = container_of(monitor_cur(), MonitorHMP, common);
>> 
>> Simpler:
>> 
>>        MonitorHMP *mon = container_of(opaque, MonitorHMP, common);
>
> opaque is void*, so it doesn't have a field 'common'.

I actually compile-tested before I sent this.  For once ;)

Here's container_of():

    #define container_of(ptr, type, member) ({                      \
            const typeof(((type *) 0)->member) *__mptr = (ptr);     \
            (type *) ((char *) __mptr - offsetof(type, member));})

Its first argument's only use is as an initializer for a pointer
variable.  Both type * and void * work fine there.

>> > diff --git a/monitor/monitor.c b/monitor/monitor.c
>> > index 125494410a..182ba136b4 100644
>> > --- a/monitor/monitor.c
>> > +++ b/monitor/monitor.c
>> > @@ -66,13 +66,24 @@ MonitorList mon_list;
>> >  int mon_refcount;
>> >  static bool monitor_destroyed;
>> >  
>> > -__thread Monitor *cur_mon;
>> > +static __thread Monitor *cur_monitor;
>> > +
>> > +Monitor *monitor_cur(void)
>> > +{
>> > +    return cur_monitor;
>> > +}
>> > +
>> > +void monitor_set_cur(Monitor *mon)
>> > +{
>> > +    cur_monitor = mon;
>> > +}
>> 
>> All uses of monitor_set_cur() look like this:
>> 
>>     old_mon = monitor_cur();
>>     monitor_set_cur(new_mon);
>>     ...
>>     monitor_set_cur(old_mon);
>> 
>> If we let monitor_set_cur() return the old value, this becomes
>> 
>>     old_mon = monitor_set_cur(new_mon);
>>     ...
>>     monitor_set_cur(old_mon);
>> 
>> I like this better.
>
> Fine with me.
>
>> > diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
>> > index 6cff1c4e1d..0cd2d864b2 100644
>> > --- a/stubs/monitor-core.c
>> > +++ b/stubs/monitor-core.c
>> > @@ -3,7 +3,10 @@
>> >  #include "qemu-common.h"
>> >  #include "qapi/qapi-emit-events.h"
>> >  
>> > -__thread Monitor *cur_mon;
>> > +Monitor *monitor_cur(void)
>> > +{
>> > +    return NULL;
>> > +}
>> 
>> Is this meant to be called?  If not, abort().
>
> error_report() and friends are supposed to be called pretty much
> everywhere, so I'd say yes.

Okay.

Am 05.08.2020 um 09:19 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 04.08.2020 um 14:46 hat Markus Armbruster geschrieben:
> >> > diff --git a/monitor/hmp.c b/monitor/hmp.c
> >> > index d598dd02bb..f609fcf75b 100644
> >> > --- a/monitor/hmp.c
> >> > +++ b/monitor/hmp.c
> >> > @@ -1301,11 +1301,11 @@ cleanup:
> >> >  static void monitor_read(void *opaque, const uint8_t *buf, int size)
> >> >  {
> >> >      MonitorHMP *mon;
> >> > -    Monitor *old_mon = cur_mon;
> >> > +    Monitor *old_mon = monitor_cur();
> >> >      int i;
> >> >  
> >> > -    cur_mon = opaque;
> >> > -    mon = container_of(cur_mon, MonitorHMP, common);
> >> > +    monitor_set_cur(opaque);
> >> > +    mon = container_of(monitor_cur(), MonitorHMP, common);
> >> 
> >> Simpler:
> >> 
> >>        MonitorHMP *mon = container_of(opaque, MonitorHMP, common);
> >
> > opaque is void*, so it doesn't have a field 'common'.
> 
> I actually compile-tested before I sent this.  For once ;)
> 
> Here's container_of():
> 
>     #define container_of(ptr, type, member) ({                      \
>             const typeof(((type *) 0)->member) *__mptr = (ptr);     \
>             (type *) ((char *) __mptr - offsetof(type, member));})
> 
> Its first argument's only use is as an initializer for a pointer
> variable.  Both type * and void * work fine there.

Ah, we just lose type checking.

That's what I get for replying from what I remember from over two months
ago. I was pretty sure I didn't like this way, but went with it because
the other way didn't work. Maybe I just assumed it didn't work, or tried
something different that actually fails. Who knows.

Kevin

Markus Armbruster <armbru@redhat.com> writes:

> Kevin Wolf <kwolf@redhat.com> writes:
>
>> cur_mon really needs to be coroutine-local as soon as we move monitor
>> command handlers to coroutines and let them yield. As a first step, just
>> remove all direct accesses to cur_mon so that we can implement this in
>> the getter function later.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
[...]
>> diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
>> index 2ca1e99f17..36fabb5e46 100644
>> --- a/tests/test-util-sockets.c
>> +++ b/tests/test-util-sockets.c
>> @@ -53,27 +53,27 @@ static void test_fd_is_socket_good(void)
>>  static int mon_fd = -1;
>>  static const char *mon_fdname;
>>  
>> -int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
>> -{
>> -    g_assert(cur_mon);
>> -    g_assert(mon == cur_mon);
>> -    if (mon_fd == -1 || !g_str_equal(mon_fdname, fdname)) {
>> -        error_setg(errp, "No fd named %s", fdname);
>> -        return -1;
>> -    }
>> -    return dup(mon_fd);
>> -}
>> -
>>  /* Syms in libqemustub.a are discarded at .o file granularity.
>>   * To replace monitor_get_fd() we must ensure everything in
>>   * stubs/monitor.c is defined, to make sure monitor.o is discarded
>>   * otherwise we get duplicate syms at link time.
>>   */
>>  __thread Monitor *cur_mon;
>
> Hmm.  Since monitor.o's @cur_mon now has internal linkage, the comment
> doesn't apply to @cur_mon anymore.  Easy to fix: move the variable
> before the comment.  Bonus: you don't have to move monitor_get_fd()
> then.
>
> Hmm^2, the comment is stale:
>
> * "libqemustub.a"
>
>   Gone since Commit ebedb37c8d "Makefile: Remove libqemustub.a".  Almost
>   three years.  git-grep finds three more occurences, all bogus.

Thomas posted a patch:

    Subject: [PATCH 07/11] Get rid of the libqemustub.a remainders
    Message-Id: <20200804170055.2851-8-thuth@redhat.com>

>
> * "stubs/monitor.c"
>
>   Commit 6ede81d576 "stubs: Update monitor stubs for
>   qemu-storage-daemon" moved stuff from stubs/monitor.c to
>   monitor-core.c.
>
> * "we must ensure everything in stubs/monitor.c is defined"
>
>   We don't.

These two remain.

> Mind to clean that up beforehand?

[...]