Re: [Qemu-devel] [PATCH] make screendump an async command

[Anthony Liguori <aliguori@us.ibm.com>]

Alon Levy <alevy@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 <alevy@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