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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon, 17 Jun 2013 09:43:07 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> Gerd Hoffmann <kraxel@redhat.com> 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 <kraxel@redhat.com>
> > ---
> >  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'd agree with this.

> 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
>