Kevin Wolf <kw...@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 <kw...@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");
