[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 35/38] console: make screendump asynchronous
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v3 35/38] console: make screendump asynchronous |
Date: |
Thu, 19 Apr 2018 18:05:45 +0200 |
Hi
On Thu, Apr 12, 2018 at 4:48 PM, Dr. David Alan Gilbert
<address@hidden> wrote:
> * Marc-André Lureau (address@hidden) 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 <address@hidden>
>
> 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.
>
I should have updated the commit message, but the following patches do
introduce async-QMP support for HMP (by suspending HMP monitor, just
like QMP).
> 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 / address@hidden / Manchester, UK
>
--
Marc-André Lureau