[Qemu-devel] [RfC PATCH 0/2] new screendump qmp command

[Qemu-devel] [RfC PATCH 0/2] new screendump qmp command

[Gerd Hoffmann]

  Hi,

Just a rough + incomplete draft for now, to show the idea.

Series adds a new screendump qmp command.  This does (a) allow
to specify the device we want dump from, (b) does the actual
image writing in a bottom half and (c) sends a qmp event when
(not done yet, just a comment for now).

It also adds a graphics_hw_update() variant with completion
notification (by calling a bottom half).

qxl then could stick the pointer to the notification BH into the
QXLCookie struct and schedule the BH when spice server finished
updating the screen.

Gerd Hoffmann (2):
  display update with notification
  console: add screendump-device qmp cmd

 include/ui/console.h |    2 ++
 qapi-schema.json     |   15 +++++++++++
 qmp-commands.hx      |   25 +++++++++++++++++++
 ui/console.c         |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 109 insertions(+)

-- 
1.7.9.7

[Qemu-devel] [RfC PATCH 1/2] display update with notification

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/ui/console.h |    2 ++
 ui/console.c         |   15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/ui/console.h b/include/ui/console.h
index 092b9be..4860687 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -273,6 +273,7 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
 typedef struct GraphicHwOps {
     void (*invalidate)(void *opaque);
     void (*gfx_update)(void *opaque);
+    void (*gfx_update_notify)(void *opaque, QEMUBH *notify);
     void (*text_update)(void *opaque, console_ch_t *text);
     void (*update_interval)(void *opaque, uint64_t interval);
 } GraphicHwOps;
@@ -282,6 +283,7 @@ QemuConsole *graphic_console_init(DeviceState *dev,
                                   void *opaque);
 
 void graphic_hw_update(QemuConsole *con);
+void graphic_hw_update_notify(QemuConsole *con, QEMUBH *notify);
 void graphic_hw_invalidate(QemuConsole *con);
 void graphic_hw_text_update(QemuConsole *con, console_ch_t *chardata);
 
diff --git a/ui/console.c b/ui/console.c
index 07d4d63..020805c 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -255,6 +255,21 @@ void graphic_hw_update(QemuConsole *con)
     }
 }
 
+void graphic_hw_update_notify(QemuConsole *con, QEMUBH *notify)
+{
+    assert(con != NULL);
+
+    if (con->hw_ops->gfx_update_notify) {
+        con->hw_ops->gfx_update_notify(con->hw, notify);
+        return;
+    }
+
+    if (con->hw_ops->gfx_update) {
+        con->hw_ops->gfx_update(con->hw);
+    }
+    qemu_bh_schedule(notify);
+}
+
 void graphic_hw_invalidate(QemuConsole *con)
 {
     if (!con) {
-- 
1.7.9.7

Re: [Qemu-devel] [RfC PATCH 1/2] display update with notification

[Anthony Liguori]

Little more context please.

Regards,

Anthony Liguori

Gerd Hoffmann <kraxel@redhat.com> writes:

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/ui/console.h |    2 ++
>  ui/console.c         |   15 +++++++++++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 092b9be..4860687 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -273,6 +273,7 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
>  typedef struct GraphicHwOps {
>      void (*invalidate)(void *opaque);
>      void (*gfx_update)(void *opaque);
> +    void (*gfx_update_notify)(void *opaque, QEMUBH *notify);
>      void (*text_update)(void *opaque, console_ch_t *text);
>      void (*update_interval)(void *opaque, uint64_t interval);
>  } GraphicHwOps;
> @@ -282,6 +283,7 @@ QemuConsole *graphic_console_init(DeviceState *dev,
>                                    void *opaque);
>  
>  void graphic_hw_update(QemuConsole *con);
> +void graphic_hw_update_notify(QemuConsole *con, QEMUBH *notify);
>  void graphic_hw_invalidate(QemuConsole *con);
>  void graphic_hw_text_update(QemuConsole *con, console_ch_t *chardata);
>  
> diff --git a/ui/console.c b/ui/console.c
> index 07d4d63..020805c 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -255,6 +255,21 @@ void graphic_hw_update(QemuConsole *con)
>      }
>  }
>  
> +void graphic_hw_update_notify(QemuConsole *con, QEMUBH *notify)
> +{
> +    assert(con != NULL);
> +
> +    if (con->hw_ops->gfx_update_notify) {
> +        con->hw_ops->gfx_update_notify(con->hw, notify);
> +        return;
> +    }
> +
> +    if (con->hw_ops->gfx_update) {
> +        con->hw_ops->gfx_update(con->hw);
> +    }
> +    qemu_bh_schedule(notify);
> +}
> +
>  void graphic_hw_invalidate(QemuConsole *con)
>  {
>      if (!con) {
> -- 
> 1.7.9.7

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

[Gerd Hoffmann]

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;
+};
+
+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

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

[Anthony Liguori]

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

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

[Luiz Capitulino]

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
> 

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

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

On 06/17/13 16:50, Luiz Capitulino wrote:
>>> > > +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.

Something like the attached patch?  Which is just the bare minimum I'll
need for screendump.  Basically a one-off bottom half with some monitor
infrastructure (job id, error handling).  So it isn't for big jobs, but
for small jobs which have to wait for something before they execute
(spice-server, guest action, whatever).

If you wanna some more context: screendump on top of that is here:
https://www.kraxel.org/cgit/qemu/log/?h=rebase/pixman

[ not tested yet ]

cheers,
  Gerd

[-- Attachment #2: 0001-draft-monitor-job.patch --]
[-- Type: text/plain, Size: 3882 bytes --]

>From bf1f7ec8a2baacc497e188ac38b24e5b0387b143 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 18 Jun 2013 09:21:03 +0200
Subject: [PATCH 1/3] [draft] monitor job

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/monitor/monitor.h |   10 +++++++
 monitor.c                 |   66 +++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json          |   10 +++++++
 3 files changed, 86 insertions(+)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 1a6cfcf..6f5ec7e 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -47,6 +47,7 @@ typedef enum MonitorEvent {
     QEVENT_BALLOON_CHANGE,
     QEVENT_SPICE_MIGRATE_COMPLETED,
     QEVENT_GUEST_PANICKED,
+    QEVENT_JOB_COMPLETED,
 
     /* Add to 'monitor_event_names' array in monitor.c when
      * defining new events here */
@@ -100,4 +101,13 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
 int monitor_fdset_dup_fd_remove(int dup_fd);
 int monitor_fdset_dup_fd_find(int dup_fd);
 
+typedef struct monitor_job monitor_job;
+typedef void (*monitor_job_func)(void *opaque, Error **errp);
+
+uint64_t monitor_job_new_id(void);
+monitor_job *monitor_job_alloc(uint64_t id, monitor_job_func func,
+                               void *opaque);
+void monitor_job_queue(monitor_job *job);
+void monitor_job_cancel(monitor_job *job);
+
 #endif /* !MONITOR_H */
diff --git a/monitor.c b/monitor.c
index 70ae8f5..9a25c1e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -496,6 +496,7 @@ static const char *monitor_event_names[] = {
     [QEVENT_BALLOON_CHANGE] = "BALLOON_CHANGE",
     [QEVENT_SPICE_MIGRATE_COMPLETED] = "SPICE_MIGRATE_COMPLETED",
     [QEVENT_GUEST_PANICKED] = "GUEST_PANICKED",
+    [QEVENT_JOB_COMPLETED] = "JOB_COMPLETED",
 };
 QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
 
@@ -4863,3 +4864,68 @@ QemuOptsList qemu_mon_opts = {
         { /* end of list */ }
     },
 };
+
+struct monitor_job {
+    uint64_t id;
+    monitor_job_func func;
+    void *opaque;
+    QEMUBH *bh;
+};
+
+static void monitor_job_event(monitor_job *job, const char *result, Error *err)
+{
+    QObject *data;
+
+    data = qobject_from_jsonf("{ 'job-id': %" PRId64 ", 'result': %s }",
+                              job->id, result);
+    if (error_is_set(&err)) {
+        /* TODO: add error details */
+    }
+    monitor_protocol_event(QEVENT_JOB_COMPLETED, data);
+    qobject_decref(data);
+}
+
+static void monitor_job_bh(void *opaque)
+{
+    monitor_job *job = opaque;
+    Error *local_err = NULL;
+    const char *result;
+
+    job->func(job->opaque, &local_err);
+    result = error_is_set(&local_err) ? "success" : "failure";
+    monitor_job_event(job, result, local_err);
+    qemu_bh_delete(job->bh);
+    g_free(job);
+}
+
+uint64_t monitor_job_new_id(void)
+{
+    static uint64_t nextid = 1;
+    return nextid++;
+}
+
+monitor_job *monitor_job_alloc(uint64_t id, monitor_job_func func,
+                               void *opaque)
+{
+    monitor_job *job = g_new0(monitor_job, 1);
+
+    job->id = id;
+    job->func = func;
+    job->opaque = opaque;
+    return job;
+}
+
+void monitor_job_queue(monitor_job *job)
+{
+    job->bh = qemu_bh_new(monitor_job_bh, job);
+    qemu_bh_schedule(job->bh);
+}
+
+void monitor_job_cancel(monitor_job *job)
+{
+    monitor_job_event(job, "canceled", NULL);
+    if (job->bh) {
+        qemu_bh_delete(job->bh);
+    }
+    g_free(job);
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index aced724..a449a43 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2515,6 +2515,16 @@
     'str': 'str' } }
 
 ##
+# @MonitorJob
+#
+# Monitor job id.
+#
+# Since 1.6
+##
+{ 'type': 'MonitorJob',
+  'data': { 'job-id': 'int' } }
+
+##
 # @NetdevUserOptions
 #
 # Use the user mode network stack which requires no administrator privilege to
-- 
1.7.9.7

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

[Luiz Capitulino]

On Tue, 18 Jun 2013 11:24:54 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On 06/17/13 16:50, Luiz Capitulino wrote:
> >>> > > +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.
> 
> Something like the attached patch?  Which is just the bare minimum I'll
> need for screendump.  Basically a one-off bottom half with some monitor
> infrastructure (job id, error handling).  So it isn't for big jobs, but
> for small jobs which have to wait for something before they execute
> (spice-server, guest action, whatever).

I only skimmed over the patch, but you need QMP commands to cancel
and to query running jobs. Also, please move all this stuff to qmp.c
and do s/monitor/qmp rename.

Lastly, is it possible to have the block job QMP API on top of this
new QMP job API?

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

[Gerd Hoffmann]

  Hi,

>> Something like the attached patch?  Which is just the bare minimum I'll
>> need for screendump.  Basically a one-off bottom half with some monitor
>> infrastructure (job id, error handling).  So it isn't for big jobs, but
>> for small jobs which have to wait for something before they execute
>> (spice-server, guest action, whatever).
> 
> I only skimmed over the patch, but you need QMP commands to cancel
> and to query running jobs.

Sure, can easily go on top, just need to stuff the monitor_jobs into a
list and allow ops on it.  Just want to make sure the direction I'm
heading to is fine.

> Also, please move all this stuff to qmp.c
> and do s/monitor/qmp rename.

Will do.

> Lastly, is it possible to have the block job QMP API on top of this
> new QMP job API?

Don't think so.  As mentioned above this does short-running jobs as
bottom half whereas block jobs are running in coroutines ...

cheers,
  Gerd

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

[Stefan Hajnoczi]

On Tue, Jun 18, 2013 at 03:24:33PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >> Something like the attached patch?  Which is just the bare minimum I'll
> >> need for screendump.  Basically a one-off bottom half with some monitor
> >> infrastructure (job id, error handling).  So it isn't for big jobs, but
> >> for small jobs which have to wait for something before they execute
> >> (spice-server, guest action, whatever).
> > 
> > I only skimmed over the patch, but you need QMP commands to cancel
> > and to query running jobs.
> 
> Sure, can easily go on top, just need to stuff the monitor_jobs into a
> list and allow ops on it.  Just want to make sure the direction I'm
> heading to is fine.
> 
> > Also, please move all this stuff to qmp.c
> > and do s/monitor/qmp rename.
> 
> Will do.
> 
> > Lastly, is it possible to have the block job QMP API on top of this
> > new QMP job API?
> 
> Don't think so.  As mentioned above this does short-running jobs as
> bottom half whereas block jobs are running in coroutines ...

Maybe we really should do a proper QMP jobs API.  Using a BH makes sense
for the screenshot case but is orthogonal to the QMP job API.

It may be possible to map block jobs onto the QMP jobs API.  Of course
the blockjob-specific QMP APIs will remain for compatibility.

Here's a quick overview of blockjob commands to give a feel for their
scope:

drive-mirror <args...>

Starts a block job and returns nothing.  Note that blockjobs do not have
ids because they are bound 1:1 with a block device.  This was a design
mistake, we should really allow for multiple jobs per block device.

query-block-jobs

Return info on active block jobs.

block-job-cancel

Mark a block job as cancelled.  It will cancel at the earliest
opportunity.

QMP Events BLOCK_JOB_COMPLETED and BLOCK_JOB_CANCELLED

Async events.

QMP Event BLOCK_JOB_ERROR

Error reporting with BlockDevOnError error handling policy information
(i.e. is the job paused waiting for the administrator to fix storage and
then continue it?).

Stefan

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

[Gerd Hoffmann]

  Hi,

> Maybe we really should do a proper QMP jobs API.  Using a BH makes sense
> for the screenshot case but is orthogonal to the QMP job API.

I'll drop that from my todo list for now.  No way I can finish this off
before my family summer vacation.  I'm also hoping that someone who
knows qapi + jobs stuff better than me picks this up.  Should that not
happen I might be able to have a closer look somewhen in august ...

cheers,
  Gerd

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]

On 06/18/2013 03:24 AM, Gerd Hoffmann wrote:

>>>> >> > 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.
> Something like the attached patch?  Which is just the bare minimum I'll
> need for screendump.  Basically a one-off bottom half with some monitor
> infrastructure (job id, error handling).  So it isn't for big jobs, but
> for small jobs which have to wait for something before they execute
> (spice-server, guest action, whatever).
> 
> If you wanna some more context: screendump on top of that is here:
> https://www.kraxel.org/cgit/qemu/log/?h=rebase/pixman
> 

> +
> +void monitor_job_cancel(monitor_job *job)
> +{
> +    monitor_job_event(job, "canceled", NULL);
> +    if (job->bh) {
> +        qemu_bh_delete(job->bh);
> +    }
> +    g_free(job);
> +}

What calls this command?

> diff --git a/qapi-schema.json b/qapi-schema.json
> index aced724..a449a43 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2515,6 +2515,16 @@
>      'str': 'str' } }
>  
>  ##
> +# @MonitorJob
> +#
> +# Monitor job id.
> +#

No mention of @job-id?

> +# Since 1.6
> +##
> +{ 'type': 'MonitorJob',
> +  'data': { 'job-id': 'int' } }

For a type, this looks okay; but where are the commands that use this
type?  I take it new commands (like screendump) would return a
MonitorJob, and that we need a query-monitor-jobs as well as a
monitor-job-cancel.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2155 bytes --]

On 06/17/2013 08:01 AM, Gerd Hoffmann wrote:
> 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' }}

Does this command need to return a job id, and does there need to be an
event when it is completed?  Can filename include /dev/fdset (that is,
the file is opened via qemu_open, so we can use fd passing to pass in a
pipe fd rather than requiring a trip through the filesystem)?


> +
> +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);

Yes, ppm_save uses qemu_open(), so fd passing should be possible.

> +    /* TODO: send qmp completion (or error) event */

Aha, this explains why it is still rfc.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]