Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async

[Alon Levy <alevy@redhat.com>]

On Mon, Mar 05, 2012 at 08:33:06AM -0600, Anthony Liguori wrote:
> On 03/05/2012 08:16 AM, Alon Levy wrote:
> >adds a handler for the following qmp screendump-async command.
> >
> >graphics_console_init signature change required touching every user, but
> >no implementation of the new vga_hw_screen_dump_async_ptr is provided
> >in this patch.
> >
> >Signed-off-by: Alon Levy<alevy@redhat.com>
> >---
> >  console.c            |    4 ++++
> >  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 ++
> >  25 files changed, 45 insertions(+), 23 deletions(-)
> >
> >diff --git a/console.c b/console.c
> >index 25d8acb..9a49e93 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;
> >
> >@@ -1403,6 +1404,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 +1424,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);
> 
> 
> async in QEMU doesn't mean "generate a QMP event when you're done".
> It should mean execute this closure when you finish (function
> pointer + opaque).
> 
> The QMP event should be dispatched from the closure such that the
> screendump code doesn't have to have a direct dependency on QMP.

Yes, I can add a closure (I could put the filename there, that's the
only thing we need to have there right now).

Note that there is already a function wrapping the specific QEVENT used.

> 
> Regards,
> 
> Anthony Liguori