[Qemu-devel] [PATCH 14/17] monitor: Decouple terminals

[Jan Kiszka]

Currently all registered (and activate) monitor terminals work in
broadcast mode: Everyone sees what someone else types on some other
terminal and what the monitor reports back. This model is broken when
you have a management monitor terminal that is automatically operated
and some other terminal used for independent guest inspection. Such an
additional terminal can be a multiplexed device channel or a gdb
frontend connected to QEMU's stub.

Therefor, this patch decouples the buffers and states of all monitor
terminals, allowing the user to operate them independently. The basic
idea is stolen from Jason Wessel: When starting to handle a monitor
command or some terminal event, the current monitor terminal is set to
the one associated with the underlying char device, letting all
succeeding monitor_printf show up on only this selected terminal.

There are still two asynchronous monitor writers: some error reporting
in VNC's audio_add and the log-to-monitor feature of the audio
subsystem. In order to keep them happy, the concept of a default monitor
terminal is used, which is either the dedicated monitor terminal or the
first registered mux. If we can agree on converting or dropping
asynchronous monitor users, we would also be able to drop the default
terminal concept again.

As the patch requires to rework the monitor suspension interface, it
also takes the freedom to make it "truely suspending" now (so far
suspending meant suppressing the prompt, but inputs were still
processed).

Signed-off-by: Jan Kiszka <address@hidden>
---

 migration-exec.c |   11 +---
 migration-tcp.c  |   11 +---
 migration.c      |   12 +++-
 migration.h      |    4 +
 monitor.c        |  159 +++++++++++++++++++++++++++++++-----------------------
 monitor.h        |    8 ++-
 qemu-char.c      |    2 -
 readline.c       |    3 -
 readline.h       |    1 
 vl.c             |    2 -
 10 files changed, 119 insertions(+), 94 deletions(-)

diff --git a/migration-exec.c b/migration-exec.c
index 2c92340..d2c79d6 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -55,7 +55,7 @@ static int exec_close(FdMigrationState *s)
 
 MigrationState *exec_start_outgoing_migration(const char *command,
                                              int64_t bandwidth_limit,
-                                             int async)
+                                             int detach)
 {
     FdMigrationState *s;
     FILE *f;
@@ -89,14 +89,11 @@ MigrationState *exec_start_outgoing_migration(const char 
*command,
     s->mig_state.release = migrate_fd_release;
 
     s->state = MIG_STATE_ACTIVE;
-    s->detach = !async;
+    s->mon_resume_handle = NULL;
     s->bandwidth_limit = bandwidth_limit;
 
-    if (s->detach == 1) {
-        dprintf("detaching from monitor\n");
-        monitor_suspend();
-        s->detach = 2;
-    }
+    if (!detach)
+        migrate_fd_monitor_suspend(s);
 
     migrate_fd_connect(s);
     return &s->mig_state;
diff --git a/migration-tcp.c b/migration-tcp.c
index c5e102c..6c414ac 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -79,7 +79,7 @@ static void tcp_wait_for_connect(void *opaque)
 
 MigrationState *tcp_start_outgoing_migration(const char *host_port,
                                              int64_t bandwidth_limit,
-                                             int async)
+                                             int detach)
 {
     struct sockaddr_in addr;
     FdMigrationState *s;
@@ -98,7 +98,7 @@ MigrationState *tcp_start_outgoing_migration(const char 
*host_port,
     s->mig_state.release = migrate_fd_release;
 
     s->state = MIG_STATE_ACTIVE;
-    s->detach = !async;
+    s->mon_resume_handle = NULL;
     s->bandwidth_limit = bandwidth_limit;
     s->fd = socket(PF_INET, SOCK_STREAM, 0);
     if (s->fd == -1) {
@@ -108,11 +108,8 @@ MigrationState *tcp_start_outgoing_migration(const char 
*host_port,
 
     socket_set_nonblock(s->fd);
 
-    if (s->detach == 1) {
-        dprintf("detaching from monitor\n");
-        monitor_suspend();
-        s->detach = 2;
-    }
+    if (!detach)
+        migrate_fd_monitor_suspend(s);
 
     do {
         ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
diff --git a/migration.c b/migration.c
index a7f9355..18c1492 100644
--- a/migration.c
+++ b/migration.c
@@ -125,6 +125,12 @@ void do_info_migrate(void)
 
 /* shared migration helpers */
 
+void migrate_fd_monitor_suspend(FdMigrationState *s)
+{
+    s->mon_resume_handle = monitor_suspend();
+    dprintf("suspending monitor\n");
+}
+
 void migrate_fd_error(FdMigrationState *s)
 {
     dprintf("setting error state\n");
@@ -145,10 +151,8 @@ void migrate_fd_cleanup(FdMigrationState *s)
         close(s->fd);
 
     /* Don't resume monitor until we've flushed all of the buffers */
-    if (s->detach == 2) {
-        monitor_resume();
-        s->detach = 0;
-    }
+    if (s->mon_resume_handle)
+        monitor_resume(s->mon_resume_handle);
 
     s->fd = -1;
 }
diff --git a/migration.h b/migration.h
index d9771ad..0ed7818 100644
--- a/migration.h
+++ b/migration.h
@@ -37,7 +37,7 @@ struct FdMigrationState
     int64_t bandwidth_limit;
     QEMUFile *file;
     int fd;
-    int detach;
+    void *mon_resume_handle;
     int state;
     int (*get_error)(struct FdMigrationState*);
     int (*close)(struct FdMigrationState*);
@@ -67,6 +67,8 @@ MigrationState *tcp_start_outgoing_migration(const char 
*host_port,
                                             int64_t bandwidth_limit,
                                             int detach);
 
+void migrate_fd_monitor_suspend(FdMigrationState *s);
+
 void migrate_fd_error(FdMigrationState *s);
 
 void migrate_fd_cleanup(FdMigrationState *s);
diff --git a/monitor.c b/monitor.c
index 7b844ac..498b223 100644
--- a/monitor.c
+++ b/monitor.c
@@ -69,39 +69,45 @@ typedef struct term_cmd_t {
 
 typedef struct MonitorTerm {
     CharDriverState *chr;
+    int flags;
+    int suspend_cnt;
+    uint8_t term_outbuf[1024];
+    int term_outbuf_index;
+    ReadLineState *rs;
+    CPUState *mon_cpu;
+    BlockDriverCompletionFunc *password_completion_cb;
+    void *password_opaque;
     LIST_ENTRY(MonitorTerm) entry;
 } MonitorTerm;
 
 static LIST_HEAD(term_list, MonitorTerm) term_list;
+static MonitorTerm *cur_term;
 
 static const term_cmd_t term_cmds[];
 static const term_cmd_t info_cmds[];
 
-static uint8_t term_outbuf[1024];
-static int term_outbuf_index;
-static BlockDriverCompletionFunc *password_completion_cb;
-static void *password_opaque;
-ReadLineState *rs;
+static void monitor_command_cb(void *opaque, const char *cmdline);
 
-static void monitor_start_input(void);
-
-static CPUState *mon_cpu = NULL;
+static void monitor_read_command(MonitorTerm *term, int show_prompt)
+{
+    readline_start(term->rs, "(qemu) ", 0, monitor_command_cb, NULL);
+    if (show_prompt)
+        readline_show_prompt(term->rs);
+}
 
 static void monitor_read_password(ReadLineFunc *readline_func, void *opaque)
 {
-    readline_start(rs, "Password: ", 1, readline_func, opaque);
+    readline_start(cur_term->rs, "Password: ", 1, readline_func, opaque);
+    /* prompt is printed on return from the command handler */
 }
 
 void monitor_flush(void)
 {
-    MonitorTerm *term;
-
-    if (term_outbuf_index > 0) {
-        LIST_FOREACH(term, &term_list, entry) {
-            if (term->chr->focus == 0)
-                qemu_chr_write(term->chr, term_outbuf, term_outbuf_index);
-        }
-        term_outbuf_index = 0;
+    if (cur_term && cur_term->term_outbuf_index != 0
+        && cur_term->chr->focus == 0) {
+        qemu_chr_write(cur_term->chr, cur_term->term_outbuf,
+                       cur_term->term_outbuf_index);
+        cur_term->term_outbuf_index = 0;
     }
 }
 
@@ -109,15 +115,19 @@ void monitor_flush(void)
 static void monitor_puts(const char *str)
 {
     char c;
+
+    if (!cur_term)
+        return;
+
     for(;;) {
         c = *str++;
         if (c == '\0')
             break;
         if (c == '\n')
-            term_outbuf[term_outbuf_index++] = '\r';
-        term_outbuf[term_outbuf_index++] = c;
-        if (term_outbuf_index >= (sizeof(term_outbuf) - 1) ||
-            c == '\n')
+            cur_term->term_outbuf[cur_term->term_outbuf_index++] = '\r';
+        cur_term->term_outbuf[cur_term->term_outbuf_index++] = c;
+        if (cur_term->term_outbuf_index >= (sizeof(cur_term->term_outbuf) - 1)
+            || c == '\n')
             monitor_flush();
     }
 }
@@ -301,7 +311,7 @@ static int mon_set_cpu(int cpu_index)
 
     for(env = first_cpu; env != NULL; env = env->next_cpu) {
         if (env->cpu_index == cpu_index) {
-            mon_cpu = env;
+            cur_term->mon_cpu = env;
             return 0;
         }
     }
@@ -310,10 +320,10 @@ static int mon_set_cpu(int cpu_index)
 
 static CPUState *mon_get_cpu(void)
 {
-    if (!mon_cpu) {
+    if (!cur_term->mon_cpu) {
         mon_set_cpu(0);
     }
-    return mon_cpu;
+    return cur_term->mon_cpu;
 }
 
 static void do_info_registers(void)
@@ -340,7 +350,7 @@ static void do_info_cpus(void)
 
     for(env = first_cpu; env != NULL; env = env->next_cpu) {
         monitor_printf("%c CPU #%d:",
-                       (env == mon_cpu) ? '*' : ' ',
+                       (env == cur_term->mon_cpu) ? '*' : ' ',
                        env->cpu_index);
 #if defined(TARGET_I386)
         monitor_printf(" pc=0x" TARGET_FMT_lx,
@@ -377,7 +387,7 @@ static void do_info_history (void)
 
     i = 0;
     for(;;) {
-        str = readline_get_history(rs, i);
+        str = readline_get_history(cur_term->rs, i);
         if (!str)
             break;
        monitor_printf("%d: '%s'\n", i, str);
@@ -459,7 +469,7 @@ static void change_vnc_password_cb(void *opaque, const char 
*password)
     if (vnc_display_password(NULL, password) < 0)
         monitor_printf("could not set VNC server password\n");
 
-    monitor_start_input();
+    monitor_read_command(cur_term, 1);
 }
 
 static void do_change_vnc(const char *target, const char *arg)
@@ -2629,7 +2639,7 @@ static void cmd_completion(const char *name, const char 
*list)
         memcpy(cmd, pstart, len);
         cmd[len] = '\0';
         if (name[0] == '\0' || !strncmp(name, cmd, strlen(name))) {
-            readline_add_completion(rs, cmd);
+            readline_add_completion(cur_term->rs, cmd);
         }
         if (*p == '\0')
             break;
@@ -2682,7 +2692,7 @@ static void file_completion(const char *input)
             stat(file, &sb);
             if(S_ISDIR(sb.st_mode))
                 pstrcat(file, sizeof(file), "/");
-            readline_add_completion(rs, file);
+            readline_add_completion(cur_term->rs, file);
         }
     }
     closedir(ffs);
@@ -2695,7 +2705,7 @@ static void block_completion_it(void *opaque, 
BlockDriverState *bs)
 
     if (input[0] == '\0' ||
         !strncmp(name, (char *)input, strlen(input))) {
-        readline_add_completion(rs, name);
+        readline_add_completion(cur_term->rs, name);
     }
 }
 
@@ -2755,7 +2765,7 @@ static void monitor_find_completion(const char *cmdline)
             cmdname = "";
         else
             cmdname = args[0];
-        readline_set_completion_index(rs, strlen(cmdname));
+        readline_set_completion_index(cur_term->rs, strlen(cmdname));
         for(cmd = term_cmds; cmd->name != NULL; cmd++) {
             cmd_completion(cmdname, cmd->name);
         }
@@ -2779,23 +2789,23 @@ static void monitor_find_completion(const char *cmdline)
         switch(*ptype) {
         case 'F':
             /* file completion */
-            readline_set_completion_index(rs, strlen(str));
+            readline_set_completion_index(cur_term->rs, strlen(str));
             file_completion(str);
             break;
         case 'B':
             /* block device name completion */
-            readline_set_completion_index(rs, strlen(str));
+            readline_set_completion_index(cur_term->rs, strlen(str));
             bdrv_iterate(block_completion_it, (void *)str);
             break;
         case 's':
             /* XXX: more generic ? */
             if (!strcmp(cmd->name, "info")) {
-                readline_set_completion_index(rs, strlen(str));
+                readline_set_completion_index(cur_term->rs, strlen(str));
                 for(cmd = info_cmds; cmd->name != NULL; cmd++) {
                     cmd_completion(str, cmd->name);
                 }
             } else if (!strcmp(cmd->name, "sendkey")) {
-                readline_set_completion_index(rs, strlen(str));
+                readline_set_completion_index(cur_term->rs, strlen(str));
                 for(key = key_defs; key->name != NULL; key++) {
                     cmd_completion(str, key->name);
                 }
@@ -2811,58 +2821,68 @@ static void monitor_find_completion(const char *cmdline)
 
 static int term_can_read(void *opaque)
 {
-    return 128;
+    MonitorTerm *term = opaque;
+
+    return (term->suspend_cnt == 0) ? 128 : 0;
 }
 
 static void term_read(void *opaque, const uint8_t *buf, int size)
 {
+    MonitorTerm *old_term = cur_term;
     int i;
-    for(i = 0; i < size; i++)
-        readline_handle_byte(rs, buf[i]);
-}
 
-static int monitor_suspended;
+    cur_term = opaque;
 
-static void monitor_handle_command1(void *opaque, const char *cmdline)
-{
-    monitor_handle_command(cmdline);
-    if (!monitor_suspended)
-        monitor_start_input();
-    else
-        monitor_suspended = 2;
+    for (i = 0; i < size; i++)
+        readline_handle_byte(cur_term->rs, buf[i]);
+
+    cur_term = old_term;
 }
 
-void monitor_suspend(void)
+static void monitor_command_cb(void *opaque, const char *cmdline)
 {
-    monitor_suspended = 1;
+    monitor_suspend();
+    monitor_handle_command(cmdline);
+    monitor_resume(cur_term);
 }
 
-void monitor_resume(void)
+void *monitor_suspend(void)
 {
-    if (monitor_suspended == 2)
-        monitor_start_input();
-    monitor_suspended = 0;
+    cur_term->suspend_cnt++;
+    return cur_term;
 }
 
-static void monitor_start_input(void)
+void monitor_resume(void *handle)
 {
-    readline_start(rs, "(qemu) ", 0, monitor_handle_command1, NULL);
+    MonitorTerm *old_term = cur_term;
+
+    cur_term = handle;
+
+    if (--cur_term->suspend_cnt == 0)
+        readline_show_prompt(cur_term->rs);
+
+    cur_term = old_term;
 }
 
 static void term_event(void *opaque, int event)
 {
+    MonitorTerm *old_term = cur_term;
+
     if (event != CHR_EVENT_RESET)
        return;
 
+    cur_term = opaque;
+
     monitor_printf("QEMU %s monitor - type 'help' for more information\n",
                    QEMU_VERSION);
-    monitor_start_input();
-}
+    monitor_resume(cur_term);
 
-static int is_first_init = 1;
+    cur_term = old_term;
+}
 
-void monitor_init(CharDriverState *chr)
+void monitor_init(CharDriverState *chr, int flags)
 {
+    static int is_first_init = 1;
     MonitorTerm *term;
 
     if (is_first_init) {
@@ -2873,13 +2893,16 @@ void monitor_init(CharDriverState *chr)
     term = qemu_mallocz(sizeof(*term));
 
     term->chr = chr;
-    rs = readline_init(monitor_find_completion);
+    term->flags = flags;
+    term->suspend_cnt = 1; /* resume on reset */
+    term->rs = readline_init(monitor_find_completion);
+    monitor_read_command(term, 0);
 
-    qemu_chr_add_handlers(chr, term_can_read, term_read, term_event, NULL);
+    qemu_chr_add_handlers(chr, term_can_read, term_read, term_event, term);
 
     LIST_INSERT_HEAD(&term_list, term, entry);
-
-    readline_start(rs, "", 0, monitor_handle_command1, NULL);
+    if (!cur_term || (flags & MONITOR_IS_DEFAULT))
+        cur_term = term;
 }
 
 static void bdrv_password_cb(void *opaque, const char *password)
@@ -2891,10 +2914,10 @@ static void bdrv_password_cb(void *opaque, const char 
*password)
         monitor_printf("invalid password\n");
         ret = -EPERM;
     }
-    if (password_completion_cb)
-        password_completion_cb(password_opaque, ret);
+    if (cur_term->password_completion_cb)
+        cur_term->password_completion_cb(cur_term->password_opaque, ret);
 
-    monitor_start_input();
+    monitor_read_command(cur_term, 1);
 }
 
 void monitor_read_bdrv_key_start(BlockDriverState *bs,
@@ -2910,8 +2933,8 @@ void monitor_read_bdrv_key_start(BlockDriverState *bs,
     monitor_printf("%s (%s) is encrypted.\n", bdrv_get_device_name(bs),
                    bdrv_get_encrypted_filename(bs));
 
-    password_completion_cb = completion_cb;
-    password_opaque = opaque;
+    cur_term->password_completion_cb = completion_cb;
+    cur_term->password_opaque = opaque;
 
     monitor_read_password(bdrv_password_cb, bs);
 }
diff --git a/monitor.h b/monitor.h
index 4b35a25..7af267d 100644
--- a/monitor.h
+++ b/monitor.h
@@ -4,10 +4,12 @@
 #include "qemu-char.h"
 #include "block.h"
 
-void monitor_init(CharDriverState *chr);
-void monitor_suspend(void);
-void monitor_resume(void);
+/* flags for monitor_init */
+#define MONITOR_IS_DEFAULT    0x01
 
+void monitor_init(CharDriverState *chr, int flags);
+void *monitor_suspend(void);
+void monitor_resume(void *handle);
 void monitor_read_bdrv_key_start(BlockDriverState *bs,
                                  BlockDriverCompletionFunc *completion_cb,
                                  void *opaque);
diff --git a/qemu-char.c b/qemu-char.c
index 5c2ee4e..db2fb44 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2095,7 +2095,7 @@ CharDriverState *qemu_chr_open(const char *label, const 
char *filename, void (*i
         chr = qemu_chr_open(label, p, NULL);
         if (chr) {
             chr = qemu_chr_open_mux(chr);
-            monitor_init(chr);
+            monitor_init(chr, 0);
         } else {
             printf("Unable to open driver: %s\n", p);
         }
diff --git a/readline.c b/readline.c
index 6404674..a0b234d 100644
--- a/readline.c
+++ b/readline.c
@@ -31,7 +31,7 @@
 
 #define printf do_not_use_printf
 
-static void readline_show_prompt(ReadLineState *rs)
+void readline_show_prompt(ReadLineState *rs)
 {
     monitor_printf("%s", rs->prompt);
     monitor_flush();
@@ -445,7 +445,6 @@ void readline_start(ReadLineState *rs, const char *prompt, 
int read_password,
     rs->readline_opaque = opaque;
     rs->read_password = read_password;
     readline_restart(rs);
-    readline_show_prompt(rs);
 }
 
 void readline_restart(ReadLineState *rs)
diff --git a/readline.h b/readline.h
index e385ac4..5247025 100644
--- a/readline.h
+++ b/readline.h
@@ -41,6 +41,7 @@ const char *readline_get_history(ReadLineState *rs, unsigned 
int index);
 void readline_start(ReadLineState *rs, const char *prompt, int read_password,
                     ReadLineFunc *readline_func, void *opaque);
 void readline_restart(ReadLineState *rs);
+void readline_show_prompt(ReadLineState *rs);
 ReadLineState *readline_init(ReadLineCompletionFunc *completion_finder);
 
 #endif /* !READLINE_H */
diff --git a/vl.c b/vl.c
index f5b6171..a80a25b 100644
--- a/vl.c
+++ b/vl.c
@@ -5594,7 +5594,7 @@ int main(int argc, char **argv, char **envp)
     text_consoles_set_display(display_state);
 
     if (monitor_device && monitor_hd)
-        monitor_init(monitor_hd);
+        monitor_init(monitor_hd, MONITOR_IS_DEFAULT);
 
     for(i = 0; i < MAX_SERIAL_PORTS; i++) {
         const char *devname = serial_devices[i];