Patchwork [2/2] qmp: revert "add set_echo implementation for qemu_chr_stdio"

login
register
mail settings
Submitter Pavel Hrdina
Date May 30, 2012, 10:01 a.m.
Message ID <50512eddcf839053fa5bf8f5d09f35c40ff35cef.1338229697.git.phrdina@redhat.com>
Download mbox | patch
Permalink /patch/161926/
State New
Headers show

Comments

Pavel Hrdina - May 30, 2012, 10:01 a.m.
This reverts commit bb002513a9bd2bff169c3d431a8f00c5b2e3aa99 because this code is not used
in order that we use readline mode for '-qmp stdio'.

Conflicts:

	qemu-char.c

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 qemu-char.c |   26 ++++++++++----------------
 1 files changed, 10 insertions(+), 16 deletions(-)
Eric Blake - May 30, 2012, 12:49 p.m.
On 05/30/2012 04:01 AM, Pavel Hrdina wrote:
> This reverts commit bb002513a9bd2bff169c3d431a8f00c5b2e3aa99 because this code is not used
> in order that we use readline mode for '-qmp stdio'.
> 
> Conflicts:
> 
> 	qemu-char.c

This is evidence that you've rebased locally, but as none of us know
what the patch was like before you resolved your rebase conflicts, you
should drop this portion of the commit message before posting upstream.
 Conflict designations are primarily useful on public backporting
branches, rather than on initial submissions for upstream.

As to the series itself, will readline mode add overhead?  Libvirt wants
to use QMP monitor without the overhead of remembering issued commands,
and if the difference for turning on readline is significant, then it
needs to be an opt-in option and not the default.
Pavel Hrdina - May 30, 2012, 1:07 p.m.
On 05/30/2012 02:49 PM, Eric Blake wrote:
> On 05/30/2012 04:01 AM, Pavel Hrdina wrote:
>> This reverts commit bb002513a9bd2bff169c3d431a8f00c5b2e3aa99 because this code is not used
>> in order that we use readline mode for '-qmp stdio'.
>>
>> Conflicts:
>>
>> 	qemu-char.c
> This is evidence that you've rebased locally, but as none of us know
> what the patch was like before you resolved your rebase conflicts, you
> should drop this portion of the commit message before posting upstream.
>   Conflict designations are primarily useful on public backporting
> branches, rather than on initial submissions for upstream.
Thanks, I'll fix the commit message.
> As to the series itself, will readline mode add overhead?  Libvirt wants
> to use QMP monitor without the overhead of remembering issued commands,
> and if the difference for turning on readline is significant, then it
> needs to be an opt-in option and not the default.
>
No, it isn't. The readline mode with control mode is used only if you 
start qemu with '-qmp stdio' option. Libvirt uses sockets for 
communication with qemu.
Paolo Bonzini - May 31, 2012, 10:37 a.m.
Il 30/05/2012 14:49, Eric Blake ha scritto:
> On 05/30/2012 04:01 AM, Pavel Hrdina wrote:
>> This reverts commit bb002513a9bd2bff169c3d431a8f00c5b2e3aa99 because this code is not used
>> in order that we use readline mode for '-qmp stdio'.
>>
>> Conflicts:
>>
>> 	qemu-char.c
> 
> This is evidence that you've rebased locally

Note that if your workflow is rebase-heavy (mine is) you can use the
prepare-commit-msg hook to comment out such lines.  The sample hook
already has the code (.git/hooks/prepare-commit-msg.sample).

> As to the series itself, will readline mode add overhead?  Libvirt wants
> to use QMP monitor without the overhead of remembering issued commands,
> and if the difference for turning on readline is significant, then it
> needs to be an opt-in option and not the default.

The patch should be a no-op unless you use -qmp stdio.

Paolo

Patch

diff --git a/qemu-char.c b/qemu-char.c
index fe1126f..ba336f4 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -727,7 +727,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 bool stdio_allow_signal;
 
 static void term_exit(void)
 {
@@ -735,24 +734,22 @@  static void term_exit(void)
     fcntl(0, F_SETFL, old_fd0_flags);
 }
 
-static void qemu_chr_set_echo_stdio(CharDriverState *chr, bool echo)
+static void term_init(QemuOpts *opts)
 {
     struct termios tty;
 
     tty = oldtty;
-    if (!echo) {
-        tty.c_iflag &= ~(IGNBRK|BRKINT|PARMRK|ISTRIP
+    tty.c_iflag &= ~(IGNBRK|BRKINT|PARMRK|ISTRIP
                           |INLCR|IGNCR|ICRNL|IXON);
-        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;
-    }
+    tty.c_oflag |= OPOST;
+    tty.c_lflag &= ~(ECHO|ECHONL|ICANON|IEXTEN);
     /* if graphical mode, we allow Ctrl-C handling */
-    if (!stdio_allow_signal)
+    if (!qemu_opt_get_bool(opts, "signal", display_type != DT_NOGRAPHIC))
         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);
 }
@@ -781,12 +778,9 @@  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++;
-    stdio_allow_signal = qemu_opt_get_bool(opts, "signal",
-                                           display_type != DT_NOGRAPHIC);
-    qemu_chr_fe_set_echo(chr, false);
+    term_init(opts);
 
     return chr;
 }