Re: [Qemu-devel] [RfC PATCH 2/2] console: add screendump-device qmp cmd

[Anthony Liguori]

Gerd Hoffmann <address@hidden> writes:

> Adds a screendump-device qmp command, which has an additional 'device'
> parameter.  This way it is possible to specify the device you want a
> screendump from.
>
> For the hmp monitor an optional device parameter has been added to the
> esisting screendump command.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=903910
>
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>  qapi-schema.json |   15 +++++++++++++++
>  qmp-commands.hx  |   25 +++++++++++++++++++++++++
>  ui/console.c     |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 92 insertions(+)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index adcf801..719dc6e 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3125,6 +3125,21 @@
>  { 'command': 'screendump', 'data': {'filename': 'str'} }
>  
>  ##
> +# @screendump-device:
> +#
> +# Write a PPM from the specified device to a file.
> +#
> +# @filename: the path of a new PPM file to store the image
> +# @device: #optional device to take the screenshot from
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 1.6
> +##
> +{ 'command': 'screendump-device', 'data': {'filename': 'str',
> +                                           '*device' : '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 8e69fba..6361561 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -167,6 +167,31 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "screendump-device",
> +        .args_type  = "filename:F,device:B?",
> +        .mhandler.cmd_new = qmp_marshal_input_screendump_device,
> +    },
> +
> +SQMP
> +screendump-device
> +-----------------
> +
> +Save screen into PPM image.
> +
> +Arguments:
> +
> +- "filename": file path (json-string)
> +- "device": video device (json-string, optional)
> +
> +Example:
> +
> +-> { "execute": "screendump-device",
> +     "arguments": { "filename": "/tmp/image", "device" : "video0" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "stop",
>          .args_type  = "",
>          .mhandler.cmd_new = qmp_marshal_input_stop,
> diff --git a/ui/console.c b/ui/console.c
> index 020805c..1895acf 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -343,6 +343,58 @@ void qmp_screendump(const char *filename, Error **errp)
>      ppm_save(filename, surface, errp);
>  }
>  
> +struct screendump_job {
> +    QEMUBH *bh;
> +    QemuConsole *con;
> +    char *filename;
> +};

We have a job API in the block layer.  Would it make sense to have a
QMP-level job interface?

I don't think we want to replace the block layer API, but I think having
a simple interface that returns a job id, allows querying jobs
(including whether they are cancelable), canceling cancelable jobs, and
then a single event notifying completion of jobs, would solve a lot of
problems in the current interface.

Regards,

Anthony Liguori

> +static void qmp_screendump_bh(void *opaque)
> +{
> +    Error *local_err;
> +    struct screendump_job *j = opaque;
> +    DisplaySurface *surface;
> +
> +    surface = qemu_console_surface(j->con);
> +    ppm_save(j->filename, surface, &local_err);
> +    /* TODO: send qmp completion (or error) event */
> +    qemu_bh_delete(j->bh);
> +    free(j->filename);
> +    free(j);
> +}
> +
> +void qmp_screendump_device(const char *filename,
> +                           bool has_device, const char *device, Error **errp)
> +{
> +    struct screendump_job *j;
> +    QemuConsole *con;
> +
> +    if (has_device) {
> +        DeviceState *dev = qdev_find_recursive(sysbus_get_default(), device);
> +        if (NULL == dev) {
> +            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +            return;
> +        }
> +        con = qemu_console_lookup_by_device(dev);
> +        if (NULL == con) {
> +            error_setg(errp, "There is no QemuConsole linked to %s", device);
> +            return;
> +        }
> +    } else {
> +        con = qemu_console_lookup_by_index(0);
> +        if (con == NULL) {
> +            error_setg(errp, "There is no QemuConsole I can screendump 
> from.");
> +            return;
> +        }
> +    }
> +
> +    j = g_new0(struct screendump_job, 1);
> +    j->bh = qemu_bh_new(qmp_screendump_bh, j);
> +    j->con = con;
> +    j->filename = g_strdup(filename);
> +    graphic_hw_update_notify(con, j->bh);
> +}
> +
>  void graphic_hw_text_update(QemuConsole *con, console_ch_t *chardata)
>  {
>      if (!con) {
> -- 
> 1.7.9.7