diff mbox

[BUG] GTK terminal is broken

Message ID 51F6EE2A.9050607@redhat.com
State New
Headers show

Commit Message

Laszlo Ersek July 29, 2013, 10:35 p.m. UTC 
On 07/29/13 20:18, Stefan Weil wrote:
> Am 29.07.2013 20:05, schrieb Anthony Liguori:
>> Stefan Weil <sw@weilnetz.de> writes:
>>
>>> Hello,
>>>
>>> maybe most developers will already have noticed that the terminal output
>>> in QEMU's GTK user interface is broken. As far as I know, it never worked,
>>> but as there are working alternatives, I did not care much and forgot to
>>> report the issues.
>>>
>>> See these snapshots which show the problems with QEMU's GTK terminals:
>>>
>>> http://qemu.weilnetz.de/test/bugs/qemu-gtk-demo1.png
>>>
>>> Here the first line of the QEMUmonitor is only partially visible.
>>> Tested with Cygwin/X on W64 host, QEMU running on Debian Linux.
>>> Other X servers (e.g. native X on Debian Linux) don't show this
>>> problem.
>> It's a GTK/Cygwin/X bug.
>>
>> We use a vbox without anything particularly fancy happening.  Looks like
>> something is calculating layout incorrectly.
>>
>> Regards,
>>
>> Anthony Liguori
> 
> I miss a comment on the more important next two points.
> They are _not_ related to Cygwin / X.
> 
> Do you investigate them?

I did some investigation, and the results are not pretty. I'm using
RHEL-6.4 host OS (kernel, gtk, vte etc), with fresh upstream qemu. The
command line (started as a mere user --> TCG) was

./qemu-system-x86_64 -cdrom
/home/virt-images/isos/Fedora-18-x86_64-Live-XFCE.iso

Please find the attached debug patch (it has a small functional change
as well, I'll discuss it below).

(a) The patch extends the qemu_hexdump() function with ASCII strings. I
wanted to ask you to run your tests with this patch applied, and try to
correlate the hexdumps with the breakage on the screen.

However, when testing the patch myself, I ran into problems.

(b) The patch adds a qemu_set_nonblock(slave_fd) call to gd_vc_init(). I
tested the patch both with and without this hunk.

The key is to produce big output for VTE. For that purpose I waited
until the Fedora 18 kernel enabled paging, and then issued "info tlb".

("info tlb" should be familiar from a recent monitor bugfix, as test
case / trigger, but in bleeding edge qemu the monitor is OK.)

So, if I do not add (b), the debug patch starts to spew data to stderr,
and after a while qemu hangs hard, in the write() call in gd_vc_chr_write().

If I add (b), then the same happens initially, followed by qemu spinning
(ie. entering and exiting rapidly, without making any progress)
gd_vc_chr_write(), with write() returning -1/EAGAIN.

VTE seems to expect the owner of the slave side to run in a different
process or thread. While browsing vte_terminal_io_read(), I found some
throttling code in the read path, where VTE tries to keep one child
process (maybe running in one tab?) to monopolize it.

I think this could explain the deadlock / busy wait behavior. In qemu
both sides of the terminal are manipulated by the same thread, and both
file descriptors are (should be) added to the same GLib main loop. At
some point VTE would be able read the master side but refuses to, the
pty buffer becomes full, the slave-side write() in gd_vc_chr_write()
blocks, and VTE never gets back control to release the throttle.

This doesn't explain why qemu never recovers when the slave side is set
to non-blocking mode (ie. with (b) added). I suspect that although we
manage to run circles around the main loop in this case, we either never
reach the master-fd read in VTE, or VTE always decides it's still too
early to read.

Laszlo
From a598fb00caf89a02eb64145cdada559f9405235b Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Mon, 29 Jul 2013 21:27:04 +0200
Subject: [PATCH] gd_vc_chr_write(): print hexdump, set nonblock


Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ui/gtk.c       |   14 +++++++++++++-
 util/hexdump.c |   39 ++++++++++++++++++++++++++++++++-------
 2 files changed, 45 insertions(+), 8 deletions(-)

Comments

Stefan Weil Aug. 28, 2013, 5:50 p.m. UTC | #1 
Am 30.07.2013 00:35, schrieb Laszlo Ersek:
> On 07/29/13 20:18, Stefan Weil wrote:
>> Am 29.07.2013 20:05, schrieb Anthony Liguori:
>>> Stefan Weil <sw@weilnetz.de> writes:
>>>
>>>> Hello,
>>>>
>>>> maybe most developers will already have noticed that the terminal output
>>>> in QEMU's GTK user interface is broken. As far as I know, it never worked,
>>>> but as there are working alternatives, I did not care much and forgot to
>>>> report the issues.
>>>>
>>>> See these snapshots which show the problems with QEMU's GTK terminals:
>>>>
>>>> http://qemu.weilnetz.de/test/bugs/qemu-gtk-demo1.png
>>>>
>>>> Here the first line of the QEMUmonitor is only partially visible.
>>>> Tested with Cygwin/X on W64 host, QEMU running on Debian Linux.
>>>> Other X servers (e.g. native X on Debian Linux) don't show this
>>>> problem.
>>> It's a GTK/Cygwin/X bug.
>>>
>>> We use a vbox without anything particularly fancy happening.  Looks like
>>> something is calculating layout incorrectly.
>>>
>>> Regards,
>>>
>>> Anthony Liguori
>> I miss a comment on the more important next two points.
>> They are _not_ related to Cygwin / X.
>>
>> Do you investigate them?
> I did some investigation, and the results are not pretty. I'm using
> RHEL-6.4 host OS (kernel, gtk, vte etc), with fresh upstream qemu. The
> command line (started as a mere user --> TCG) was
>
> ./qemu-system-x86_64 -cdrom
> /home/virt-images/isos/Fedora-18-x86_64-Live-XFCE.iso
>
> Please find the attached debug patch (it has a small functional change
> as well, I'll discuss it below).
>
> (a) The patch extends the qemu_hexdump() function with ASCII strings. I
> wanted to ask you to run your tests with this patch applied, and try to
> correlate the hexdumps with the breakage on the screen.
>
> However, when testing the patch myself, I ran into problems.
>
> (b) The patch adds a qemu_set_nonblock(slave_fd) call to gd_vc_init(). I
> tested the patch both with and without this hunk.
>
> The key is to produce big output for VTE. For that purpose I waited
> until the Fedora 18 kernel enabled paging, and then issued "info tlb".
>
> ("info tlb" should be familiar from a recent monitor bugfix, as test
> case / trigger, but in bleeding edge qemu the monitor is OK.)
>
> So, if I do not add (b), the debug patch starts to spew data to stderr,
> and after a while qemu hangs hard, in the write() call in gd_vc_chr_write().
>
> If I add (b), then the same happens initially, followed by qemu spinning
> (ie. entering and exiting rapidly, without making any progress)
> gd_vc_chr_write(), with write() returning -1/EAGAIN.
>
> VTE seems to expect the owner of the slave side to run in a different
> process or thread. While browsing vte_terminal_io_read(), I found some
> throttling code in the read path, where VTE tries to keep one child
> process (maybe running in one tab?) to monopolize it.
>
> I think this could explain the deadlock / busy wait behavior. In qemu
> both sides of the terminal are manipulated by the same thread, and both
> file descriptors are (should be) added to the same GLib main loop. At
> some point VTE would be able read the master side but refuses to, the
> pty buffer becomes full, the slave-side write() in gd_vc_chr_write()
> blocks, and VTE never gets back control to release the throttle.
>
> This doesn't explain why qemu never recovers when the slave side is set
> to non-blocking mode (ie. with (b) added). I suspect that although we
> manage to run circles around the main loop in this case, we either never
> reach the master-fd read in VTE, or VTE always decides it's still too
> early to read.
>
> Laszlo

Thanks for your analysis. For me, it confirms that terminal
emulation with GTK is currently unusable.

Here is one more obvious bug (tested on Linux):

1. Run qemu-system-i386 -L pc-bios with GTK UI.
2. Select "Show tabs".
3. Unselect "Show tabs".

In 3, blank lines are added on top and on the bottom of the BIOS boot text,
so the screen in 3 is higher than in 1 although both should be identical.

Stefan
diff mbox

Patch

diff --git a/ui/gtk.c b/ui/gtk.c
index c38146f..c88bd37 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -35,6 +35,7 @@ 
 #define LOCALEDIR "po"
 
 #include "qemu-common.h"
+#include "qemu/sockets.h"
 
 #ifdef CONFIG_PRAGMA_DIAGNOSTIC_AVAILABLE
 /* Work around an -Wstrict-prototypes warning in GTK headers */
@@ -1119,8 +1120,18 @@  static gboolean gd_focus_out_event(GtkWidget *widget,
 static int gd_vc_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     VirtualConsole *vc = chr->opaque;
+    int written, errno_save;
 
-    return write(vc->fd, buf, len);
+    errno = 0;
+    written = write(vc->fd, buf, len);
+    errno_save = errno;
+
+    fprintf(stderr, "%s: len=%d written=%d errno=%d\n", __FUNCTION__, len,
+            written, errno);
+    qemu_hexdump((const char *)buf, stderr, __FUNCTION__, len);
+
+    errno = errno_save;
+    return written;
 }
 
 static int nb_vcs;
@@ -1213,6 +1224,7 @@  static GSList *gd_vc_init(GtkDisplayState *s, VirtualConsole *vc, int index, GSL
     vte_terminal_set_size(VTE_TERMINAL(vc->terminal), 80, 25);
 
     vc->fd = slave_fd;
+    qemu_set_nonblock(slave_fd);
     vc->chr->opaque = vc;
     vc->scrolled_window = scrolled_window;
 
diff --git a/util/hexdump.c b/util/hexdump.c
index 969b340..994fd69 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -13,25 +13,50 @@ 
  * GNU GPL, version 2 or (at your option) any later version.
  */
 
+#include <ctype.h>
 #include "qemu-common.h"
 
+static void asciiize(const char *buf, FILE *fp, size_t size, unsigned *col)
+{
+    size_t i;
+
+    while (*col < 4 + 1 + 4 * (1 + 4 * (1 + 2))) {
+        fputc(' ', fp);
+        ++*col;
+    }
+    *col = 0;
+
+    fprintf(fp, "  '");
+    for (i = 0; i < size; ++i) {
+      unsigned char c = buf[i];
+
+      fputc(isprint(c) ? c : '.', fp);
+    }
+    fprintf(fp, "'\n");
+}
+
 void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t size)
 {
     unsigned int b;
+    unsigned pos, col;
 
     for (b = 0; b < size; b++) {
-        if ((b % 16) == 0) {
+        pos = b % 16;
+
+        if (pos == 0) {
             fprintf(fp, "%s: %04x:", prefix, b);
+            col = 5;
         }
         if ((b % 4) == 0) {
-            fprintf(fp, " ");
+            col += fprintf(fp, " ");
         }
-        fprintf(fp, " %02x", (unsigned char)buf[b]);
-        if ((b % 16) == 15) {
-            fprintf(fp, "\n");
+        col += fprintf(fp, " %02x", (unsigned char)buf[b]);
+        if (pos == 15) {
+          asciiize(buf + (b - 15), fp, 16, &col);
         }
     }
-    if ((b % 16) != 0) {
-        fprintf(fp, "\n");
+    pos = b % 16;
+    if (pos != 0) {
+        asciiize(buf + (b - pos), fp, pos, &col);
     }
 }