diff mbox series

[v4,17/20] console: make screendump asynchronous

Message ID 20190409161009.6322-18-marcandre.lureau@redhat.com
State New
Headers show
Series monitor: add asynchronous command type | expand

Commit Message

Marc-André Lureau April 9, 2019, 4:10 p.m. UTC
Make screendump asynchronous to provide correct screendumps.

For now, HMP doesn't have async support, so it has to remain
synchronous and potentially incorrect to avoid races (following
patches will add HMP asynchronous commands)

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1230527

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qapi/ui.json         |   3 +-
 include/ui/console.h |   3 ++
 hmp.c                |   2 +-
 ui/console.c         | 103 +++++++++++++++++++++++++++++++++++++++----
 4 files changed, 100 insertions(+), 11 deletions(-)

Comments

Gerd Hoffmann April 10, 2019, 8:48 a.m. UTC | #1
> +static void qmp_screendump_finish(QemuConsole *con, struct qmp_screendump *dump)
> +{
> +    Error *err = NULL;
> +    DisplaySurface *surface;
> +    Monitor *prev_mon = cur_mon;

Why this is needed?

> +        /*
> +         * FIXME: async save with coroutine? it would have to copy or
> +         * lock the surface.
> +         */
> +        ppm_save(dump->filename, surface, &err);

DisplaySurface is just a thin layer above pixman images these days.
Pixman images are reference counted, so you can
pixman_image_ref(surface->image) to make sure it doesn't disappear
underneath you, then pass the pixman image to ppm_save.

cheers,
  Gerd
Marc-André Lureau July 15, 2019, 7:09 p.m. UTC | #2
On Wed, Apr 10, 2019 at 12:49 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > +static void qmp_screendump_finish(QemuConsole *con, struct qmp_screendump *dump)
> > +{
> > +    Error *err = NULL;
> > +    DisplaySurface *surface;
> > +    Monitor *prev_mon = cur_mon;
>
> Why this is needed?
>

ppm_save() calls qemu_open() which may lookup fd associated to the
monitor: monitor_fdset_get_fd().

Interestingly, it seems fdset are not coupled with the current
monitor, so it's probably unnecessary to update the monitor to the one
associated with the command invocation.

> > +        /*
> > +         * FIXME: async save with coroutine? it would have to copy or
> > +         * lock the surface.
> > +         */
> > +        ppm_save(dump->filename, surface, &err);
>
> DisplaySurface is just a thin layer above pixman images these days.
> Pixman images are reference counted, so you can
> pixman_image_ref(surface->image) to make sure it doesn't disappear
> underneath you, then pass the pixman image to ppm_save.

ppm_save() is still synchronous. I suppose you suggested that for a
future async version. (note that in this case, ref the surface is
probably not sufficient, as it could be mutated while it is being
saved)
Gerd Hoffmann July 30, 2019, 11:23 a.m. UTC | #3
> > > +        /*
> > > +         * FIXME: async save with coroutine? it would have to copy or
> > > +         * lock the surface.
> > > +         */
> > > +        ppm_save(dump->filename, surface, &err);
> >
> > DisplaySurface is just a thin layer above pixman images these days.
> > Pixman images are reference counted, so you can
> > pixman_image_ref(surface->image) to make sure it doesn't disappear
> > underneath you, then pass the pixman image to ppm_save.
> 
> ppm_save() is still synchronous. I suppose you suggested that for a
> future async version.

Yes.

> (note that in this case, ref the surface is
> probably not sufficient, as it could be mutated while it is being
> saved)

That can happen anyway.  The display surface / pixman image can be backed
by guest-writable memory (stdvga vram for example) and even when holding
the qemu lock the guest vcpu can write there ...

cheers,
  GerdY
diff mbox series

Patch

diff --git a/qapi/ui.json b/qapi/ui.json
index 59e412139a..cbb3979172 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 9415adadfe..2649c3cbfe 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -74,6 +74,9 @@  typedef struct MouseTransformInfo {
 } 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 8eec768088..9428c90c5d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2380,7 +2380,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 3a7f71021f..620cd14b80 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;
 
@@ -166,6 +173,8 @@  struct QemuConsole {
     uint8_t out_fifo_buf[16];
     QEMUTimer *kbd_timer;
 
+    QLIST_HEAD(, qmp_screendump) qmp_screendumps;
+
     QTAILQ_ENTRY(QemuConsole) next;
 };
 
@@ -192,6 +201,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)
 {
@@ -258,8 +269,48 @@  static void gui_setup_refresh(DisplayState *ds)
     ds->have_text = have_text;
 }
 
+static void qmp_screendump_finish(QemuConsole *con, struct qmp_screendump *dump)
+{
+    Error *err = NULL;
+    DisplaySurface *surface;
+    Monitor *prev_mon = cur_mon;
+
+    if (qmp_return_is_cancelled(dump->ret)) {
+        goto cleanup;
+    }
+
+    cur_mon = qmp_return_get_monitor(dump->ret);
+    surface = qemu_console_surface(con);
+    if (!surface) {
+        error_setg(&err, "no surface");
+    } else {
+        /*
+         * FIXME: async save with coroutine? it would have to copy or
+         * lock the surface.
+         */
+        ppm_save(dump->filename, surface, &err);
+    }
+
+    if (err) {
+        qmp_return_error(dump->ret, err);
+    } else {
+        qmp_screendump_return(dump->ret);
+    }
+    cur_mon = prev_mon;
+
+cleanup:
+    g_free(dump->filename);
+    QLIST_REMOVE(dump, link);
+    g_free(dump);
+}
+
 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);
+    }
 }
 
 void graphic_hw_update(QemuConsole *con)
@@ -355,30 +406,41 @@  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);
     if (!surface) {
@@ -389,6 +451,28 @@  void qmp_screendump(const char *filename, bool has_device, const char *device,
     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;
+    struct qmp_screendump *dump = NULL;
+    QemuConsole *con = get_console(has_device, device, has_head, head, &err);
+
+    if (!con) {
+        qmp_return_error(qret, err);
+        return;
+    }
+
+    dump = g_new(struct qmp_screendump, 1);
+    dump->filename = g_strdup(filename);
+    dump->ret = qret;
+    QLIST_INSERT_HEAD(&con->qmp_screendumps, dump, link);
+
+    graphic_hw_update(con);
+}
+
 void graphic_hw_text_update(QemuConsole *con, console_ch_t *chardata)
 {
     if (!con) {
@@ -1293,6 +1377,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,