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

[Luiz Capitulino <lcapitulino@redhat.com>]

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.