[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] make screendump an async command
From: |
Alon Levy |
Subject: |
Re: [Qemu-devel] [PATCH] make screendump an async command |
Date: |
Thu, 13 Jun 2013 16:35:03 -0400 (EDT) |
> 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 <address@hidden>
> > ---
> > 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?
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);
(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;
> >
>
>
>