From patchwork Mon Jul 29 22:35:22 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laszlo Ersek X-Patchwork-Id: 263095 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id DE0D52C0104 for ; Tue, 30 Jul 2013 08:33:26 +1000 (EST) Received: from localhost ([::1]:57445 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3w0K-0002pI-ST for incoming@patchwork.ozlabs.org; Mon, 29 Jul 2013 18:33:24 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58491) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3vzx-0002kQ-TF for qemu-devel@nongnu.org; Mon, 29 Jul 2013 18:33:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V3vzs-0008O7-VV for qemu-devel@nongnu.org; Mon, 29 Jul 2013 18:33:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:25229) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3vzs-0008Nx-OQ for qemu-devel@nongnu.org; Mon, 29 Jul 2013 18:32:56 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r6TMWtNH017287 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 29 Jul 2013 18:32:56 -0400 Received: from lacos-laptop.usersys.redhat.com (vpn1-6-104.ams2.redhat.com [10.36.6.104]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r6TMWsWN026727; Mon, 29 Jul 2013 18:32:55 -0400 Message-ID: <51F6EE2A.9050607@redhat.com> Date: Tue, 30 Jul 2013 00:35:22 +0200 From: Laszlo Ersek User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130621 Thunderbird/17.0.7 MIME-Version: 1.0 To: Stefan Weil References: <51F6927E.5040202@weilnetz.de> <871u6htf5r.fsf@codemonkey.ws> <51F6B1FC.7060707@weilnetz.de> In-Reply-To: <51F6B1FC.7060707@weilnetz.de> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: Anthony Liguori , qemu-devel Subject: Re: [Qemu-devel] [BUG] GTK terminal is broken X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On 07/29/13 20:18, Stefan Weil wrote: > Am 29.07.2013 20:05, schrieb Anthony Liguori: >> Stefan Weil 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 Date: Mon, 29 Jul 2013 21:27:04 +0200 Subject: [PATCH] gd_vc_chr_write(): print hexdump, set nonblock Signed-off-by: Laszlo Ersek --- ui/gtk.c | 14 +++++++++++++- util/hexdump.c | 39 ++++++++++++++++++++++++++++++++------- 2 files changed, 45 insertions(+), 8 deletions(-) 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 #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); } }