Re: [Qemu-devel] [PATCH v3 35/38] console: make screendump asynchronous
Hi On Thu, Apr 12, 2018 at 4:49 PM Dr. David Alan Gilbert wrote: > > * 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 > > 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. The calling context is in QmpReturn: it will handle further dispatch on the associated session (it has the associated cb+data, if you will). gfx_update_async() doesn't have cb+data to associate the call with a finish handler. Further down, spice doesn't keep the request "context" either. All it does is call graphic_hw_update_done() when update is finished, that's why we walk the list of pending screendumps here. We could move the pending list of async screendumps down to spice (and other console renderers), but that would make the code more complicated & duplicated for no strong reason. > > (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). Yes, in theory it could. In practice that won't happen today, but we may change things later and not realize we "broke" screendump. So I will rearrange the code to add it to the pending list before calling graphic_hw_update(). > > Also, do we need a list of outstanding screendumps, or should we just > have a list of async commands. We have both, mostly for simplicity and flexibility reasons. If we removed the outstanding screendump list, we would have to query it from the session. Thanks for the review, I'll send an update revision. -- Marc-André Lureau
Re: [Qemu-devel] [PATCH v3 35/38] console: make screendump asynchronous
Hi
On Thu, Apr 12, 2018 at 4:48 PM, Dr. David Alan Gilbert
wrote:
> * 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
>
> 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);
>> +
>>
Re: [Qemu-devel] [PATCH v3 35/38] console: make screendump asynchronous
* 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
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(c
