From patchwork Thu Jun 13 20:35:03 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alon Levy X-Patchwork-Id: 251158 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 1F91D2C0085 for ; Fri, 14 Jun 2013 06:35:31 +1000 (EST) Received: from localhost ([::1]:54325 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UnEEy-0001YO-W3 for incoming@patchwork.ozlabs.org; Thu, 13 Jun 2013 16:35:28 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41387) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UnEEc-0001Vd-Ml for qemu-devel@nongnu.org; Thu, 13 Jun 2013 16:35:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UnEEb-0006Mq-90 for qemu-devel@nongnu.org; Thu, 13 Jun 2013 16:35:06 -0400 Received: from mx4-phx2.redhat.com ([209.132.183.25]:35281) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UnEEa-0006JO-Vb for qemu-devel@nongnu.org; Thu, 13 Jun 2013 16:35:05 -0400 Received: from zmail15.collab.prod.int.phx2.redhat.com (zmail15.collab.prod.int.phx2.redhat.com [10.5.83.17]) by mx4-phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r5DKZ4ad001757; Thu, 13 Jun 2013 16:35:04 -0400 Date: Thu, 13 Jun 2013 16:35:03 -0400 (EDT) From: Alon Levy To: Paolo Bonzini Message-ID: <523291450.19138315.1371155703116.JavaMail.root@redhat.com> In-Reply-To: <51BA26A6.2020000@redhat.com> References: <1371151644-22308-1-git-send-email-alevy@redhat.com> <51BA26A6.2020000@redhat.com> MIME-Version: 1.0 X-Originating-IP: [10.5.82.11] X-Mailer: Zimbra 8.0.3_GA_5664 (ZimbraWebClient - FF21 (Linux)/8.0.3_GA_5664) Thread-Topic: make screendump an async command Thread-Index: +gzWvlwrzFGb6jlL7uq+Jxzn719QCg== X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6.x X-Received-From: 209.132.183.25 Cc: airlied@redhat.com, aliguori@us.ibm.com, qemu-devel@nongnu.org, kraxel@redhat.com Subject: Re: [Qemu-devel] [PATCH] make screendump an async command 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 > Il 13/06/2013 15:27, Alon Levy ha scritto: > > This fixes the broken screendump behavior for qxl devices in native mode > > since 81fb6f1504fb9ef71f2382f44af34756668296e8. > > > > Note: due to QAPI not generating async commands yet I had to remove the > > schema screendump definition. > > > > Related RHBZ: 973374 > > This patch is not enough to fix said bz, with the linux qxl driver you get > > garbage still, just not out of date garbage. > > > > Signed-off-by: Alon Levy > > --- > > hmp.c | 2 +- > > hw/display/qxl-render.c | 1 + > > hw/display/vga.c | 1 + > > include/qapi/qmp/qerror.h | 6 +++++ > > include/ui/console.h | 10 ++++++++ > > qapi-schema.json | 13 ----------- > > qmp-commands.hx | 3 ++- > > ui/console.c | 58 > > ++++++++++++++++++++++++++++++++++++++++++++++- > > 8 files changed, 78 insertions(+), 16 deletions(-) > > > > diff --git a/hmp.c b/hmp.c > > index 494a9aa..2a37975 100644 > > --- a/hmp.c > > +++ b/hmp.c > > @@ -1346,7 +1346,7 @@ void hmp_screen_dump(Monitor *mon, const QDict > > *qdict) > > const char *filename = qdict_get_str(qdict, "filename"); > > Error *err = NULL; > > > > - qmp_screendump(filename, &err); > > + hmp_screen_dump_helper(filename, &err); > > hmp_handle_error(mon, &err); > > } > > > > diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c > > index f511a62..d719448 100644 > > --- a/hw/display/qxl-render.c > > +++ b/hw/display/qxl-render.c > > @@ -139,6 +139,7 @@ static void > > qxl_render_update_area_unlocked(PCIQXLDevice *qxl) > > qxl->dirty[i].bottom - qxl->dirty[i].top); > > } > > qxl->num_dirty_rects = 0; > > + console_screendump_complete(vga->con); > > } > > > > /* > > diff --git a/hw/display/vga.c b/hw/display/vga.c > > index 21a108d..1fc52eb 100644 > > --- a/hw/display/vga.c > > +++ b/hw/display/vga.c > > @@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque) > > break; > > } > > } > > + console_screendump_complete(s->con); > > } > > > > /* force a full display refresh */ > > diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h > > index 6c0a18d..1bd7efa 100644 > > --- a/include/qapi/qmp/qerror.h > > +++ b/include/qapi/qmp/qerror.h > > @@ -237,6 +237,12 @@ void assert_no_error(Error *err); > > #define QERR_VIRTFS_FEATURE_BLOCKS_MIGRATION \ > > ERROR_CLASS_GENERIC_ERROR, "Migration is disabled when VirtFS export > > path '%s' is mounted in the guest using mount_tag '%s'" > > > > +#define QERR_SCREENDUMP_NOT_AVAILABLE \ > > + ERROR_CLASS_GENERIC_ERROR, "There is no QemuConsole I can screendump > > from" > > s/QemuConsole/graphical console/ > > > +#define QERR_SCREENDUMP_IN_PROGRESS \ > > + ERROR_CLASS_GENERIC_ERROR, "Existing screendump in progress" > > + > > Please use error_setg instead. I'm not sure what you are proposing. Something like this? (minus defining err in the inner scope) > > > #define QERR_SOCKET_CONNECT_FAILED \ > > ERROR_CLASS_GENERIC_ERROR, "Failed to connect to socket" > > > > diff --git a/include/ui/console.h b/include/ui/console.h > > index 4307b5f..643da45 100644 > > --- a/include/ui/console.h > > +++ b/include/ui/console.h > > @@ -341,4 +341,14 @@ int index_from_keycode(int code); > > void early_gtk_display_init(void); > > void gtk_display_init(DisplayState *ds); > > > > +/* hw/display/vga.c hw/display/qxl.c */ > > +void console_screendump_complete(QemuConsole *con); > > + > > +/* monitor.c */ > > +int qmp_screendump(Monitor *mon, const QDict *qdict, > > + MonitorCompletion cb, void *opaque); > > + > > +/* hmp.c */ > > +void hmp_screen_dump_helper(const char *filename, Error **errp); > > + > > #endif > > diff --git a/qapi-schema.json b/qapi-schema.json > > index 5ad6894..f5fdc2f 100644 > > --- a/qapi-schema.json > > +++ b/qapi-schema.json > > @@ -3112,19 +3112,6 @@ > > 'data': { 'keys': ['KeyValue'], '*hold-time': 'int' } } > > > > ## > > -# @screendump: > > -# > > -# Write a PPM of the VGA screen to a file. > > -# > > -# @filename: the path of a new PPM file to store the image > > -# > > -# Returns: Nothing on success > > -# > > -# Since: 0.14.0 > > -## > > -{ 'command': 'screendump', 'data': {'filename': 'str'} } > > Please just comment out the declaration with an explanation. > > Paolo > > > - > > -## > > # @nbd-server-start: > > # > > # Start an NBD server listening on the given host and port. Block > > diff --git a/qmp-commands.hx b/qmp-commands.hx > > index 8cea5e5..bde8f43 100644 > > --- a/qmp-commands.hx > > +++ b/qmp-commands.hx > > @@ -146,7 +146,8 @@ EQMP > > { > > .name = "screendump", > > .args_type = "filename:F", > > - .mhandler.cmd_new = qmp_marshal_input_screendump, > > + .mhandler.cmd_async = qmp_screendump, > > + .flags = MONITOR_CMD_ASYNC, > > }, > > > > SQMP > > diff --git a/ui/console.c b/ui/console.c > > index 28bba6d..0efd588 100644 > > --- a/ui/console.c > > +++ b/ui/console.c > > @@ -116,6 +116,12 @@ typedef enum { > > struct QemuConsole { > > Object parent; > > > > + struct { > > + char *filename; > > + MonitorCompletion *completion_cb; > > + void *completion_opaque; > > + } screendump; > > + > > int index; > > console_type_t console_type; > > DisplayState *ds; > > @@ -313,11 +319,61 @@ write_err: > > goto out; > > } > > > > -void qmp_screendump(const char *filename, Error **errp) > > +int qmp_screendump(Monitor *mon, const QDict *qdict, > > + MonitorCompletion cb, void *opaque) > > { > > QemuConsole *con = qemu_console_lookup_by_index(0); > > + const char *filename = qdict_get_str(qdict, "filename"); > > + > > + if (con == NULL) { > > + qerror_report(QERR_SCREENDUMP_NOT_AVAILABLE); > > + return -1; > > + } > > + if (filename == NULL) { > > + qerror_report(QERR_MISSING_PARAMETER, "filename"); > > + return -1; > > + } > > + if (con->screendump.filename != NULL) { > > + qerror_report(QERR_SCREENDUMP_IN_PROGRESS); > > + return -1; > > + } > > + con->screendump.filename = g_strdup(filename); > > + con->screendump.completion_cb = cb; > > + con->screendump.completion_opaque = opaque; > > + > > + graphic_hw_update(con); > > + return 0; > > +} > > + > > +void console_screendump_complete(QemuConsole *con) > > +{ > > + Error *errp = NULL; > > DisplaySurface *surface; > > > > + if (!con->screendump.filename) { > > + return; > > + } > > + assert(con->screendump.completion_cb); > > + surface = qemu_console_surface(con); > > + ppm_save(con->screendump.filename, surface, &errp); > > + if (errp) { > > + /* > > + * TODO: return data? raise error somehow? read qmp-spec for async > > + * error reporting. > > + */ > > + } > > + con->screendump.completion_cb(con->screendump.completion_opaque, > > NULL); > > + g_free(con->screendump.filename); > > + con->screendump.filename = NULL; > > + con->screendump.completion_cb = NULL; > > + con->screendump.completion_opaque = NULL; > > +} > > + > > +void hmp_screen_dump_helper(const char *filename, Error **errp) > > +{ > > + DisplaySurface *surface; > > + QemuConsole *con = qemu_console_lookup_by_index(0); > > + > > if (con == NULL) { > > error_setg(errp, "There is no QemuConsole I can screendump > > from."); > > return; > > > > > diff --git a/ui/console.c b/ui/console.c index 0efd588..3a1e6ed 100644 --- a/ui/console.c +++ b/ui/console.c @@ -334,7 +334,9 @@ int qmp_screendump(Monitor *mon, const QDict *qdict, return -1; } if (con->screendump.filename != NULL) { - qerror_report(QERR_SCREENDUMP_IN_PROGRESS); + QError *err = NULL; + error_setg(err, "Existing screendump in progress"); + monitor_set_error(mon, err); return -1; } con->screendump.filename = g_strdup(filename);