* Marc-André Lureau (marcandre.lur...@redhat.com) wrote:
> Make screendump asynchronous to provide correct screendumps.
>
> HMP doesn't have async support, so it has to remain synchronous and
> potentially incorrect to avoid potential races.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1230527
>
> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
Hi,
Looking at this and the previous pair of patches, I think we'd be
better if we defined that async commands took a callback function
pointer and data that they call.
Here we've got 'graphic_hw_update_done' that explicitly walks qmp
lists; but I think it's better just to pass graphic_hw_update() the
completion data together with a callback function and it calls that when
it's done.
(Also isn't it a little bit of a race, calling graphic_hw_update() and
*then* adding the entry to the list? Can't it end up calling the _done()
before we've added it to the list).
Also, do we need a list of outstanding screendumps, or should we just
have a list of async commands.
It wouldn't seem hard to use the async-QMP command from the HMP
and then just provide a way to tell whether it had finished.
Dave
> ---
> qapi/ui.json | 3 +-
> include/ui/console.h | 3 ++
> hmp.c | 2 +-
> ui/console.c | 99 ++++++++++++++++++++++++++++++++++++++++----
> 4 files changed, 96 insertions(+), 11 deletions(-)
>
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 5d01ad4304..4d2c326fb9 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -96,7 +96,8 @@
> #
> ##
> { 'command': 'screendump',
> - 'data': {'filename': 'str', '*device': 'str', '*head': 'int'} }
> + 'data': {'filename': 'str', '*device': 'str', '*head': 'int'},
> + 'async': true }
>
> ##
> # == Spice
> diff --git a/include/ui/console.h b/include/ui/console.h
> index fc21621656..0c02190963 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -74,6 +74,9 @@ struct MouseTransformInfo {
> };
>
> void hmp_mouse_set(Monitor *mon, const QDict *qdict);
> +void hmp_screendump_sync(const char *filename,
> + bool has_device, const char *device,
> + bool has_head, int64_t head, Error **errp);
>
> /* keysym is a unicode code except for special keys (see QEMU_KEY_xxx
> constants) */
> diff --git a/hmp.c b/hmp.c
> index 679467d85a..da9008fe63 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2144,7 +2144,7 @@ void hmp_screendump(Monitor *mon, const QDict *qdict)
> int64_t head = qdict_get_try_int(qdict, "head", 0);
> Error *err = NULL;
>
> - qmp_screendump(filename, id != NULL, id, id != NULL, head, &err);
> + hmp_screendump_sync(filename, id != NULL, id, id != NULL, head, &err);
> hmp_handle_error(mon, &err);
> }
>
> diff --git a/ui/console.c b/ui/console.c
> index 29234605a7..da51861191 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -32,6 +32,7 @@
> #include "chardev/char-fe.h"
> #include "trace.h"
> #include "exec/memory.h"
> +#include "monitor/monitor.h"
>
> #define DEFAULT_BACKSCROLL 512
> #define CONSOLE_CURSOR_PERIOD 500
> @@ -116,6 +117,12 @@ typedef enum {
> TEXT_CONSOLE_FIXED_SIZE
> } console_type_t;
>
> +struct qmp_screendump {
> + gchar *filename;
> + QmpReturn *ret;
> + QLIST_ENTRY(qmp_screendump) link;
> +};
> +
> struct QemuConsole {
> Object parent;
>
> @@ -165,6 +172,8 @@ struct QemuConsole {
> QEMUFIFO out_fifo;
> uint8_t out_fifo_buf[16];
> QEMUTimer *kbd_timer;
> +
> + QLIST_HEAD(, qmp_screendump) qmp_screendumps;
> };
>
> struct DisplayState {
> @@ -190,6 +199,8 @@ static void dpy_refresh(DisplayState *s);
> static DisplayState *get_alloc_displaystate(void);
> static void text_console_update_cursor_timer(void);
> static void text_console_update_cursor(void *opaque);
> +static void ppm_save(const char *filename, DisplaySurface *ds,
> + Error **errp);
>
> static void gui_update(void *opaque)
> {
> @@ -256,8 +267,42 @@ static void gui_setup_refresh(DisplayState *ds)
> ds->have_text = have_text;
> }
>
> +static void qmp_screendump_finish(QemuConsole *con, const char *filename,
> + QmpReturn *ret)
> +{
> + Error *err = NULL;
> + DisplaySurface *surface;
> + Monitor *prev_mon = cur_mon;
> +
> + if (qmp_return_is_cancelled(ret)) {
> + return;
> + }
> +
> + cur_mon = qmp_return_get_monitor(ret);
> + surface = qemu_console_surface(con);
> +
> + /* FIXME: async save with coroutine? it would have to copy or lock
> + * the surface. */
> + ppm_save(filename, surface, &err);
> +
> + if (err) {
> + qmp_return_error(ret, err);
> + } else {
> + qmp_screendump_return(ret);
> + }
> + cur_mon = prev_mon;
> +}
> +
> void graphic_hw_update_done(QemuConsole *con)
> {
> + struct qmp_screendump *dump, *next;
> +
> + QLIST_FOREACH_SAFE(dump, &con->qmp_screendumps, link, next) {
> + qmp_screendump_finish(con, dump->filename, dump->ret);
> + g_free(dump->filename);
> + QLIST_REMOVE(dump, link);
> + g_free(dump);
> + }
> }
>
> bool graphic_hw_update(QemuConsole *con)
> @@ -358,35 +403,70 @@ write_err:
> goto out;
> }
>
> -void qmp_screendump(const char *filename, bool has_device, const char
> *device,
> - bool has_head, int64_t head, Error **errp)
> +
> +static QemuConsole *get_console(bool has_device, const char *device,
> + bool has_head, int64_t head, Error **errp)
> {
> - QemuConsole *con;
> - DisplaySurface *surface;
> + QemuConsole *con = NULL;
>
> if (has_device) {
> con = qemu_console_lookup_by_device_name(device, has_head ? head : 0,
> errp);
> - if (!con) {
> - return;
> - }
> } else {
> if (has_head) {
> error_setg(errp, "'head' must be specified together with
> 'device'");
> - return;
> + return NULL;
> }
> con = qemu_console_lookup_by_index(0);
> if (!con) {
> error_setg(errp, "There is no console to take a screendump
> from");
> - return;
> }
> }
>
> + return con;
> +}
> +
> +void hmp_screendump_sync(const char *filename,
> + bool has_device, const char *device,
> + bool has_head, int64_t head, Error **errp)
> +{
> + DisplaySurface *surface;
> + QemuConsole *con = get_console(has_device, device, has_head, head, errp);
> +
> + if (!con) {
> + return;
> + }
> + /* This may not complete the drawing with Spice, you may have
> + * glitches or outdated dumps, use qmp instead! */
> graphic_hw_update(con);
> surface = qemu_console_surface(con);
> ppm_save(filename, surface, errp);
> }
>
> +void qmp_screendump(const char *filename,
> + bool has_device, const char *device,
> + bool has_head, int64_t head,
> + QmpReturn *qret)
> +{
> + Error *err = NULL;
> + bool async;
> + QemuConsole *con = get_console(has_device, device, has_head, head, &err);
> +
> + if (!con) {
> + qmp_return_error(qret, err);
> + return;
> + }
> + async = graphic_hw_update(con);
> + if (async) {
> + struct qmp_screendump *dump = g_new(struct qmp_screendump, 1);
> + dump->filename = g_strdup(filename);
> + dump->ret = qret;
> + QLIST_INSERT_HEAD(&con->qmp_screendumps, dump, link);
> + } else {
> + qmp_screendump_finish(con, filename, qret);
> + }
> +}
> +
> void graphic_hw_text_update(QemuConsole *con, console_ch_t *chardata)
> {
> if (!con) {
> @@ -1280,6 +1360,7 @@ static QemuConsole *new_console(DisplayState *ds,
> console_type_t console_type,
> obj = object_new(TYPE_QEMU_CONSOLE);
> s = QEMU_CONSOLE(obj);
> s->head = head;
> + QLIST_INIT(&s->qmp_screendumps);
> object_property_add_link(obj, "device", TYPE_DEVICE,
> (Object **)&s->device,
> object_property_allow_set_link,
> --
> 2.17.0.rc1.1.g4c4f2b46a3
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
