[Qemu-devel] [PATCH 1/4] qapi-schema: fix typos and explain 'spice' auth

[Qemu-devel] [PATCH 1/4] qapi-schema: fix typos and explain 'spice' auth

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 qapi-schema.json |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index d0b6792..72b17f1 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -616,12 +616,13 @@
 # @connection-id: SPICE connection id number.  All channels with the same id
 #                 belong to the same SPICE session.
 #
-# @connection-type: SPICE channel type number.  "1" is the main control channel,
-#                   filter for this one if you want track spice sessions only
+# @connection-type: SPICE channel type number.  "1" is the main control
+#                   channel, filter for this one if you want to track spice
+#                   sessions only
 #
-# @channel-id: SPICE channel ID number.  Usually "0", might be different needed
-#              when multiple channels of the same type exist, such as multiple
-#              display channels in a multihead setup
+# @channel-id: SPICE channel ID number.  Usually "0", might be different when
+#                   multiple channels of the same type exist, such as multiple
+#                   display channels in a multihead setup
 #
 # @tls: true if the channel is encrypted, false otherwise.
 #
@@ -649,8 +650,9 @@
 # @tls-port: #optional The SPICE server's TLS port number.
 #
 # @auth: #optional the current authentication type used by the server
-#        'none' if no authentication is being used
-#        'spice' (TODO: describe)
+#        'none'  if no authentication is being used
+#        'spice' uses SASL or direct TLS authentication, depending on command
+#                line options
 #
 # @channels: a list of @SpiceChannel for each active spice channel
 #
@@ -1216,7 +1218,7 @@
 { 'command': 'migrate_set_speed', 'data': {'value': 'int'} }
 
 ##
-# @DevicePropertyInfo:
+# @ObjectPropertyInfo:
 #
 # @name: the name of the property
 #
-- 
1.7.9.1

[Qemu-devel] [PATCH 2/4] qjson.h: include compiler.h for GCC_FMT_ATTR

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 qjson.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/qjson.h b/qjson.h
index 65b10ea..1190d8a 100644
--- a/qjson.h
+++ b/qjson.h
@@ -15,6 +15,7 @@
 #define QJSON_H
 
 #include <stdarg.h>
+#include "compiler.h"
 #include "qobject.h"
 #include "qstring.h"
 
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 2/4] qjson.h: include compiler.h for GCC_FMT_ATTR

[Luiz Capitulino]

On Fri, 24 Feb 2012 23:22:03 +0200
Alon Levy <alevy@redhat.com> wrote:

> Signed-off-by: Alon Levy <alevy@redhat.com>

Acked-by: Luiz Capitulino <lcapitulino@redhat.com>

Or, I can apply it in my queue if you allow me to cherry pick.


> ---
>  qjson.h |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/qjson.h b/qjson.h
> index 65b10ea..1190d8a 100644
> --- a/qjson.h
> +++ b/qjson.h
> @@ -15,6 +15,7 @@
>  #define QJSON_H
>  
>  #include <stdarg.h>
> +#include "compiler.h"
>  #include "qobject.h"
>  #include "qstring.h"
>  

Re: [Qemu-devel] [PATCH 2/4] qjson.h: include compiler.h for GCC_FMT_ATTR

[Alon Levy]

On Tue, Feb 28, 2012 at 04:58:46PM -0300, Luiz Capitulino wrote:
> On Fri, 24 Feb 2012 23:22:03 +0200
> Alon Levy <alevy@redhat.com> wrote:
> 
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> 
> Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
> 
> Or, I can apply it in my queue if you allow me to cherry pick.
> 

Sure.

> 
> > ---
> >  qjson.h |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/qjson.h b/qjson.h
> > index 65b10ea..1190d8a 100644
> > --- a/qjson.h
> > +++ b/qjson.h
> > @@ -15,6 +15,7 @@
> >  #define QJSON_H
> >  
> >  #include <stdarg.h>
> > +#include "compiler.h"
> >  #include "qobject.h"
> >  #include "qstring.h"
> >  
> 

[Qemu-devel] [PATCH 3/4] monitor, console: add QEVENT_SCREEN_DUMP_COMPLETE

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 QMP/qmp-events.txt |   14 ++++++++++++++
 console.c          |   18 ++++++++++++++++++
 console.h          |    1 +
 monitor.c          |    2 ++
 monitor.h          |    1 +
 5 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index 9286af5..0d1c9f4 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -335,3 +335,17 @@ Example:
                "len": 10737418240, "offset": 134217728,
                "speed": 0 },
      "timestamp": { "seconds": 1267061043, "microseconds": 959568 } }
+
+SCREEN_DUMP_COMPLETE
+--------------------
+
+Emitted when screen-dump-async completes.
+
+Data:
+
+- "filename": Name of file containing screen dump (json-string)
+
+Example:
+
+{ "event": "SCREEN_DUMP_COMPLETE",
+     "data": { "filename": "/tmp/a.ppm" } }
diff --git a/console.c b/console.c
index 6a463f5..6750538 100644
--- a/console.c
+++ b/console.c
@@ -24,6 +24,8 @@
 #include "qemu-common.h"
 #include "console.h"
 #include "qemu-timer.h"
+#include "qjson.h"
+#include "monitor.h"
 
 //#define DEBUG_CONSOLE
 #define DEFAULT_BACKSCROLL 512
@@ -1707,3 +1709,19 @@ PixelFormat qemu_default_pixelformat(int bpp)
     }
     return pf;
 }
+
+void monitor_protocol_screen_dump_complete_event(const char *filename)
+{
+    QObject *event_data;
+    /*
+     * TODO: Only good for a single user, or a very aware user
+     * that changes the filename to distinguish between different
+     * screendumps.
+     * Should include support for different monitors on the same device,
+     * and for different devices. Got stuck trying to figure a canonical string
+     * represnetation of a device.
+     */
+    event_data = qobject_from_jsonf("{ 'filename': %s }", filename);
+    monitor_protocol_event(QEVENT_SCREEN_DUMP_COMPLETE, event_data);
+    qobject_decref(event_data);
+}
diff --git a/console.h b/console.h
index a95b581..c22803c 100644
--- a/console.h
+++ b/console.h
@@ -353,6 +353,7 @@ void vga_hw_update(void);
 void vga_hw_invalidate(void);
 void vga_hw_screen_dump(const char *filename);
 void vga_hw_text_update(console_ch_t *chardata);
+void monitor_protocol_screen_dump_complete_event(const char *filename);
 
 int is_graphic_console(void);
 int is_fixedsize_console(void);
diff --git a/monitor.c b/monitor.c
index 953e748..1a65c41 100644
--- a/monitor.c
+++ b/monitor.c
@@ -493,6 +493,8 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
             break;
         case QEVENT_WAKEUP:
             event_name = "WAKEUP";
+        case QEVENT_SCREEN_DUMP_COMPLETE:
+            event_name = "SCREEN_DUMP_COMPLETE";
             break;
         default:
             abort();
diff --git a/monitor.h b/monitor.h
index 0d49800..227ebf2 100644
--- a/monitor.h
+++ b/monitor.h
@@ -41,6 +41,7 @@ typedef enum MonitorEvent {
     QEVENT_DEVICE_TRAY_MOVED,
     QEVENT_SUSPEND,
     QEVENT_WAKEUP,
+    QEVENT_SCREEN_DUMP_COMPLETE,
     QEVENT_MAX,
 } MonitorEvent;
 
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 3/4] monitor, console: add QEVENT_SCREEN_DUMP_COMPLETE

[Luiz Capitulino]

On Fri, 24 Feb 2012 23:22:04 +0200
Alon Levy <alevy@redhat.com> wrote:

> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  QMP/qmp-events.txt |   14 ++++++++++++++
>  console.c          |   18 ++++++++++++++++++
>  console.h          |    1 +
>  monitor.c          |    2 ++
>  monitor.h          |    1 +
>  5 files changed, 36 insertions(+), 0 deletions(-)
> 
> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> index 9286af5..0d1c9f4 100644
> --- a/QMP/qmp-events.txt
> +++ b/QMP/qmp-events.txt
> @@ -335,3 +335,17 @@ Example:
>                 "len": 10737418240, "offset": 134217728,
>                 "speed": 0 },
>       "timestamp": { "seconds": 1267061043, "microseconds": 959568 } }
> +
> +SCREEN_DUMP_COMPLETE
> +--------------------
> +
> +Emitted when screen-dump-async completes.
> +
> +Data:
> +
> +- "filename": Name of file containing screen dump (json-string)
> +
> +Example:
> +
> +{ "event": "SCREEN_DUMP_COMPLETE",
> +     "data": { "filename": "/tmp/a.ppm" } }

This example is incomplete, a QMP event contains more fields.

> diff --git a/console.c b/console.c
> index 6a463f5..6750538 100644
> --- a/console.c
> +++ b/console.c
> @@ -24,6 +24,8 @@
>  #include "qemu-common.h"
>  #include "console.h"
>  #include "qemu-timer.h"
> +#include "qjson.h"
> +#include "monitor.h"
>  
>  //#define DEBUG_CONSOLE
>  #define DEFAULT_BACKSCROLL 512
> @@ -1707,3 +1709,19 @@ PixelFormat qemu_default_pixelformat(int bpp)
>      }
>      return pf;
>  }
> +
> +void monitor_protocol_screen_dump_complete_event(const char *filename)
> +{
> +    QObject *event_data;
> +    /*
> +     * TODO: Only good for a single user, or a very aware user
> +     * that changes the filename to distinguish between different
> +     * screendumps.

This comment pertains to the event documentation.

> +     * Should include support for different monitors on the same device,
> +     * and for different devices. Got stuck trying to figure a canonical string
> +     * represnetation of a device.
> +     */

I think the above comment is not worth it.

> +    event_data = qobject_from_jsonf("{ 'filename': %s }", filename);
> +    monitor_protocol_event(QEVENT_SCREEN_DUMP_COMPLETE, event_data);
> +    qobject_decref(event_data);
> +}
> diff --git a/console.h b/console.h
> index a95b581..c22803c 100644
> --- a/console.h
> +++ b/console.h
> @@ -353,6 +353,7 @@ void vga_hw_update(void);
>  void vga_hw_invalidate(void);
>  void vga_hw_screen_dump(const char *filename);
>  void vga_hw_text_update(console_ch_t *chardata);
> +void monitor_protocol_screen_dump_complete_event(const char *filename);
>  
>  int is_graphic_console(void);
>  int is_fixedsize_console(void);
> diff --git a/monitor.c b/monitor.c
> index 953e748..1a65c41 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -493,6 +493,8 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
>              break;
>          case QEVENT_WAKEUP:
>              event_name = "WAKEUP";
> +        case QEVENT_SCREEN_DUMP_COMPLETE:
> +            event_name = "SCREEN_DUMP_COMPLETE";
>              break;
>          default:
>              abort();
> diff --git a/monitor.h b/monitor.h
> index 0d49800..227ebf2 100644
> --- a/monitor.h
> +++ b/monitor.h
> @@ -41,6 +41,7 @@ typedef enum MonitorEvent {
>      QEVENT_DEVICE_TRAY_MOVED,
>      QEVENT_SUSPEND,
>      QEVENT_WAKEUP,
> +    QEVENT_SCREEN_DUMP_COMPLETE,
>      QEVENT_MAX,
>  } MonitorEvent;
>  

Re: [Qemu-devel] [PATCH 3/4] monitor, console: add QEVENT_SCREEN_DUMP_COMPLETE

[Alon Levy]

On Tue, Feb 28, 2012 at 05:01:13PM -0300, Luiz Capitulino wrote:
> On Fri, 24 Feb 2012 23:22:04 +0200
> Alon Levy <alevy@redhat.com> wrote:
> 
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> >  QMP/qmp-events.txt |   14 ++++++++++++++
> >  console.c          |   18 ++++++++++++++++++
> >  console.h          |    1 +
> >  monitor.c          |    2 ++
> >  monitor.h          |    1 +
> >  5 files changed, 36 insertions(+), 0 deletions(-)
> > 
> > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> > index 9286af5..0d1c9f4 100644
> > --- a/QMP/qmp-events.txt
> > +++ b/QMP/qmp-events.txt
> > @@ -335,3 +335,17 @@ Example:
> >                 "len": 10737418240, "offset": 134217728,
> >                 "speed": 0 },
> >       "timestamp": { "seconds": 1267061043, "microseconds": 959568 } }
> > +
> > +SCREEN_DUMP_COMPLETE
> > +--------------------
> > +
> > +Emitted when screen-dump-async completes.
> > +
> > +Data:
> > +
> > +- "filename": Name of file containing screen dump (json-string)
> > +
> > +Example:
> > +
> > +{ "event": "SCREEN_DUMP_COMPLETE",
> > +     "data": { "filename": "/tmp/a.ppm" } }
> 
> This example is incomplete, a QMP event contains more fields.

timestamp, right?

> 
> > diff --git a/console.c b/console.c
> > index 6a463f5..6750538 100644
> > --- a/console.c
> > +++ b/console.c
> > @@ -24,6 +24,8 @@
> >  #include "qemu-common.h"
> >  #include "console.h"
> >  #include "qemu-timer.h"
> > +#include "qjson.h"
> > +#include "monitor.h"
> >  
> >  //#define DEBUG_CONSOLE
> >  #define DEFAULT_BACKSCROLL 512
> > @@ -1707,3 +1709,19 @@ PixelFormat qemu_default_pixelformat(int bpp)
> >      }
> >      return pf;
> >  }
> > +
> > +void monitor_protocol_screen_dump_complete_event(const char *filename)
> > +{
> > +    QObject *event_data;
> > +    /*
> > +     * TODO: Only good for a single user, or a very aware user
> > +     * that changes the filename to distinguish between different
> > +     * screendumps.
> 
> This comment pertains to the event documentation.

Move to documentation, check.

> 
> > +     * Should include support for different monitors on the same device,
> > +     * and for different devices. Got stuck trying to figure a canonical string
> > +     * represnetation of a device.
> > +     */
> 
> I think the above comment is not worth it.

This half of the comment? ok.

> 
> > +    event_data = qobject_from_jsonf("{ 'filename': %s }", filename);
> > +    monitor_protocol_event(QEVENT_SCREEN_DUMP_COMPLETE, event_data);
> > +    qobject_decref(event_data);
> > +}
> > diff --git a/console.h b/console.h
> > index a95b581..c22803c 100644
> > --- a/console.h
> > +++ b/console.h
> > @@ -353,6 +353,7 @@ void vga_hw_update(void);
> >  void vga_hw_invalidate(void);
> >  void vga_hw_screen_dump(const char *filename);
> >  void vga_hw_text_update(console_ch_t *chardata);
> > +void monitor_protocol_screen_dump_complete_event(const char *filename);
> >  
> >  int is_graphic_console(void);
> >  int is_fixedsize_console(void);
> > diff --git a/monitor.c b/monitor.c
> > index 953e748..1a65c41 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -493,6 +493,8 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
> >              break;
> >          case QEVENT_WAKEUP:
> >              event_name = "WAKEUP";
> > +        case QEVENT_SCREEN_DUMP_COMPLETE:
> > +            event_name = "SCREEN_DUMP_COMPLETE";
> >              break;
> >          default:
> >              abort();
> > diff --git a/monitor.h b/monitor.h
> > index 0d49800..227ebf2 100644
> > --- a/monitor.h
> > +++ b/monitor.h
> > @@ -41,6 +41,7 @@ typedef enum MonitorEvent {
> >      QEVENT_DEVICE_TRAY_MOVED,
> >      QEVENT_SUSPEND,
> >      QEVENT_WAKEUP,
> > +    QEVENT_SCREEN_DUMP_COMPLETE,
> >      QEVENT_MAX,
> >  } MonitorEvent;
> >  
> 

Re: [Qemu-devel] [PATCH 3/4] monitor, console: add QEVENT_SCREEN_DUMP_COMPLETE

[Luiz Capitulino]

On Tue, 28 Feb 2012 22:51:44 +0200
Alon Levy <alevy@redhat.com> wrote:

> On Tue, Feb 28, 2012 at 05:01:13PM -0300, Luiz Capitulino wrote:
> > On Fri, 24 Feb 2012 23:22:04 +0200
> > Alon Levy <alevy@redhat.com> wrote:
> > 
> > > Signed-off-by: Alon Levy <alevy@redhat.com>
> > > ---
> > >  QMP/qmp-events.txt |   14 ++++++++++++++
> > >  console.c          |   18 ++++++++++++++++++
> > >  console.h          |    1 +
> > >  monitor.c          |    2 ++
> > >  monitor.h          |    1 +
> > >  5 files changed, 36 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> > > index 9286af5..0d1c9f4 100644
> > > --- a/QMP/qmp-events.txt
> > > +++ b/QMP/qmp-events.txt
> > > @@ -335,3 +335,17 @@ Example:
> > >                 "len": 10737418240, "offset": 134217728,
> > >                 "speed": 0 },
> > >       "timestamp": { "seconds": 1267061043, "microseconds": 959568 } }
> > > +
> > > +SCREEN_DUMP_COMPLETE
> > > +--------------------
> > > +
> > > +Emitted when screen-dump-async completes.
> > > +
> > > +Data:
> > > +
> > > +- "filename": Name of file containing screen dump (json-string)
> > > +
> > > +Example:
> > > +
> > > +{ "event": "SCREEN_DUMP_COMPLETE",
> > > +     "data": { "filename": "/tmp/a.ppm" } }
> > 
> > This example is incomplete, a QMP event contains more fields.
> 
> timestamp, right?

Yes.

[Qemu-devel] [PATCH 4/4] add qmp screendump-async

[Alon Levy]

This is an across the board change since I wanted to keep the existing
(good imo) single graphic_console_init callback setter, instead of
introducing a new cb that isn't set by it but instead by a second
initialization function.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 console.c            |   25 +++++++++++++++++++++++--
 console.h            |    5 +++++
 hw/blizzard.c        |    2 +-
 hw/cirrus_vga.c      |    4 ++--
 hw/exynos4210_fimd.c |    3 ++-
 hw/g364fb.c          |    2 +-
 hw/jazz_led.c        |    3 ++-
 hw/milkymist-vgafb.c |    2 +-
 hw/musicpal.c        |    2 +-
 hw/omap_dss.c        |    4 +++-
 hw/omap_lcdc.c       |    2 +-
 hw/pl110.c           |    2 +-
 hw/pxa2xx_lcd.c      |    2 +-
 hw/qxl.c             |    3 ++-
 hw/sm501.c           |    4 ++--
 hw/ssd0303.c         |    2 +-
 hw/ssd0323.c         |    2 +-
 hw/tc6393xb.c        |    1 +
 hw/tcx.c             |    4 ++--
 hw/vga-isa-mm.c      |    3 ++-
 hw/vga-isa.c         |    3 ++-
 hw/vga-pci.c         |    3 ++-
 hw/vga_int.h         |    1 +
 hw/vmware_vga.c      |    3 ++-
 hw/xenfb.c           |    2 ++
 monitor.c            |    5 +++++
 qapi-schema.json     |   20 ++++++++++++++++++++
 qmp-commands.hx      |   26 ++++++++++++++++++++++++++
 28 files changed, 115 insertions(+), 25 deletions(-)

diff --git a/console.c b/console.c
index 6750538..ced2aa7 100644
--- a/console.c
+++ b/console.c
@@ -124,6 +124,7 @@ struct TextConsole {
     vga_hw_update_ptr hw_update;
     vga_hw_invalidate_ptr hw_invalidate;
     vga_hw_screen_dump_ptr hw_screen_dump;
+    vga_hw_screen_dump_async_ptr hw_screen_dump_async;
     vga_hw_text_update_ptr hw_text_update;
     void *hw;
 
@@ -175,8 +176,9 @@ void vga_hw_invalidate(void)
         active_console->hw_invalidate(active_console->hw);
 }
 
-void vga_hw_screen_dump(const char *filename)
+static void vga_hw_screen_dump_helper(const char *filename, bool async)
 {
+    bool event_sent = false;
     TextConsole *previous_active_console;
     bool cswitch;
 
@@ -188,17 +190,33 @@ void vga_hw_screen_dump(const char *filename)
     if (cswitch) {
         console_select(0);
     }
-    if (consoles[0] && consoles[0]->hw_screen_dump) {
+    if (async && consoles[0] && consoles[0]->hw_screen_dump_async) {
+        consoles[0]->hw_screen_dump_async(consoles[0]->hw, filename, cswitch);
+        event_sent = true;
+    } else if (consoles[0] && consoles[0]->hw_screen_dump) {
         consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
     } else {
         error_report("screen dump not implemented");
     }
+    if (async && !event_sent) {
+        monitor_protocol_screen_dump_complete_event(filename);
+    }
 
     if (cswitch) {
         console_select(previous_active_console->index);
     }
 }
 
+void vga_hw_screen_dump(const char *filename)
+{
+    vga_hw_screen_dump_helper(filename, false);
+}
+
+void vga_hw_screen_dump_async(const char *filename)
+{
+    vga_hw_screen_dump_helper(filename, true);
+}
+
 void vga_hw_text_update(console_ch_t *chardata)
 {
     if (active_console && active_console->hw_text_update)
@@ -1403,6 +1421,8 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
                                    vga_hw_invalidate_ptr invalidate,
                                    vga_hw_screen_dump_ptr screen_dump,
                                    vga_hw_text_update_ptr text_update,
+                                   vga_hw_screen_dump_async_ptr
+                                                    screen_dump_async,
                                    void *opaque)
 {
     TextConsole *s;
@@ -1421,6 +1441,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
     s->hw_update = update;
     s->hw_invalidate = invalidate;
     s->hw_screen_dump = screen_dump;
+    s->hw_screen_dump_async = screen_dump_async;
     s->hw_text_update = text_update;
     s->hw = opaque;
 
diff --git a/console.h b/console.h
index c22803c..3cf28c0 100644
--- a/console.h
+++ b/console.h
@@ -341,17 +341,22 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
 typedef void (*vga_hw_update_ptr)(void *);
 typedef void (*vga_hw_invalidate_ptr)(void *);
 typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
+typedef void (*vga_hw_screen_dump_async_ptr)(void *, const char *filename,
+                                             bool cswitch);
 typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
 
 DisplayState *graphic_console_init(vga_hw_update_ptr update,
                                    vga_hw_invalidate_ptr invalidate,
                                    vga_hw_screen_dump_ptr screen_dump,
                                    vga_hw_text_update_ptr text_update,
+                                   vga_hw_screen_dump_async_ptr
+                                                    screen_dump_async,
                                    void *opaque);
 
 void vga_hw_update(void);
 void vga_hw_invalidate(void);
 void vga_hw_screen_dump(const char *filename);
+void vga_hw_screen_dump_async(const char *filename);
 void vga_hw_text_update(console_ch_t *chardata);
 void monitor_protocol_screen_dump_complete_event(const char *filename);
 
diff --git a/hw/blizzard.c b/hw/blizzard.c
index c7d844d..cc51045 100644
--- a/hw/blizzard.c
+++ b/hw/blizzard.c
@@ -963,7 +963,7 @@ void *s1d13745_init(qemu_irq gpio_int)
 
     s->state = graphic_console_init(blizzard_update_display,
                                  blizzard_invalidate_display,
-                                 blizzard_screen_dump, NULL, s);
+                                 blizzard_screen_dump, NULL, NULL, s);
 
     switch (ds_get_bits_per_pixel(s->state)) {
     case 0:
diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 4edcb94..d2a9c27 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -2900,7 +2900,7 @@ static int vga_initfn(ISADevice *dev)
                        isa_address_space(dev));
     s->ds = graphic_console_init(s->update, s->invalidate,
                                  s->screen_dump, s->text_update,
-                                 s);
+                                 s->screen_dump_async, s);
     rom_add_vga(VGABIOS_CIRRUS_FILENAME);
     /* XXX ISA-LFB support */
     /* FIXME not qdev yet */
@@ -2941,7 +2941,7 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
      cirrus_init_common(s, device_id, 1, pci_address_space(dev));
      s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
                                       s->vga.screen_dump, s->vga.text_update,
-                                      &s->vga);
+                                      s->vga.screen_dump_async, &s->vga);
 
      /* setup PCI */
 
diff --git a/hw/exynos4210_fimd.c b/hw/exynos4210_fimd.c
index 3313f00..2053ed0 100644
--- a/hw/exynos4210_fimd.c
+++ b/hw/exynos4210_fimd.c
@@ -1898,7 +1898,8 @@ static int exynos4210_fimd_init(SysBusDevice *dev)
             "exynos4210.fimd", FIMD_REGS_SIZE);
     sysbus_init_mmio(dev, &s->iomem);
     s->console = graphic_console_init(exynos4210_fimd_update,
-                                  exynos4210_fimd_invalidate, NULL, NULL, s);
+                                  exynos4210_fimd_invalidate,
+                                  NULL, NULL, NULL, s);
 
     return 0;
 }
diff --git a/hw/g364fb.c b/hw/g364fb.c
index 3a0b68f..8ae40d8 100644
--- a/hw/g364fb.c
+++ b/hw/g364fb.c
@@ -517,7 +517,7 @@ static void g364fb_init(DeviceState *dev, G364State *s)
 
     s->ds = graphic_console_init(g364fb_update_display,
                                  g364fb_invalidate_display,
-                                 g364fb_screen_dump, NULL, s);
+                                 g364fb_screen_dump, NULL, NULL, s);
 
     memory_region_init_io(&s->mem_ctrl, &g364fb_ctrl_ops, s, "ctrl", 0x180000);
     memory_region_init_ram_ptr(&s->mem_vram, "vram",
diff --git a/hw/jazz_led.c b/hw/jazz_led.c
index 6486523..bb5daf7 100644
--- a/hw/jazz_led.c
+++ b/hw/jazz_led.c
@@ -251,7 +251,8 @@ static int jazz_led_init(SysBusDevice *dev)
     s->ds = graphic_console_init(jazz_led_update_display,
                                  jazz_led_invalidate_display,
                                  NULL,
-                                 jazz_led_text_update, s);
+                                 jazz_led_text_update,
+                                 NULL, s);
 
     return 0;
 }
diff --git a/hw/milkymist-vgafb.c b/hw/milkymist-vgafb.c
index 69afd72..ae018d1 100644
--- a/hw/milkymist-vgafb.c
+++ b/hw/milkymist-vgafb.c
@@ -276,7 +276,7 @@ static int milkymist_vgafb_init(SysBusDevice *dev)
 
     s->ds = graphic_console_init(vgafb_update_display,
                                  vgafb_invalidate_display,
-                                 NULL, NULL, s);
+                                 NULL, NULL, NULL, s);
 
     return 0;
 }
diff --git a/hw/musicpal.c b/hw/musicpal.c
index 187a1ae..2fc391b 100644
--- a/hw/musicpal.c
+++ b/hw/musicpal.c
@@ -611,7 +611,7 @@ static int musicpal_lcd_init(SysBusDevice *dev)
     sysbus_init_mmio(dev, &s->iomem);
 
     s->ds = graphic_console_init(lcd_refresh, lcd_invalidate,
-                                 NULL, NULL, s);
+                                 NULL, NULL, NULL, s);
     qemu_console_resize(s->ds, 128*3, 64*3);
 
     qdev_init_gpio_in(&dev->qdev, musicpal_lcd_gpio_brigthness_in, 3);
diff --git a/hw/omap_dss.c b/hw/omap_dss.c
index 86ed6ea..0e8243e 100644
--- a/hw/omap_dss.c
+++ b/hw/omap_dss.c
@@ -1072,7 +1072,9 @@ struct omap_dss_s *omap_dss_init(struct omap_target_agent_s *ta,
 
 #if 0
     s->state = graphic_console_init(omap_update_display,
-                                    omap_invalidate_display, omap_screen_dump, s);
+                                    omap_invalidate_display,
+                                    omap_screen_dump,
+                                    NULL, NULL, s);
 #endif
 
     return s;
diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
index f172093..d91e88d 100644
--- a/hw/omap_lcdc.c
+++ b/hw/omap_lcdc.c
@@ -452,7 +452,7 @@ struct omap_lcd_panel_s *omap_lcdc_init(MemoryRegion *sysmem,
 
     s->state = graphic_console_init(omap_update_display,
                                     omap_invalidate_display,
-                                    omap_screen_dump, NULL, s);
+                                    omap_screen_dump, NULL, NULL, s);
 
     return s;
 }
diff --git a/hw/pl110.c b/hw/pl110.c
index f94608c..32ee89c 100644
--- a/hw/pl110.c
+++ b/hw/pl110.c
@@ -451,7 +451,7 @@ static int pl110_init(SysBusDevice *dev)
     qdev_init_gpio_in(&s->busdev.qdev, pl110_mux_ctrl_set, 1);
     s->ds = graphic_console_init(pl110_update_display,
                                  pl110_invalidate_display,
-                                 NULL, NULL, s);
+                                 NULL, NULL, NULL, s);
     return 0;
 }
 
diff --git a/hw/pxa2xx_lcd.c b/hw/pxa2xx_lcd.c
index fcbdfb3..5345fb4 100644
--- a/hw/pxa2xx_lcd.c
+++ b/hw/pxa2xx_lcd.c
@@ -1004,7 +1004,7 @@ PXA2xxLCDState *pxa2xx_lcdc_init(MemoryRegion *sysmem,
 
     s->ds = graphic_console_init(pxa2xx_update_display,
                                  pxa2xx_invalidate_display,
-                                 NULL, NULL, s);
+                                 NULL, NULL, NULL, s);
 
     switch (ds_get_bits_per_pixel(s->ds)) {
     case 0:
diff --git a/hw/qxl.c b/hw/qxl.c
index d83d245..61a7edd 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1727,7 +1727,8 @@ static int qxl_init_primary(PCIDevice *dev)
     portio_list_add(qxl_vga_port_list, pci_address_space_io(dev), 0x3b0);
 
     vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
-                                   qxl_hw_screen_dump, qxl_hw_text_update, qxl);
+                                   qxl_hw_screen_dump, qxl_hw_text_update,
+                                   NULL, qxl);
     qemu_spice_display_init_common(&qxl->ssd, vga->ds);
 
     qxl0 = qxl;
diff --git a/hw/sm501.c b/hw/sm501.c
index 786e076..8b150be 100644
--- a/hw/sm501.c
+++ b/hw/sm501.c
@@ -1442,6 +1442,6 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base,
     }
 
     /* create qemu graphic console */
-    s->ds = graphic_console_init(sm501_update_display, NULL,
-				 NULL, NULL, s);
+    s->ds = graphic_console_init(sm501_update_display,
+                                 NULL, NULL, NULL, NULL, s);
 }
diff --git a/hw/ssd0303.c b/hw/ssd0303.c
index 4e1ee6e..05e6686 100644
--- a/hw/ssd0303.c
+++ b/hw/ssd0303.c
@@ -289,7 +289,7 @@ static int ssd0303_init(I2CSlave *i2c)
 
     s->ds = graphic_console_init(ssd0303_update_display,
                                  ssd0303_invalidate_display,
-                                 NULL, NULL, s);
+                                 NULL, NULL, NULL, s);
     qemu_console_resize(s->ds, 96 * MAGNIFY, 16 * MAGNIFY);
     return 0;
 }
diff --git a/hw/ssd0323.c b/hw/ssd0323.c
index b0b2e94..5e3491f 100644
--- a/hw/ssd0323.c
+++ b/hw/ssd0323.c
@@ -330,7 +330,7 @@ static int ssd0323_init(SSISlave *dev)
     s->row_end = 79;
     s->ds = graphic_console_init(ssd0323_update_display,
                                  ssd0323_invalidate_display,
-                                 NULL, NULL, s);
+                                 NULL, NULL, NULL, s);
     qemu_console_resize(s->ds, 128 * MAGNIFY, 64 * MAGNIFY);
 
     qdev_init_gpio_in(&dev->qdev, ssd0323_cd, 1);
diff --git a/hw/tc6393xb.c b/hw/tc6393xb.c
index 420925c..853b180 100644
--- a/hw/tc6393xb.c
+++ b/hw/tc6393xb.c
@@ -581,6 +581,7 @@ TC6393xbState *tc6393xb_init(MemoryRegion *sysmem, uint32_t base, qemu_irq irq)
             NULL, /* invalidate */
             NULL, /* screen_dump */
             NULL, /* text_update */
+            NULL, /* screen_dump_async */
             s);
 
     return s;
diff --git a/hw/tcx.c b/hw/tcx.c
index ac7dcb4..9214dec 100644
--- a/hw/tcx.c
+++ b/hw/tcx.c
@@ -558,7 +558,7 @@ static int tcx_init1(SysBusDevice *dev)
 
         s->ds = graphic_console_init(tcx24_update_display,
                                      tcx24_invalidate_display,
-                                     tcx24_screen_dump, NULL, s);
+                                     tcx24_screen_dump, NULL, NULL, s);
     } else {
         /* THC 8 bit (dummy) */
         memory_region_init_io(&s->thc8, &dummy_ops, s, "tcx.thc8",
@@ -567,7 +567,7 @@ static int tcx_init1(SysBusDevice *dev)
 
         s->ds = graphic_console_init(tcx_update_display,
                                      tcx_invalidate_display,
-                                     tcx_screen_dump, NULL, s);
+                                     tcx_screen_dump, NULL, NULL, s);
     }
 
     qemu_console_resize(s->ds, s->width, s->height);
diff --git a/hw/vga-isa-mm.c b/hw/vga-isa-mm.c
index f8984c6..87b8fc6 100644
--- a/hw/vga-isa-mm.c
+++ b/hw/vga-isa-mm.c
@@ -132,7 +132,8 @@ int isa_vga_mm_init(target_phys_addr_t vram_base,
     vga_mm_init(s, vram_base, ctrl_base, it_shift, address_space);
 
     s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
-                                     s->vga.screen_dump, s->vga.text_update, s);
+                                     s->vga.screen_dump, s->vga.text_update,
+                                     s->vga.screen_dump_async, s);
 
     vga_init_vbe(&s->vga, address_space);
     return 0;
diff --git a/hw/vga-isa.c b/hw/vga-isa.c
index 4bcc4db..a362ed2 100644
--- a/hw/vga-isa.c
+++ b/hw/vga-isa.c
@@ -61,7 +61,8 @@ static int vga_initfn(ISADevice *dev)
                                         vga_io_memory, 1);
     memory_region_set_coalescing(vga_io_memory);
     s->ds = graphic_console_init(s->update, s->invalidate,
-                                 s->screen_dump, s->text_update, s);
+                                 s->screen_dump, s->text_update,
+                                 s->screen_dump_async, s);
 
     vga_init_vbe(s, isa_address_space(dev));
     /* ROM BIOS */
diff --git a/hw/vga-pci.c b/hw/vga-pci.c
index 465b643..8c8dd53 100644
--- a/hw/vga-pci.c
+++ b/hw/vga-pci.c
@@ -57,7 +57,8 @@ static int pci_vga_initfn(PCIDevice *dev)
      vga_init(s, pci_address_space(dev), pci_address_space_io(dev), true);
 
      s->ds = graphic_console_init(s->update, s->invalidate,
-                                  s->screen_dump, s->text_update, s);
+                                  s->screen_dump, s->text_update,
+                                  s->screen_dump_async, s);
 
      /* XXX: VGA_RAM_SIZE must be a power of two */
      pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->vram);
diff --git a/hw/vga_int.h b/hw/vga_int.h
index 7685b2b..4486cfb 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -161,6 +161,7 @@ typedef struct VGACommonState {
     vga_hw_invalidate_ptr invalidate;
     vga_hw_screen_dump_ptr screen_dump;
     vga_hw_text_update_ptr text_update;
+    vga_hw_screen_dump_async_ptr screen_dump_async;
     /* hardware mouse cursor support */
     uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32];
     void (*cursor_invalidate)(struct VGACommonState *s);
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 142d9f4..db96e61 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1087,7 +1087,8 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size,
     s->vga.ds = graphic_console_init(vmsvga_update_display,
                                      vmsvga_invalidate_display,
                                      vmsvga_screen_dump,
-                                     vmsvga_text_update, s);
+                                     vmsvga_text_update,
+                                     NULL, s);
 
 
     s->fifo_size = SVGA_FIFO_SIZE;
diff --git a/hw/xenfb.c b/hw/xenfb.c
index 1bcf171..ad9f0d7 100644
--- a/hw/xenfb.c
+++ b/hw/xenfb.c
@@ -906,6 +906,7 @@ static int fb_initialise(struct XenDevice *xendev)
                                         xenfb_invalidate,
                                         NULL,
                                         NULL,
+                                        NULL,
                                         fb);
         fb->have_console = 1;
     }
@@ -1015,6 +1016,7 @@ wait_more:
                                     xenfb_invalidate,
                                     NULL,
                                     NULL,
+                                    NULL,
                                     fb);
     fb->have_console = 1;
 
diff --git a/monitor.c b/monitor.c
index 1a65c41..91a76a1 100644
--- a/monitor.c
+++ b/monitor.c
@@ -901,6 +901,11 @@ static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
     return 0;
 }
 
+void qmp_screendump_async(const char *filename, Error **errp)
+{
+    vga_hw_screen_dump_async(filename);
+}
+
 static void do_logfile(Monitor *mon, const QDict *qdict)
 {
     cpu_set_log_filename(qdict_get_str(qdict, "filename"));
diff --git a/qapi-schema.json b/qapi-schema.json
index 72b17f1..a5873cb 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1595,3 +1595,23 @@
 { 'command': 'qom-list-types',
   'data': { '*implements': 'str', '*abstract': 'bool' },
   'returns': [ 'ObjectTypeInfo' ] }
+
+##
+## @screendump-async:
+#
+# This command will perform a screen dump of the first console to the givem
+# filename. The additional parameters are unused at this time.
+#
+# @filename  name of output file to write screen dump to
+#
+# Since: 1.1
+#
+# Notes: This command is experimental and may change syntax in future releases.
+#
+# This command is the same as the qmp/hmp screendump command, except that on
+# successful completion of the scren dump the SCREEN_DUMP_COMPLETE event is
+# emitted.
+#
+##
+{ 'command': 'screendump-async',
+  'data': { 'filename': 'str' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 705f704..ea0d051 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -170,6 +170,32 @@ Example:
 EQMP
 
     {
+        .name       = "screendump-async",
+        .args_type  = "filename:F",
+        .params     = "filename",
+        .help       = "save screen into PPM image 'filename'",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = qmp_marshal_input_screendump_async,
+    },
+
+SQMP
+screendump-async
+----------------
+
+Save screen into PPM image. Fires a SCREEN_DUMP_COMPLETE event on completion.
+
+Arguments:
+
+- "filename": file path (json-string)
+
+Example:
+
+-> { "execute": "screendump-async", "arguments": { "filename": "/tmp/image" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "stop",
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_input_stop,
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 4/4] add qmp screendump-async

[Anthony Liguori]

On 02/24/2012 03:22 PM, Alon Levy wrote:
> This is an across the board change since I wanted to keep the existing
> (good imo) single graphic_console_init callback setter, instead of
> introducing a new cb that isn't set by it but instead by a second
> initialization function.
>
> Signed-off-by: Alon Levy<alevy@redhat.com>

What's the rationale for this?

I really don't want a plethora of pseudo-async commands.

Regards,

Anthony Liguori

> ---
>   console.c            |   25 +++++++++++++++++++++++--
>   console.h            |    5 +++++
>   hw/blizzard.c        |    2 +-
>   hw/cirrus_vga.c      |    4 ++--
>   hw/exynos4210_fimd.c |    3 ++-
>   hw/g364fb.c          |    2 +-
>   hw/jazz_led.c        |    3 ++-
>   hw/milkymist-vgafb.c |    2 +-
>   hw/musicpal.c        |    2 +-
>   hw/omap_dss.c        |    4 +++-
>   hw/omap_lcdc.c       |    2 +-
>   hw/pl110.c           |    2 +-
>   hw/pxa2xx_lcd.c      |    2 +-
>   hw/qxl.c             |    3 ++-
>   hw/sm501.c           |    4 ++--
>   hw/ssd0303.c         |    2 +-
>   hw/ssd0323.c         |    2 +-
>   hw/tc6393xb.c        |    1 +
>   hw/tcx.c             |    4 ++--
>   hw/vga-isa-mm.c      |    3 ++-
>   hw/vga-isa.c         |    3 ++-
>   hw/vga-pci.c         |    3 ++-
>   hw/vga_int.h         |    1 +
>   hw/vmware_vga.c      |    3 ++-
>   hw/xenfb.c           |    2 ++
>   monitor.c            |    5 +++++
>   qapi-schema.json     |   20 ++++++++++++++++++++
>   qmp-commands.hx      |   26 ++++++++++++++++++++++++++
>   28 files changed, 115 insertions(+), 25 deletions(-)
>
> diff --git a/console.c b/console.c
> index 6750538..ced2aa7 100644
> --- a/console.c
> +++ b/console.c
> @@ -124,6 +124,7 @@ struct TextConsole {
>       vga_hw_update_ptr hw_update;
>       vga_hw_invalidate_ptr hw_invalidate;
>       vga_hw_screen_dump_ptr hw_screen_dump;
> +    vga_hw_screen_dump_async_ptr hw_screen_dump_async;
>       vga_hw_text_update_ptr hw_text_update;
>       void *hw;
>
> @@ -175,8 +176,9 @@ void vga_hw_invalidate(void)
>           active_console->hw_invalidate(active_console->hw);
>   }
>
> -void vga_hw_screen_dump(const char *filename)
> +static void vga_hw_screen_dump_helper(const char *filename, bool async)
>   {
> +    bool event_sent = false;
>       TextConsole *previous_active_console;
>       bool cswitch;
>
> @@ -188,17 +190,33 @@ void vga_hw_screen_dump(const char *filename)
>       if (cswitch) {
>           console_select(0);
>       }
> -    if (consoles[0]&&  consoles[0]->hw_screen_dump) {
> +    if (async&&  consoles[0]&&  consoles[0]->hw_screen_dump_async) {
> +        consoles[0]->hw_screen_dump_async(consoles[0]->hw, filename, cswitch);
> +        event_sent = true;
> +    } else if (consoles[0]&&  consoles[0]->hw_screen_dump) {
>           consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
>       } else {
>           error_report("screen dump not implemented");
>       }
> +    if (async&&  !event_sent) {
> +        monitor_protocol_screen_dump_complete_event(filename);
> +    }
>
>       if (cswitch) {
>           console_select(previous_active_console->index);
>       }
>   }
>
> +void vga_hw_screen_dump(const char *filename)
> +{
> +    vga_hw_screen_dump_helper(filename, false);
> +}
> +
> +void vga_hw_screen_dump_async(const char *filename)
> +{
> +    vga_hw_screen_dump_helper(filename, true);
> +}
> +
>   void vga_hw_text_update(console_ch_t *chardata)
>   {
>       if (active_console&&  active_console->hw_text_update)
> @@ -1403,6 +1421,8 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
>                                      vga_hw_invalidate_ptr invalidate,
>                                      vga_hw_screen_dump_ptr screen_dump,
>                                      vga_hw_text_update_ptr text_update,
> +                                   vga_hw_screen_dump_async_ptr
> +                                                    screen_dump_async,
>                                      void *opaque)
>   {
>       TextConsole *s;
> @@ -1421,6 +1441,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
>       s->hw_update = update;
>       s->hw_invalidate = invalidate;
>       s->hw_screen_dump = screen_dump;
> +    s->hw_screen_dump_async = screen_dump_async;
>       s->hw_text_update = text_update;
>       s->hw = opaque;
>
> diff --git a/console.h b/console.h
> index c22803c..3cf28c0 100644
> --- a/console.h
> +++ b/console.h
> @@ -341,17 +341,22 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
>   typedef void (*vga_hw_update_ptr)(void *);
>   typedef void (*vga_hw_invalidate_ptr)(void *);
>   typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
> +typedef void (*vga_hw_screen_dump_async_ptr)(void *, const char *filename,
> +                                             bool cswitch);
>   typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
>
>   DisplayState *graphic_console_init(vga_hw_update_ptr update,
>                                      vga_hw_invalidate_ptr invalidate,
>                                      vga_hw_screen_dump_ptr screen_dump,
>                                      vga_hw_text_update_ptr text_update,
> +                                   vga_hw_screen_dump_async_ptr
> +                                                    screen_dump_async,
>                                      void *opaque);
>
>   void vga_hw_update(void);
>   void vga_hw_invalidate(void);
>   void vga_hw_screen_dump(const char *filename);
> +void vga_hw_screen_dump_async(const char *filename);
>   void vga_hw_text_update(console_ch_t *chardata);
>   void monitor_protocol_screen_dump_complete_event(const char *filename);
>
> diff --git a/hw/blizzard.c b/hw/blizzard.c
> index c7d844d..cc51045 100644
> --- a/hw/blizzard.c
> +++ b/hw/blizzard.c
> @@ -963,7 +963,7 @@ void *s1d13745_init(qemu_irq gpio_int)
>
>       s->state = graphic_console_init(blizzard_update_display,
>                                    blizzard_invalidate_display,
> -                                 blizzard_screen_dump, NULL, s);
> +                                 blizzard_screen_dump, NULL, NULL, s);
>
>       switch (ds_get_bits_per_pixel(s->state)) {
>       case 0:
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 4edcb94..d2a9c27 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -2900,7 +2900,7 @@ static int vga_initfn(ISADevice *dev)
>                          isa_address_space(dev));
>       s->ds = graphic_console_init(s->update, s->invalidate,
>                                    s->screen_dump, s->text_update,
> -                                 s);
> +                                 s->screen_dump_async, s);
>       rom_add_vga(VGABIOS_CIRRUS_FILENAME);
>       /* XXX ISA-LFB support */
>       /* FIXME not qdev yet */
> @@ -2941,7 +2941,7 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
>        cirrus_init_common(s, device_id, 1, pci_address_space(dev));
>        s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
>                                         s->vga.screen_dump, s->vga.text_update,
> -&s->vga);
> +                                      s->vga.screen_dump_async,&s->vga);
>
>        /* setup PCI */
>
> diff --git a/hw/exynos4210_fimd.c b/hw/exynos4210_fimd.c
> index 3313f00..2053ed0 100644
> --- a/hw/exynos4210_fimd.c
> +++ b/hw/exynos4210_fimd.c
> @@ -1898,7 +1898,8 @@ static int exynos4210_fimd_init(SysBusDevice *dev)
>               "exynos4210.fimd", FIMD_REGS_SIZE);
>       sysbus_init_mmio(dev,&s->iomem);
>       s->console = graphic_console_init(exynos4210_fimd_update,
> -                                  exynos4210_fimd_invalidate, NULL, NULL, s);
> +                                  exynos4210_fimd_invalidate,
> +                                  NULL, NULL, NULL, s);
>
>       return 0;
>   }
> diff --git a/hw/g364fb.c b/hw/g364fb.c
> index 3a0b68f..8ae40d8 100644
> --- a/hw/g364fb.c
> +++ b/hw/g364fb.c
> @@ -517,7 +517,7 @@ static void g364fb_init(DeviceState *dev, G364State *s)
>
>       s->ds = graphic_console_init(g364fb_update_display,
>                                    g364fb_invalidate_display,
> -                                 g364fb_screen_dump, NULL, s);
> +                                 g364fb_screen_dump, NULL, NULL, s);
>
>       memory_region_init_io(&s->mem_ctrl,&g364fb_ctrl_ops, s, "ctrl", 0x180000);
>       memory_region_init_ram_ptr(&s->mem_vram, "vram",
> diff --git a/hw/jazz_led.c b/hw/jazz_led.c
> index 6486523..bb5daf7 100644
> --- a/hw/jazz_led.c
> +++ b/hw/jazz_led.c
> @@ -251,7 +251,8 @@ static int jazz_led_init(SysBusDevice *dev)
>       s->ds = graphic_console_init(jazz_led_update_display,
>                                    jazz_led_invalidate_display,
>                                    NULL,
> -                                 jazz_led_text_update, s);
> +                                 jazz_led_text_update,
> +                                 NULL, s);
>
>       return 0;
>   }
> diff --git a/hw/milkymist-vgafb.c b/hw/milkymist-vgafb.c
> index 69afd72..ae018d1 100644
> --- a/hw/milkymist-vgafb.c
> +++ b/hw/milkymist-vgafb.c
> @@ -276,7 +276,7 @@ static int milkymist_vgafb_init(SysBusDevice *dev)
>
>       s->ds = graphic_console_init(vgafb_update_display,
>                                    vgafb_invalidate_display,
> -                                 NULL, NULL, s);
> +                                 NULL, NULL, NULL, s);
>
>       return 0;
>   }
> diff --git a/hw/musicpal.c b/hw/musicpal.c
> index 187a1ae..2fc391b 100644
> --- a/hw/musicpal.c
> +++ b/hw/musicpal.c
> @@ -611,7 +611,7 @@ static int musicpal_lcd_init(SysBusDevice *dev)
>       sysbus_init_mmio(dev,&s->iomem);
>
>       s->ds = graphic_console_init(lcd_refresh, lcd_invalidate,
> -                                 NULL, NULL, s);
> +                                 NULL, NULL, NULL, s);
>       qemu_console_resize(s->ds, 128*3, 64*3);
>
>       qdev_init_gpio_in(&dev->qdev, musicpal_lcd_gpio_brigthness_in, 3);
> diff --git a/hw/omap_dss.c b/hw/omap_dss.c
> index 86ed6ea..0e8243e 100644
> --- a/hw/omap_dss.c
> +++ b/hw/omap_dss.c
> @@ -1072,7 +1072,9 @@ struct omap_dss_s *omap_dss_init(struct omap_target_agent_s *ta,
>
>   #if 0
>       s->state = graphic_console_init(omap_update_display,
> -                                    omap_invalidate_display, omap_screen_dump, s);
> +                                    omap_invalidate_display,
> +                                    omap_screen_dump,
> +                                    NULL, NULL, s);
>   #endif
>
>       return s;
> diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
> index f172093..d91e88d 100644
> --- a/hw/omap_lcdc.c
> +++ b/hw/omap_lcdc.c
> @@ -452,7 +452,7 @@ struct omap_lcd_panel_s *omap_lcdc_init(MemoryRegion *sysmem,
>
>       s->state = graphic_console_init(omap_update_display,
>                                       omap_invalidate_display,
> -                                    omap_screen_dump, NULL, s);
> +                                    omap_screen_dump, NULL, NULL, s);
>
>       return s;
>   }
> diff --git a/hw/pl110.c b/hw/pl110.c
> index f94608c..32ee89c 100644
> --- a/hw/pl110.c
> +++ b/hw/pl110.c
> @@ -451,7 +451,7 @@ static int pl110_init(SysBusDevice *dev)
>       qdev_init_gpio_in(&s->busdev.qdev, pl110_mux_ctrl_set, 1);
>       s->ds = graphic_console_init(pl110_update_display,
>                                    pl110_invalidate_display,
> -                                 NULL, NULL, s);
> +                                 NULL, NULL, NULL, s);
>       return 0;
>   }
>
> diff --git a/hw/pxa2xx_lcd.c b/hw/pxa2xx_lcd.c
> index fcbdfb3..5345fb4 100644
> --- a/hw/pxa2xx_lcd.c
> +++ b/hw/pxa2xx_lcd.c
> @@ -1004,7 +1004,7 @@ PXA2xxLCDState *pxa2xx_lcdc_init(MemoryRegion *sysmem,
>
>       s->ds = graphic_console_init(pxa2xx_update_display,
>                                    pxa2xx_invalidate_display,
> -                                 NULL, NULL, s);
> +                                 NULL, NULL, NULL, s);
>
>       switch (ds_get_bits_per_pixel(s->ds)) {
>       case 0:
> diff --git a/hw/qxl.c b/hw/qxl.c
> index d83d245..61a7edd 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -1727,7 +1727,8 @@ static int qxl_init_primary(PCIDevice *dev)
>       portio_list_add(qxl_vga_port_list, pci_address_space_io(dev), 0x3b0);
>
>       vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
> -                                   qxl_hw_screen_dump, qxl_hw_text_update, qxl);
> +                                   qxl_hw_screen_dump, qxl_hw_text_update,
> +                                   NULL, qxl);
>       qemu_spice_display_init_common(&qxl->ssd, vga->ds);
>
>       qxl0 = qxl;
> diff --git a/hw/sm501.c b/hw/sm501.c
> index 786e076..8b150be 100644
> --- a/hw/sm501.c
> +++ b/hw/sm501.c
> @@ -1442,6 +1442,6 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base,
>       }
>
>       /* create qemu graphic console */
> -    s->ds = graphic_console_init(sm501_update_display, NULL,
> -				 NULL, NULL, s);
> +    s->ds = graphic_console_init(sm501_update_display,
> +                                 NULL, NULL, NULL, NULL, s);
>   }
> diff --git a/hw/ssd0303.c b/hw/ssd0303.c
> index 4e1ee6e..05e6686 100644
> --- a/hw/ssd0303.c
> +++ b/hw/ssd0303.c
> @@ -289,7 +289,7 @@ static int ssd0303_init(I2CSlave *i2c)
>
>       s->ds = graphic_console_init(ssd0303_update_display,
>                                    ssd0303_invalidate_display,
> -                                 NULL, NULL, s);
> +                                 NULL, NULL, NULL, s);
>       qemu_console_resize(s->ds, 96 * MAGNIFY, 16 * MAGNIFY);
>       return 0;
>   }
> diff --git a/hw/ssd0323.c b/hw/ssd0323.c
> index b0b2e94..5e3491f 100644
> --- a/hw/ssd0323.c
> +++ b/hw/ssd0323.c
> @@ -330,7 +330,7 @@ static int ssd0323_init(SSISlave *dev)
>       s->row_end = 79;
>       s->ds = graphic_console_init(ssd0323_update_display,
>                                    ssd0323_invalidate_display,
> -                                 NULL, NULL, s);
> +                                 NULL, NULL, NULL, s);
>       qemu_console_resize(s->ds, 128 * MAGNIFY, 64 * MAGNIFY);
>
>       qdev_init_gpio_in(&dev->qdev, ssd0323_cd, 1);
> diff --git a/hw/tc6393xb.c b/hw/tc6393xb.c
> index 420925c..853b180 100644
> --- a/hw/tc6393xb.c
> +++ b/hw/tc6393xb.c
> @@ -581,6 +581,7 @@ TC6393xbState *tc6393xb_init(MemoryRegion *sysmem, uint32_t base, qemu_irq irq)
>               NULL, /* invalidate */
>               NULL, /* screen_dump */
>               NULL, /* text_update */
> +            NULL, /* screen_dump_async */
>               s);
>
>       return s;
> diff --git a/hw/tcx.c b/hw/tcx.c
> index ac7dcb4..9214dec 100644
> --- a/hw/tcx.c
> +++ b/hw/tcx.c
> @@ -558,7 +558,7 @@ static int tcx_init1(SysBusDevice *dev)
>
>           s->ds = graphic_console_init(tcx24_update_display,
>                                        tcx24_invalidate_display,
> -                                     tcx24_screen_dump, NULL, s);
> +                                     tcx24_screen_dump, NULL, NULL, s);
>       } else {
>           /* THC 8 bit (dummy) */
>           memory_region_init_io(&s->thc8,&dummy_ops, s, "tcx.thc8",
> @@ -567,7 +567,7 @@ static int tcx_init1(SysBusDevice *dev)
>
>           s->ds = graphic_console_init(tcx_update_display,
>                                        tcx_invalidate_display,
> -                                     tcx_screen_dump, NULL, s);
> +                                     tcx_screen_dump, NULL, NULL, s);
>       }
>
>       qemu_console_resize(s->ds, s->width, s->height);
> diff --git a/hw/vga-isa-mm.c b/hw/vga-isa-mm.c
> index f8984c6..87b8fc6 100644
> --- a/hw/vga-isa-mm.c
> +++ b/hw/vga-isa-mm.c
> @@ -132,7 +132,8 @@ int isa_vga_mm_init(target_phys_addr_t vram_base,
>       vga_mm_init(s, vram_base, ctrl_base, it_shift, address_space);
>
>       s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
> -                                     s->vga.screen_dump, s->vga.text_update, s);
> +                                     s->vga.screen_dump, s->vga.text_update,
> +                                     s->vga.screen_dump_async, s);
>
>       vga_init_vbe(&s->vga, address_space);
>       return 0;
> diff --git a/hw/vga-isa.c b/hw/vga-isa.c
> index 4bcc4db..a362ed2 100644
> --- a/hw/vga-isa.c
> +++ b/hw/vga-isa.c
> @@ -61,7 +61,8 @@ static int vga_initfn(ISADevice *dev)
>                                           vga_io_memory, 1);
>       memory_region_set_coalescing(vga_io_memory);
>       s->ds = graphic_console_init(s->update, s->invalidate,
> -                                 s->screen_dump, s->text_update, s);
> +                                 s->screen_dump, s->text_update,
> +                                 s->screen_dump_async, s);
>
>       vga_init_vbe(s, isa_address_space(dev));
>       /* ROM BIOS */
> diff --git a/hw/vga-pci.c b/hw/vga-pci.c
> index 465b643..8c8dd53 100644
> --- a/hw/vga-pci.c
> +++ b/hw/vga-pci.c
> @@ -57,7 +57,8 @@ static int pci_vga_initfn(PCIDevice *dev)
>        vga_init(s, pci_address_space(dev), pci_address_space_io(dev), true);
>
>        s->ds = graphic_console_init(s->update, s->invalidate,
> -                                  s->screen_dump, s->text_update, s);
> +                                  s->screen_dump, s->text_update,
> +                                  s->screen_dump_async, s);
>
>        /* XXX: VGA_RAM_SIZE must be a power of two */
>        pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH,&s->vram);
> diff --git a/hw/vga_int.h b/hw/vga_int.h
> index 7685b2b..4486cfb 100644
> --- a/hw/vga_int.h
> +++ b/hw/vga_int.h
> @@ -161,6 +161,7 @@ typedef struct VGACommonState {
>       vga_hw_invalidate_ptr invalidate;
>       vga_hw_screen_dump_ptr screen_dump;
>       vga_hw_text_update_ptr text_update;
> +    vga_hw_screen_dump_async_ptr screen_dump_async;
>       /* hardware mouse cursor support */
>       uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32];
>       void (*cursor_invalidate)(struct VGACommonState *s);
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index 142d9f4..db96e61 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -1087,7 +1087,8 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size,
>       s->vga.ds = graphic_console_init(vmsvga_update_display,
>                                        vmsvga_invalidate_display,
>                                        vmsvga_screen_dump,
> -                                     vmsvga_text_update, s);
> +                                     vmsvga_text_update,
> +                                     NULL, s);
>
>
>       s->fifo_size = SVGA_FIFO_SIZE;
> diff --git a/hw/xenfb.c b/hw/xenfb.c
> index 1bcf171..ad9f0d7 100644
> --- a/hw/xenfb.c
> +++ b/hw/xenfb.c
> @@ -906,6 +906,7 @@ static int fb_initialise(struct XenDevice *xendev)
>                                           xenfb_invalidate,
>                                           NULL,
>                                           NULL,
> +                                        NULL,
>                                           fb);
>           fb->have_console = 1;
>       }
> @@ -1015,6 +1016,7 @@ wait_more:
>                                       xenfb_invalidate,
>                                       NULL,
>                                       NULL,
> +                                    NULL,
>                                       fb);
>       fb->have_console = 1;
>
> diff --git a/monitor.c b/monitor.c
> index 1a65c41..91a76a1 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -901,6 +901,11 @@ static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
>       return 0;
>   }
>
> +void qmp_screendump_async(const char *filename, Error **errp)
> +{
> +    vga_hw_screen_dump_async(filename);
> +}
> +
>   static void do_logfile(Monitor *mon, const QDict *qdict)
>   {
>       cpu_set_log_filename(qdict_get_str(qdict, "filename"));
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 72b17f1..a5873cb 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1595,3 +1595,23 @@
>   { 'command': 'qom-list-types',
>     'data': { '*implements': 'str', '*abstract': 'bool' },
>     'returns': [ 'ObjectTypeInfo' ] }
> +
> +##
> +## @screendump-async:
> +#
> +# This command will perform a screen dump of the first console to the givem
> +# filename. The additional parameters are unused at this time.
> +#
> +# @filename  name of output file to write screen dump to
> +#
> +# Since: 1.1
> +#
> +# Notes: This command is experimental and may change syntax in future releases.
> +#
> +# This command is the same as the qmp/hmp screendump command, except that on
> +# successful completion of the scren dump the SCREEN_DUMP_COMPLETE event is
> +# emitted.
> +#
> +##
> +{ 'command': 'screendump-async',
> +  'data': { 'filename': 'str' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 705f704..ea0d051 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -170,6 +170,32 @@ Example:
>   EQMP
>
>       {
> +        .name       = "screendump-async",
> +        .args_type  = "filename:F",
> +        .params     = "filename",
> +        .help       = "save screen into PPM image 'filename'",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = qmp_marshal_input_screendump_async,
> +    },
> +
> +SQMP
> +screendump-async
> +----------------
> +
> +Save screen into PPM image. Fires a SCREEN_DUMP_COMPLETE event on completion.
> +
> +Arguments:
> +
> +- "filename": file path (json-string)
> +
> +Example:
> +
> +->  { "execute": "screendump-async", "arguments": { "filename": "/tmp/image" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>           .name       = "stop",
>           .args_type  = "",
>           .mhandler.cmd_new = qmp_marshal_input_stop,

Re: [Qemu-devel] [PATCH 4/4] add qmp screendump-async

[Alon Levy]

On Fri, Feb 24, 2012 at 04:40:15PM -0600, Anthony Liguori wrote:
> On 02/24/2012 03:22 PM, Alon Levy wrote:
> >This is an across the board change since I wanted to keep the existing
> >(good imo) single graphic_console_init callback setter, instead of
> >introducing a new cb that isn't set by it but instead by a second
> >initialization function.
> >
> >Signed-off-by: Alon Levy<alevy@redhat.com>
> 
> What's the rationale for this?

There is a hang possible with the current screendump command, qxl, a
spice client using libvirt and spice-gtk such as virt-viewer /
remote-viewer, where you have:
1. libvirt waiting for screendump to complete
2. screendump waiting for spice server thread to render
3. spice server thread waiting for spice client to read messages
4. spice client == libvirt client, waiting for screendump completion

The solution to this is to have 2 become asyncronous. See
http://patchwork.ozlabs.org/patch/142985/

The result of 2 becoming async is that now screendump completes before
the framebuffer has been updated, resulting in a screendump of an older
state. To really fix it I need to have the screendump (ppm_save) done
with the newer state, which is what
http://patchwork.ozlabs.org/patch/142977/ does. But for the user
(libvirt/autotest) to know when the file is ready there now needs to be
some event. That's what this series does.

> 
> I really don't want a plethora of pseudo-async commands.
> 
> Regards,
> 
> Anthony Liguori
> 
> >---
> >  console.c            |   25 +++++++++++++++++++++++--
> >  console.h            |    5 +++++
> >  hw/blizzard.c        |    2 +-
> >  hw/cirrus_vga.c      |    4 ++--
> >  hw/exynos4210_fimd.c |    3 ++-
> >  hw/g364fb.c          |    2 +-
> >  hw/jazz_led.c        |    3 ++-
> >  hw/milkymist-vgafb.c |    2 +-
> >  hw/musicpal.c        |    2 +-
> >  hw/omap_dss.c        |    4 +++-
> >  hw/omap_lcdc.c       |    2 +-
> >  hw/pl110.c           |    2 +-
> >  hw/pxa2xx_lcd.c      |    2 +-
> >  hw/qxl.c             |    3 ++-
> >  hw/sm501.c           |    4 ++--
> >  hw/ssd0303.c         |    2 +-
> >  hw/ssd0323.c         |    2 +-
> >  hw/tc6393xb.c        |    1 +
> >  hw/tcx.c             |    4 ++--
> >  hw/vga-isa-mm.c      |    3 ++-
> >  hw/vga-isa.c         |    3 ++-
> >  hw/vga-pci.c         |    3 ++-
> >  hw/vga_int.h         |    1 +
> >  hw/vmware_vga.c      |    3 ++-
> >  hw/xenfb.c           |    2 ++
> >  monitor.c            |    5 +++++
> >  qapi-schema.json     |   20 ++++++++++++++++++++
> >  qmp-commands.hx      |   26 ++++++++++++++++++++++++++
> >  28 files changed, 115 insertions(+), 25 deletions(-)
> >
> >diff --git a/console.c b/console.c
> >index 6750538..ced2aa7 100644
> >--- a/console.c
> >+++ b/console.c
> >@@ -124,6 +124,7 @@ struct TextConsole {
> >      vga_hw_update_ptr hw_update;
> >      vga_hw_invalidate_ptr hw_invalidate;
> >      vga_hw_screen_dump_ptr hw_screen_dump;
> >+    vga_hw_screen_dump_async_ptr hw_screen_dump_async;
> >      vga_hw_text_update_ptr hw_text_update;
> >      void *hw;
> >
> >@@ -175,8 +176,9 @@ void vga_hw_invalidate(void)
> >          active_console->hw_invalidate(active_console->hw);
> >  }
> >
> >-void vga_hw_screen_dump(const char *filename)
> >+static void vga_hw_screen_dump_helper(const char *filename, bool async)
> >  {
> >+    bool event_sent = false;
> >      TextConsole *previous_active_console;
> >      bool cswitch;
> >
> >@@ -188,17 +190,33 @@ void vga_hw_screen_dump(const char *filename)
> >      if (cswitch) {
> >          console_select(0);
> >      }
> >-    if (consoles[0]&&  consoles[0]->hw_screen_dump) {
> >+    if (async&&  consoles[0]&&  consoles[0]->hw_screen_dump_async) {
> >+        consoles[0]->hw_screen_dump_async(consoles[0]->hw, filename, cswitch);
> >+        event_sent = true;
> >+    } else if (consoles[0]&&  consoles[0]->hw_screen_dump) {
> >          consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
> >      } else {
> >          error_report("screen dump not implemented");
> >      }
> >+    if (async&&  !event_sent) {
> >+        monitor_protocol_screen_dump_complete_event(filename);
> >+    }
> >
> >      if (cswitch) {
> >          console_select(previous_active_console->index);
> >      }
> >  }
> >
> >+void vga_hw_screen_dump(const char *filename)
> >+{
> >+    vga_hw_screen_dump_helper(filename, false);
> >+}
> >+
> >+void vga_hw_screen_dump_async(const char *filename)
> >+{
> >+    vga_hw_screen_dump_helper(filename, true);
> >+}
> >+
> >  void vga_hw_text_update(console_ch_t *chardata)
> >  {
> >      if (active_console&&  active_console->hw_text_update)
> >@@ -1403,6 +1421,8 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
> >                                     vga_hw_invalidate_ptr invalidate,
> >                                     vga_hw_screen_dump_ptr screen_dump,
> >                                     vga_hw_text_update_ptr text_update,
> >+                                   vga_hw_screen_dump_async_ptr
> >+                                                    screen_dump_async,
> >                                     void *opaque)
> >  {
> >      TextConsole *s;
> >@@ -1421,6 +1441,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
> >      s->hw_update = update;
> >      s->hw_invalidate = invalidate;
> >      s->hw_screen_dump = screen_dump;
> >+    s->hw_screen_dump_async = screen_dump_async;
> >      s->hw_text_update = text_update;
> >      s->hw = opaque;
> >
> >diff --git a/console.h b/console.h
> >index c22803c..3cf28c0 100644
> >--- a/console.h
> >+++ b/console.h
> >@@ -341,17 +341,22 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
> >  typedef void (*vga_hw_update_ptr)(void *);
> >  typedef void (*vga_hw_invalidate_ptr)(void *);
> >  typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
> >+typedef void (*vga_hw_screen_dump_async_ptr)(void *, const char *filename,
> >+                                             bool cswitch);
> >  typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
> >
> >  DisplayState *graphic_console_init(vga_hw_update_ptr update,
> >                                     vga_hw_invalidate_ptr invalidate,
> >                                     vga_hw_screen_dump_ptr screen_dump,
> >                                     vga_hw_text_update_ptr text_update,
> >+                                   vga_hw_screen_dump_async_ptr
> >+                                                    screen_dump_async,
> >                                     void *opaque);
> >
> >  void vga_hw_update(void);
> >  void vga_hw_invalidate(void);
> >  void vga_hw_screen_dump(const char *filename);
> >+void vga_hw_screen_dump_async(const char *filename);
> >  void vga_hw_text_update(console_ch_t *chardata);
> >  void monitor_protocol_screen_dump_complete_event(const char *filename);
> >
> >diff --git a/hw/blizzard.c b/hw/blizzard.c
> >index c7d844d..cc51045 100644
> >--- a/hw/blizzard.c
> >+++ b/hw/blizzard.c
> >@@ -963,7 +963,7 @@ void *s1d13745_init(qemu_irq gpio_int)
> >
> >      s->state = graphic_console_init(blizzard_update_display,
> >                                   blizzard_invalidate_display,
> >-                                 blizzard_screen_dump, NULL, s);
> >+                                 blizzard_screen_dump, NULL, NULL, s);
> >
> >      switch (ds_get_bits_per_pixel(s->state)) {
> >      case 0:
> >diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> >index 4edcb94..d2a9c27 100644
> >--- a/hw/cirrus_vga.c
> >+++ b/hw/cirrus_vga.c
> >@@ -2900,7 +2900,7 @@ static int vga_initfn(ISADevice *dev)
> >                         isa_address_space(dev));
> >      s->ds = graphic_console_init(s->update, s->invalidate,
> >                                   s->screen_dump, s->text_update,
> >-                                 s);
> >+                                 s->screen_dump_async, s);
> >      rom_add_vga(VGABIOS_CIRRUS_FILENAME);
> >      /* XXX ISA-LFB support */
> >      /* FIXME not qdev yet */
> >@@ -2941,7 +2941,7 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
> >       cirrus_init_common(s, device_id, 1, pci_address_space(dev));
> >       s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
> >                                        s->vga.screen_dump, s->vga.text_update,
> >-&s->vga);
> >+                                      s->vga.screen_dump_async,&s->vga);
> >
> >       /* setup PCI */
> >
> >diff --git a/hw/exynos4210_fimd.c b/hw/exynos4210_fimd.c
> >index 3313f00..2053ed0 100644
> >--- a/hw/exynos4210_fimd.c
> >+++ b/hw/exynos4210_fimd.c
> >@@ -1898,7 +1898,8 @@ static int exynos4210_fimd_init(SysBusDevice *dev)
> >              "exynos4210.fimd", FIMD_REGS_SIZE);
> >      sysbus_init_mmio(dev,&s->iomem);
> >      s->console = graphic_console_init(exynos4210_fimd_update,
> >-                                  exynos4210_fimd_invalidate, NULL, NULL, s);
> >+                                  exynos4210_fimd_invalidate,
> >+                                  NULL, NULL, NULL, s);
> >
> >      return 0;
> >  }
> >diff --git a/hw/g364fb.c b/hw/g364fb.c
> >index 3a0b68f..8ae40d8 100644
> >--- a/hw/g364fb.c
> >+++ b/hw/g364fb.c
> >@@ -517,7 +517,7 @@ static void g364fb_init(DeviceState *dev, G364State *s)
> >
> >      s->ds = graphic_console_init(g364fb_update_display,
> >                                   g364fb_invalidate_display,
> >-                                 g364fb_screen_dump, NULL, s);
> >+                                 g364fb_screen_dump, NULL, NULL, s);
> >
> >      memory_region_init_io(&s->mem_ctrl,&g364fb_ctrl_ops, s, "ctrl", 0x180000);
> >      memory_region_init_ram_ptr(&s->mem_vram, "vram",
> >diff --git a/hw/jazz_led.c b/hw/jazz_led.c
> >index 6486523..bb5daf7 100644
> >--- a/hw/jazz_led.c
> >+++ b/hw/jazz_led.c
> >@@ -251,7 +251,8 @@ static int jazz_led_init(SysBusDevice *dev)
> >      s->ds = graphic_console_init(jazz_led_update_display,
> >                                   jazz_led_invalidate_display,
> >                                   NULL,
> >-                                 jazz_led_text_update, s);
> >+                                 jazz_led_text_update,
> >+                                 NULL, s);
> >
> >      return 0;
> >  }
> >diff --git a/hw/milkymist-vgafb.c b/hw/milkymist-vgafb.c
> >index 69afd72..ae018d1 100644
> >--- a/hw/milkymist-vgafb.c
> >+++ b/hw/milkymist-vgafb.c
> >@@ -276,7 +276,7 @@ static int milkymist_vgafb_init(SysBusDevice *dev)
> >
> >      s->ds = graphic_console_init(vgafb_update_display,
> >                                   vgafb_invalidate_display,
> >-                                 NULL, NULL, s);
> >+                                 NULL, NULL, NULL, s);
> >
> >      return 0;
> >  }
> >diff --git a/hw/musicpal.c b/hw/musicpal.c
> >index 187a1ae..2fc391b 100644
> >--- a/hw/musicpal.c
> >+++ b/hw/musicpal.c
> >@@ -611,7 +611,7 @@ static int musicpal_lcd_init(SysBusDevice *dev)
> >      sysbus_init_mmio(dev,&s->iomem);
> >
> >      s->ds = graphic_console_init(lcd_refresh, lcd_invalidate,
> >-                                 NULL, NULL, s);
> >+                                 NULL, NULL, NULL, s);
> >      qemu_console_resize(s->ds, 128*3, 64*3);
> >
> >      qdev_init_gpio_in(&dev->qdev, musicpal_lcd_gpio_brigthness_in, 3);
> >diff --git a/hw/omap_dss.c b/hw/omap_dss.c
> >index 86ed6ea..0e8243e 100644
> >--- a/hw/omap_dss.c
> >+++ b/hw/omap_dss.c
> >@@ -1072,7 +1072,9 @@ struct omap_dss_s *omap_dss_init(struct omap_target_agent_s *ta,
> >
> >  #if 0
> >      s->state = graphic_console_init(omap_update_display,
> >-                                    omap_invalidate_display, omap_screen_dump, s);
> >+                                    omap_invalidate_display,
> >+                                    omap_screen_dump,
> >+                                    NULL, NULL, s);
> >  #endif
> >
> >      return s;
> >diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
> >index f172093..d91e88d 100644
> >--- a/hw/omap_lcdc.c
> >+++ b/hw/omap_lcdc.c
> >@@ -452,7 +452,7 @@ struct omap_lcd_panel_s *omap_lcdc_init(MemoryRegion *sysmem,
> >
> >      s->state = graphic_console_init(omap_update_display,
> >                                      omap_invalidate_display,
> >-                                    omap_screen_dump, NULL, s);
> >+                                    omap_screen_dump, NULL, NULL, s);
> >
> >      return s;
> >  }
> >diff --git a/hw/pl110.c b/hw/pl110.c
> >index f94608c..32ee89c 100644
> >--- a/hw/pl110.c
> >+++ b/hw/pl110.c
> >@@ -451,7 +451,7 @@ static int pl110_init(SysBusDevice *dev)
> >      qdev_init_gpio_in(&s->busdev.qdev, pl110_mux_ctrl_set, 1);
> >      s->ds = graphic_console_init(pl110_update_display,
> >                                   pl110_invalidate_display,
> >-                                 NULL, NULL, s);
> >+                                 NULL, NULL, NULL, s);
> >      return 0;
> >  }
> >
> >diff --git a/hw/pxa2xx_lcd.c b/hw/pxa2xx_lcd.c
> >index fcbdfb3..5345fb4 100644
> >--- a/hw/pxa2xx_lcd.c
> >+++ b/hw/pxa2xx_lcd.c
> >@@ -1004,7 +1004,7 @@ PXA2xxLCDState *pxa2xx_lcdc_init(MemoryRegion *sysmem,
> >
> >      s->ds = graphic_console_init(pxa2xx_update_display,
> >                                   pxa2xx_invalidate_display,
> >-                                 NULL, NULL, s);
> >+                                 NULL, NULL, NULL, s);
> >
> >      switch (ds_get_bits_per_pixel(s->ds)) {
> >      case 0:
> >diff --git a/hw/qxl.c b/hw/qxl.c
> >index d83d245..61a7edd 100644
> >--- a/hw/qxl.c
> >+++ b/hw/qxl.c
> >@@ -1727,7 +1727,8 @@ static int qxl_init_primary(PCIDevice *dev)
> >      portio_list_add(qxl_vga_port_list, pci_address_space_io(dev), 0x3b0);
> >
> >      vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
> >-                                   qxl_hw_screen_dump, qxl_hw_text_update, qxl);
> >+                                   qxl_hw_screen_dump, qxl_hw_text_update,
> >+                                   NULL, qxl);
> >      qemu_spice_display_init_common(&qxl->ssd, vga->ds);
> >
> >      qxl0 = qxl;
> >diff --git a/hw/sm501.c b/hw/sm501.c
> >index 786e076..8b150be 100644
> >--- a/hw/sm501.c
> >+++ b/hw/sm501.c
> >@@ -1442,6 +1442,6 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base,
> >      }
> >
> >      /* create qemu graphic console */
> >-    s->ds = graphic_console_init(sm501_update_display, NULL,
> >-				 NULL, NULL, s);
> >+    s->ds = graphic_console_init(sm501_update_display,
> >+                                 NULL, NULL, NULL, NULL, s);
> >  }
> >diff --git a/hw/ssd0303.c b/hw/ssd0303.c
> >index 4e1ee6e..05e6686 100644
> >--- a/hw/ssd0303.c
> >+++ b/hw/ssd0303.c
> >@@ -289,7 +289,7 @@ static int ssd0303_init(I2CSlave *i2c)
> >
> >      s->ds = graphic_console_init(ssd0303_update_display,
> >                                   ssd0303_invalidate_display,
> >-                                 NULL, NULL, s);
> >+                                 NULL, NULL, NULL, s);
> >      qemu_console_resize(s->ds, 96 * MAGNIFY, 16 * MAGNIFY);
> >      return 0;
> >  }
> >diff --git a/hw/ssd0323.c b/hw/ssd0323.c
> >index b0b2e94..5e3491f 100644
> >--- a/hw/ssd0323.c
> >+++ b/hw/ssd0323.c
> >@@ -330,7 +330,7 @@ static int ssd0323_init(SSISlave *dev)
> >      s->row_end = 79;
> >      s->ds = graphic_console_init(ssd0323_update_display,
> >                                   ssd0323_invalidate_display,
> >-                                 NULL, NULL, s);
> >+                                 NULL, NULL, NULL, s);
> >      qemu_console_resize(s->ds, 128 * MAGNIFY, 64 * MAGNIFY);
> >
> >      qdev_init_gpio_in(&dev->qdev, ssd0323_cd, 1);
> >diff --git a/hw/tc6393xb.c b/hw/tc6393xb.c
> >index 420925c..853b180 100644
> >--- a/hw/tc6393xb.c
> >+++ b/hw/tc6393xb.c
> >@@ -581,6 +581,7 @@ TC6393xbState *tc6393xb_init(MemoryRegion *sysmem, uint32_t base, qemu_irq irq)
> >              NULL, /* invalidate */
> >              NULL, /* screen_dump */
> >              NULL, /* text_update */
> >+            NULL, /* screen_dump_async */
> >              s);
> >
> >      return s;
> >diff --git a/hw/tcx.c b/hw/tcx.c
> >index ac7dcb4..9214dec 100644
> >--- a/hw/tcx.c
> >+++ b/hw/tcx.c
> >@@ -558,7 +558,7 @@ static int tcx_init1(SysBusDevice *dev)
> >
> >          s->ds = graphic_console_init(tcx24_update_display,
> >                                       tcx24_invalidate_display,
> >-                                     tcx24_screen_dump, NULL, s);
> >+                                     tcx24_screen_dump, NULL, NULL, s);
> >      } else {
> >          /* THC 8 bit (dummy) */
> >          memory_region_init_io(&s->thc8,&dummy_ops, s, "tcx.thc8",
> >@@ -567,7 +567,7 @@ static int tcx_init1(SysBusDevice *dev)
> >
> >          s->ds = graphic_console_init(tcx_update_display,
> >                                       tcx_invalidate_display,
> >-                                     tcx_screen_dump, NULL, s);
> >+                                     tcx_screen_dump, NULL, NULL, s);
> >      }
> >
> >      qemu_console_resize(s->ds, s->width, s->height);
> >diff --git a/hw/vga-isa-mm.c b/hw/vga-isa-mm.c
> >index f8984c6..87b8fc6 100644
> >--- a/hw/vga-isa-mm.c
> >+++ b/hw/vga-isa-mm.c
> >@@ -132,7 +132,8 @@ int isa_vga_mm_init(target_phys_addr_t vram_base,
> >      vga_mm_init(s, vram_base, ctrl_base, it_shift, address_space);
> >
> >      s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
> >-                                     s->vga.screen_dump, s->vga.text_update, s);
> >+                                     s->vga.screen_dump, s->vga.text_update,
> >+                                     s->vga.screen_dump_async, s);
> >
> >      vga_init_vbe(&s->vga, address_space);
> >      return 0;
> >diff --git a/hw/vga-isa.c b/hw/vga-isa.c
> >index 4bcc4db..a362ed2 100644
> >--- a/hw/vga-isa.c
> >+++ b/hw/vga-isa.c
> >@@ -61,7 +61,8 @@ static int vga_initfn(ISADevice *dev)
> >                                          vga_io_memory, 1);
> >      memory_region_set_coalescing(vga_io_memory);
> >      s->ds = graphic_console_init(s->update, s->invalidate,
> >-                                 s->screen_dump, s->text_update, s);
> >+                                 s->screen_dump, s->text_update,
> >+                                 s->screen_dump_async, s);
> >
> >      vga_init_vbe(s, isa_address_space(dev));
> >      /* ROM BIOS */
> >diff --git a/hw/vga-pci.c b/hw/vga-pci.c
> >index 465b643..8c8dd53 100644
> >--- a/hw/vga-pci.c
> >+++ b/hw/vga-pci.c
> >@@ -57,7 +57,8 @@ static int pci_vga_initfn(PCIDevice *dev)
> >       vga_init(s, pci_address_space(dev), pci_address_space_io(dev), true);
> >
> >       s->ds = graphic_console_init(s->update, s->invalidate,
> >-                                  s->screen_dump, s->text_update, s);
> >+                                  s->screen_dump, s->text_update,
> >+                                  s->screen_dump_async, s);
> >
> >       /* XXX: VGA_RAM_SIZE must be a power of two */
> >       pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH,&s->vram);
> >diff --git a/hw/vga_int.h b/hw/vga_int.h
> >index 7685b2b..4486cfb 100644
> >--- a/hw/vga_int.h
> >+++ b/hw/vga_int.h
> >@@ -161,6 +161,7 @@ typedef struct VGACommonState {
> >      vga_hw_invalidate_ptr invalidate;
> >      vga_hw_screen_dump_ptr screen_dump;
> >      vga_hw_text_update_ptr text_update;
> >+    vga_hw_screen_dump_async_ptr screen_dump_async;
> >      /* hardware mouse cursor support */
> >      uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32];
> >      void (*cursor_invalidate)(struct VGACommonState *s);
> >diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> >index 142d9f4..db96e61 100644
> >--- a/hw/vmware_vga.c
> >+++ b/hw/vmware_vga.c
> >@@ -1087,7 +1087,8 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size,
> >      s->vga.ds = graphic_console_init(vmsvga_update_display,
> >                                       vmsvga_invalidate_display,
> >                                       vmsvga_screen_dump,
> >-                                     vmsvga_text_update, s);
> >+                                     vmsvga_text_update,
> >+                                     NULL, s);
> >
> >
> >      s->fifo_size = SVGA_FIFO_SIZE;
> >diff --git a/hw/xenfb.c b/hw/xenfb.c
> >index 1bcf171..ad9f0d7 100644
> >--- a/hw/xenfb.c
> >+++ b/hw/xenfb.c
> >@@ -906,6 +906,7 @@ static int fb_initialise(struct XenDevice *xendev)
> >                                          xenfb_invalidate,
> >                                          NULL,
> >                                          NULL,
> >+                                        NULL,
> >                                          fb);
> >          fb->have_console = 1;
> >      }
> >@@ -1015,6 +1016,7 @@ wait_more:
> >                                      xenfb_invalidate,
> >                                      NULL,
> >                                      NULL,
> >+                                    NULL,
> >                                      fb);
> >      fb->have_console = 1;
> >
> >diff --git a/monitor.c b/monitor.c
> >index 1a65c41..91a76a1 100644
> >--- a/monitor.c
> >+++ b/monitor.c
> >@@ -901,6 +901,11 @@ static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >      return 0;
> >  }
> >
> >+void qmp_screendump_async(const char *filename, Error **errp)
> >+{
> >+    vga_hw_screen_dump_async(filename);
> >+}
> >+
> >  static void do_logfile(Monitor *mon, const QDict *qdict)
> >  {
> >      cpu_set_log_filename(qdict_get_str(qdict, "filename"));
> >diff --git a/qapi-schema.json b/qapi-schema.json
> >index 72b17f1..a5873cb 100644
> >--- a/qapi-schema.json
> >+++ b/qapi-schema.json
> >@@ -1595,3 +1595,23 @@
> >  { 'command': 'qom-list-types',
> >    'data': { '*implements': 'str', '*abstract': 'bool' },
> >    'returns': [ 'ObjectTypeInfo' ] }
> >+
> >+##
> >+## @screendump-async:
> >+#
> >+# This command will perform a screen dump of the first console to the givem
> >+# filename. The additional parameters are unused at this time.
> >+#
> >+# @filename  name of output file to write screen dump to
> >+#
> >+# Since: 1.1
> >+#
> >+# Notes: This command is experimental and may change syntax in future releases.
> >+#
> >+# This command is the same as the qmp/hmp screendump command, except that on
> >+# successful completion of the scren dump the SCREEN_DUMP_COMPLETE event is
> >+# emitted.
> >+#
> >+##
> >+{ 'command': 'screendump-async',
> >+  'data': { 'filename': 'str' } }
> >diff --git a/qmp-commands.hx b/qmp-commands.hx
> >index 705f704..ea0d051 100644
> >--- a/qmp-commands.hx
> >+++ b/qmp-commands.hx
> >@@ -170,6 +170,32 @@ Example:
> >  EQMP
> >
> >      {
> >+        .name       = "screendump-async",
> >+        .args_type  = "filename:F",
> >+        .params     = "filename",
> >+        .help       = "save screen into PPM image 'filename'",
> >+        .user_print = monitor_user_noop,
> >+        .mhandler.cmd_new = qmp_marshal_input_screendump_async,
> >+    },
> >+
> >+SQMP
> >+screendump-async
> >+----------------
> >+
> >+Save screen into PPM image. Fires a SCREEN_DUMP_COMPLETE event on completion.
> >+
> >+Arguments:
> >+
> >+- "filename": file path (json-string)
> >+
> >+Example:
> >+
> >+->  { "execute": "screendump-async", "arguments": { "filename": "/tmp/image" } }
> >+<- { "return": {} }
> >+
> >+EQMP
> >+
> >+    {
> >          .name       = "stop",
> >          .args_type  = "",
> >          .mhandler.cmd_new = qmp_marshal_input_stop,
> 
> 

Re: [Qemu-devel] [PATCH 4/4] add qmp screendump-async

[Luiz Capitulino]

On Sat, 25 Feb 2012 10:46:07 +0200
Alon Levy <alevy@redhat.com> wrote:

> On Fri, Feb 24, 2012 at 04:40:15PM -0600, Anthony Liguori wrote:
> > On 02/24/2012 03:22 PM, Alon Levy wrote:
> > >This is an across the board change since I wanted to keep the existing
> > >(good imo) single graphic_console_init callback setter, instead of
> > >introducing a new cb that isn't set by it but instead by a second
> > >initialization function.
> > >
> > >Signed-off-by: Alon Levy<alevy@redhat.com>
> > 
> > What's the rationale for this?
> 
> There is a hang possible with the current screendump command, qxl, a
> spice client using libvirt and spice-gtk such as virt-viewer /
> remote-viewer, where you have:
> 1. libvirt waiting for screendump to complete
> 2. screendump waiting for spice server thread to render
> 3. spice server thread waiting for spice client to read messages

Which messages?

> 4. spice client == libvirt client, waiting for screendump completion

The way I had understood this problem is that qxl takes a long time to
perform a screen dump, which would cause the global mutex to be held for
a long time. If this is really serious, then a async command for it
makes sense IMO.

Now, the problem you describe above is different and maybe it shouldn't
be solved at QMP level. Can you describe it in more detail?

> > I really don't want a plethora of pseudo-async commands.

Agreed, but this is not supposed to be pseudo-async. If a command returns
immediately and sends an event when it later completes, it's really async.

Besides, we have agreed on adding new command + event for commands that
really need to be async.

Re: [Qemu-devel] [PATCH 4/4] add qmp screendump-async

[Gerd Hoffmann]

  Hi,

>> There is a hang possible with the current screendump command, qxl, a
>> spice client using libvirt and spice-gtk such as virt-viewer /
>> remote-viewer, where you have:
>> 1. libvirt waiting for screendump to complete
>> 2. screendump waiting for spice server thread to render
>> 3. spice server thread waiting for spice client to read messages
> 
> Which messages?

spice protocol.

>> 4. spice client == libvirt client, waiting for screendump completion
> 
> The way I had understood this problem is that qxl takes a long time to
> perform a screen dump, which would cause the global mutex to be held for
> a long time. If this is really serious, then a async command for it
> makes sense IMO.

That describes it pretty good.  Normal workflow is that spice-server
sends the rendering commands received from the guest off to the spice
client and the actual rendering happens there.  spice-server also keeps
track of the rendering commands so it can actually render on the host
side too of needed (called 'local rendering').  But it doesn't do that
unless it has to to avoid wasting cycles.  Events which need local
rendering are:  (1) display on sdl/vnc, (2) screendump monitor command,
 (3) guest requesting screen content (happens when doing screen shots
for example).

So when issuing the screendump monitor command we have to ask
spice-server to render the screen for us, when it is done we can write
out the screen dump.  We don't want block the iothread while the spice
server is busy does the rendering.

Additionally the local rendering can be delayed by a spice-client
connected via slow network link.  This is (3) in alon's list above.
There are plans to fix that, but it isn't trivial to do due to the way
spice-server keeps track of the rendering commands.  It can delay the
rendering several seconds.  But even without that I've seen delays
around 50ms, which is still way too much to have iothread blocked.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 4/4] add qmp screendump-async

[Luiz Capitulino]

On Wed, 29 Feb 2012 08:49:10 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> >> There is a hang possible with the current screendump command, qxl, a
> >> spice client using libvirt and spice-gtk such as virt-viewer /
> >> remote-viewer, where you have:
> >> 1. libvirt waiting for screendump to complete
> >> 2. screendump waiting for spice server thread to render
> >> 3. spice server thread waiting for spice client to read messages
> > 
> > Which messages?
> 
> spice protocol.
> 
> >> 4. spice client == libvirt client, waiting for screendump completion
> > 
> > The way I had understood this problem is that qxl takes a long time to
> > perform a screen dump, which would cause the global mutex to be held for
> > a long time. If this is really serious, then a async command for it
> > makes sense IMO.
> 
> That describes it pretty good.  Normal workflow is that spice-server
> sends the rendering commands received from the guest off to the spice
> client and the actual rendering happens there.  spice-server also keeps
> track of the rendering commands so it can actually render on the host
> side too of needed (called 'local rendering').  But it doesn't do that
> unless it has to to avoid wasting cycles.  Events which need local
> rendering are:  (1) display on sdl/vnc, (2) screendump monitor command,
>  (3) guest requesting screen content (happens when doing screen shots
> for example).
> 
> So when issuing the screendump monitor command we have to ask
> spice-server to render the screen for us, when it is done we can write
> out the screen dump.  We don't want block the iothread while the spice
> server is busy does the rendering.

Anthony, the only way I can think of solving this is making the screendump
asynchronous. Do you have a different suggestion?

> 
> Additionally the local rendering can be delayed by a spice-client
> connected via slow network link.  This is (3) in alon's list above.
> There are plans to fix that, but it isn't trivial to do due to the way
> spice-server keeps track of the rendering commands.  It can delay the
> rendering several seconds.  But even without that I've seen delays
> around 50ms, which is still way too much to have iothread blocked.
> 
> cheers,
>   Gerd
> 

Re: [Qemu-devel] [PATCH 4/4] add qmp screendump-async

[Alon Levy]

On Tue, Feb 28, 2012 at 05:10:39PM -0300, Luiz Capitulino wrote:
> On Sat, 25 Feb 2012 10:46:07 +0200
> Alon Levy <alevy@redhat.com> wrote:
> 
> > On Fri, Feb 24, 2012 at 04:40:15PM -0600, Anthony Liguori wrote:
> > > On 02/24/2012 03:22 PM, Alon Levy wrote:
> > > >This is an across the board change since I wanted to keep the existing
> > > >(good imo) single graphic_console_init callback setter, instead of
> > > >introducing a new cb that isn't set by it but instead by a second
> > > >initialization function.
> > > >
> > > >Signed-off-by: Alon Levy<alevy@redhat.com>
> > > 
> > > What's the rationale for this?
> > 
> > There is a hang possible with the current screendump command, qxl, a
> > spice client using libvirt and spice-gtk such as virt-viewer /
> > remote-viewer, where you have:
> > 1. libvirt waiting for screendump to complete
> > 2. screendump waiting for spice server thread to render
> > 3. spice server thread waiting for spice client to read messages
> 
> Which messages?

spice display channel messages.

> 
> > 4. spice client == libvirt client, waiting for screendump completion
> 
> The way I had understood this problem is that qxl takes a long time to
> perform a screen dump, which would cause the global mutex to be held for
> a long time. If this is really serious, then a async command for it
> makes sense IMO.

That is true, but it is not the immediate problem the bz is dealing with
- if it was only this there would be no hang.

> 
> Now, the problem you describe above is different and maybe it shouldn't
> be solved at QMP level. Can you describe it in more detail?
> 

I can just point to the diagram I put into the mail from october:

"""
This fixes a hang that is caused when the spice client is also a qemu
monitor client and the client is single threaded:


  io thread                   worker thread                client

   <---------------------------------------------------- screendump
  do_screen_dump-> read------->
                               flush, read
                               client socket------------->
"""

I have not managed to reproduce this myself, so I'm cc'ing Marc-Andre
who has and reported the fix of using an asyncronous operation
internally (unrelated to the asynchronous qevent in this patchset)
between the monitor thread and the spice thread fixed the problem.

> > > I really don't want a plethora of pseudo-async commands.
> 
> Agreed, but this is not supposed to be pseudo-async. If a command returns
> immediately and sends an event when it later completes, it's really async.
Thanks for saying this. I didn't understand the pseudo-async angle.

Re: [Qemu-devel] [PATCH 4/4] add qmp screendump-async

[Luiz Capitulino]

On Wed, 29 Feb 2012 10:15:53 +0200
Alon Levy <alevy@redhat.com> wrote:

> On Tue, Feb 28, 2012 at 05:10:39PM -0300, Luiz Capitulino wrote:
> > On Sat, 25 Feb 2012 10:46:07 +0200
> > Alon Levy <alevy@redhat.com> wrote:
> > 
> > > On Fri, Feb 24, 2012 at 04:40:15PM -0600, Anthony Liguori wrote:
> > > > On 02/24/2012 03:22 PM, Alon Levy wrote:
> > > > >This is an across the board change since I wanted to keep the existing
> > > > >(good imo) single graphic_console_init callback setter, instead of
> > > > >introducing a new cb that isn't set by it but instead by a second
> > > > >initialization function.
> > > > >
> > > > >Signed-off-by: Alon Levy<alevy@redhat.com>
> > > > 
> > > > What's the rationale for this?
> > > 
> > > There is a hang possible with the current screendump command, qxl, a
> > > spice client using libvirt and spice-gtk such as virt-viewer /
> > > remote-viewer, where you have:
> > > 1. libvirt waiting for screendump to complete
> > > 2. screendump waiting for spice server thread to render
> > > 3. spice server thread waiting for spice client to read messages
> > 
> > Which messages?
> 
> spice display channel messages.
> 
> > 
> > > 4. spice client == libvirt client, waiting for screendump completion
> > 
> > The way I had understood this problem is that qxl takes a long time to
> > perform a screen dump, which would cause the global mutex to be held for
> > a long time. If this is really serious, then a async command for it
> > makes sense IMO.
> 
> That is true, but it is not the immediate problem the bz is dealing with
> - if it was only this there would be no hang.

Well, this kind of hang always smells like a spice threading synchronization
problem to me. I thought that I'd be capable of showing that if I really
understood what was going on, but I can't, even with your diagram.

An asynchronous command solves the global mutex contention problem, but I
think this hang should be further investigated, otherwise the async command
risks just hiding the real problem.

Re: [Qemu-devel] [PATCH 4/4] add qmp screendump-async

[Alon Levy]

On Wed, Feb 29, 2012 at 11:58:37AM -0300, Luiz Capitulino wrote:
> On Wed, 29 Feb 2012 10:15:53 +0200
> Alon Levy <alevy@redhat.com> wrote:
> 
> > On Tue, Feb 28, 2012 at 05:10:39PM -0300, Luiz Capitulino wrote:
> > > On Sat, 25 Feb 2012 10:46:07 +0200
> > > Alon Levy <alevy@redhat.com> wrote:
> > > 
> > > > On Fri, Feb 24, 2012 at 04:40:15PM -0600, Anthony Liguori wrote:
> > > > > On 02/24/2012 03:22 PM, Alon Levy wrote:
> > > > > >This is an across the board change since I wanted to keep the existing
> > > > > >(good imo) single graphic_console_init callback setter, instead of
> > > > > >introducing a new cb that isn't set by it but instead by a second
> > > > > >initialization function.
> > > > > >
> > > > > >Signed-off-by: Alon Levy<alevy@redhat.com>
> > > > > 
> > > > > What's the rationale for this?
> > > > 
> > > > There is a hang possible with the current screendump command, qxl, a
> > > > spice client using libvirt and spice-gtk such as virt-viewer /
> > > > remote-viewer, where you have:
> > > > 1. libvirt waiting for screendump to complete
> > > > 2. screendump waiting for spice server thread to render
> > > > 3. spice server thread waiting for spice client to read messages
> > > 
> > > Which messages?
> > 
> > spice display channel messages.
> > 
> > > 
> > > > 4. spice client == libvirt client, waiting for screendump completion
> > > 
> > > The way I had understood this problem is that qxl takes a long time to
> > > perform a screen dump, which would cause the global mutex to be held for
> > > a long time. If this is really serious, then a async command for it
> > > makes sense IMO.
> > 
> > That is true, but it is not the immediate problem the bz is dealing with
> > - if it was only this there would be no hang.
> 
> Well, this kind of hang always smells like a spice threading synchronization
> problem to me. I thought that I'd be capable of showing that if I really
> understood what was going on, but I can't, even with your diagram.

It isn't a spice threading synchronization problem, it's much simpler
then that: spice server flushes messages to the spice client, so the
client not handling them causes it to sleep for a very long time (the
timeout is 150 seconds)

server: handle_dev_update/flush_display_commands

The client in this case is not reading from the socket. I couldn't
reproduce, but I imagined it was simply waiting for completion of the
screendump command before it came around to reading the spice sockets.

> 
> An asynchronous command solves the global mutex contention problem, but I
> think this hang should be further investigated, otherwise the async command
> risks just hiding the real problem.

Re: [Qemu-devel] [PATCH 4/4] add qmp screendump-async

[Luiz Capitulino]

On Fri, 24 Feb 2012 23:22:05 +0200
Alon Levy <alevy@redhat.com> wrote:

> This is an across the board change since I wanted to keep the existing
> (good imo) single graphic_console_init callback setter, instead of
> introducing a new cb that isn't set by it but instead by a second
> initialization function.
> 
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  console.c            |   25 +++++++++++++++++++++++--
>  console.h            |    5 +++++
>  hw/blizzard.c        |    2 +-
>  hw/cirrus_vga.c      |    4 ++--
>  hw/exynos4210_fimd.c |    3 ++-
>  hw/g364fb.c          |    2 +-
>  hw/jazz_led.c        |    3 ++-
>  hw/milkymist-vgafb.c |    2 +-
>  hw/musicpal.c        |    2 +-
>  hw/omap_dss.c        |    4 +++-
>  hw/omap_lcdc.c       |    2 +-
>  hw/pl110.c           |    2 +-
>  hw/pxa2xx_lcd.c      |    2 +-
>  hw/qxl.c             |    3 ++-
>  hw/sm501.c           |    4 ++--
>  hw/ssd0303.c         |    2 +-
>  hw/ssd0323.c         |    2 +-
>  hw/tc6393xb.c        |    1 +
>  hw/tcx.c             |    4 ++--
>  hw/vga-isa-mm.c      |    3 ++-
>  hw/vga-isa.c         |    3 ++-
>  hw/vga-pci.c         |    3 ++-
>  hw/vga_int.h         |    1 +
>  hw/vmware_vga.c      |    3 ++-
>  hw/xenfb.c           |    2 ++
>  monitor.c            |    5 +++++
>  qapi-schema.json     |   20 ++++++++++++++++++++
>  qmp-commands.hx      |   26 ++++++++++++++++++++++++++
>  28 files changed, 115 insertions(+), 25 deletions(-)
> 
> diff --git a/console.c b/console.c
> index 6750538..ced2aa7 100644
> --- a/console.c
> +++ b/console.c
> @@ -124,6 +124,7 @@ struct TextConsole {
>      vga_hw_update_ptr hw_update;
>      vga_hw_invalidate_ptr hw_invalidate;
>      vga_hw_screen_dump_ptr hw_screen_dump;
> +    vga_hw_screen_dump_async_ptr hw_screen_dump_async;
>      vga_hw_text_update_ptr hw_text_update;
>      void *hw;
>  
> @@ -175,8 +176,9 @@ void vga_hw_invalidate(void)
>          active_console->hw_invalidate(active_console->hw);
>  }
>  
> -void vga_hw_screen_dump(const char *filename)
> +static void vga_hw_screen_dump_helper(const char *filename, bool async)
>  {
> +    bool event_sent = false;
>      TextConsole *previous_active_console;
>      bool cswitch;
>  
> @@ -188,17 +190,33 @@ void vga_hw_screen_dump(const char *filename)
>      if (cswitch) {
>          console_select(0);
>      }
> -    if (consoles[0] && consoles[0]->hw_screen_dump) {
> +    if (async && consoles[0] && consoles[0]->hw_screen_dump_async) {
> +        consoles[0]->hw_screen_dump_async(consoles[0]->hw, filename, cswitch);
> +        event_sent = true;
> +    } else if (consoles[0] && consoles[0]->hw_screen_dump) {
>          consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
>      } else {
>          error_report("screen dump not implemented");
>      }
> +    if (async && !event_sent) {
> +        monitor_protocol_screen_dump_complete_event(filename);
> +    }

This is wrong. The event should be emitted by hw_screen_dump_async() when it
completes.

Also, this patch should be split into a least two patches. One patch does the
console changes and the other one adds the qmp async command. It's also possible
to make more patches by having individual devices' implementation of
hw_screen_dump_async() in their own patches.

>  
>      if (cswitch) {
>          console_select(previous_active_console->index);
>      }
>  }
>  
> +void vga_hw_screen_dump(const char *filename)
> +{
> +    vga_hw_screen_dump_helper(filename, false);
> +}
> +
> +void vga_hw_screen_dump_async(const char *filename)
> +{
> +    vga_hw_screen_dump_helper(filename, true);
> +}
> +
>  void vga_hw_text_update(console_ch_t *chardata)
>  {
>      if (active_console && active_console->hw_text_update)
> @@ -1403,6 +1421,8 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
>                                     vga_hw_invalidate_ptr invalidate,
>                                     vga_hw_screen_dump_ptr screen_dump,
>                                     vga_hw_text_update_ptr text_update,
> +                                   vga_hw_screen_dump_async_ptr
> +                                                    screen_dump_async,
>                                     void *opaque)
>  {
>      TextConsole *s;
> @@ -1421,6 +1441,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
>      s->hw_update = update;
>      s->hw_invalidate = invalidate;
>      s->hw_screen_dump = screen_dump;
> +    s->hw_screen_dump_async = screen_dump_async;
>      s->hw_text_update = text_update;
>      s->hw = opaque;
>  
> diff --git a/console.h b/console.h
> index c22803c..3cf28c0 100644
> --- a/console.h
> +++ b/console.h
> @@ -341,17 +341,22 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
>  typedef void (*vga_hw_update_ptr)(void *);
>  typedef void (*vga_hw_invalidate_ptr)(void *);
>  typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
> +typedef void (*vga_hw_screen_dump_async_ptr)(void *, const char *filename,
> +                                             bool cswitch);
>  typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
>  
>  DisplayState *graphic_console_init(vga_hw_update_ptr update,
>                                     vga_hw_invalidate_ptr invalidate,
>                                     vga_hw_screen_dump_ptr screen_dump,
>                                     vga_hw_text_update_ptr text_update,
> +                                   vga_hw_screen_dump_async_ptr
> +                                                    screen_dump_async,
>                                     void *opaque);
>  
>  void vga_hw_update(void);
>  void vga_hw_invalidate(void);
>  void vga_hw_screen_dump(const char *filename);
> +void vga_hw_screen_dump_async(const char *filename);
>  void vga_hw_text_update(console_ch_t *chardata);
>  void monitor_protocol_screen_dump_complete_event(const char *filename);
>  
> diff --git a/hw/blizzard.c b/hw/blizzard.c
> index c7d844d..cc51045 100644
> --- a/hw/blizzard.c
> +++ b/hw/blizzard.c
> @@ -963,7 +963,7 @@ void *s1d13745_init(qemu_irq gpio_int)
>  
>      s->state = graphic_console_init(blizzard_update_display,
>                                   blizzard_invalidate_display,
> -                                 blizzard_screen_dump, NULL, s);
> +                                 blizzard_screen_dump, NULL, NULL, s);
>  
>      switch (ds_get_bits_per_pixel(s->state)) {
>      case 0:
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 4edcb94..d2a9c27 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -2900,7 +2900,7 @@ static int vga_initfn(ISADevice *dev)
>                         isa_address_space(dev));
>      s->ds = graphic_console_init(s->update, s->invalidate,
>                                   s->screen_dump, s->text_update,
> -                                 s);
> +                                 s->screen_dump_async, s);
>      rom_add_vga(VGABIOS_CIRRUS_FILENAME);
>      /* XXX ISA-LFB support */
>      /* FIXME not qdev yet */
> @@ -2941,7 +2941,7 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
>       cirrus_init_common(s, device_id, 1, pci_address_space(dev));
>       s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
>                                        s->vga.screen_dump, s->vga.text_update,
> -                                      &s->vga);
> +                                      s->vga.screen_dump_async, &s->vga);
>  
>       /* setup PCI */
>  
> diff --git a/hw/exynos4210_fimd.c b/hw/exynos4210_fimd.c
> index 3313f00..2053ed0 100644
> --- a/hw/exynos4210_fimd.c
> +++ b/hw/exynos4210_fimd.c
> @@ -1898,7 +1898,8 @@ static int exynos4210_fimd_init(SysBusDevice *dev)
>              "exynos4210.fimd", FIMD_REGS_SIZE);
>      sysbus_init_mmio(dev, &s->iomem);
>      s->console = graphic_console_init(exynos4210_fimd_update,
> -                                  exynos4210_fimd_invalidate, NULL, NULL, s);
> +                                  exynos4210_fimd_invalidate,
> +                                  NULL, NULL, NULL, s);
>  
>      return 0;
>  }
> diff --git a/hw/g364fb.c b/hw/g364fb.c
> index 3a0b68f..8ae40d8 100644
> --- a/hw/g364fb.c
> +++ b/hw/g364fb.c
> @@ -517,7 +517,7 @@ static void g364fb_init(DeviceState *dev, G364State *s)
>  
>      s->ds = graphic_console_init(g364fb_update_display,
>                                   g364fb_invalidate_display,
> -                                 g364fb_screen_dump, NULL, s);
> +                                 g364fb_screen_dump, NULL, NULL, s);
>  
>      memory_region_init_io(&s->mem_ctrl, &g364fb_ctrl_ops, s, "ctrl", 0x180000);
>      memory_region_init_ram_ptr(&s->mem_vram, "vram",
> diff --git a/hw/jazz_led.c b/hw/jazz_led.c
> index 6486523..bb5daf7 100644
> --- a/hw/jazz_led.c
> +++ b/hw/jazz_led.c
> @@ -251,7 +251,8 @@ static int jazz_led_init(SysBusDevice *dev)
>      s->ds = graphic_console_init(jazz_led_update_display,
>                                   jazz_led_invalidate_display,
>                                   NULL,
> -                                 jazz_led_text_update, s);
> +                                 jazz_led_text_update,
> +                                 NULL, s);
>  
>      return 0;
>  }
> diff --git a/hw/milkymist-vgafb.c b/hw/milkymist-vgafb.c
> index 69afd72..ae018d1 100644
> --- a/hw/milkymist-vgafb.c
> +++ b/hw/milkymist-vgafb.c
> @@ -276,7 +276,7 @@ static int milkymist_vgafb_init(SysBusDevice *dev)
>  
>      s->ds = graphic_console_init(vgafb_update_display,
>                                   vgafb_invalidate_display,
> -                                 NULL, NULL, s);
> +                                 NULL, NULL, NULL, s);
>  
>      return 0;
>  }
> diff --git a/hw/musicpal.c b/hw/musicpal.c
> index 187a1ae..2fc391b 100644
> --- a/hw/musicpal.c
> +++ b/hw/musicpal.c
> @@ -611,7 +611,7 @@ static int musicpal_lcd_init(SysBusDevice *dev)
>      sysbus_init_mmio(dev, &s->iomem);
>  
>      s->ds = graphic_console_init(lcd_refresh, lcd_invalidate,
> -                                 NULL, NULL, s);
> +                                 NULL, NULL, NULL, s);
>      qemu_console_resize(s->ds, 128*3, 64*3);
>  
>      qdev_init_gpio_in(&dev->qdev, musicpal_lcd_gpio_brigthness_in, 3);
> diff --git a/hw/omap_dss.c b/hw/omap_dss.c
> index 86ed6ea..0e8243e 100644
> --- a/hw/omap_dss.c
> +++ b/hw/omap_dss.c
> @@ -1072,7 +1072,9 @@ struct omap_dss_s *omap_dss_init(struct omap_target_agent_s *ta,
>  
>  #if 0
>      s->state = graphic_console_init(omap_update_display,
> -                                    omap_invalidate_display, omap_screen_dump, s);
> +                                    omap_invalidate_display,
> +                                    omap_screen_dump,
> +                                    NULL, NULL, s);
>  #endif
>  
>      return s;
> diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
> index f172093..d91e88d 100644
> --- a/hw/omap_lcdc.c
> +++ b/hw/omap_lcdc.c
> @@ -452,7 +452,7 @@ struct omap_lcd_panel_s *omap_lcdc_init(MemoryRegion *sysmem,
>  
>      s->state = graphic_console_init(omap_update_display,
>                                      omap_invalidate_display,
> -                                    omap_screen_dump, NULL, s);
> +                                    omap_screen_dump, NULL, NULL, s);
>  
>      return s;
>  }
> diff --git a/hw/pl110.c b/hw/pl110.c
> index f94608c..32ee89c 100644
> --- a/hw/pl110.c
> +++ b/hw/pl110.c
> @@ -451,7 +451,7 @@ static int pl110_init(SysBusDevice *dev)
>      qdev_init_gpio_in(&s->busdev.qdev, pl110_mux_ctrl_set, 1);
>      s->ds = graphic_console_init(pl110_update_display,
>                                   pl110_invalidate_display,
> -                                 NULL, NULL, s);
> +                                 NULL, NULL, NULL, s);
>      return 0;
>  }
>  
> diff --git a/hw/pxa2xx_lcd.c b/hw/pxa2xx_lcd.c
> index fcbdfb3..5345fb4 100644
> --- a/hw/pxa2xx_lcd.c
> +++ b/hw/pxa2xx_lcd.c
> @@ -1004,7 +1004,7 @@ PXA2xxLCDState *pxa2xx_lcdc_init(MemoryRegion *sysmem,
>  
>      s->ds = graphic_console_init(pxa2xx_update_display,
>                                   pxa2xx_invalidate_display,
> -                                 NULL, NULL, s);
> +                                 NULL, NULL, NULL, s);
>  
>      switch (ds_get_bits_per_pixel(s->ds)) {
>      case 0:
> diff --git a/hw/qxl.c b/hw/qxl.c
> index d83d245..61a7edd 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -1727,7 +1727,8 @@ static int qxl_init_primary(PCIDevice *dev)
>      portio_list_add(qxl_vga_port_list, pci_address_space_io(dev), 0x3b0);
>  
>      vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
> -                                   qxl_hw_screen_dump, qxl_hw_text_update, qxl);
> +                                   qxl_hw_screen_dump, qxl_hw_text_update,
> +                                   NULL, qxl);
>      qemu_spice_display_init_common(&qxl->ssd, vga->ds);
>  
>      qxl0 = qxl;
> diff --git a/hw/sm501.c b/hw/sm501.c
> index 786e076..8b150be 100644
> --- a/hw/sm501.c
> +++ b/hw/sm501.c
> @@ -1442,6 +1442,6 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base,
>      }
>  
>      /* create qemu graphic console */
> -    s->ds = graphic_console_init(sm501_update_display, NULL,
> -				 NULL, NULL, s);
> +    s->ds = graphic_console_init(sm501_update_display,
> +                                 NULL, NULL, NULL, NULL, s);
>  }
> diff --git a/hw/ssd0303.c b/hw/ssd0303.c
> index 4e1ee6e..05e6686 100644
> --- a/hw/ssd0303.c
> +++ b/hw/ssd0303.c
> @@ -289,7 +289,7 @@ static int ssd0303_init(I2CSlave *i2c)
>  
>      s->ds = graphic_console_init(ssd0303_update_display,
>                                   ssd0303_invalidate_display,
> -                                 NULL, NULL, s);
> +                                 NULL, NULL, NULL, s);
>      qemu_console_resize(s->ds, 96 * MAGNIFY, 16 * MAGNIFY);
>      return 0;
>  }
> diff --git a/hw/ssd0323.c b/hw/ssd0323.c
> index b0b2e94..5e3491f 100644
> --- a/hw/ssd0323.c
> +++ b/hw/ssd0323.c
> @@ -330,7 +330,7 @@ static int ssd0323_init(SSISlave *dev)
>      s->row_end = 79;
>      s->ds = graphic_console_init(ssd0323_update_display,
>                                   ssd0323_invalidate_display,
> -                                 NULL, NULL, s);
> +                                 NULL, NULL, NULL, s);
>      qemu_console_resize(s->ds, 128 * MAGNIFY, 64 * MAGNIFY);
>  
>      qdev_init_gpio_in(&dev->qdev, ssd0323_cd, 1);
> diff --git a/hw/tc6393xb.c b/hw/tc6393xb.c
> index 420925c..853b180 100644
> --- a/hw/tc6393xb.c
> +++ b/hw/tc6393xb.c
> @@ -581,6 +581,7 @@ TC6393xbState *tc6393xb_init(MemoryRegion *sysmem, uint32_t base, qemu_irq irq)
>              NULL, /* invalidate */
>              NULL, /* screen_dump */
>              NULL, /* text_update */
> +            NULL, /* screen_dump_async */
>              s);
>  
>      return s;
> diff --git a/hw/tcx.c b/hw/tcx.c
> index ac7dcb4..9214dec 100644
> --- a/hw/tcx.c
> +++ b/hw/tcx.c
> @@ -558,7 +558,7 @@ static int tcx_init1(SysBusDevice *dev)
>  
>          s->ds = graphic_console_init(tcx24_update_display,
>                                       tcx24_invalidate_display,
> -                                     tcx24_screen_dump, NULL, s);
> +                                     tcx24_screen_dump, NULL, NULL, s);
>      } else {
>          /* THC 8 bit (dummy) */
>          memory_region_init_io(&s->thc8, &dummy_ops, s, "tcx.thc8",
> @@ -567,7 +567,7 @@ static int tcx_init1(SysBusDevice *dev)
>  
>          s->ds = graphic_console_init(tcx_update_display,
>                                       tcx_invalidate_display,
> -                                     tcx_screen_dump, NULL, s);
> +                                     tcx_screen_dump, NULL, NULL, s);
>      }
>  
>      qemu_console_resize(s->ds, s->width, s->height);
> diff --git a/hw/vga-isa-mm.c b/hw/vga-isa-mm.c
> index f8984c6..87b8fc6 100644
> --- a/hw/vga-isa-mm.c
> +++ b/hw/vga-isa-mm.c
> @@ -132,7 +132,8 @@ int isa_vga_mm_init(target_phys_addr_t vram_base,
>      vga_mm_init(s, vram_base, ctrl_base, it_shift, address_space);
>  
>      s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
> -                                     s->vga.screen_dump, s->vga.text_update, s);
> +                                     s->vga.screen_dump, s->vga.text_update,
> +                                     s->vga.screen_dump_async, s);
>  
>      vga_init_vbe(&s->vga, address_space);
>      return 0;
> diff --git a/hw/vga-isa.c b/hw/vga-isa.c
> index 4bcc4db..a362ed2 100644
> --- a/hw/vga-isa.c
> +++ b/hw/vga-isa.c
> @@ -61,7 +61,8 @@ static int vga_initfn(ISADevice *dev)
>                                          vga_io_memory, 1);
>      memory_region_set_coalescing(vga_io_memory);
>      s->ds = graphic_console_init(s->update, s->invalidate,
> -                                 s->screen_dump, s->text_update, s);
> +                                 s->screen_dump, s->text_update,
> +                                 s->screen_dump_async, s);
>  
>      vga_init_vbe(s, isa_address_space(dev));
>      /* ROM BIOS */
> diff --git a/hw/vga-pci.c b/hw/vga-pci.c
> index 465b643..8c8dd53 100644
> --- a/hw/vga-pci.c
> +++ b/hw/vga-pci.c
> @@ -57,7 +57,8 @@ static int pci_vga_initfn(PCIDevice *dev)
>       vga_init(s, pci_address_space(dev), pci_address_space_io(dev), true);
>  
>       s->ds = graphic_console_init(s->update, s->invalidate,
> -                                  s->screen_dump, s->text_update, s);
> +                                  s->screen_dump, s->text_update,
> +                                  s->screen_dump_async, s);
>  
>       /* XXX: VGA_RAM_SIZE must be a power of two */
>       pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->vram);
> diff --git a/hw/vga_int.h b/hw/vga_int.h
> index 7685b2b..4486cfb 100644
> --- a/hw/vga_int.h
> +++ b/hw/vga_int.h
> @@ -161,6 +161,7 @@ typedef struct VGACommonState {
>      vga_hw_invalidate_ptr invalidate;
>      vga_hw_screen_dump_ptr screen_dump;
>      vga_hw_text_update_ptr text_update;
> +    vga_hw_screen_dump_async_ptr screen_dump_async;
>      /* hardware mouse cursor support */
>      uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32];
>      void (*cursor_invalidate)(struct VGACommonState *s);
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index 142d9f4..db96e61 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -1087,7 +1087,8 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size,
>      s->vga.ds = graphic_console_init(vmsvga_update_display,
>                                       vmsvga_invalidate_display,
>                                       vmsvga_screen_dump,
> -                                     vmsvga_text_update, s);
> +                                     vmsvga_text_update,
> +                                     NULL, s);
>  
>  
>      s->fifo_size = SVGA_FIFO_SIZE;
> diff --git a/hw/xenfb.c b/hw/xenfb.c
> index 1bcf171..ad9f0d7 100644
> --- a/hw/xenfb.c
> +++ b/hw/xenfb.c
> @@ -906,6 +906,7 @@ static int fb_initialise(struct XenDevice *xendev)
>                                          xenfb_invalidate,
>                                          NULL,
>                                          NULL,
> +                                        NULL,
>                                          fb);
>          fb->have_console = 1;
>      }
> @@ -1015,6 +1016,7 @@ wait_more:
>                                      xenfb_invalidate,
>                                      NULL,
>                                      NULL,
> +                                    NULL,
>                                      fb);
>      fb->have_console = 1;
>  
> diff --git a/monitor.c b/monitor.c
> index 1a65c41..91a76a1 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -901,6 +901,11 @@ static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
>      return 0;
>  }
>  
> +void qmp_screendump_async(const char *filename, Error **errp)
> +{
> +    vga_hw_screen_dump_async(filename);
> +}
> +
>  static void do_logfile(Monitor *mon, const QDict *qdict)
>  {
>      cpu_set_log_filename(qdict_get_str(qdict, "filename"));
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 72b17f1..a5873cb 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1595,3 +1595,23 @@
>  { 'command': 'qom-list-types',
>    'data': { '*implements': 'str', '*abstract': 'bool' },
>    'returns': [ 'ObjectTypeInfo' ] }
> +
> +##
> +## @screendump-async:
> +#
> +# This command will perform a screen dump of the first console to the givem
> +# filename. The additional parameters are unused at this time.

Isn't it interesting to allow the mngt app to specify the console?

> +#
> +# @filename  name of output file to write screen dump to
> +#
> +# Since: 1.1
> +#
> +# Notes: This command is experimental and may change syntax in future releases.

Why is this needed?

> +#
> +# This command is the same as the qmp/hmp screendump command, except that on
> +# successful completion of the scren dump the SCREEN_DUMP_COMPLETE event is
> +# emitted.

The real difference is that it's asynchronous, while the other is synchronous.

> +#
> +##
> +{ 'command': 'screendump-async',
> +  'data': { 'filename': 'str' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 705f704..ea0d051 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -170,6 +170,32 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "screendump-async",
> +        .args_type  = "filename:F",
> +        .params     = "filename",
> +        .help       = "save screen into PPM image 'filename'",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = qmp_marshal_input_screendump_async,
> +    },
> +
> +SQMP
> +screendump-async
> +----------------
> +
> +Save screen into PPM image. Fires a SCREEN_DUMP_COMPLETE event on completion.
> +
> +Arguments:
> +
> +- "filename": file path (json-string)
> +
> +Example:
> +
> +-> { "execute": "screendump-async", "arguments": { "filename": "/tmp/image" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "stop",
>          .args_type  = "",
>          .mhandler.cmd_new = qmp_marshal_input_stop,

Re: [Qemu-devel] [PATCH 4/4] add qmp screendump-async

[Alon Levy]

On Tue, Feb 28, 2012 at 05:05:32PM -0300, Luiz Capitulino wrote:
> On Fri, 24 Feb 2012 23:22:05 +0200
> Alon Levy <alevy@redhat.com> wrote:
> 
> > This is an across the board change since I wanted to keep the existing
> > (good imo) single graphic_console_init callback setter, instead of
> > introducing a new cb that isn't set by it but instead by a second
> > initialization function.
> > 
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> >  console.c            |   25 +++++++++++++++++++++++--
> >  console.h            |    5 +++++
> >  hw/blizzard.c        |    2 +-
> >  hw/cirrus_vga.c      |    4 ++--
> >  hw/exynos4210_fimd.c |    3 ++-
> >  hw/g364fb.c          |    2 +-
> >  hw/jazz_led.c        |    3 ++-
> >  hw/milkymist-vgafb.c |    2 +-
> >  hw/musicpal.c        |    2 +-
> >  hw/omap_dss.c        |    4 +++-
> >  hw/omap_lcdc.c       |    2 +-
> >  hw/pl110.c           |    2 +-
> >  hw/pxa2xx_lcd.c      |    2 +-
> >  hw/qxl.c             |    3 ++-
> >  hw/sm501.c           |    4 ++--
> >  hw/ssd0303.c         |    2 +-
> >  hw/ssd0323.c         |    2 +-
> >  hw/tc6393xb.c        |    1 +
> >  hw/tcx.c             |    4 ++--
> >  hw/vga-isa-mm.c      |    3 ++-
> >  hw/vga-isa.c         |    3 ++-
> >  hw/vga-pci.c         |    3 ++-
> >  hw/vga_int.h         |    1 +
> >  hw/vmware_vga.c      |    3 ++-
> >  hw/xenfb.c           |    2 ++
> >  monitor.c            |    5 +++++
> >  qapi-schema.json     |   20 ++++++++++++++++++++
> >  qmp-commands.hx      |   26 ++++++++++++++++++++++++++
> >  28 files changed, 115 insertions(+), 25 deletions(-)
> > 
> > diff --git a/console.c b/console.c
> > index 6750538..ced2aa7 100644
> > --- a/console.c
> > +++ b/console.c
> > @@ -124,6 +124,7 @@ struct TextConsole {
> >      vga_hw_update_ptr hw_update;
> >      vga_hw_invalidate_ptr hw_invalidate;
> >      vga_hw_screen_dump_ptr hw_screen_dump;
> > +    vga_hw_screen_dump_async_ptr hw_screen_dump_async;
> >      vga_hw_text_update_ptr hw_text_update;
> >      void *hw;
> >  
> > @@ -175,8 +176,9 @@ void vga_hw_invalidate(void)
> >          active_console->hw_invalidate(active_console->hw);
> >  }
> >  
> > -void vga_hw_screen_dump(const char *filename)
> > +static void vga_hw_screen_dump_helper(const char *filename, bool async)
> >  {
> > +    bool event_sent = false;
> >      TextConsole *previous_active_console;
> >      bool cswitch;
> >  
> > @@ -188,17 +190,33 @@ void vga_hw_screen_dump(const char *filename)
> >      if (cswitch) {
> >          console_select(0);
> >      }
> > -    if (consoles[0] && consoles[0]->hw_screen_dump) {
> > +    if (async && consoles[0] && consoles[0]->hw_screen_dump_async) {
> > +        consoles[0]->hw_screen_dump_async(consoles[0]->hw, filename, cswitch);
> > +        event_sent = true;
> > +    } else if (consoles[0] && consoles[0]->hw_screen_dump) {
> >          consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
> >      } else {
> >          error_report("screen dump not implemented");
> >      }
> > +    if (async && !event_sent) {
> > +        monitor_protocol_screen_dump_complete_event(filename);
> > +    }
> 
> This is wrong. The event should be emitted by hw_screen_dump_async() when it
> completes.

So you think it's better to add a intermediate callback there? ok, I'll try to
do that.

> 
> Also, this patch should be split into a least two patches. One patch does the
> console changes and the other one adds the qmp async command. It's also possible
> to make more patches by having individual devices' implementation of
> hw_screen_dump_async() in their own patches.

will do all the splits.

> 
> >  
> >      if (cswitch) {
> >          console_select(previous_active_console->index);
> >      }
> >  }
> >  
> > +void vga_hw_screen_dump(const char *filename)
> > +{
> > +    vga_hw_screen_dump_helper(filename, false);
> > +}
> > +
> > +void vga_hw_screen_dump_async(const char *filename)
> > +{
> > +    vga_hw_screen_dump_helper(filename, true);
> > +}
> > +
> >  void vga_hw_text_update(console_ch_t *chardata)
> >  {
> >      if (active_console && active_console->hw_text_update)
> > @@ -1403,6 +1421,8 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
> >                                     vga_hw_invalidate_ptr invalidate,
> >                                     vga_hw_screen_dump_ptr screen_dump,
> >                                     vga_hw_text_update_ptr text_update,
> > +                                   vga_hw_screen_dump_async_ptr
> > +                                                    screen_dump_async,
> >                                     void *opaque)
> >  {
> >      TextConsole *s;
> > @@ -1421,6 +1441,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
> >      s->hw_update = update;
> >      s->hw_invalidate = invalidate;
> >      s->hw_screen_dump = screen_dump;
> > +    s->hw_screen_dump_async = screen_dump_async;
> >      s->hw_text_update = text_update;
> >      s->hw = opaque;
> >  
> > diff --git a/console.h b/console.h
> > index c22803c..3cf28c0 100644
> > --- a/console.h
> > +++ b/console.h
> > @@ -341,17 +341,22 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
> >  typedef void (*vga_hw_update_ptr)(void *);
> >  typedef void (*vga_hw_invalidate_ptr)(void *);
> >  typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
> > +typedef void (*vga_hw_screen_dump_async_ptr)(void *, const char *filename,
> > +                                             bool cswitch);
> >  typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
> >  
> >  DisplayState *graphic_console_init(vga_hw_update_ptr update,
> >                                     vga_hw_invalidate_ptr invalidate,
> >                                     vga_hw_screen_dump_ptr screen_dump,
> >                                     vga_hw_text_update_ptr text_update,
> > +                                   vga_hw_screen_dump_async_ptr
> > +                                                    screen_dump_async,
> >                                     void *opaque);
> >  
> >  void vga_hw_update(void);
> >  void vga_hw_invalidate(void);
> >  void vga_hw_screen_dump(const char *filename);
> > +void vga_hw_screen_dump_async(const char *filename);
> >  void vga_hw_text_update(console_ch_t *chardata);
> >  void monitor_protocol_screen_dump_complete_event(const char *filename);
> >  
> > diff --git a/hw/blizzard.c b/hw/blizzard.c
> > index c7d844d..cc51045 100644
> > --- a/hw/blizzard.c
> > +++ b/hw/blizzard.c
> > @@ -963,7 +963,7 @@ void *s1d13745_init(qemu_irq gpio_int)
> >  
> >      s->state = graphic_console_init(blizzard_update_display,
> >                                   blizzard_invalidate_display,
> > -                                 blizzard_screen_dump, NULL, s);
> > +                                 blizzard_screen_dump, NULL, NULL, s);
> >  
> >      switch (ds_get_bits_per_pixel(s->state)) {
> >      case 0:
> > diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> > index 4edcb94..d2a9c27 100644
> > --- a/hw/cirrus_vga.c
> > +++ b/hw/cirrus_vga.c
> > @@ -2900,7 +2900,7 @@ static int vga_initfn(ISADevice *dev)
> >                         isa_address_space(dev));
> >      s->ds = graphic_console_init(s->update, s->invalidate,
> >                                   s->screen_dump, s->text_update,
> > -                                 s);
> > +                                 s->screen_dump_async, s);
> >      rom_add_vga(VGABIOS_CIRRUS_FILENAME);
> >      /* XXX ISA-LFB support */
> >      /* FIXME not qdev yet */
> > @@ -2941,7 +2941,7 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
> >       cirrus_init_common(s, device_id, 1, pci_address_space(dev));
> >       s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
> >                                        s->vga.screen_dump, s->vga.text_update,
> > -                                      &s->vga);
> > +                                      s->vga.screen_dump_async, &s->vga);
> >  
> >       /* setup PCI */
> >  
> > diff --git a/hw/exynos4210_fimd.c b/hw/exynos4210_fimd.c
> > index 3313f00..2053ed0 100644
> > --- a/hw/exynos4210_fimd.c
> > +++ b/hw/exynos4210_fimd.c
> > @@ -1898,7 +1898,8 @@ static int exynos4210_fimd_init(SysBusDevice *dev)
> >              "exynos4210.fimd", FIMD_REGS_SIZE);
> >      sysbus_init_mmio(dev, &s->iomem);
> >      s->console = graphic_console_init(exynos4210_fimd_update,
> > -                                  exynos4210_fimd_invalidate, NULL, NULL, s);
> > +                                  exynos4210_fimd_invalidate,
> > +                                  NULL, NULL, NULL, s);
> >  
> >      return 0;
> >  }
> > diff --git a/hw/g364fb.c b/hw/g364fb.c
> > index 3a0b68f..8ae40d8 100644
> > --- a/hw/g364fb.c
> > +++ b/hw/g364fb.c
> > @@ -517,7 +517,7 @@ static void g364fb_init(DeviceState *dev, G364State *s)
> >  
> >      s->ds = graphic_console_init(g364fb_update_display,
> >                                   g364fb_invalidate_display,
> > -                                 g364fb_screen_dump, NULL, s);
> > +                                 g364fb_screen_dump, NULL, NULL, s);
> >  
> >      memory_region_init_io(&s->mem_ctrl, &g364fb_ctrl_ops, s, "ctrl", 0x180000);
> >      memory_region_init_ram_ptr(&s->mem_vram, "vram",
> > diff --git a/hw/jazz_led.c b/hw/jazz_led.c
> > index 6486523..bb5daf7 100644
> > --- a/hw/jazz_led.c
> > +++ b/hw/jazz_led.c
> > @@ -251,7 +251,8 @@ static int jazz_led_init(SysBusDevice *dev)
> >      s->ds = graphic_console_init(jazz_led_update_display,
> >                                   jazz_led_invalidate_display,
> >                                   NULL,
> > -                                 jazz_led_text_update, s);
> > +                                 jazz_led_text_update,
> > +                                 NULL, s);
> >  
> >      return 0;
> >  }
> > diff --git a/hw/milkymist-vgafb.c b/hw/milkymist-vgafb.c
> > index 69afd72..ae018d1 100644
> > --- a/hw/milkymist-vgafb.c
> > +++ b/hw/milkymist-vgafb.c
> > @@ -276,7 +276,7 @@ static int milkymist_vgafb_init(SysBusDevice *dev)
> >  
> >      s->ds = graphic_console_init(vgafb_update_display,
> >                                   vgafb_invalidate_display,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >  
> >      return 0;
> >  }
> > diff --git a/hw/musicpal.c b/hw/musicpal.c
> > index 187a1ae..2fc391b 100644
> > --- a/hw/musicpal.c
> > +++ b/hw/musicpal.c
> > @@ -611,7 +611,7 @@ static int musicpal_lcd_init(SysBusDevice *dev)
> >      sysbus_init_mmio(dev, &s->iomem);
> >  
> >      s->ds = graphic_console_init(lcd_refresh, lcd_invalidate,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >      qemu_console_resize(s->ds, 128*3, 64*3);
> >  
> >      qdev_init_gpio_in(&dev->qdev, musicpal_lcd_gpio_brigthness_in, 3);
> > diff --git a/hw/omap_dss.c b/hw/omap_dss.c
> > index 86ed6ea..0e8243e 100644
> > --- a/hw/omap_dss.c
> > +++ b/hw/omap_dss.c
> > @@ -1072,7 +1072,9 @@ struct omap_dss_s *omap_dss_init(struct omap_target_agent_s *ta,
> >  
> >  #if 0
> >      s->state = graphic_console_init(omap_update_display,
> > -                                    omap_invalidate_display, omap_screen_dump, s);
> > +                                    omap_invalidate_display,
> > +                                    omap_screen_dump,
> > +                                    NULL, NULL, s);
> >  #endif
> >  
> >      return s;
> > diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
> > index f172093..d91e88d 100644
> > --- a/hw/omap_lcdc.c
> > +++ b/hw/omap_lcdc.c
> > @@ -452,7 +452,7 @@ struct omap_lcd_panel_s *omap_lcdc_init(MemoryRegion *sysmem,
> >  
> >      s->state = graphic_console_init(omap_update_display,
> >                                      omap_invalidate_display,
> > -                                    omap_screen_dump, NULL, s);
> > +                                    omap_screen_dump, NULL, NULL, s);
> >  
> >      return s;
> >  }
> > diff --git a/hw/pl110.c b/hw/pl110.c
> > index f94608c..32ee89c 100644
> > --- a/hw/pl110.c
> > +++ b/hw/pl110.c
> > @@ -451,7 +451,7 @@ static int pl110_init(SysBusDevice *dev)
> >      qdev_init_gpio_in(&s->busdev.qdev, pl110_mux_ctrl_set, 1);
> >      s->ds = graphic_console_init(pl110_update_display,
> >                                   pl110_invalidate_display,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >      return 0;
> >  }
> >  
> > diff --git a/hw/pxa2xx_lcd.c b/hw/pxa2xx_lcd.c
> > index fcbdfb3..5345fb4 100644
> > --- a/hw/pxa2xx_lcd.c
> > +++ b/hw/pxa2xx_lcd.c
> > @@ -1004,7 +1004,7 @@ PXA2xxLCDState *pxa2xx_lcdc_init(MemoryRegion *sysmem,
> >  
> >      s->ds = graphic_console_init(pxa2xx_update_display,
> >                                   pxa2xx_invalidate_display,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >  
> >      switch (ds_get_bits_per_pixel(s->ds)) {
> >      case 0:
> > diff --git a/hw/qxl.c b/hw/qxl.c
> > index d83d245..61a7edd 100644
> > --- a/hw/qxl.c
> > +++ b/hw/qxl.c
> > @@ -1727,7 +1727,8 @@ static int qxl_init_primary(PCIDevice *dev)
> >      portio_list_add(qxl_vga_port_list, pci_address_space_io(dev), 0x3b0);
> >  
> >      vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
> > -                                   qxl_hw_screen_dump, qxl_hw_text_update, qxl);
> > +                                   qxl_hw_screen_dump, qxl_hw_text_update,
> > +                                   NULL, qxl);
> >      qemu_spice_display_init_common(&qxl->ssd, vga->ds);
> >  
> >      qxl0 = qxl;
> > diff --git a/hw/sm501.c b/hw/sm501.c
> > index 786e076..8b150be 100644
> > --- a/hw/sm501.c
> > +++ b/hw/sm501.c
> > @@ -1442,6 +1442,6 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base,
> >      }
> >  
> >      /* create qemu graphic console */
> > -    s->ds = graphic_console_init(sm501_update_display, NULL,
> > -				 NULL, NULL, s);
> > +    s->ds = graphic_console_init(sm501_update_display,
> > +                                 NULL, NULL, NULL, NULL, s);
> >  }
> > diff --git a/hw/ssd0303.c b/hw/ssd0303.c
> > index 4e1ee6e..05e6686 100644
> > --- a/hw/ssd0303.c
> > +++ b/hw/ssd0303.c
> > @@ -289,7 +289,7 @@ static int ssd0303_init(I2CSlave *i2c)
> >  
> >      s->ds = graphic_console_init(ssd0303_update_display,
> >                                   ssd0303_invalidate_display,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >      qemu_console_resize(s->ds, 96 * MAGNIFY, 16 * MAGNIFY);
> >      return 0;
> >  }
> > diff --git a/hw/ssd0323.c b/hw/ssd0323.c
> > index b0b2e94..5e3491f 100644
> > --- a/hw/ssd0323.c
> > +++ b/hw/ssd0323.c
> > @@ -330,7 +330,7 @@ static int ssd0323_init(SSISlave *dev)
> >      s->row_end = 79;
> >      s->ds = graphic_console_init(ssd0323_update_display,
> >                                   ssd0323_invalidate_display,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >      qemu_console_resize(s->ds, 128 * MAGNIFY, 64 * MAGNIFY);
> >  
> >      qdev_init_gpio_in(&dev->qdev, ssd0323_cd, 1);
> > diff --git a/hw/tc6393xb.c b/hw/tc6393xb.c
> > index 420925c..853b180 100644
> > --- a/hw/tc6393xb.c
> > +++ b/hw/tc6393xb.c
> > @@ -581,6 +581,7 @@ TC6393xbState *tc6393xb_init(MemoryRegion *sysmem, uint32_t base, qemu_irq irq)
> >              NULL, /* invalidate */
> >              NULL, /* screen_dump */
> >              NULL, /* text_update */
> > +            NULL, /* screen_dump_async */
> >              s);
> >  
> >      return s;
> > diff --git a/hw/tcx.c b/hw/tcx.c
> > index ac7dcb4..9214dec 100644
> > --- a/hw/tcx.c
> > +++ b/hw/tcx.c
> > @@ -558,7 +558,7 @@ static int tcx_init1(SysBusDevice *dev)
> >  
> >          s->ds = graphic_console_init(tcx24_update_display,
> >                                       tcx24_invalidate_display,
> > -                                     tcx24_screen_dump, NULL, s);
> > +                                     tcx24_screen_dump, NULL, NULL, s);
> >      } else {
> >          /* THC 8 bit (dummy) */
> >          memory_region_init_io(&s->thc8, &dummy_ops, s, "tcx.thc8",
> > @@ -567,7 +567,7 @@ static int tcx_init1(SysBusDevice *dev)
> >  
> >          s->ds = graphic_console_init(tcx_update_display,
> >                                       tcx_invalidate_display,
> > -                                     tcx_screen_dump, NULL, s);
> > +                                     tcx_screen_dump, NULL, NULL, s);
> >      }
> >  
> >      qemu_console_resize(s->ds, s->width, s->height);
> > diff --git a/hw/vga-isa-mm.c b/hw/vga-isa-mm.c
> > index f8984c6..87b8fc6 100644
> > --- a/hw/vga-isa-mm.c
> > +++ b/hw/vga-isa-mm.c
> > @@ -132,7 +132,8 @@ int isa_vga_mm_init(target_phys_addr_t vram_base,
> >      vga_mm_init(s, vram_base, ctrl_base, it_shift, address_space);
> >  
> >      s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
> > -                                     s->vga.screen_dump, s->vga.text_update, s);
> > +                                     s->vga.screen_dump, s->vga.text_update,
> > +                                     s->vga.screen_dump_async, s);
> >  
> >      vga_init_vbe(&s->vga, address_space);
> >      return 0;
> > diff --git a/hw/vga-isa.c b/hw/vga-isa.c
> > index 4bcc4db..a362ed2 100644
> > --- a/hw/vga-isa.c
> > +++ b/hw/vga-isa.c
> > @@ -61,7 +61,8 @@ static int vga_initfn(ISADevice *dev)
> >                                          vga_io_memory, 1);
> >      memory_region_set_coalescing(vga_io_memory);
> >      s->ds = graphic_console_init(s->update, s->invalidate,
> > -                                 s->screen_dump, s->text_update, s);
> > +                                 s->screen_dump, s->text_update,
> > +                                 s->screen_dump_async, s);
> >  
> >      vga_init_vbe(s, isa_address_space(dev));
> >      /* ROM BIOS */
> > diff --git a/hw/vga-pci.c b/hw/vga-pci.c
> > index 465b643..8c8dd53 100644
> > --- a/hw/vga-pci.c
> > +++ b/hw/vga-pci.c
> > @@ -57,7 +57,8 @@ static int pci_vga_initfn(PCIDevice *dev)
> >       vga_init(s, pci_address_space(dev), pci_address_space_io(dev), true);
> >  
> >       s->ds = graphic_console_init(s->update, s->invalidate,
> > -                                  s->screen_dump, s->text_update, s);
> > +                                  s->screen_dump, s->text_update,
> > +                                  s->screen_dump_async, s);
> >  
> >       /* XXX: VGA_RAM_SIZE must be a power of two */
> >       pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->vram);
> > diff --git a/hw/vga_int.h b/hw/vga_int.h
> > index 7685b2b..4486cfb 100644
> > --- a/hw/vga_int.h
> > +++ b/hw/vga_int.h
> > @@ -161,6 +161,7 @@ typedef struct VGACommonState {
> >      vga_hw_invalidate_ptr invalidate;
> >      vga_hw_screen_dump_ptr screen_dump;
> >      vga_hw_text_update_ptr text_update;
> > +    vga_hw_screen_dump_async_ptr screen_dump_async;
> >      /* hardware mouse cursor support */
> >      uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32];
> >      void (*cursor_invalidate)(struct VGACommonState *s);
> > diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> > index 142d9f4..db96e61 100644
> > --- a/hw/vmware_vga.c
> > +++ b/hw/vmware_vga.c
> > @@ -1087,7 +1087,8 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size,
> >      s->vga.ds = graphic_console_init(vmsvga_update_display,
> >                                       vmsvga_invalidate_display,
> >                                       vmsvga_screen_dump,
> > -                                     vmsvga_text_update, s);
> > +                                     vmsvga_text_update,
> > +                                     NULL, s);
> >  
> >  
> >      s->fifo_size = SVGA_FIFO_SIZE;
> > diff --git a/hw/xenfb.c b/hw/xenfb.c
> > index 1bcf171..ad9f0d7 100644
> > --- a/hw/xenfb.c
> > +++ b/hw/xenfb.c
> > @@ -906,6 +906,7 @@ static int fb_initialise(struct XenDevice *xendev)
> >                                          xenfb_invalidate,
> >                                          NULL,
> >                                          NULL,
> > +                                        NULL,
> >                                          fb);
> >          fb->have_console = 1;
> >      }
> > @@ -1015,6 +1016,7 @@ wait_more:
> >                                      xenfb_invalidate,
> >                                      NULL,
> >                                      NULL,
> > +                                    NULL,
> >                                      fb);
> >      fb->have_console = 1;
> >  
> > diff --git a/monitor.c b/monitor.c
> > index 1a65c41..91a76a1 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -901,6 +901,11 @@ static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >      return 0;
> >  }
> >  
> > +void qmp_screendump_async(const char *filename, Error **errp)
> > +{
> > +    vga_hw_screen_dump_async(filename);
> > +}
> > +
> >  static void do_logfile(Monitor *mon, const QDict *qdict)
> >  {
> >      cpu_set_log_filename(qdict_get_str(qdict, "filename"));
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 72b17f1..a5873cb 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1595,3 +1595,23 @@
> >  { 'command': 'qom-list-types',
> >    'data': { '*implements': 'str', '*abstract': 'bool' },
> >    'returns': [ 'ObjectTypeInfo' ] }
> > +
> > +##
> > +## @screendump-async:
> > +#
> > +# This command will perform a screen dump of the first console to the givem
> > +# filename. The additional parameters are unused at this time.
> 
> Isn't it interesting to allow the mngt app to specify the console?

Will add.

> 
> > +#
> > +# @filename  name of output file to write screen dump to
> > +#
> > +# Since: 1.1
> > +#
> > +# Notes: This command is experimental and may change syntax in future releases.
> 
> Why is this needed?

Because I wanted an option to specify the device and the monitor in the
device (or a general index for a device, interpreted as a monitor for
video output devices like vga), but I wasn't sure if we have a string
representation of a device yet.

> 
> > +#
> > +# This command is the same as the qmp/hmp screendump command, except that on
> > +# successful completion of the scren dump the SCREEN_DUMP_COMPLETE event is
> > +# emitted.
> 
> The real difference is that it's asynchronous, while the other is synchronous.

Yes, I was trying to be more specific. I'll add that line.

> 
> > +#
> > +##
> > +{ 'command': 'screendump-async',
> > +  'data': { 'filename': 'str' } }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 705f704..ea0d051 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -170,6 +170,32 @@ Example:
> >  EQMP
> >  
> >      {
> > +        .name       = "screendump-async",
> > +        .args_type  = "filename:F",
> > +        .params     = "filename",
> > +        .help       = "save screen into PPM image 'filename'",
> > +        .user_print = monitor_user_noop,
> > +        .mhandler.cmd_new = qmp_marshal_input_screendump_async,
> > +    },
> > +
> > +SQMP
> > +screendump-async
> > +----------------
> > +
> > +Save screen into PPM image. Fires a SCREEN_DUMP_COMPLETE event on completion.
> > +
> > +Arguments:
> > +
> > +- "filename": file path (json-string)
> > +
> > +Example:
> > +
> > +-> { "execute": "screendump-async", "arguments": { "filename": "/tmp/image" } }
> > +<- { "return": {} }
> > +
> > +EQMP
> > +
> > +    {
> >          .name       = "stop",
> >          .args_type  = "",
> >          .mhandler.cmd_new = qmp_marshal_input_stop,
> 
> 

Re: [Qemu-devel] [PATCH 4/4] add qmp screendump-async

[Luiz Capitulino]

On Wed, 29 Feb 2012 10:39:29 +0200
Alon Levy <alevy@redhat.com> wrote:

> On Tue, Feb 28, 2012 at 05:05:32PM -0300, Luiz Capitulino wrote:
> > On Fri, 24 Feb 2012 23:22:05 +0200
> > Alon Levy <alevy@redhat.com> wrote:
> > 
> > > This is an across the board change since I wanted to keep the existing
> > > (good imo) single graphic_console_init callback setter, instead of
> > > introducing a new cb that isn't set by it but instead by a second
> > > initialization function.
> > > 
> > > Signed-off-by: Alon Levy <alevy@redhat.com>
> > > ---
> > >  console.c            |   25 +++++++++++++++++++++++--
> > >  console.h            |    5 +++++
> > >  hw/blizzard.c        |    2 +-
> > >  hw/cirrus_vga.c      |    4 ++--
> > >  hw/exynos4210_fimd.c |    3 ++-
> > >  hw/g364fb.c          |    2 +-
> > >  hw/jazz_led.c        |    3 ++-
> > >  hw/milkymist-vgafb.c |    2 +-
> > >  hw/musicpal.c        |    2 +-
> > >  hw/omap_dss.c        |    4 +++-
> > >  hw/omap_lcdc.c       |    2 +-
> > >  hw/pl110.c           |    2 +-
> > >  hw/pxa2xx_lcd.c      |    2 +-
> > >  hw/qxl.c             |    3 ++-
> > >  hw/sm501.c           |    4 ++--
> > >  hw/ssd0303.c         |    2 +-
> > >  hw/ssd0323.c         |    2 +-
> > >  hw/tc6393xb.c        |    1 +
> > >  hw/tcx.c             |    4 ++--
> > >  hw/vga-isa-mm.c      |    3 ++-
> > >  hw/vga-isa.c         |    3 ++-
> > >  hw/vga-pci.c         |    3 ++-
> > >  hw/vga_int.h         |    1 +
> > >  hw/vmware_vga.c      |    3 ++-
> > >  hw/xenfb.c           |    2 ++
> > >  monitor.c            |    5 +++++
> > >  qapi-schema.json     |   20 ++++++++++++++++++++
> > >  qmp-commands.hx      |   26 ++++++++++++++++++++++++++
> > >  28 files changed, 115 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/console.c b/console.c
> > > index 6750538..ced2aa7 100644
> > > --- a/console.c
> > > +++ b/console.c
> > > @@ -124,6 +124,7 @@ struct TextConsole {
> > >      vga_hw_update_ptr hw_update;
> > >      vga_hw_invalidate_ptr hw_invalidate;
> > >      vga_hw_screen_dump_ptr hw_screen_dump;
> > > +    vga_hw_screen_dump_async_ptr hw_screen_dump_async;
> > >      vga_hw_text_update_ptr hw_text_update;
> > >      void *hw;
> > >  
> > > @@ -175,8 +176,9 @@ void vga_hw_invalidate(void)
> > >          active_console->hw_invalidate(active_console->hw);
> > >  }
> > >  
> > > -void vga_hw_screen_dump(const char *filename)
> > > +static void vga_hw_screen_dump_helper(const char *filename, bool async)
> > >  {
> > > +    bool event_sent = false;
> > >      TextConsole *previous_active_console;
> > >      bool cswitch;
> > >  
> > > @@ -188,17 +190,33 @@ void vga_hw_screen_dump(const char *filename)
> > >      if (cswitch) {
> > >          console_select(0);
> > >      }
> > > -    if (consoles[0] && consoles[0]->hw_screen_dump) {
> > > +    if (async && consoles[0] && consoles[0]->hw_screen_dump_async) {
> > > +        consoles[0]->hw_screen_dump_async(consoles[0]->hw, filename, cswitch);
> > > +        event_sent = true;
> > > +    } else if (consoles[0] && consoles[0]->hw_screen_dump) {
> > >          consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
> > >      } else {
> > >          error_report("screen dump not implemented");
> > >      }
> > > +    if (async && !event_sent) {
> > > +        monitor_protocol_screen_dump_complete_event(filename);
> > > +    }
> > 
> > This is wrong. The event should be emitted by hw_screen_dump_async() when it
> > completes.
> 
> So you think it's better to add a intermediate callback there? ok, I'll try to
> do that.

A completion callback, yes. But I'd wait for a conclusion in the other
thread regarding if we should add the asynchronous command or not.

Re: [Qemu-devel] [PATCH 4/4] add qmp screendump-async

[Alon Levy]

On Tue, Feb 28, 2012 at 05:05:32PM -0300, Luiz Capitulino wrote:
> On Fri, 24 Feb 2012 23:22:05 +0200
> Alon Levy <alevy@redhat.com> wrote:
> 
> > This is an across the board change since I wanted to keep the existing
> > (good imo) single graphic_console_init callback setter, instead of
> > introducing a new cb that isn't set by it but instead by a second
> > initialization function.
> > 
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> >  console.c            |   25 +++++++++++++++++++++++--
> >  console.h            |    5 +++++
> >  hw/blizzard.c        |    2 +-
> >  hw/cirrus_vga.c      |    4 ++--
> >  hw/exynos4210_fimd.c |    3 ++-
> >  hw/g364fb.c          |    2 +-
> >  hw/jazz_led.c        |    3 ++-
> >  hw/milkymist-vgafb.c |    2 +-
> >  hw/musicpal.c        |    2 +-
> >  hw/omap_dss.c        |    4 +++-
> >  hw/omap_lcdc.c       |    2 +-
> >  hw/pl110.c           |    2 +-
> >  hw/pxa2xx_lcd.c      |    2 +-
> >  hw/qxl.c             |    3 ++-
> >  hw/sm501.c           |    4 ++--
> >  hw/ssd0303.c         |    2 +-
> >  hw/ssd0323.c         |    2 +-
> >  hw/tc6393xb.c        |    1 +
> >  hw/tcx.c             |    4 ++--
> >  hw/vga-isa-mm.c      |    3 ++-
> >  hw/vga-isa.c         |    3 ++-
> >  hw/vga-pci.c         |    3 ++-
> >  hw/vga_int.h         |    1 +
> >  hw/vmware_vga.c      |    3 ++-
> >  hw/xenfb.c           |    2 ++
> >  monitor.c            |    5 +++++
> >  qapi-schema.json     |   20 ++++++++++++++++++++
> >  qmp-commands.hx      |   26 ++++++++++++++++++++++++++
> >  28 files changed, 115 insertions(+), 25 deletions(-)
> > 
> > diff --git a/console.c b/console.c
> > index 6750538..ced2aa7 100644
> > --- a/console.c
> > +++ b/console.c
> > @@ -124,6 +124,7 @@ struct TextConsole {
> >      vga_hw_update_ptr hw_update;
> >      vga_hw_invalidate_ptr hw_invalidate;
> >      vga_hw_screen_dump_ptr hw_screen_dump;
> > +    vga_hw_screen_dump_async_ptr hw_screen_dump_async;
> >      vga_hw_text_update_ptr hw_text_update;
> >      void *hw;
> >  
> > @@ -175,8 +176,9 @@ void vga_hw_invalidate(void)
> >          active_console->hw_invalidate(active_console->hw);
> >  }
> >  
> > -void vga_hw_screen_dump(const char *filename)
> > +static void vga_hw_screen_dump_helper(const char *filename, bool async)
> >  {
> > +    bool event_sent = false;
> >      TextConsole *previous_active_console;
> >      bool cswitch;
> >  
> > @@ -188,17 +190,33 @@ void vga_hw_screen_dump(const char *filename)
> >      if (cswitch) {
> >          console_select(0);
> >      }
> > -    if (consoles[0] && consoles[0]->hw_screen_dump) {
> > +    if (async && consoles[0] && consoles[0]->hw_screen_dump_async) {
> > +        consoles[0]->hw_screen_dump_async(consoles[0]->hw, filename, cswitch);
> > +        event_sent = true;
> > +    } else if (consoles[0] && consoles[0]->hw_screen_dump) {
> >          consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
> >      } else {
> >          error_report("screen dump not implemented");
> >      }
> > +    if (async && !event_sent) {
> > +        monitor_protocol_screen_dump_complete_event(filename);
> > +    }
> 
> This is wrong. The event should be emitted by hw_screen_dump_async() when it
> completes.

I misunderstood before. The variable names may be misleading, but this
is the logic:
 if async requested:
  if console has hw_screen_dump_async callback use it
  else use hw_screen_dump, and send the event ourselves.

So actually it's exactly what you wanted, i.e. that hw_screen_dump_async
will be responsible for calling the monitor callback, but with the added
benefit that any implementation of hw_screen_dump is automatically an
implmementation of the new async interface. The point is that the only
backends that should change implementation are those that actually need
it, i.e. at this time only qxl.

> 
> Also, this patch should be split into a least two patches. One patch does the
> console changes and the other one adds the qmp async command. It's also possible
> to make more patches by having individual devices' implementation of
> hw_screen_dump_async() in their own patches.
> 
> >  
> >      if (cswitch) {
> >          console_select(previous_active_console->index);
> >      }
> >  }
> >  
> > +void vga_hw_screen_dump(const char *filename)
> > +{
> > +    vga_hw_screen_dump_helper(filename, false);
> > +}
> > +
> > +void vga_hw_screen_dump_async(const char *filename)
> > +{
> > +    vga_hw_screen_dump_helper(filename, true);
> > +}
> > +
> >  void vga_hw_text_update(console_ch_t *chardata)
> >  {
> >      if (active_console && active_console->hw_text_update)
> > @@ -1403,6 +1421,8 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
> >                                     vga_hw_invalidate_ptr invalidate,
> >                                     vga_hw_screen_dump_ptr screen_dump,
> >                                     vga_hw_text_update_ptr text_update,
> > +                                   vga_hw_screen_dump_async_ptr
> > +                                                    screen_dump_async,
> >                                     void *opaque)
> >  {
> >      TextConsole *s;
> > @@ -1421,6 +1441,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
> >      s->hw_update = update;
> >      s->hw_invalidate = invalidate;
> >      s->hw_screen_dump = screen_dump;
> > +    s->hw_screen_dump_async = screen_dump_async;
> >      s->hw_text_update = text_update;
> >      s->hw = opaque;
> >  
> > diff --git a/console.h b/console.h
> > index c22803c..3cf28c0 100644
> > --- a/console.h
> > +++ b/console.h
> > @@ -341,17 +341,22 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
> >  typedef void (*vga_hw_update_ptr)(void *);
> >  typedef void (*vga_hw_invalidate_ptr)(void *);
> >  typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
> > +typedef void (*vga_hw_screen_dump_async_ptr)(void *, const char *filename,
> > +                                             bool cswitch);
> >  typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
> >  
> >  DisplayState *graphic_console_init(vga_hw_update_ptr update,
> >                                     vga_hw_invalidate_ptr invalidate,
> >                                     vga_hw_screen_dump_ptr screen_dump,
> >                                     vga_hw_text_update_ptr text_update,
> > +                                   vga_hw_screen_dump_async_ptr
> > +                                                    screen_dump_async,
> >                                     void *opaque);
> >  
> >  void vga_hw_update(void);
> >  void vga_hw_invalidate(void);
> >  void vga_hw_screen_dump(const char *filename);
> > +void vga_hw_screen_dump_async(const char *filename);
> >  void vga_hw_text_update(console_ch_t *chardata);
> >  void monitor_protocol_screen_dump_complete_event(const char *filename);
> >  
> > diff --git a/hw/blizzard.c b/hw/blizzard.c
> > index c7d844d..cc51045 100644
> > --- a/hw/blizzard.c
> > +++ b/hw/blizzard.c
> > @@ -963,7 +963,7 @@ void *s1d13745_init(qemu_irq gpio_int)
> >  
> >      s->state = graphic_console_init(blizzard_update_display,
> >                                   blizzard_invalidate_display,
> > -                                 blizzard_screen_dump, NULL, s);
> > +                                 blizzard_screen_dump, NULL, NULL, s);
> >  
> >      switch (ds_get_bits_per_pixel(s->state)) {
> >      case 0:
> > diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> > index 4edcb94..d2a9c27 100644
> > --- a/hw/cirrus_vga.c
> > +++ b/hw/cirrus_vga.c
> > @@ -2900,7 +2900,7 @@ static int vga_initfn(ISADevice *dev)
> >                         isa_address_space(dev));
> >      s->ds = graphic_console_init(s->update, s->invalidate,
> >                                   s->screen_dump, s->text_update,
> > -                                 s);
> > +                                 s->screen_dump_async, s);
> >      rom_add_vga(VGABIOS_CIRRUS_FILENAME);
> >      /* XXX ISA-LFB support */
> >      /* FIXME not qdev yet */
> > @@ -2941,7 +2941,7 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
> >       cirrus_init_common(s, device_id, 1, pci_address_space(dev));
> >       s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
> >                                        s->vga.screen_dump, s->vga.text_update,
> > -                                      &s->vga);
> > +                                      s->vga.screen_dump_async, &s->vga);
> >  
> >       /* setup PCI */
> >  
> > diff --git a/hw/exynos4210_fimd.c b/hw/exynos4210_fimd.c
> > index 3313f00..2053ed0 100644
> > --- a/hw/exynos4210_fimd.c
> > +++ b/hw/exynos4210_fimd.c
> > @@ -1898,7 +1898,8 @@ static int exynos4210_fimd_init(SysBusDevice *dev)
> >              "exynos4210.fimd", FIMD_REGS_SIZE);
> >      sysbus_init_mmio(dev, &s->iomem);
> >      s->console = graphic_console_init(exynos4210_fimd_update,
> > -                                  exynos4210_fimd_invalidate, NULL, NULL, s);
> > +                                  exynos4210_fimd_invalidate,
> > +                                  NULL, NULL, NULL, s);
> >  
> >      return 0;
> >  }
> > diff --git a/hw/g364fb.c b/hw/g364fb.c
> > index 3a0b68f..8ae40d8 100644
> > --- a/hw/g364fb.c
> > +++ b/hw/g364fb.c
> > @@ -517,7 +517,7 @@ static void g364fb_init(DeviceState *dev, G364State *s)
> >  
> >      s->ds = graphic_console_init(g364fb_update_display,
> >                                   g364fb_invalidate_display,
> > -                                 g364fb_screen_dump, NULL, s);
> > +                                 g364fb_screen_dump, NULL, NULL, s);
> >  
> >      memory_region_init_io(&s->mem_ctrl, &g364fb_ctrl_ops, s, "ctrl", 0x180000);
> >      memory_region_init_ram_ptr(&s->mem_vram, "vram",
> > diff --git a/hw/jazz_led.c b/hw/jazz_led.c
> > index 6486523..bb5daf7 100644
> > --- a/hw/jazz_led.c
> > +++ b/hw/jazz_led.c
> > @@ -251,7 +251,8 @@ static int jazz_led_init(SysBusDevice *dev)
> >      s->ds = graphic_console_init(jazz_led_update_display,
> >                                   jazz_led_invalidate_display,
> >                                   NULL,
> > -                                 jazz_led_text_update, s);
> > +                                 jazz_led_text_update,
> > +                                 NULL, s);
> >  
> >      return 0;
> >  }
> > diff --git a/hw/milkymist-vgafb.c b/hw/milkymist-vgafb.c
> > index 69afd72..ae018d1 100644
> > --- a/hw/milkymist-vgafb.c
> > +++ b/hw/milkymist-vgafb.c
> > @@ -276,7 +276,7 @@ static int milkymist_vgafb_init(SysBusDevice *dev)
> >  
> >      s->ds = graphic_console_init(vgafb_update_display,
> >                                   vgafb_invalidate_display,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >  
> >      return 0;
> >  }
> > diff --git a/hw/musicpal.c b/hw/musicpal.c
> > index 187a1ae..2fc391b 100644
> > --- a/hw/musicpal.c
> > +++ b/hw/musicpal.c
> > @@ -611,7 +611,7 @@ static int musicpal_lcd_init(SysBusDevice *dev)
> >      sysbus_init_mmio(dev, &s->iomem);
> >  
> >      s->ds = graphic_console_init(lcd_refresh, lcd_invalidate,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >      qemu_console_resize(s->ds, 128*3, 64*3);
> >  
> >      qdev_init_gpio_in(&dev->qdev, musicpal_lcd_gpio_brigthness_in, 3);
> > diff --git a/hw/omap_dss.c b/hw/omap_dss.c
> > index 86ed6ea..0e8243e 100644
> > --- a/hw/omap_dss.c
> > +++ b/hw/omap_dss.c
> > @@ -1072,7 +1072,9 @@ struct omap_dss_s *omap_dss_init(struct omap_target_agent_s *ta,
> >  
> >  #if 0
> >      s->state = graphic_console_init(omap_update_display,
> > -                                    omap_invalidate_display, omap_screen_dump, s);
> > +                                    omap_invalidate_display,
> > +                                    omap_screen_dump,
> > +                                    NULL, NULL, s);
> >  #endif
> >  
> >      return s;
> > diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
> > index f172093..d91e88d 100644
> > --- a/hw/omap_lcdc.c
> > +++ b/hw/omap_lcdc.c
> > @@ -452,7 +452,7 @@ struct omap_lcd_panel_s *omap_lcdc_init(MemoryRegion *sysmem,
> >  
> >      s->state = graphic_console_init(omap_update_display,
> >                                      omap_invalidate_display,
> > -                                    omap_screen_dump, NULL, s);
> > +                                    omap_screen_dump, NULL, NULL, s);
> >  
> >      return s;
> >  }
> > diff --git a/hw/pl110.c b/hw/pl110.c
> > index f94608c..32ee89c 100644
> > --- a/hw/pl110.c
> > +++ b/hw/pl110.c
> > @@ -451,7 +451,7 @@ static int pl110_init(SysBusDevice *dev)
> >      qdev_init_gpio_in(&s->busdev.qdev, pl110_mux_ctrl_set, 1);
> >      s->ds = graphic_console_init(pl110_update_display,
> >                                   pl110_invalidate_display,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >      return 0;
> >  }
> >  
> > diff --git a/hw/pxa2xx_lcd.c b/hw/pxa2xx_lcd.c
> > index fcbdfb3..5345fb4 100644
> > --- a/hw/pxa2xx_lcd.c
> > +++ b/hw/pxa2xx_lcd.c
> > @@ -1004,7 +1004,7 @@ PXA2xxLCDState *pxa2xx_lcdc_init(MemoryRegion *sysmem,
> >  
> >      s->ds = graphic_console_init(pxa2xx_update_display,
> >                                   pxa2xx_invalidate_display,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >  
> >      switch (ds_get_bits_per_pixel(s->ds)) {
> >      case 0:
> > diff --git a/hw/qxl.c b/hw/qxl.c
> > index d83d245..61a7edd 100644
> > --- a/hw/qxl.c
> > +++ b/hw/qxl.c
> > @@ -1727,7 +1727,8 @@ static int qxl_init_primary(PCIDevice *dev)
> >      portio_list_add(qxl_vga_port_list, pci_address_space_io(dev), 0x3b0);
> >  
> >      vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
> > -                                   qxl_hw_screen_dump, qxl_hw_text_update, qxl);
> > +                                   qxl_hw_screen_dump, qxl_hw_text_update,
> > +                                   NULL, qxl);
> >      qemu_spice_display_init_common(&qxl->ssd, vga->ds);
> >  
> >      qxl0 = qxl;
> > diff --git a/hw/sm501.c b/hw/sm501.c
> > index 786e076..8b150be 100644
> > --- a/hw/sm501.c
> > +++ b/hw/sm501.c
> > @@ -1442,6 +1442,6 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base,
> >      }
> >  
> >      /* create qemu graphic console */
> > -    s->ds = graphic_console_init(sm501_update_display, NULL,
> > -				 NULL, NULL, s);
> > +    s->ds = graphic_console_init(sm501_update_display,
> > +                                 NULL, NULL, NULL, NULL, s);
> >  }
> > diff --git a/hw/ssd0303.c b/hw/ssd0303.c
> > index 4e1ee6e..05e6686 100644
> > --- a/hw/ssd0303.c
> > +++ b/hw/ssd0303.c
> > @@ -289,7 +289,7 @@ static int ssd0303_init(I2CSlave *i2c)
> >  
> >      s->ds = graphic_console_init(ssd0303_update_display,
> >                                   ssd0303_invalidate_display,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >      qemu_console_resize(s->ds, 96 * MAGNIFY, 16 * MAGNIFY);
> >      return 0;
> >  }
> > diff --git a/hw/ssd0323.c b/hw/ssd0323.c
> > index b0b2e94..5e3491f 100644
> > --- a/hw/ssd0323.c
> > +++ b/hw/ssd0323.c
> > @@ -330,7 +330,7 @@ static int ssd0323_init(SSISlave *dev)
> >      s->row_end = 79;
> >      s->ds = graphic_console_init(ssd0323_update_display,
> >                                   ssd0323_invalidate_display,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >      qemu_console_resize(s->ds, 128 * MAGNIFY, 64 * MAGNIFY);
> >  
> >      qdev_init_gpio_in(&dev->qdev, ssd0323_cd, 1);
> > diff --git a/hw/tc6393xb.c b/hw/tc6393xb.c
> > index 420925c..853b180 100644
> > --- a/hw/tc6393xb.c
> > +++ b/hw/tc6393xb.c
> > @@ -581,6 +581,7 @@ TC6393xbState *tc6393xb_init(MemoryRegion *sysmem, uint32_t base, qemu_irq irq)
> >              NULL, /* invalidate */
> >              NULL, /* screen_dump */
> >              NULL, /* text_update */
> > +            NULL, /* screen_dump_async */
> >              s);
> >  
> >      return s;
> > diff --git a/hw/tcx.c b/hw/tcx.c
> > index ac7dcb4..9214dec 100644
> > --- a/hw/tcx.c
> > +++ b/hw/tcx.c
> > @@ -558,7 +558,7 @@ static int tcx_init1(SysBusDevice *dev)
> >  
> >          s->ds = graphic_console_init(tcx24_update_display,
> >                                       tcx24_invalidate_display,
> > -                                     tcx24_screen_dump, NULL, s);
> > +                                     tcx24_screen_dump, NULL, NULL, s);
> >      } else {
> >          /* THC 8 bit (dummy) */
> >          memory_region_init_io(&s->thc8, &dummy_ops, s, "tcx.thc8",
> > @@ -567,7 +567,7 @@ static int tcx_init1(SysBusDevice *dev)
> >  
> >          s->ds = graphic_console_init(tcx_update_display,
> >                                       tcx_invalidate_display,
> > -                                     tcx_screen_dump, NULL, s);
> > +                                     tcx_screen_dump, NULL, NULL, s);
> >      }
> >  
> >      qemu_console_resize(s->ds, s->width, s->height);
> > diff --git a/hw/vga-isa-mm.c b/hw/vga-isa-mm.c
> > index f8984c6..87b8fc6 100644
> > --- a/hw/vga-isa-mm.c
> > +++ b/hw/vga-isa-mm.c
> > @@ -132,7 +132,8 @@ int isa_vga_mm_init(target_phys_addr_t vram_base,
> >      vga_mm_init(s, vram_base, ctrl_base, it_shift, address_space);
> >  
> >      s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
> > -                                     s->vga.screen_dump, s->vga.text_update, s);
> > +                                     s->vga.screen_dump, s->vga.text_update,
> > +                                     s->vga.screen_dump_async, s);
> >  
> >      vga_init_vbe(&s->vga, address_space);
> >      return 0;
> > diff --git a/hw/vga-isa.c b/hw/vga-isa.c
> > index 4bcc4db..a362ed2 100644
> > --- a/hw/vga-isa.c
> > +++ b/hw/vga-isa.c
> > @@ -61,7 +61,8 @@ static int vga_initfn(ISADevice *dev)
> >                                          vga_io_memory, 1);
> >      memory_region_set_coalescing(vga_io_memory);
> >      s->ds = graphic_console_init(s->update, s->invalidate,
> > -                                 s->screen_dump, s->text_update, s);
> > +                                 s->screen_dump, s->text_update,
> > +                                 s->screen_dump_async, s);
> >  
> >      vga_init_vbe(s, isa_address_space(dev));
> >      /* ROM BIOS */
> > diff --git a/hw/vga-pci.c b/hw/vga-pci.c
> > index 465b643..8c8dd53 100644
> > --- a/hw/vga-pci.c
> > +++ b/hw/vga-pci.c
> > @@ -57,7 +57,8 @@ static int pci_vga_initfn(PCIDevice *dev)
> >       vga_init(s, pci_address_space(dev), pci_address_space_io(dev), true);
> >  
> >       s->ds = graphic_console_init(s->update, s->invalidate,
> > -                                  s->screen_dump, s->text_update, s);
> > +                                  s->screen_dump, s->text_update,
> > +                                  s->screen_dump_async, s);
> >  
> >       /* XXX: VGA_RAM_SIZE must be a power of two */
> >       pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->vram);
> > diff --git a/hw/vga_int.h b/hw/vga_int.h
> > index 7685b2b..4486cfb 100644
> > --- a/hw/vga_int.h
> > +++ b/hw/vga_int.h
> > @@ -161,6 +161,7 @@ typedef struct VGACommonState {
> >      vga_hw_invalidate_ptr invalidate;
> >      vga_hw_screen_dump_ptr screen_dump;
> >      vga_hw_text_update_ptr text_update;
> > +    vga_hw_screen_dump_async_ptr screen_dump_async;
> >      /* hardware mouse cursor support */
> >      uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32];
> >      void (*cursor_invalidate)(struct VGACommonState *s);
> > diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> > index 142d9f4..db96e61 100644
> > --- a/hw/vmware_vga.c
> > +++ b/hw/vmware_vga.c
> > @@ -1087,7 +1087,8 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size,
> >      s->vga.ds = graphic_console_init(vmsvga_update_display,
> >                                       vmsvga_invalidate_display,
> >                                       vmsvga_screen_dump,
> > -                                     vmsvga_text_update, s);
> > +                                     vmsvga_text_update,
> > +                                     NULL, s);
> >  
> >  
> >      s->fifo_size = SVGA_FIFO_SIZE;
> > diff --git a/hw/xenfb.c b/hw/xenfb.c
> > index 1bcf171..ad9f0d7 100644
> > --- a/hw/xenfb.c
> > +++ b/hw/xenfb.c
> > @@ -906,6 +906,7 @@ static int fb_initialise(struct XenDevice *xendev)
> >                                          xenfb_invalidate,
> >                                          NULL,
> >                                          NULL,
> > +                                        NULL,
> >                                          fb);
> >          fb->have_console = 1;
> >      }
> > @@ -1015,6 +1016,7 @@ wait_more:
> >                                      xenfb_invalidate,
> >                                      NULL,
> >                                      NULL,
> > +                                    NULL,
> >                                      fb);
> >      fb->have_console = 1;
> >  
> > diff --git a/monitor.c b/monitor.c
> > index 1a65c41..91a76a1 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -901,6 +901,11 @@ static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >      return 0;
> >  }
> >  
> > +void qmp_screendump_async(const char *filename, Error **errp)
> > +{
> > +    vga_hw_screen_dump_async(filename);
> > +}
> > +
> >  static void do_logfile(Monitor *mon, const QDict *qdict)
> >  {
> >      cpu_set_log_filename(qdict_get_str(qdict, "filename"));
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 72b17f1..a5873cb 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1595,3 +1595,23 @@
> >  { 'command': 'qom-list-types',
> >    'data': { '*implements': 'str', '*abstract': 'bool' },
> >    'returns': [ 'ObjectTypeInfo' ] }
> > +
> > +##
> > +## @screendump-async:
> > +#
> > +# This command will perform a screen dump of the first console to the givem
> > +# filename. The additional parameters are unused at this time.
> 
> Isn't it interesting to allow the mngt app to specify the console?
> 
> > +#
> > +# @filename  name of output file to write screen dump to
> > +#
> > +# Since: 1.1
> > +#
> > +# Notes: This command is experimental and may change syntax in future releases.
> 
> Why is this needed?
> 
> > +#
> > +# This command is the same as the qmp/hmp screendump command, except that on
> > +# successful completion of the scren dump the SCREEN_DUMP_COMPLETE event is
> > +# emitted.
> 
> The real difference is that it's asynchronous, while the other is synchronous.
> 
> > +#
> > +##
> > +{ 'command': 'screendump-async',
> > +  'data': { 'filename': 'str' } }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 705f704..ea0d051 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -170,6 +170,32 @@ Example:
> >  EQMP
> >  
> >      {
> > +        .name       = "screendump-async",
> > +        .args_type  = "filename:F",
> > +        .params     = "filename",
> > +        .help       = "save screen into PPM image 'filename'",
> > +        .user_print = monitor_user_noop,
> > +        .mhandler.cmd_new = qmp_marshal_input_screendump_async,
> > +    },
> > +
> > +SQMP
> > +screendump-async
> > +----------------
> > +
> > +Save screen into PPM image. Fires a SCREEN_DUMP_COMPLETE event on completion.
> > +
> > +Arguments:
> > +
> > +- "filename": file path (json-string)
> > +
> > +Example:
> > +
> > +-> { "execute": "screendump-async", "arguments": { "filename": "/tmp/image" } }
> > +<- { "return": {} }
> > +
> > +EQMP
> > +
> > +    {
> >          .name       = "stop",
> >          .args_type  = "",
> >          .mhandler.cmd_new = qmp_marshal_input_stop,
> 
> 

Re: [Qemu-devel] [PATCH 4/4] add qmp screendump-async

[Alon Levy]

On Tue, Feb 28, 2012 at 05:05:32PM -0300, Luiz Capitulino wrote:
> On Fri, 24 Feb 2012 23:22:05 +0200
> Alon Levy <alevy@redhat.com> wrote:
> 
> > This is an across the board change since I wanted to keep the existing
> > (good imo) single graphic_console_init callback setter, instead of
> > introducing a new cb that isn't set by it but instead by a second
> > initialization function.
> > 
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> >  console.c            |   25 +++++++++++++++++++++++--
> >  console.h            |    5 +++++
> >  hw/blizzard.c        |    2 +-
> >  hw/cirrus_vga.c      |    4 ++--
> >  hw/exynos4210_fimd.c |    3 ++-
> >  hw/g364fb.c          |    2 +-
> >  hw/jazz_led.c        |    3 ++-
> >  hw/milkymist-vgafb.c |    2 +-
> >  hw/musicpal.c        |    2 +-
> >  hw/omap_dss.c        |    4 +++-
> >  hw/omap_lcdc.c       |    2 +-
> >  hw/pl110.c           |    2 +-
> >  hw/pxa2xx_lcd.c      |    2 +-
> >  hw/qxl.c             |    3 ++-
> >  hw/sm501.c           |    4 ++--
> >  hw/ssd0303.c         |    2 +-
> >  hw/ssd0323.c         |    2 +-
> >  hw/tc6393xb.c        |    1 +
> >  hw/tcx.c             |    4 ++--
> >  hw/vga-isa-mm.c      |    3 ++-
> >  hw/vga-isa.c         |    3 ++-
> >  hw/vga-pci.c         |    3 ++-
> >  hw/vga_int.h         |    1 +
> >  hw/vmware_vga.c      |    3 ++-
> >  hw/xenfb.c           |    2 ++
> >  monitor.c            |    5 +++++
> >  qapi-schema.json     |   20 ++++++++++++++++++++
> >  qmp-commands.hx      |   26 ++++++++++++++++++++++++++
> >  28 files changed, 115 insertions(+), 25 deletions(-)
> > 
> > diff --git a/console.c b/console.c
> > index 6750538..ced2aa7 100644
> > --- a/console.c
> > +++ b/console.c
> > @@ -124,6 +124,7 @@ struct TextConsole {
> >      vga_hw_update_ptr hw_update;
> >      vga_hw_invalidate_ptr hw_invalidate;
> >      vga_hw_screen_dump_ptr hw_screen_dump;
> > +    vga_hw_screen_dump_async_ptr hw_screen_dump_async;
> >      vga_hw_text_update_ptr hw_text_update;
> >      void *hw;
> >  
> > @@ -175,8 +176,9 @@ void vga_hw_invalidate(void)
> >          active_console->hw_invalidate(active_console->hw);
> >  }
> >  
> > -void vga_hw_screen_dump(const char *filename)
> > +static void vga_hw_screen_dump_helper(const char *filename, bool async)
> >  {
> > +    bool event_sent = false;
> >      TextConsole *previous_active_console;
> >      bool cswitch;
> >  
> > @@ -188,17 +190,33 @@ void vga_hw_screen_dump(const char *filename)
> >      if (cswitch) {
> >          console_select(0);
> >      }
> > -    if (consoles[0] && consoles[0]->hw_screen_dump) {
> > +    if (async && consoles[0] && consoles[0]->hw_screen_dump_async) {
> > +        consoles[0]->hw_screen_dump_async(consoles[0]->hw, filename, cswitch);
> > +        event_sent = true;
> > +    } else if (consoles[0] && consoles[0]->hw_screen_dump) {
> >          consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
> >      } else {
> >          error_report("screen dump not implemented");
> >      }
> > +    if (async && !event_sent) {
> > +        monitor_protocol_screen_dump_complete_event(filename);
> > +    }
> 
> This is wrong. The event should be emitted by hw_screen_dump_async() when it
> completes.
> 
> Also, this patch should be split into a least two patches. One patch does the
> console changes and the other one adds the qmp async command. It's also possible
> to make more patches by having individual devices' implementation of
> hw_screen_dump_async() in their own patches.

There are no implementations of hw_screen_dump_async in this patch, but
I'll do the first split you suggested.

> 
> >  
> >      if (cswitch) {
> >          console_select(previous_active_console->index);
> >      }
> >  }
> >  
> > +void vga_hw_screen_dump(const char *filename)
> > +{
> > +    vga_hw_screen_dump_helper(filename, false);
> > +}
> > +
> > +void vga_hw_screen_dump_async(const char *filename)
> > +{
> > +    vga_hw_screen_dump_helper(filename, true);
> > +}
> > +
> >  void vga_hw_text_update(console_ch_t *chardata)
> >  {
> >      if (active_console && active_console->hw_text_update)
> > @@ -1403,6 +1421,8 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
> >                                     vga_hw_invalidate_ptr invalidate,
> >                                     vga_hw_screen_dump_ptr screen_dump,
> >                                     vga_hw_text_update_ptr text_update,
> > +                                   vga_hw_screen_dump_async_ptr
> > +                                                    screen_dump_async,
> >                                     void *opaque)
> >  {
> >      TextConsole *s;
> > @@ -1421,6 +1441,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update,
> >      s->hw_update = update;
> >      s->hw_invalidate = invalidate;
> >      s->hw_screen_dump = screen_dump;
> > +    s->hw_screen_dump_async = screen_dump_async;
> >      s->hw_text_update = text_update;
> >      s->hw = opaque;
> >  
> > diff --git a/console.h b/console.h
> > index c22803c..3cf28c0 100644
> > --- a/console.h
> > +++ b/console.h
> > @@ -341,17 +341,22 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
> >  typedef void (*vga_hw_update_ptr)(void *);
> >  typedef void (*vga_hw_invalidate_ptr)(void *);
> >  typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
> > +typedef void (*vga_hw_screen_dump_async_ptr)(void *, const char *filename,
> > +                                             bool cswitch);
> >  typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
> >  
> >  DisplayState *graphic_console_init(vga_hw_update_ptr update,
> >                                     vga_hw_invalidate_ptr invalidate,
> >                                     vga_hw_screen_dump_ptr screen_dump,
> >                                     vga_hw_text_update_ptr text_update,
> > +                                   vga_hw_screen_dump_async_ptr
> > +                                                    screen_dump_async,
> >                                     void *opaque);
> >  
> >  void vga_hw_update(void);
> >  void vga_hw_invalidate(void);
> >  void vga_hw_screen_dump(const char *filename);
> > +void vga_hw_screen_dump_async(const char *filename);
> >  void vga_hw_text_update(console_ch_t *chardata);
> >  void monitor_protocol_screen_dump_complete_event(const char *filename);
> >  
> > diff --git a/hw/blizzard.c b/hw/blizzard.c
> > index c7d844d..cc51045 100644
> > --- a/hw/blizzard.c
> > +++ b/hw/blizzard.c
> > @@ -963,7 +963,7 @@ void *s1d13745_init(qemu_irq gpio_int)
> >  
> >      s->state = graphic_console_init(blizzard_update_display,
> >                                   blizzard_invalidate_display,
> > -                                 blizzard_screen_dump, NULL, s);
> > +                                 blizzard_screen_dump, NULL, NULL, s);
> >  
> >      switch (ds_get_bits_per_pixel(s->state)) {
> >      case 0:
> > diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> > index 4edcb94..d2a9c27 100644
> > --- a/hw/cirrus_vga.c
> > +++ b/hw/cirrus_vga.c
> > @@ -2900,7 +2900,7 @@ static int vga_initfn(ISADevice *dev)
> >                         isa_address_space(dev));
> >      s->ds = graphic_console_init(s->update, s->invalidate,
> >                                   s->screen_dump, s->text_update,
> > -                                 s);
> > +                                 s->screen_dump_async, s);
> >      rom_add_vga(VGABIOS_CIRRUS_FILENAME);
> >      /* XXX ISA-LFB support */
> >      /* FIXME not qdev yet */
> > @@ -2941,7 +2941,7 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
> >       cirrus_init_common(s, device_id, 1, pci_address_space(dev));
> >       s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
> >                                        s->vga.screen_dump, s->vga.text_update,
> > -                                      &s->vga);
> > +                                      s->vga.screen_dump_async, &s->vga);
> >  
> >       /* setup PCI */
> >  
> > diff --git a/hw/exynos4210_fimd.c b/hw/exynos4210_fimd.c
> > index 3313f00..2053ed0 100644
> > --- a/hw/exynos4210_fimd.c
> > +++ b/hw/exynos4210_fimd.c
> > @@ -1898,7 +1898,8 @@ static int exynos4210_fimd_init(SysBusDevice *dev)
> >              "exynos4210.fimd", FIMD_REGS_SIZE);
> >      sysbus_init_mmio(dev, &s->iomem);
> >      s->console = graphic_console_init(exynos4210_fimd_update,
> > -                                  exynos4210_fimd_invalidate, NULL, NULL, s);
> > +                                  exynos4210_fimd_invalidate,
> > +                                  NULL, NULL, NULL, s);
> >  
> >      return 0;
> >  }
> > diff --git a/hw/g364fb.c b/hw/g364fb.c
> > index 3a0b68f..8ae40d8 100644
> > --- a/hw/g364fb.c
> > +++ b/hw/g364fb.c
> > @@ -517,7 +517,7 @@ static void g364fb_init(DeviceState *dev, G364State *s)
> >  
> >      s->ds = graphic_console_init(g364fb_update_display,
> >                                   g364fb_invalidate_display,
> > -                                 g364fb_screen_dump, NULL, s);
> > +                                 g364fb_screen_dump, NULL, NULL, s);
> >  
> >      memory_region_init_io(&s->mem_ctrl, &g364fb_ctrl_ops, s, "ctrl", 0x180000);
> >      memory_region_init_ram_ptr(&s->mem_vram, "vram",
> > diff --git a/hw/jazz_led.c b/hw/jazz_led.c
> > index 6486523..bb5daf7 100644
> > --- a/hw/jazz_led.c
> > +++ b/hw/jazz_led.c
> > @@ -251,7 +251,8 @@ static int jazz_led_init(SysBusDevice *dev)
> >      s->ds = graphic_console_init(jazz_led_update_display,
> >                                   jazz_led_invalidate_display,
> >                                   NULL,
> > -                                 jazz_led_text_update, s);
> > +                                 jazz_led_text_update,
> > +                                 NULL, s);
> >  
> >      return 0;
> >  }
> > diff --git a/hw/milkymist-vgafb.c b/hw/milkymist-vgafb.c
> > index 69afd72..ae018d1 100644
> > --- a/hw/milkymist-vgafb.c
> > +++ b/hw/milkymist-vgafb.c
> > @@ -276,7 +276,7 @@ static int milkymist_vgafb_init(SysBusDevice *dev)
> >  
> >      s->ds = graphic_console_init(vgafb_update_display,
> >                                   vgafb_invalidate_display,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >  
> >      return 0;
> >  }
> > diff --git a/hw/musicpal.c b/hw/musicpal.c
> > index 187a1ae..2fc391b 100644
> > --- a/hw/musicpal.c
> > +++ b/hw/musicpal.c
> > @@ -611,7 +611,7 @@ static int musicpal_lcd_init(SysBusDevice *dev)
> >      sysbus_init_mmio(dev, &s->iomem);
> >  
> >      s->ds = graphic_console_init(lcd_refresh, lcd_invalidate,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >      qemu_console_resize(s->ds, 128*3, 64*3);
> >  
> >      qdev_init_gpio_in(&dev->qdev, musicpal_lcd_gpio_brigthness_in, 3);
> > diff --git a/hw/omap_dss.c b/hw/omap_dss.c
> > index 86ed6ea..0e8243e 100644
> > --- a/hw/omap_dss.c
> > +++ b/hw/omap_dss.c
> > @@ -1072,7 +1072,9 @@ struct omap_dss_s *omap_dss_init(struct omap_target_agent_s *ta,
> >  
> >  #if 0
> >      s->state = graphic_console_init(omap_update_display,
> > -                                    omap_invalidate_display, omap_screen_dump, s);
> > +                                    omap_invalidate_display,
> > +                                    omap_screen_dump,
> > +                                    NULL, NULL, s);
> >  #endif
> >  
> >      return s;
> > diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
> > index f172093..d91e88d 100644
> > --- a/hw/omap_lcdc.c
> > +++ b/hw/omap_lcdc.c
> > @@ -452,7 +452,7 @@ struct omap_lcd_panel_s *omap_lcdc_init(MemoryRegion *sysmem,
> >  
> >      s->state = graphic_console_init(omap_update_display,
> >                                      omap_invalidate_display,
> > -                                    omap_screen_dump, NULL, s);
> > +                                    omap_screen_dump, NULL, NULL, s);
> >  
> >      return s;
> >  }
> > diff --git a/hw/pl110.c b/hw/pl110.c
> > index f94608c..32ee89c 100644
> > --- a/hw/pl110.c
> > +++ b/hw/pl110.c
> > @@ -451,7 +451,7 @@ static int pl110_init(SysBusDevice *dev)
> >      qdev_init_gpio_in(&s->busdev.qdev, pl110_mux_ctrl_set, 1);
> >      s->ds = graphic_console_init(pl110_update_display,
> >                                   pl110_invalidate_display,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >      return 0;
> >  }
> >  
> > diff --git a/hw/pxa2xx_lcd.c b/hw/pxa2xx_lcd.c
> > index fcbdfb3..5345fb4 100644
> > --- a/hw/pxa2xx_lcd.c
> > +++ b/hw/pxa2xx_lcd.c
> > @@ -1004,7 +1004,7 @@ PXA2xxLCDState *pxa2xx_lcdc_init(MemoryRegion *sysmem,
> >  
> >      s->ds = graphic_console_init(pxa2xx_update_display,
> >                                   pxa2xx_invalidate_display,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >  
> >      switch (ds_get_bits_per_pixel(s->ds)) {
> >      case 0:
> > diff --git a/hw/qxl.c b/hw/qxl.c
> > index d83d245..61a7edd 100644
> > --- a/hw/qxl.c
> > +++ b/hw/qxl.c
> > @@ -1727,7 +1727,8 @@ static int qxl_init_primary(PCIDevice *dev)
> >      portio_list_add(qxl_vga_port_list, pci_address_space_io(dev), 0x3b0);
> >  
> >      vga->ds = graphic_console_init(qxl_hw_update, qxl_hw_invalidate,
> > -                                   qxl_hw_screen_dump, qxl_hw_text_update, qxl);
> > +                                   qxl_hw_screen_dump, qxl_hw_text_update,
> > +                                   NULL, qxl);
> >      qemu_spice_display_init_common(&qxl->ssd, vga->ds);
> >  
> >      qxl0 = qxl;
> > diff --git a/hw/sm501.c b/hw/sm501.c
> > index 786e076..8b150be 100644
> > --- a/hw/sm501.c
> > +++ b/hw/sm501.c
> > @@ -1442,6 +1442,6 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base,
> >      }
> >  
> >      /* create qemu graphic console */
> > -    s->ds = graphic_console_init(sm501_update_display, NULL,
> > -				 NULL, NULL, s);
> > +    s->ds = graphic_console_init(sm501_update_display,
> > +                                 NULL, NULL, NULL, NULL, s);
> >  }
> > diff --git a/hw/ssd0303.c b/hw/ssd0303.c
> > index 4e1ee6e..05e6686 100644
> > --- a/hw/ssd0303.c
> > +++ b/hw/ssd0303.c
> > @@ -289,7 +289,7 @@ static int ssd0303_init(I2CSlave *i2c)
> >  
> >      s->ds = graphic_console_init(ssd0303_update_display,
> >                                   ssd0303_invalidate_display,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >      qemu_console_resize(s->ds, 96 * MAGNIFY, 16 * MAGNIFY);
> >      return 0;
> >  }
> > diff --git a/hw/ssd0323.c b/hw/ssd0323.c
> > index b0b2e94..5e3491f 100644
> > --- a/hw/ssd0323.c
> > +++ b/hw/ssd0323.c
> > @@ -330,7 +330,7 @@ static int ssd0323_init(SSISlave *dev)
> >      s->row_end = 79;
> >      s->ds = graphic_console_init(ssd0323_update_display,
> >                                   ssd0323_invalidate_display,
> > -                                 NULL, NULL, s);
> > +                                 NULL, NULL, NULL, s);
> >      qemu_console_resize(s->ds, 128 * MAGNIFY, 64 * MAGNIFY);
> >  
> >      qdev_init_gpio_in(&dev->qdev, ssd0323_cd, 1);
> > diff --git a/hw/tc6393xb.c b/hw/tc6393xb.c
> > index 420925c..853b180 100644
> > --- a/hw/tc6393xb.c
> > +++ b/hw/tc6393xb.c
> > @@ -581,6 +581,7 @@ TC6393xbState *tc6393xb_init(MemoryRegion *sysmem, uint32_t base, qemu_irq irq)
> >              NULL, /* invalidate */
> >              NULL, /* screen_dump */
> >              NULL, /* text_update */
> > +            NULL, /* screen_dump_async */
> >              s);
> >  
> >      return s;
> > diff --git a/hw/tcx.c b/hw/tcx.c
> > index ac7dcb4..9214dec 100644
> > --- a/hw/tcx.c
> > +++ b/hw/tcx.c
> > @@ -558,7 +558,7 @@ static int tcx_init1(SysBusDevice *dev)
> >  
> >          s->ds = graphic_console_init(tcx24_update_display,
> >                                       tcx24_invalidate_display,
> > -                                     tcx24_screen_dump, NULL, s);
> > +                                     tcx24_screen_dump, NULL, NULL, s);
> >      } else {
> >          /* THC 8 bit (dummy) */
> >          memory_region_init_io(&s->thc8, &dummy_ops, s, "tcx.thc8",
> > @@ -567,7 +567,7 @@ static int tcx_init1(SysBusDevice *dev)
> >  
> >          s->ds = graphic_console_init(tcx_update_display,
> >                                       tcx_invalidate_display,
> > -                                     tcx_screen_dump, NULL, s);
> > +                                     tcx_screen_dump, NULL, NULL, s);
> >      }
> >  
> >      qemu_console_resize(s->ds, s->width, s->height);
> > diff --git a/hw/vga-isa-mm.c b/hw/vga-isa-mm.c
> > index f8984c6..87b8fc6 100644
> > --- a/hw/vga-isa-mm.c
> > +++ b/hw/vga-isa-mm.c
> > @@ -132,7 +132,8 @@ int isa_vga_mm_init(target_phys_addr_t vram_base,
> >      vga_mm_init(s, vram_base, ctrl_base, it_shift, address_space);
> >  
> >      s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
> > -                                     s->vga.screen_dump, s->vga.text_update, s);
> > +                                     s->vga.screen_dump, s->vga.text_update,
> > +                                     s->vga.screen_dump_async, s);
> >  
> >      vga_init_vbe(&s->vga, address_space);
> >      return 0;
> > diff --git a/hw/vga-isa.c b/hw/vga-isa.c
> > index 4bcc4db..a362ed2 100644
> > --- a/hw/vga-isa.c
> > +++ b/hw/vga-isa.c
> > @@ -61,7 +61,8 @@ static int vga_initfn(ISADevice *dev)
> >                                          vga_io_memory, 1);
> >      memory_region_set_coalescing(vga_io_memory);
> >      s->ds = graphic_console_init(s->update, s->invalidate,
> > -                                 s->screen_dump, s->text_update, s);
> > +                                 s->screen_dump, s->text_update,
> > +                                 s->screen_dump_async, s);
> >  
> >      vga_init_vbe(s, isa_address_space(dev));
> >      /* ROM BIOS */
> > diff --git a/hw/vga-pci.c b/hw/vga-pci.c
> > index 465b643..8c8dd53 100644
> > --- a/hw/vga-pci.c
> > +++ b/hw/vga-pci.c
> > @@ -57,7 +57,8 @@ static int pci_vga_initfn(PCIDevice *dev)
> >       vga_init(s, pci_address_space(dev), pci_address_space_io(dev), true);
> >  
> >       s->ds = graphic_console_init(s->update, s->invalidate,
> > -                                  s->screen_dump, s->text_update, s);
> > +                                  s->screen_dump, s->text_update,
> > +                                  s->screen_dump_async, s);
> >  
> >       /* XXX: VGA_RAM_SIZE must be a power of two */
> >       pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->vram);
> > diff --git a/hw/vga_int.h b/hw/vga_int.h
> > index 7685b2b..4486cfb 100644
> > --- a/hw/vga_int.h
> > +++ b/hw/vga_int.h
> > @@ -161,6 +161,7 @@ typedef struct VGACommonState {
> >      vga_hw_invalidate_ptr invalidate;
> >      vga_hw_screen_dump_ptr screen_dump;
> >      vga_hw_text_update_ptr text_update;
> > +    vga_hw_screen_dump_async_ptr screen_dump_async;
> >      /* hardware mouse cursor support */
> >      uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32];
> >      void (*cursor_invalidate)(struct VGACommonState *s);
> > diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> > index 142d9f4..db96e61 100644
> > --- a/hw/vmware_vga.c
> > +++ b/hw/vmware_vga.c
> > @@ -1087,7 +1087,8 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size,
> >      s->vga.ds = graphic_console_init(vmsvga_update_display,
> >                                       vmsvga_invalidate_display,
> >                                       vmsvga_screen_dump,
> > -                                     vmsvga_text_update, s);
> > +                                     vmsvga_text_update,
> > +                                     NULL, s);
> >  
> >  
> >      s->fifo_size = SVGA_FIFO_SIZE;
> > diff --git a/hw/xenfb.c b/hw/xenfb.c
> > index 1bcf171..ad9f0d7 100644
> > --- a/hw/xenfb.c
> > +++ b/hw/xenfb.c
> > @@ -906,6 +906,7 @@ static int fb_initialise(struct XenDevice *xendev)
> >                                          xenfb_invalidate,
> >                                          NULL,
> >                                          NULL,
> > +                                        NULL,
> >                                          fb);
> >          fb->have_console = 1;
> >      }
> > @@ -1015,6 +1016,7 @@ wait_more:
> >                                      xenfb_invalidate,
> >                                      NULL,
> >                                      NULL,
> > +                                    NULL,
> >                                      fb);
> >      fb->have_console = 1;
> >  
> > diff --git a/monitor.c b/monitor.c
> > index 1a65c41..91a76a1 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -901,6 +901,11 @@ static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >      return 0;
> >  }
> >  
> > +void qmp_screendump_async(const char *filename, Error **errp)
> > +{
> > +    vga_hw_screen_dump_async(filename);
> > +}
> > +
> >  static void do_logfile(Monitor *mon, const QDict *qdict)
> >  {
> >      cpu_set_log_filename(qdict_get_str(qdict, "filename"));
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 72b17f1..a5873cb 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1595,3 +1595,23 @@
> >  { 'command': 'qom-list-types',
> >    'data': { '*implements': 'str', '*abstract': 'bool' },
> >    'returns': [ 'ObjectTypeInfo' ] }
> > +
> > +##
> > +## @screendump-async:
> > +#
> > +# This command will perform a screen dump of the first console to the givem
> > +# filename. The additional parameters are unused at this time.
> 
> Isn't it interesting to allow the mngt app to specify the console?
> 
> > +#
> > +# @filename  name of output file to write screen dump to
> > +#
> > +# Since: 1.1
> > +#
> > +# Notes: This command is experimental and may change syntax in future releases.
> 
> Why is this needed?
> 
> > +#
> > +# This command is the same as the qmp/hmp screendump command, except that on
> > +# successful completion of the scren dump the SCREEN_DUMP_COMPLETE event is
> > +# emitted.
> 
> The real difference is that it's asynchronous, while the other is synchronous.
> 
> > +#
> > +##
> > +{ 'command': 'screendump-async',
> > +  'data': { 'filename': 'str' } }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 705f704..ea0d051 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -170,6 +170,32 @@ Example:
> >  EQMP
> >  
> >      {
> > +        .name       = "screendump-async",
> > +        .args_type  = "filename:F",
> > +        .params     = "filename",
> > +        .help       = "save screen into PPM image 'filename'",
> > +        .user_print = monitor_user_noop,
> > +        .mhandler.cmd_new = qmp_marshal_input_screendump_async,
> > +    },
> > +
> > +SQMP
> > +screendump-async
> > +----------------
> > +
> > +Save screen into PPM image. Fires a SCREEN_DUMP_COMPLETE event on completion.
> > +
> > +Arguments:
> > +
> > +- "filename": file path (json-string)
> > +
> > +Example:
> > +
> > +-> { "execute": "screendump-async", "arguments": { "filename": "/tmp/image" } }
> > +<- { "return": {} }
> > +
> > +EQMP
> > +
> > +    {
> >          .name       = "stop",
> >          .args_type  = "",
> >          .mhandler.cmd_new = qmp_marshal_input_stop,
> 
> 

Re: [Qemu-devel] [PATCH 1/4] qapi-schema: fix typos and explain 'spice' auth

[Alon Levy]

On Fri, Feb 24, 2012 at 11:22:02PM +0200, Alon Levy wrote:
> Signed-off-by: Alon Levy <alevy@redhat.com>

Ping.

> ---
>  qapi-schema.json |   18 ++++++++++--------
>  1 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index d0b6792..72b17f1 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -616,12 +616,13 @@
>  # @connection-id: SPICE connection id number.  All channels with the same id
>  #                 belong to the same SPICE session.
>  #
> -# @connection-type: SPICE channel type number.  "1" is the main control channel,
> -#                   filter for this one if you want track spice sessions only
> +# @connection-type: SPICE channel type number.  "1" is the main control
> +#                   channel, filter for this one if you want to track spice
> +#                   sessions only
>  #
> -# @channel-id: SPICE channel ID number.  Usually "0", might be different needed
> -#              when multiple channels of the same type exist, such as multiple
> -#              display channels in a multihead setup
> +# @channel-id: SPICE channel ID number.  Usually "0", might be different when
> +#                   multiple channels of the same type exist, such as multiple
> +#                   display channels in a multihead setup
>  #
>  # @tls: true if the channel is encrypted, false otherwise.
>  #
> @@ -649,8 +650,9 @@
>  # @tls-port: #optional The SPICE server's TLS port number.
>  #
>  # @auth: #optional the current authentication type used by the server
> -#        'none' if no authentication is being used
> -#        'spice' (TODO: describe)
> +#        'none'  if no authentication is being used
> +#        'spice' uses SASL or direct TLS authentication, depending on command
> +#                line options
>  #
>  # @channels: a list of @SpiceChannel for each active spice channel
>  #
> @@ -1216,7 +1218,7 @@
>  { 'command': 'migrate_set_speed', 'data': {'value': 'int'} }
>  
>  ##
> -# @DevicePropertyInfo:
> +# @ObjectPropertyInfo:
>  #
>  # @name: the name of the property
>  #
> -- 
> 1.7.9.1
> 
> 

Re: [Qemu-devel] [PATCH 1/4] qapi-schema: fix typos and explain 'spice' auth

[Luiz Capitulino]

On Fri, 24 Feb 2012 23:22:02 +0200
Alon Levy <alevy@redhat.com> wrote:

> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
>  qapi-schema.json |   18 ++++++++++--------
>  1 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index d0b6792..72b17f1 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -616,12 +616,13 @@
>  # @connection-id: SPICE connection id number.  All channels with the same id
>  #                 belong to the same SPICE session.
>  #
> -# @connection-type: SPICE channel type number.  "1" is the main control channel,
> -#                   filter for this one if you want track spice sessions only
> +# @connection-type: SPICE channel type number.  "1" is the main control
> +#                   channel, filter for this one if you want to track spice
> +#                   sessions only
>  #
> -# @channel-id: SPICE channel ID number.  Usually "0", might be different needed
> -#              when multiple channels of the same type exist, such as multiple
> -#              display channels in a multihead setup
> +# @channel-id: SPICE channel ID number.  Usually "0", might be different when
> +#                   multiple channels of the same type exist, such as multiple
> +#                   display channels in a multihead setup
>  #
>  # @tls: true if the channel is encrypted, false otherwise.
>  #
> @@ -649,8 +650,9 @@
>  # @tls-port: #optional The SPICE server's TLS port number.
>  #
>  # @auth: #optional the current authentication type used by the server
> -#        'none' if no authentication is being used
> -#        'spice' (TODO: describe)
> +#        'none'  if no authentication is being used
> +#        'spice' uses SASL or direct TLS authentication, depending on command
> +#                line options
>  #
>  # @channels: a list of @SpiceChannel for each active spice channel
>  #
> @@ -1216,7 +1218,7 @@
>  { 'command': 'migrate_set_speed', 'data': {'value': 'int'} }
>  
>  ##
> -# @DevicePropertyInfo:
> +# @ObjectPropertyInfo:
>  #
>  # @name: the name of the property
>  #

This last hunk is unrelated to this series.

Re: [Qemu-devel] [PATCH 1/4] qapi-schema: fix typos and explain 'spice' auth

[Alon Levy]

On Tue, Feb 28, 2012 at 04:57:10PM -0300, Luiz Capitulino wrote:
> On Fri, 24 Feb 2012 23:22:02 +0200
> Alon Levy <alevy@redhat.com> wrote:
> 
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> >  qapi-schema.json |   18 ++++++++++--------
> >  1 files changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index d0b6792..72b17f1 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -616,12 +616,13 @@
> >  # @connection-id: SPICE connection id number.  All channels with the same id
> >  #                 belong to the same SPICE session.
> >  #
> > -# @connection-type: SPICE channel type number.  "1" is the main control channel,
> > -#                   filter for this one if you want track spice sessions only
> > +# @connection-type: SPICE channel type number.  "1" is the main control
> > +#                   channel, filter for this one if you want to track spice
> > +#                   sessions only
> >  #
> > -# @channel-id: SPICE channel ID number.  Usually "0", might be different needed
> > -#              when multiple channels of the same type exist, such as multiple
> > -#              display channels in a multihead setup
> > +# @channel-id: SPICE channel ID number.  Usually "0", might be different when
> > +#                   multiple channels of the same type exist, such as multiple
> > +#                   display channels in a multihead setup
> >  #
> >  # @tls: true if the channel is encrypted, false otherwise.
> >  #
> > @@ -649,8 +650,9 @@
> >  # @tls-port: #optional The SPICE server's TLS port number.
> >  #
> >  # @auth: #optional the current authentication type used by the server
> > -#        'none' if no authentication is being used
> > -#        'spice' (TODO: describe)
> > +#        'none'  if no authentication is being used
> > +#        'spice' uses SASL or direct TLS authentication, depending on command
> > +#                line options
> >  #
> >  # @channels: a list of @SpiceChannel for each active spice channel
> >  #
> > @@ -1216,7 +1218,7 @@
> >  { 'command': 'migrate_set_speed', 'data': {'value': 'int'} }
> >  
> >  ##
> > -# @DevicePropertyInfo:
> > +# @ObjectPropertyInfo:
> >  #
> >  # @name: the name of the property
> >  #
> 
> This last hunk is unrelated to this series.

I'll send it separately.