Alon Levy <al...@redhat.com> writes:
> 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 <al...@redhat.com>
Async commands are badly broken with respect to error handling.
This needs to be done after we get the new QMP server.
Why is QXL unable to do a synchronous screendump?
Regards,
Anthony Liguori
> ---
> 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"
> +
> +#define QERR_SCREENDUMP_IN_PROGRESS \
> + ERROR_CLASS_GENERIC_ERROR, "Existing screendump in progress"
> +
> #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'} }
> -
> -##
> # @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;
> --
> 1.8.2.1
