Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
monitor: do not use mb_read/mb_set for suspend_cnt 
Clean up monitor_event to just use monitor_suspend/monitor_resume,
using mon->mux_out to protect against incorrect nesting (especially
on startup).

The only remaining case of reading suspend_cnt is in the can_read
callback, which is just advisory and can use qatomic_read.

As an extra benefit, mux_out is now simply protected by mon_lock.
Also, moving the prompt to the beginning of the main loop removes
it from the output in some error cases where QEMU does not actually
start successfully.  It is not a full fix and it would be nice to
also remove the monitor heading, but this is already a small (though
unintentional) improvement.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
  • Loading branch information
@bonzini
bonzini committed May 25, 2023
1 parent 4cb96b9 commit 6ee7c82
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 35 deletions.
35 changes: 16 additions & 19 deletions monitor/hmp.c
View file
Expand Up @@ -1401,45 +1401,42 @@ static void monitor_read(void *opaque, const uint8_t *buf, int size)
static void monitor_event(void *opaque, QEMUChrEvent event)
{
Monitor *mon = opaque;
MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);

switch (event) {
case CHR_EVENT_MUX_IN:
qemu_mutex_lock(&mon->mon_lock);
mon->mux_out = 0;
qemu_mutex_unlock(&mon->mon_lock);
if (mon->reset_seen) {
readline_restart(hmp_mon->rs);
if (mon->mux_out) {
mon->mux_out = 0;
monitor_resume(mon);
monitor_flush(mon);
} else {
qatomic_mb_set(&mon->suspend_cnt, 0);
}
qemu_mutex_unlock(&mon->mon_lock);
break;

case CHR_EVENT_MUX_OUT:
if (mon->reset_seen) {
if (qatomic_mb_read(&mon->suspend_cnt) == 0) {
monitor_printf(mon, "\n");
qemu_mutex_lock(&mon->mon_lock);
if (!mon->mux_out) {
if (mon->reset_seen && !mon->suspend_cnt) {
monitor_puts_locked(mon, "\n");
} else {
monitor_flush_locked(mon);
}
monitor_flush(mon);
monitor_suspend(mon);
} else {
qatomic_inc(&mon->suspend_cnt);
mon->mux_out = 1;
}
qemu_mutex_lock(&mon->mon_lock);
mon->mux_out = 1;
qemu_mutex_unlock(&mon->mon_lock);
break;

case CHR_EVENT_OPENED:
monitor_printf(mon, "QEMU %s monitor - type 'help' for more "
"information\n", QEMU_VERSION);
qemu_mutex_lock(&mon->mon_lock);
mon->reset_seen = 1;
if (!mon->mux_out) {
readline_restart(hmp_mon->rs);
readline_show_prompt(hmp_mon->rs);
/* Suspend-resume forces the prompt to be printed. */
monitor_suspend(mon);
monitor_resume(mon);
}
mon->reset_seen = 1;
qemu_mutex_unlock(&mon->mon_lock);
mon_refcount++;
break;

Expand Down
3 changes: 1 addition & 2 deletions monitor/monitor-internal.h
View file
Expand Up @@ -94,7 +94,6 @@ typedef struct HMPCommand {

struct Monitor {
CharBackend chr;
int reset_seen;
int suspend_cnt; /* Needs to be accessed atomically */
bool is_qmp;
bool skip_flush;
Expand All @@ -115,8 +114,8 @@ struct Monitor {
QLIST_HEAD(, mon_fd_t) fds;
GString *outbuf;
guint out_watch;
/* Read under either BQL or mon_lock, written with BQL+mon_lock. */
int mux_out;
int reset_seen;
};

struct MonitorHMP {
Expand Down
9 changes: 7 additions & 2 deletions monitor/monitor.c
View file
Expand Up @@ -569,10 +569,15 @@ static void monitor_accept_input(void *opaque)
{
Monitor *mon = opaque;

if (!monitor_is_qmp(mon)) {
qemu_mutex_lock(&mon->mon_lock);
if (!monitor_is_qmp(mon) && mon->reset_seen) {
MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
assert(hmp_mon->rs);
readline_restart(hmp_mon->rs);
qemu_mutex_unlock(&mon->mon_lock);
readline_show_prompt(hmp_mon->rs);
} else {
qemu_mutex_unlock(&mon->mon_lock);
}

qemu_chr_fe_accept_input(&mon->chr);
Expand Down Expand Up @@ -603,7 +608,7 @@ int monitor_can_read(void *opaque)
{
Monitor *mon = opaque;

return !qatomic_mb_read(&mon->suspend_cnt);
return !qatomic_read(&mon->suspend_cnt);
}

void monitor_list_append(Monitor *mon)
Expand Down
4 changes: 2 additions & 2 deletions tests/qemu-iotests/051.out
View file
Expand Up @@ -74,7 +74,7 @@ QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=foo#12: Invalid node-name: 'fo

Testing: -device virtio-scsi -device scsi-hd
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) QEMU_PROG: -device scsi-hd: drive property not set
QEMU_PROG: -device scsi-hd: drive property not set


=== Overriding backing file ===
Expand Down Expand Up @@ -134,7 +134,7 @@ QEMU X.Y.Z monitor - type 'help' for more information

Testing: -drive if=virtio
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) QEMU_PROG: -drive if=virtio: Device needs media, but drive is empty
QEMU_PROG: -drive if=virtio: Device needs media, but drive is empty


=== Attach to node in non-default iothread ===
Expand Down
20 changes: 10 additions & 10 deletions tests/qemu-iotests/051.pc.out
View file
Expand Up @@ -74,7 +74,7 @@ QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=foo#12: Invalid node-name: 'fo

Testing: -device virtio-scsi -device scsi-hd
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) QEMU_PROG: -device scsi-hd: drive property not set
QEMU_PROG: -device scsi-hd: drive property not set


=== Overriding backing file ===
Expand Down Expand Up @@ -142,11 +142,11 @@ QEMU X.Y.Z monitor - type 'help' for more information

Testing: -drive if=ide
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) QEMU_PROG: Device needs media, but drive is empty
QEMU_PROG: Device needs media, but drive is empty

Testing: -drive if=virtio
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) QEMU_PROG: -drive if=virtio: Device needs media, but drive is empty
QEMU_PROG: -drive if=virtio: Device needs media, but drive is empty

Testing: -drive if=none,id=disk -device ide-cd,drive=disk
QEMU X.Y.Z monitor - type 'help' for more information
Expand All @@ -158,22 +158,22 @@ QEMU X.Y.Z monitor - type 'help' for more information

Testing: -drive if=none,id=disk -device ide-hd,drive=disk
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) QEMU_PROG: -device ide-hd,drive=disk: Device needs media, but drive is empty
QEMU_PROG: -device ide-hd,drive=disk: Device needs media, but drive is empty

Testing: -drive if=none,id=disk -device lsi53c895a -device scsi-hd,drive=disk
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) QEMU_PROG: -device scsi-hd,drive=disk: Device needs media, but drive is empty
QEMU_PROG: -device scsi-hd,drive=disk: Device needs media, but drive is empty


=== Attach to node in non-default iothread ===

Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device ide-hd,drive=disk,share-rw=on
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) QEMU_PROG: -device ide-hd,drive=disk,share-rw=on: Cannot change iothread of active block backend
QEMU_PROG: -device ide-hd,drive=disk,share-rw=on: Cannot change iothread of active block backend

Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,share-rw=on
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,share-rw=on: Cannot change iothread of active block backend
QEMU_PROG: -device virtio-blk-pci,drive=disk,share-rw=on: Cannot change iothread of active block backend

Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device lsi53c895a,id=lsi0 -device scsi-hd,bus=lsi0.0,drive=disk,share-rw=on
QEMU X.Y.Z monitor - type 'help' for more information
Expand All @@ -185,7 +185,7 @@ QEMU X.Y.Z monitor - type 'help' for more information

Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on: Cannot change iothread of active block backend
QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on: Cannot change iothread of active block backend

Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-scsi,id=virtio-scsi1,iothread=thread0 -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on
QEMU X.Y.Z monitor - type 'help' for more information
Expand All @@ -204,7 +204,7 @@ QEMU X.Y.Z monitor - type 'help' for more information

Testing: -drive file=TEST_DIR/t.qcow2,if=ide,readonly=on
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) QEMU_PROG: Block node is read-only
QEMU_PROG: Block node is read-only

Testing: -drive file=TEST_DIR/t.qcow2,if=virtio,readonly=on
QEMU X.Y.Z monitor - type 'help' for more information
Expand All @@ -220,7 +220,7 @@ QEMU X.Y.Z monitor - type 'help' for more information

Testing: -drive file=TEST_DIR/t.qcow2,if=none,id=disk,readonly=on -device ide-hd,drive=disk
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) QEMU_PROG: -device ide-hd,drive=disk: Block node is read-only
QEMU_PROG: -device ide-hd,drive=disk: Block node is read-only

Testing: -drive file=TEST_DIR/t.qcow2,if=none,id=disk,readonly=on -device lsi53c895a -device scsi-hd,drive=disk
QEMU X.Y.Z monitor - type 'help' for more information
Expand Down

0 comments on commit 6ee7c82

Please sign in to comment.