[Qemu-devel] [PATCH 0/8] make -qmp stdio usable

[Qemu-devel] [PATCH 0/8] make -qmp stdio usable

[Paolo Bonzini]

This patch series is a rewritten version of the patch to disable
raw mode on the terminal when "-qmp stdio" is given on the command-line.

The rewritten series has several advantages, including working for
text consoles ("-qmp vc2") and especially working with -chardev/-mon
without the need to add special options.  This is because in this
series it is _the monitor_ who asks to disable raw mode.

Patch 2 adds a new CharDriverState method.  Patches 3/4 implement
it for stdio, while patches 5/6 implement it for text consoles.

Patches 1, 7 and 8 are related cleanups.

Paolo Bonzini (8):
  remove broken code for tty
  add qemu_chr_set_echo
  move atexit(term_exit) to qemu_chr_open_stdio
  add set_echo implementation for qemu_chr_stdio
  create TextConsole together with the CharDeviceState
  add set_echo implementation for text consoles
  remove text_console_opts
  fix QemuOpts leak

 console.c    |   81 +++++++++++++++++++++++++++++++++++-----------------------
 monitor.c    |    1 +
 qemu-char.c  |   62 +++++++++++++++++++++++---------------------
 qemu-char.h  |    2 +
 5 files changed, 85 insertions(+), 63 deletions(-)

-- 
1.7.3.2

[Qemu-devel] [PATCH 1/8] remove broken code for tty

[Paolo Bonzini]

This code is taking the settings for a serial port and moving it to
fd 0 when qemu exits.  This is likely just cut-and-paste, rip it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index edc9ad6..5dbdafa 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1012,9 +1012,6 @@ static void tty_serial_init(int fd, int speed,
            speed, parity, data_bits, stop_bits);
 #endif
     tcgetattr (fd, &tty);
-    if (!term_atexit_done) {
-        oldtty = tty;
-    }
 
 #define check_speed(val) if (speed <= val) { spd = B##val; break; }
     speed = speed * 10 / 11;
@@ -1186,11 +1183,6 @@ static int tty_serial_ioctl(CharDriverState *chr, int cmd, void *arg)
     return 0;
 }
 
-static void tty_exit(void)
-{
-    tcsetattr(0, TCSANOW, &oldtty);
-}
-
 static void qemu_chr_close_tty(CharDriverState *chr)
 {
     FDCharDriver *s = chr->opaque;
@@ -1225,8 +1217,6 @@ static CharDriverState *qemu_chr_open_tty(QemuOpts *opts)
     }
     chr->chr_ioctl = tty_serial_ioctl;
     chr->chr_close = qemu_chr_close_tty;
-    if (!term_atexit_done++)
-        atexit(tty_exit);
     return chr;
 }
 #else  /* ! __linux__ && ! __sun__ */
-- 
1.7.3.2

[Qemu-devel] [PATCH 2/8] add qemu_chr_set_echo

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor.c   |    1 +
 qemu-char.c |    7 +++++++
 qemu-char.h |    2 ++
 3 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 5d74fe3..7bb54b8 100644
--- a/monitor.c
+++ b/monitor.c
@@ -5004,6 +5004,7 @@ void monitor_init(CharDriverState *chr, int flags)
         /* Control mode requires special handlers */
         qemu_chr_add_handlers(chr, monitor_can_read, monitor_control_read,
                               monitor_control_event, mon);
+        qemu_chr_set_echo(chr, true);
     } else {
         qemu_chr_add_handlers(chr, monitor_can_read, monitor_read,
                               monitor_event, mon);
diff --git a/qemu-char.c b/qemu-char.c
index 5dbdafa..e2a5e91 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2554,6 +2554,13 @@ CharDriverState *qemu_chr_open(const char *label, const char *filename, void (*i
     return chr;
 }
 
+void qemu_chr_set_echo(struct CharDriverState *chr, bool echo)
+{
+    if (chr->chr_set_echo) {
+        chr->chr_set_echo(chr, echo);
+    }
+}
+
 void qemu_chr_close(CharDriverState *chr)
 {
     QTAILQ_REMOVE(&chardevs, chr, next);
diff --git a/qemu-char.h b/qemu-char.h
index e6ee6c4..56d9954 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -64,6 +64,7 @@ struct CharDriverState {
     void (*chr_send_event)(struct CharDriverState *chr, int event);
     void (*chr_close)(struct CharDriverState *chr);
     void (*chr_accept_input)(struct CharDriverState *chr);
+    void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
     void *opaque;
     QEMUBH *bh;
     char *label;
@@ -76,6 +77,7 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
 CharDriverState *qemu_chr_open_opts(QemuOpts *opts,
                                     void (*init)(struct CharDriverState *s));
 CharDriverState *qemu_chr_open(const char *label, const char *filename, void (*init)(struct CharDriverState *s));
+void qemu_chr_set_echo(struct CharDriverState *chr, bool echo);
 void qemu_chr_close(CharDriverState *chr);
 void qemu_chr_printf(CharDriverState *s, const char *fmt, ...)
     GCC_FMT_ATTR(2, 3);
-- 
1.7.3.2

[Qemu-devel] [PATCH 3/8] move atexit(term_exit) and O_NONBLOCK to qemu_chr_open_stdio

[Paolo Bonzini]

In the next patch, term_init will be changed to enable or disable
echo at will.  Move extraneous stuff out of it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c |   18 ++++++++----------
 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index e2a5e91..cc20fa0 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -715,7 +715,6 @@ static void stdio_read(void *opaque)
 /* init terminal so that we can grab keys */
 static struct termios oldtty;
 static int old_fd0_flags;
-static int term_atexit_done;
 
 static void term_exit(void)
 {
@@ -727,10 +726,7 @@ static void term_init(QemuOpts *opts)
 {
     struct termios tty;
 
-    tcgetattr (0, &tty);
-    oldtty = tty;
-    old_fd0_flags = fcntl(0, F_GETFL);
-
+    tty = oldtty;
     tty.c_iflag &= ~(IGNBRK|BRKINT|PARMRK|ISTRIP
                           |INLCR|IGNCR|ICRNL|IXON);
     tty.c_oflag |= OPOST;
@@ -744,11 +740,6 @@ static void term_init(QemuOpts *opts)
     tty.c_cc[VTIME] = 0;
 
     tcsetattr (0, TCSANOW, &tty);
-
-    if (!term_atexit_done++)
-        atexit(term_exit);
-
-    fcntl(0, F_SETFL, O_NONBLOCK);
 }
 
 static void qemu_chr_close_stdio(struct CharDriverState *chr)
@@ -765,6 +756,13 @@ static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts)
 
     if (stdio_nb_clients >= STDIO_MAX_CLIENTS)
         return NULL;
+    if (stdio_nb_clients == 0) {
+        old_fd0_flags = fcntl(0, F_GETFL);
+        tcgetattr (0, &oldtty);
+        fcntl(0, F_SETFL, O_NONBLOCK);
+        atexit(term_exit);
+    }
+
     chr = qemu_chr_open_fd(0, 1);
     chr->chr_close = qemu_chr_close_stdio;
     qemu_set_fd_handler2(0, stdio_read_poll, stdio_read, NULL, chr);
-- 
1.7.3.2

[Qemu-devel] [PATCH 4/8] add set_echo implementation for qemu_chr_stdio

[Paolo Bonzini]

This also requires moving QemuOpts out of term_init.

Clearing ISIG is independent of whether echo is enabled or disabled.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c |   26 ++++++++++++++++----------
 1 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index cc20fa0..7b61a62 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -715,6 +715,7 @@ static void stdio_read(void *opaque)
 /* init terminal so that we can grab keys */
 static struct termios oldtty;
 static int old_fd0_flags;
+static bool stdio_allow_signal;
 
 static void term_exit(void)
 {
@@ -722,22 +723,24 @@ static void term_exit(void)
     fcntl(0, F_SETFL, old_fd0_flags);
 }
 
-static void term_init(QemuOpts *opts)
+static void qemu_chr_set_echo_stdio(CharDriverState *chr, bool echo)
 {
     struct termios tty;
 
     tty = oldtty;
-    tty.c_iflag &= ~(IGNBRK|BRKINT|PARMRK|ISTRIP
+    if (!echo) {
+        tty.c_iflag &= ~(IGNBRK|BRKINT|PARMRK|ISTRIP
                           |INLCR|IGNCR|ICRNL|IXON);
-    tty.c_oflag |= OPOST;
-    tty.c_lflag &= ~(ECHO|ECHONL|ICANON|IEXTEN);
+        tty.c_oflag |= OPOST;
+        tty.c_lflag &= ~(ECHO|ECHONL|ICANON|IEXTEN);
+        tty.c_cflag &= ~(CSIZE|PARENB);
+        tty.c_cflag |= CS8;
+        tty.c_cc[VMIN] = 1;
+        tty.c_cc[VTIME] = 0;
+    }
     /* if graphical mode, we allow Ctrl-C handling */
-    if (!qemu_opt_get_bool(opts, "signal", display_type != DT_NOGRAPHIC))
+    if (!stdio_allow_signal)
         tty.c_lflag &= ~ISIG;
-    tty.c_cflag &= ~(CSIZE|PARENB);
-    tty.c_cflag |= CS8;
-    tty.c_cc[VMIN] = 1;
-    tty.c_cc[VTIME] = 0;
 
     tcsetattr (0, TCSANOW, &tty);
 }
@@ -765,9 +768,12 @@ static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts)
 
     chr = qemu_chr_open_fd(0, 1);
     chr->chr_close = qemu_chr_close_stdio;
+    chr->chr_set_echo = qemu_chr_set_echo_stdio;
     qemu_set_fd_handler2(0, stdio_read_poll, stdio_read, NULL, chr);
     stdio_nb_clients++;
-    term_init(opts);
+    stdio_allow_signal = qemu_opt_get_bool(opts, "signal",
+                                           display_type != DT_NOGRAPHIC);
+    qemu_chr_set_echo(chr, false);
 
     return chr;
 }
-- 
1.7.3.2

[Qemu-devel] [PATCH 5/8] create TextConsole together with the CharDeviceState

[Paolo Bonzini]

A nicer solution would be to get rid of the opaque pointer and
use containment, but it would also be a much bigger patch.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 console.c    |   56 +++++++++++++++++++++++++++++++-------------------------
 2 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/console.c b/console.c
index c1728b1..42c2ee3 100644
--- a/console.c
+++ b/console.c
@@ -1435,35 +1435,13 @@ static QemuOpts *text_console_opts[128];
 static void text_console_do_init(CharDriverState *chr, DisplayState *ds, QemuOpts *opts)
 {
     TextConsole *s;
-    unsigned width;
-    unsigned height;
     static int color_inited;
 
-    width = qemu_opt_get_number(opts, "width", 0);
-    if (width == 0)
-        width = qemu_opt_get_number(opts, "cols", 0) * FONT_WIDTH;
-
-    height = qemu_opt_get_number(opts, "height", 0);
-    if (height == 0)
-        height = qemu_opt_get_number(opts, "rows", 0) * FONT_HEIGHT;
-
-    if (width == 0 || height == 0) {
-        s = new_console(ds, TEXT_CONSOLE);
-        width = ds_get_width(s->ds);
-        height = ds_get_height(s->ds);
-    } else {
-        s = new_console(ds, TEXT_CONSOLE_FIXED_SIZE);
-    }
+    s = chr->opaque;
 
-    if (!s) {
-        free(chr);
-        return;
-    }
-    chr->opaque = s;
     chr->chr_write = console_puts;
     chr->chr_send_event = console_send_event;
 
-    s->chr = chr;
     s->out_fifo.buf = s->out_fifo_buf;
     s->out_fifo.buf_size = sizeof(s->out_fifo_buf);
     s->kbd_timer = qemu_new_timer(rt_clock, kbd_send_chars, s);
@@ -1478,8 +1456,10 @@ static void text_console_do_init(CharDriverState *chr, DisplayState *ds, QemuOpt
     s->total_height = DEFAULT_BACKSCROLL;
     s->x = 0;
     s->y = 0;
-    s->g_width = width;
-    s->g_height = height;
+    if (s->console_type == TEXT_CONSOLE) {
+        s->g_width = ds_get_width(s->ds);
+        s->g_height = ds_get_height(s->ds);
+    }
 
     s->hw_invalidate = text_console_invalidate;
     s->hw_text_update = text_console_update;
@@ -1515,6 +1495,9 @@ static void text_console_do_init(CharDriverState *chr, DisplayState *ds, QemuOpt
 CharDriverState *text_console_init(QemuOpts *opts)
 {
     CharDriverState *chr;
+    TextConsole *s;
+    unsigned width;
+    unsigned height;
 
     chr = qemu_mallocz(sizeof(CharDriverState));
 
@@ -1526,6 +1509,29 @@ CharDriverState *text_console_init(QemuOpts *opts)
     text_console_opts[n_text_consoles] = opts;
     n_text_consoles++;
 
+    width = qemu_opt_get_number(opts, "width", 0);
+    if (width == 0)
+        width = qemu_opt_get_number(opts, "cols", 0) * FONT_WIDTH;
+
+    height = qemu_opt_get_number(opts, "height", 0);
+    if (height == 0)
+        height = qemu_opt_get_number(opts, "rows", 0) * FONT_HEIGHT;
+
+    if (width == 0 || height == 0) {
+        s = new_console(NULL, TEXT_CONSOLE);
+    } else {
+        s = new_console(NULL, TEXT_CONSOLE_FIXED_SIZE);
+    }
+
+    if (!s) {
+        free(chr);
+        return NULL;
+    }
+
+    s->chr = chr;
+    s->g_width = width;
+    s->g_height = height;
+    chr->opaque = s;
     return chr;
 }
 
-- 
1.7.3.2

[Qemu-devel] [PATCH 6/8] add set_echo implementation for text consoles

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 console.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/console.c b/console.c
index 42c2ee3..60b80ee 100644
--- a/console.c
+++ b/console.c
@@ -137,6 +137,7 @@ struct TextConsole {
     TextAttributes t_attrib; /* currently active text attributes */
     TextCell *cells;
     int text_x[2], text_y[2], cursor_invalidate;
+    int echo;
 
     int update_x0;
     int update_y0;
@@ -1177,8 +1178,14 @@ void kbd_put_keysym(int keysym)
             *q++ = '\033';
             *q++ = '[';
             *q++ = keysym & 0xff;
+        } else if (s->echo && (keysym == '\r' || keysym == '\n')) {
+            console_puts(s->chr, (const uint8_t *) "\r", 1);
+            *q++ = '\n';
         } else {
-                *q++ = keysym;
+            *q++ = keysym;
+        }
+        if (s->echo) {
+            console_puts(s->chr, buf, q - buf);
         }
         if (s->chr->chr_read) {
             qemu_fifo_write(&s->out_fifo, buf, q - buf);
@@ -1432,6 +1439,13 @@ static int n_text_consoles;
 static CharDriverState *text_consoles[128];
 static QemuOpts *text_console_opts[128];
 
+static void text_console_set_echo(CharDriverState *chr, bool echo)
+{
+    TextConsole *s = chr->opaque;
+
+    s->echo = echo;
+}
+
 static void text_console_do_init(CharDriverState *chr, DisplayState *ds, QemuOpts *opts)
 {
     TextConsole *s;
@@ -1532,6 +1546,7 @@ CharDriverState *text_console_init(QemuOpts *opts)
     s->g_width = width;
     s->g_height = height;
     chr->opaque = s;
+    chr->chr_set_echo = text_console_set_echo;
     return chr;
 }
 
-- 
1.7.3.2

[Qemu-devel] [PATCH 7/8] remove text_console_opts

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 console.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/console.c b/console.c
index 60b80ee..57d6eb5 100644
--- a/console.c
+++ b/console.c
@@ -1437,7 +1437,6 @@ void console_color_init(DisplayState *ds)
 
 static int n_text_consoles;
 static CharDriverState *text_consoles[128];
-static QemuOpts *text_console_opts[128];
 
 static void text_console_set_echo(CharDriverState *chr, bool echo)
 {
@@ -1446,7 +1445,7 @@ static void text_console_set_echo(CharDriverState *chr, bool echo)
     s->echo = echo;
 }
 
-static void text_console_do_init(CharDriverState *chr, DisplayState *ds, QemuOpts *opts)
+static void text_console_do_init(CharDriverState *chr, DisplayState *ds)
 {
     TextConsole *s;
     static int color_inited;
@@ -1520,7 +1519,6 @@ CharDriverState *text_console_init(QemuOpts *opts)
         exit(1);
     }
     text_consoles[n_text_consoles] = chr;
-    text_console_opts[n_text_consoles] = opts;
     n_text_consoles++;
 
     width = qemu_opt_get_number(opts, "width", 0);
@@ -1555,9 +1553,7 @@ void text_consoles_set_display(DisplayState *ds)
     int i;
 
     for (i = 0; i < n_text_consoles; i++) {
-        text_console_do_init(text_consoles[i], ds, text_console_opts[i]);
-        qemu_opts_del(text_console_opts[i]);
-        text_console_opts[i] = NULL;
+        text_console_do_init(text_consoles[i], ds);
     }
 
     n_text_consoles = 0;
-- 
1.7.3.2

[Qemu-devel] [PATCH 8/8] fix QemuOpts leak

[Paolo Bonzini]

Now that no backend's open function saves the passed QemuOpts, fix a leak
in the qemu_chr_open backwards-compatible parser.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 7b61a62..b99b77b 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2555,6 +2555,7 @@ CharDriverState *qemu_chr_open(const char *label, const char *filename, void (*i
     if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
         monitor_init(chr, MONITOR_USE_READLINE);
     }
+    qemu_opts_del(opts);
     return chr;
 }
 
-- 
1.7.3.2

[Qemu-devel] Re: [PATCH 0/8] make -qmp stdio usable

[Paolo Bonzini]

On 12/23/2010 01:42 PM, Paolo Bonzini wrote:
> This patch series is a rewritten version of the patch to disable
> raw mode on the terminal when "-qmp stdio" is given on the command-line.
>
> The rewritten series has several advantages, including working for
> text consoles ("-qmp vc2") and especially working with -chardev/-mon
> without the need to add special options.  This is because in this
> series it is _the monitor_ who asks to disable raw mode.
>
> Patch 2 adds a new CharDriverState method.  Patches 3/4 implement
> it for stdio, while patches 5/6 implement it for text consoles.
>
> Patches 1, 7 and 8 are related cleanups.
>
> Paolo Bonzini (8):
>    remove broken code for tty
>    add qemu_chr_set_echo
>    move atexit(term_exit) to qemu_chr_open_stdio
>    add set_echo implementation for qemu_chr_stdio
>    create TextConsole together with the CharDeviceState
>    add set_echo implementation for text consoles
>    remove text_console_opts
>    fix QemuOpts leak
>
>   console.c    |   81 +++++++++++++++++++++++++++++++++++-----------------------
>   monitor.c    |    1 +
>   qemu-char.c  |   62 +++++++++++++++++++++++---------------------
>   qemu-char.h  |    2 +
>   5 files changed, 85 insertions(+), 63 deletions(-)

Ping?

Paolo

Re: [Qemu-devel] [PATCH 0/8] make -qmp stdio usable

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> This patch series is a rewritten version of the patch to disable
> raw mode on the terminal when "-qmp stdio" is given on the command-line.
>
> The rewritten series has several advantages, including working for
> text consoles ("-qmp vc2") and especially working with -chardev/-mon
> without the need to add special options.  This is because in this
> series it is _the monitor_ who asks to disable raw mode.
>
> Patch 2 adds a new CharDriverState method.  Patches 3/4 implement
> it for stdio, while patches 5/6 implement it for text consoles.
>
> Patches 1, 7 and 8 are related cleanups.

Series looks sane to me.  However, I'm not really qualified to pass
judgement on PATCH 5+6.