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

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

[Alon Levy]

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);
 
 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 e17b0e3..da0f931 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1763,7 +1763,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;
 
-- 
1.7.9.1

[Qemu-devel] [PATCH v2 2/2] add qmp screendump-async

[Alon Levy]

Uses a new console.h function, vga_hw_screen_dump_async.

vga_hw_screen_dump_async falls back to hw_vga_screen_dump if there
is no hw_vga_screen_dump_async callback provided to graphic_console_init.

This is the only case right now, but the up side is that the interface
is already implemented.

The QEVENT_SCREEN_DUMP event is used to notify of completion.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 console.c        |   19 +++++++++++++++++--
 monitor.c        |    5 +++++
 qapi-schema.json |   20 ++++++++++++++++++++
 qmp-commands.hx  |   26 ++++++++++++++++++++++++++
 4 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/console.c b/console.c
index 9a49e93..5102573 100644
--- a/console.c
+++ b/console.c
@@ -176,7 +176,7 @@ 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)
 {
     TextConsole *previous_active_console;
     bool cswitch;
@@ -189,8 +189,13 @@ 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);
+    } else if (consoles[0] && consoles[0]->hw_screen_dump) {
         consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
+        if (async) {
+            monitor_protocol_screen_dump_complete_event(filename);
+        }
     } else {
         error_report("screen dump not implemented");
     }
@@ -200,6 +205,16 @@ void vga_hw_screen_dump(const char *filename)
     }
 }
 
+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)
diff --git a/monitor.c b/monitor.c
index a8c84c0..1c3bd2a 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 dd9e0e5..60dae67 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1633,3 +1633,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 0c9bfac..94ee1ae 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 v2 1/2] console: add hw_screen_dump_async

[Anthony Liguori]

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.

Regards,

Anthony Liguori

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

[Alon Levy]

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

[Qemu-devel] [PATCH v3 0/3] screendump async command

[Alon Levy]

v2->v3:
 pass a closure to hw_screen_dump_async

Alon Levy (3):
  monitor, console: add QEVENT_SCREEN_DUMP_COMPLETE
  console: add hw_screen_dump_async
  add qmp screendump-async

 QMP/qmp-events.txt   |   15 +++++++++++
 console.c            |   66 ++++++++++++++++++++++++++++++++++++++++++++++++-
 console.h            |    9 +++++++
 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            |    7 +++++
 monitor.h            |    1 +
 qapi-schema.json     |   20 +++++++++++++++
 qmp-commands.hx      |   26 +++++++++++++++++++
 30 files changed, 178 insertions(+), 25 deletions(-)

-- 
1.7.9.1

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

[Alon Levy]

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

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index 9286af5..49c8d77 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -335,3 +335,18 @@ 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" },
+     "timestamp": { "seconds": 1267061043, "microseconds": 959568 } }
diff --git a/console.c b/console.c
index 6a463f5..beebced 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,12 @@ PixelFormat qemu_default_pixelformat(int bpp)
     }
     return pf;
 }
+
+void monitor_protocol_screen_dump_complete_event(const char *filename)
+{
+    QObject *event_data;
+
+    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 cbdfbad..a8c84c0 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

[Qemu-devel] [PATCH v3 2/3] console: add hw_screen_dump_async

[Alon Levy]

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            |    8 ++++++++
 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, 48 insertions(+), 23 deletions(-)

diff --git a/console.c b/console.c
index beebced..1e97c4a 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..3025135 100644
--- a/console.h
+++ b/console.h
@@ -341,17 +341,25 @@ 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_cb_ptr)(void *opaque);
+typedef void (*vga_hw_screen_dump_async_ptr)(void *, const char *filename,
+                             bool cswitch,
+                             vga_hw_screen_dump_async_cb_ptr cb,
+                             void *cb_opaque);
 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 e17b0e3..da0f931 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1763,7 +1763,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;
 
-- 
1.7.9.1

[Qemu-devel] [PATCH v3 3/3] add qmp screendump-async

[Alon Levy]

Uses a new console.h function, vga_hw_screen_dump_async.

vga_hw_screen_dump_async falls back to hw_vga_screen_dump if there
is no hw_vga_screen_dump_async callback provided to graphic_console_init.

This is the only case right now, but the up side is that the interface
is already implemented.

The QEVENT_SCREEN_DUMP event is used to notify of completion.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 console.c        |   51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 monitor.c        |    5 +++++
 qapi-schema.json |   20 ++++++++++++++++++++
 qmp-commands.hx  |   26 ++++++++++++++++++++++++++
 4 files changed, 100 insertions(+), 2 deletions(-)

diff --git a/console.c b/console.c
index 1e97c4a..301ac3d 100644
--- a/console.c
+++ b/console.c
@@ -176,8 +176,38 @@ void vga_hw_invalidate(void)
         active_console->hw_invalidate(active_console->hw);
 }
 
-void vga_hw_screen_dump(const char *filename)
+typedef struct VGAHwScreenDumpAsyncCbData {
+    char *filename;
+} VGAHwScreenDumpAsyncCbData;
+
+static VGAHwScreenDumpAsyncCbData *
+vga_hw_screen_dump_async_data_new(const char *filename)
+{
+    VGAHwScreenDumpAsyncCbData *data = g_malloc0(
+            sizeof(VGAHwScreenDumpAsyncCbData));
+
+    data->filename = g_strdup(filename);
+    return data;
+}
+
+static void vga_hw_screen_dump_async_data_free(
+        VGAHwScreenDumpAsyncCbData *data)
 {
+    g_free(data->filename);
+    g_free(data);
+}
+
+static void vga_hw_screen_dump_async_cb(void *opaque)
+{
+    VGAHwScreenDumpAsyncCbData *data = opaque;
+
+    monitor_protocol_screen_dump_complete_event(data->filename);
+    vga_hw_screen_dump_async_data_free(data);
+}
+
+static void vga_hw_screen_dump_helper(const char *filename, bool async)
+{
+    VGAHwScreenDumpAsyncCbData *data;
     TextConsole *previous_active_console;
     bool cswitch;
 
@@ -189,8 +219,15 @@ 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) {
+        data = vga_hw_screen_dump_async_data_new(filename);
+        consoles[0]->hw_screen_dump_async(consoles[0]->hw, filename, cswitch,
+                                          vga_hw_screen_dump_async_cb, data);
+    } else if (consoles[0] && consoles[0]->hw_screen_dump) {
         consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
+        if (async) {
+            monitor_protocol_screen_dump_complete_event(filename);
+        }
     } else {
         error_report("screen dump not implemented");
     }
@@ -200,6 +237,16 @@ void vga_hw_screen_dump(const char *filename)
     }
 }
 
+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)
diff --git a/monitor.c b/monitor.c
index a8c84c0..1c3bd2a 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 dd9e0e5..60dae67 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1633,3 +1633,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 0c9bfac..94ee1ae 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 v3 0/3] screendump async command

[Anthony Liguori]

On 03/05/2012 09:56 AM, Alon Levy wrote:
> v2->v3:
>   pass a closure to hw_screen_dump_async
>
> Alon Levy (3):
>    monitor, console: add QEVENT_SCREEN_DUMP_COMPLETE
>    console: add hw_screen_dump_async
>    add qmp screendump-async

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

>
>   QMP/qmp-events.txt   |   15 +++++++++++
>   console.c            |   66 ++++++++++++++++++++++++++++++++++++++++++++++++-
>   console.h            |    9 +++++++
>   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            |    7 +++++
>   monitor.h            |    1 +
>   qapi-schema.json     |   20 +++++++++++++++
>   qmp-commands.hx      |   26 +++++++++++++++++++
>   30 files changed, 178 insertions(+), 25 deletions(-)
>

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

[Avi Kivity]

On 03/05/2012 04:33 PM, Anthony Liguori wrote:
>
>
> 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.
>

What about using the parallel execution facility of qmp?  It's silly to
duplicate every command X with X-async and X-COMPLETED.

-- 
error compiling committee.c: too many arguments to function

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

[Anthony Liguori]

On 03/05/2012 11:20 AM, Avi Kivity wrote:
> On 03/05/2012 04:33 PM, Anthony Liguori wrote:
>>
>>
>> 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.
>>
>
> What about using the parallel execution facility of qmp?  It's silly to
> duplicate every command X with X-async and X-COMPLETED.

We need to switch over to QAPI to get there.  We're pretty close to being there. 
  Luiz, about how long do you think before we get there?

Regards,

Anthony Liguori

>

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

[Avi Kivity]

On 03/05/2012 07:27 PM, Anthony Liguori wrote:
> On 03/05/2012 11:20 AM, Avi Kivity wrote:
>> On 03/05/2012 04:33 PM, Anthony Liguori wrote:
>>>
>>>
>>> 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.
>>>
>>
>> What about using the parallel execution facility of qmp?  It's silly to
>> duplicate every command X with X-async and X-COMPLETED.
>
> We need to switch over to QAPI to get there.

Just an implementation detail, yes?  No spec/protocol changes?

>   We're pretty close to being there.  Luiz, about how long do you
> think before we get there?

It's a pity to add new commands along the way.

-- 
error compiling committee.c: too many arguments to function

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

[Luiz Capitulino]

On Mon, 05 Mar 2012 19:29:14 +0200
Avi Kivity <avi@redhat.com> wrote:

> On 03/05/2012 07:27 PM, Anthony Liguori wrote:
> > On 03/05/2012 11:20 AM, Avi Kivity wrote:
> >> On 03/05/2012 04:33 PM, Anthony Liguori wrote:
> >>>
> >>>
> >>> 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.
> >>>
> >>
> >> What about using the parallel execution facility of qmp?  It's silly to
> >> duplicate every command X with X-async and X-COMPLETED.
> >
> > We need to switch over to QAPI to get there.
> 
> Just an implementation detail, yes?  No spec/protocol changes?

We haven't discussed it yet how to do async commands in detail, so it may or
may not have protocol changes.

I have a simple proposal in mind, but haven't submitted it yet.

> >   We're pretty close to being there.  Luiz, about how long do you
> > think before we get there?
> 
> It's a pity to add new commands along the way.

We decided not to block useful features because of the long time that's
taking to do async support properly.

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

[Anthony Liguori]

On 03/05/2012 11:29 AM, Avi Kivity wrote:
> On 03/05/2012 07:27 PM, Anthony Liguori wrote:
>> On 03/05/2012 11:20 AM, Avi Kivity wrote:
>>> On 03/05/2012 04:33 PM, Anthony Liguori wrote:
>>>>
>>>>
>>>> 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.
>>>>
>>>
>>> What about using the parallel execution facility of qmp?  It's silly to
>>> duplicate every command X with X-async and X-COMPLETED.
>>
>> We need to switch over to QAPI to get there.
>
> Just an implementation detail, yes?  No spec/protocol changes?

Correct.

>
>>    We're pretty close to being there.  Luiz, about how long do you
>> think before we get there?
>
> It's a pity to add new commands along the way.

It's more complicated than this unfortunately.

A client needs to be able to determine whether the 'screendump' command works as 
expected.  Unfortunately, when QXL was introduced, 'screendump' became broken 
across multiple releases.

screendump is the right interface, but since it was broken in multiple releases, 
we need another command for a client to determine that it's not broken.  In the 
short term, screendump_async is that.  After QAPI, it will probably be a 
screendump2.

I don't mind introducing short term commands and then deprecating it 
particularly when they are marked as such.

Regards,

Anthony Liguori

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

[Avi Kivity]

On 03/05/2012 08:16 PM, Anthony Liguori wrote:
>
>>
>>>    We're pretty close to being there.  Luiz, about how long do you
>>> think before we get there?
>>
>> It's a pity to add new commands along the way.
>
> It's more complicated than this unfortunately.
>
> A client needs to be able to determine whether the 'screendump'
> command works as expected.  Unfortunately, when QXL was introduced,
> 'screendump' became broken across multiple releases.
>
> screendump is the right interface, but since it was broken in multiple
> releases, we need another command for a client to determine that it's
> not broken.  In the short term, screendump_async is that.  After QAPI,
> it will probably be a screendump2.
>
> I don't mind introducing short term commands and then deprecating it
> particularly when they are marked as such.

Zooming out from screendump, there are several ways to deal with broken
commands:

1. introduce new commands
2. introduce capabilities that say "screendump is fixed wrt bug so-and-so"
3. fix it and backport the fix to a stable release
4. ignore the broken command and hope

My preference is 3->2->1->4, or we'll end up with screendump65535 soon.

-- 
error compiling committee.c: too many arguments to function

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

[Anthony Liguori]

On 03/05/2012 12:22 PM, Avi Kivity wrote:
> On 03/05/2012 08:16 PM, Anthony Liguori wrote:
>>
>>>
>>>>     We're pretty close to being there.  Luiz, about how long do you
>>>> think before we get there?
>>>
>>> It's a pity to add new commands along the way.
>>
>> It's more complicated than this unfortunately.
>>
>> A client needs to be able to determine whether the 'screendump'
>> command works as expected.  Unfortunately, when QXL was introduced,
>> 'screendump' became broken across multiple releases.
>>
>> screendump is the right interface, but since it was broken in multiple
>> releases, we need another command for a client to determine that it's
>> not broken.  In the short term, screendump_async is that.  After QAPI,
>> it will probably be a screendump2.
>>
>> I don't mind introducing short term commands and then deprecating it
>> particularly when they are marked as such.
>
> Zooming out from screendump, there are several ways to deal with broken
> commands:
>
> 1. introduce new commands

This is our only short term options for this specific circumstance.

> 2. introduce capabilities that say "screendump is fixed wrt bug so-and-so"

We don't have such a mechanism and I generally would prefer to avoid it. 
There's a certain shame in introducing new commands that hopefully will cause us 
to be more careful in the future.

> 3. fix it and backport the fix to a stable release

This is only not practical because it requires such an infrastructure change.

> 4. ignore the broken command and hope

5. just tell clients to rely on version information and ignore the existence of 
downstreams

This is really mostly a downstream problem, not an upstream problem.  Version 
numbers can be reliable here for upstream.

This is the approach that would be typically taken for a C library.  The flip 
side is that this is also while we end up with autotools and a bunch of compile 
time tests to determine what broken functions exist.

Regards,

Anthony Liguori

>
> My preference is 3->2->1->4, or we'll end up with screendump65535 soon.



>

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

[Luiz Capitulino]

On Mon, 05 Mar 2012 11:27:07 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 03/05/2012 11:20 AM, Avi Kivity wrote:
> > On 03/05/2012 04:33 PM, Anthony Liguori wrote:
> >>
> >>
> >> 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.
> >>
> >
> > What about using the parallel execution facility of qmp?  It's silly to
> > duplicate every command X with X-async and X-COMPLETED.
> 
> We need to switch over to QAPI to get there.  We're pretty close to being there. 
>   Luiz, about how long do you think before we get there?

It's a bit hard to tell, because it depends on upstream review plus me not
being bogged down with other stuff. But maybe in around two weeks when I
resume my work on this.

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

[Alon Levy]

On Mon, Mar 05, 2012 at 02:31:42PM -0300, Luiz Capitulino wrote:
> On Mon, 05 Mar 2012 11:27:07 -0600
> Anthony Liguori <anthony@codemonkey.ws> wrote:
> 
> > On 03/05/2012 11:20 AM, Avi Kivity wrote:
> > > On 03/05/2012 04:33 PM, Anthony Liguori wrote:
> > >>
> > >>
> > >> 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.
> > >>
> > >
> > > What about using the parallel execution facility of qmp?  It's silly to
> > > duplicate every command X with X-async and X-COMPLETED.

How would the parallel execution facility be opaque to the implementer?
screendump returns, screendump_async needs to pass a closure. You can
automatically generate any amount of code, but you can only have a
single function implementation with longjmp/coroutine, or having a
saparate thread per command but that would mean taking locks for
anything not trivial, which avoids the no-change-to-implementation. Is
this what you have in mind?

> > 
> > We need to switch over to QAPI to get there.  We're pretty close to being there. 
> >   Luiz, about how long do you think before we get there?
> 
> It's a bit hard to tell, because it depends on upstream review plus me not
> being bogged down with other stuff. But maybe in around two weeks when I
> resume my work on this.
> 

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

[Avi Kivity]

On 03/05/2012 08:09 PM, Alon Levy wrote:
> On Mon, Mar 05, 2012 at 02:31:42PM -0300, Luiz Capitulino wrote:
> > On Mon, 05 Mar 2012 11:27:07 -0600
> > Anthony Liguori <anthony@codemonkey.ws> wrote:
> > 
> > > On 03/05/2012 11:20 AM, Avi Kivity wrote:
> > > > On 03/05/2012 04:33 PM, Anthony Liguori wrote:
> > > >>
> > > >>
> > > >> 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.
> > > >>
> > > >
> > > > What about using the parallel execution facility of qmp?  It's silly to
> > > > duplicate every command X with X-async and X-COMPLETED.
>
> How would the parallel execution facility be opaque to the implementer?
> screendump returns, screendump_async needs to pass a closure. You can
> automatically generate any amount of code, but you can only have a
> single function implementation with longjmp/coroutine, or having a
> saparate thread per command but that would mean taking locks for
> anything not trivial, which avoids the no-change-to-implementation. Is
> this what you have in mind?

It would not be opaque to the implementer.  But it would avoid
introducing new commands and events, instead we have a unified mechanism
to signal completion.

-- 
error compiling committee.c: too many arguments to function

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

[Alon Levy]

On Mon, Mar 05, 2012 at 08:17:52PM +0200, Avi Kivity wrote:
> On 03/05/2012 08:09 PM, Alon Levy wrote:
> > On Mon, Mar 05, 2012 at 02:31:42PM -0300, Luiz Capitulino wrote:
> > > On Mon, 05 Mar 2012 11:27:07 -0600
> > > Anthony Liguori <anthony@codemonkey.ws> wrote:
> > > 
> > > > On 03/05/2012 11:20 AM, Avi Kivity wrote:
> > > > > On 03/05/2012 04:33 PM, Anthony Liguori wrote:
> > > > >>
> > > > >>
> > > > >> 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.
> > > > >>
> > > > >
> > > > > What about using the parallel execution facility of qmp?  It's silly to
> > > > > duplicate every command X with X-async and X-COMPLETED.
> >
> > How would the parallel execution facility be opaque to the implementer?
> > screendump returns, screendump_async needs to pass a closure. You can
> > automatically generate any amount of code, but you can only have a
> > single function implementation with longjmp/coroutine, or having a
> > saparate thread per command but that would mean taking locks for
> > anything not trivial, which avoids the no-change-to-implementation. Is
> > this what you have in mind?
> 
> It would not be opaque to the implementer.  But it would avoid
> introducing new commands and events, instead we have a unified mechanism
> to signal completion.

Yeah, that sounds good. Let's have that for 6.4.

> 
> -- 
> error compiling committee.c: too many arguments to function
> 
> 

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

[Luiz Capitulino]

On Mon, 5 Mar 2012 20:58:08 +0200
Alon Levy <alevy@redhat.com> wrote:

> On Mon, Mar 05, 2012 at 08:17:52PM +0200, Avi Kivity wrote:
> > On 03/05/2012 08:09 PM, Alon Levy wrote:
> > > On Mon, Mar 05, 2012 at 02:31:42PM -0300, Luiz Capitulino wrote:
> > > > On Mon, 05 Mar 2012 11:27:07 -0600
> > > > Anthony Liguori <anthony@codemonkey.ws> wrote:
> > > > 
> > > > > On 03/05/2012 11:20 AM, Avi Kivity wrote:
> > > > > > On 03/05/2012 04:33 PM, Anthony Liguori wrote:
> > > > > >>
> > > > > >>
> > > > > >> 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.
> > > > > >>
> > > > > >
> > > > > > What about using the parallel execution facility of qmp?  It's silly to
> > > > > > duplicate every command X with X-async and X-COMPLETED.
> > >
> > > How would the parallel execution facility be opaque to the implementer?
> > > screendump returns, screendump_async needs to pass a closure. You can
> > > automatically generate any amount of code, but you can only have a
> > > single function implementation with longjmp/coroutine, or having a
> > > saparate thread per command but that would mean taking locks for
> > > anything not trivial, which avoids the no-change-to-implementation. Is
> > > this what you have in mind?
> > 
> > It would not be opaque to the implementer.  But it would avoid
> > introducing new commands and events, instead we have a unified mechanism
> > to signal completion.
> 
> Yeah, that sounds good. Let's have that for 6.4.

That's too far away, qemu will have be re-written a dozen of times when we
get there.

But if you meant 1.2, yes, I hope to have async support for 1.2. But I must
admit that I'm having some trouble delivering things on time...

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

[Gerd Hoffmann]

  Hi,

>> How would the parallel execution facility be opaque to the implementer?
>> screendump returns, screendump_async needs to pass a closure. You can
>> automatically generate any amount of code, but you can only have a
>> single function implementation with longjmp/coroutine, or having a
>> saparate thread per command but that would mean taking locks for
>> anything not trivial, which avoids the no-change-to-implementation. Is
>> this what you have in mind?
> 
> It would not be opaque to the implementer.  But it would avoid
> introducing new commands and events, instead we have a unified mechanism
> to signal completion.

Ok.  We have a async mechanism today: .mhandler.cmd_async = ...

I know it has its problems like no cancelation and is deprecated and
all.  But still: how about using it as interim until QAPI-based async
monitor support is ready?  We could unbreak qxl screendumps without
having to introduce a new (but temporary!) screendump_async command +
completion event.

cheers,
  Gerd

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

[Alon Levy]

On Tue, Mar 06, 2012 at 08:36:34AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >> How would the parallel execution facility be opaque to the implementer?
> >> screendump returns, screendump_async needs to pass a closure. You can
> >> automatically generate any amount of code, but you can only have a
> >> single function implementation with longjmp/coroutine, or having a
> >> saparate thread per command but that would mean taking locks for
> >> anything not trivial, which avoids the no-change-to-implementation. Is
> >> this what you have in mind?
> > 
> > It would not be opaque to the implementer.  But it would avoid
> > introducing new commands and events, instead we have a unified mechanism
> > to signal completion.
> 
> Ok.  We have a async mechanism today: .mhandler.cmd_async = ...
> 
> I know it has its problems like no cancelation and is deprecated and
> all.  But still: how about using it as interim until QAPI-based async
> monitor support is ready?  We could unbreak qxl screendumps without
> having to introduce a new (but temporary!) screendump_async command +
> completion event.

With the added benefit of no libvirt changes for the temporary solution.

> 
> cheers,
>   Gerd
> 

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

[Alon Levy]

On Tue, Mar 06, 2012 at 08:36:34AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >> How would the parallel execution facility be opaque to the implementer?
> >> screendump returns, screendump_async needs to pass a closure. You can
> >> automatically generate any amount of code, but you can only have a
> >> single function implementation with longjmp/coroutine, or having a
> >> saparate thread per command but that would mean taking locks for
> >> anything not trivial, which avoids the no-change-to-implementation. Is
> >> this what you have in mind?
> > 
> > It would not be opaque to the implementer.  But it would avoid
> > introducing new commands and events, instead we have a unified mechanism
> > to signal completion.
> 
> Ok.  We have a async mechanism today: .mhandler.cmd_async = ...
> 
> I know it has its problems like no cancelation and is deprecated and
> all.  But still: how about using it as interim until QAPI-based async
> monitor support is ready?  We could unbreak qxl screendumps without
> having to introduce a new (but temporary!) screendump_async command +
> completion event.

Actually, I'm not sure this doesn't reintroduce the original problem
(which I haven't been able to reproduce):

client: screenshot <-> client libvirt <-> host libvirt

host libvirt (screendump) <-> qemu monitor -> <- spice server <-> client

> 
> cheers,
>   Gerd
> 

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

[Gerd Hoffmann]

On 03/06/12 08:56, Alon Levy wrote:
> On Tue, Mar 06, 2012 at 08:36:34AM +0100, Gerd Hoffmann wrote:
>>   Hi,
>>
>>>> How would the parallel execution facility be opaque to the implementer?
>>>> screendump returns, screendump_async needs to pass a closure. You can
>>>> automatically generate any amount of code, but you can only have a
>>>> single function implementation with longjmp/coroutine, or having a
>>>> saparate thread per command but that would mean taking locks for
>>>> anything not trivial, which avoids the no-change-to-implementation. Is
>>>> this what you have in mind?
>>>
>>> It would not be opaque to the implementer.  But it would avoid
>>> introducing new commands and events, instead we have a unified mechanism
>>> to signal completion.
>>
>> Ok.  We have a async mechanism today: .mhandler.cmd_async = ...
>>
>> I know it has its problems like no cancelation and is deprecated and
>> all.  But still: how about using it as interim until QAPI-based async
>> monitor support is ready?  We could unbreak qxl screendumps without
>> having to introduce a new (but temporary!) screendump_async command +
>> completion event.
> 
> Actually, I'm not sure this doesn't reintroduce the original problem
> (which I haven't been able to reproduce):
> 
> client: screenshot <-> client libvirt <-> host libvirt
> 
> host libvirt (screendump) <-> qemu monitor -> <- spice server <-> client

Hmm?  spice client can ask for a screendump via libvirt?

/me looks completely puzzled.

cheers,
  Gerd

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

[Alon Levy]

On Tue, Mar 06, 2012 at 09:10:00AM +0100, Gerd Hoffmann wrote:
> On 03/06/12 08:56, Alon Levy wrote:
> > On Tue, Mar 06, 2012 at 08:36:34AM +0100, Gerd Hoffmann wrote:
> >>   Hi,
> >>
> >>>> How would the parallel execution facility be opaque to the implementer?
> >>>> screendump returns, screendump_async needs to pass a closure. You can
> >>>> automatically generate any amount of code, but you can only have a
> >>>> single function implementation with longjmp/coroutine, or having a
> >>>> saparate thread per command but that would mean taking locks for
> >>>> anything not trivial, which avoids the no-change-to-implementation. Is
> >>>> this what you have in mind?
> >>>
> >>> It would not be opaque to the implementer.  But it would avoid
> >>> introducing new commands and events, instead we have a unified mechanism
> >>> to signal completion.
> >>
> >> Ok.  We have a async mechanism today: .mhandler.cmd_async = ...
> >>
> >> I know it has its problems like no cancelation and is deprecated and
> >> all.  But still: how about using it as interim until QAPI-based async
> >> monitor support is ready?  We could unbreak qxl screendumps without
> >> having to introduce a new (but temporary!) screendump_async command +
> >> completion event.
> > 
> > Actually, I'm not sure this doesn't reintroduce the original problem
> > (which I haven't been able to reproduce):
> > 
> > client: screenshot <-> client libvirt <-> host libvirt
> > 
> > host libvirt (screendump) <-> qemu monitor -> <- spice server <-> client
> 
> Hmm?  spice client can ask for a screendump via libvirt?
> 
> /me looks completely puzzled.

No, ok, bad attempt.

 "client" is a libvirt and spice client.
 client as a libvirt client asks for screenshot.
 libvirt (client side) asks from libvirt (host side)
 libvirt (host side) issues a screendump monitor command (the new
   internal async version)
 qxl_screen_dump asks spice server to render
 spice server waits for pipe to spice client to empty (lower then 50)

 but spice client, which is just "client", is blocking on completion of
 the screendump command.

 I have two problems with my own explanation:
  1. I didn't manage to reproduce it myself, and nor has Marc Andre (who
  first reported this problem) yesterday.
  2. I remember that the async monitor command I sent in October did fix
  the problem, so something with my description is wrong.

> 
> cheers,
>   Gerd
> 
> 

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

[Luiz Capitulino]

On Tue, 06 Mar 2012 08:36:34 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> >> How would the parallel execution facility be opaque to the implementer?
> >> screendump returns, screendump_async needs to pass a closure. You can
> >> automatically generate any amount of code, but you can only have a
> >> single function implementation with longjmp/coroutine, or having a
> >> saparate thread per command but that would mean taking locks for
> >> anything not trivial, which avoids the no-change-to-implementation. Is
> >> this what you have in mind?
> > 
> > It would not be opaque to the implementer.  But it would avoid
> > introducing new commands and events, instead we have a unified mechanism
> > to signal completion.
> 
> Ok.  We have a async mechanism today: .mhandler.cmd_async = ...
> 
> I know it has its problems like no cancelation and is deprecated and
> all.  But still: how about using it as interim until QAPI-based async
> monitor support is ready?  We could unbreak qxl screendumps without
> having to introduce a new (but temporary!) screendump_async command +
> completion event.

There are a few problems here, but the blocking one is that a command
can't go from sync to async. This is an incompatible change.

If you mind adding the temporary command and if this issue is so rare
that none can reproduce it, then I'd suggest to wait for 1.2.

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

[Alon Levy]

On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote:
> On Tue, 06 Mar 2012 08:36:34 +0100
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> >   Hi,
> > 
> > >> How would the parallel execution facility be opaque to the implementer?
> > >> screendump returns, screendump_async needs to pass a closure. You can
> > >> automatically generate any amount of code, but you can only have a
> > >> single function implementation with longjmp/coroutine, or having a
> > >> saparate thread per command but that would mean taking locks for
> > >> anything not trivial, which avoids the no-change-to-implementation. Is
> > >> this what you have in mind?
> > > 
> > > It would not be opaque to the implementer.  But it would avoid
> > > introducing new commands and events, instead we have a unified mechanism
> > > to signal completion.
> > 
> > Ok.  We have a async mechanism today: .mhandler.cmd_async = ...
> > 
> > I know it has its problems like no cancelation and is deprecated and
> > all.  But still: how about using it as interim until QAPI-based async
> > monitor support is ready?  We could unbreak qxl screendumps without
> > having to introduce a new (but temporary!) screendump_async command +
> > completion event.
> 
> There are a few problems here, but the blocking one is that a command
> can't go from sync to async. This is an incompatible change.
> 
> If you mind adding the temporary command and if this issue is so rare
> that none can reproduce it, then I'd suggest to wait for 1.2.
> 

There are two options really:
 1. revert the patches that changed qxl screendump to save the ppm
 before (possibly) updating the framebuffer.
 2. introduce a new command that is really async

 The third option, what Gerd proposes, doesn't break the blocking chain
 going from the A, the dual purpose spice client and libvirt client,
 through libvirt, qemu, spice and back to A.

If no one can reproduce the block then it would seem 1 makes sense.

But 1 serves a second purpose, which is to allow the guest to do io
exits while the server thread is processing the area update request
issued for the screendump. So it makes sense regardless.

=> Option 2.

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

[Anthony Liguori]

On 03/06/2012 07:16 AM, Alon Levy wrote:
> On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote:
>> On Tue, 06 Mar 2012 08:36:34 +0100
>> Gerd Hoffmann<kraxel@redhat.com>  wrote:
>>
>>>    Hi,
>>>
>>>>> How would the parallel execution facility be opaque to the implementer?
>>>>> screendump returns, screendump_async needs to pass a closure. You can
>>>>> automatically generate any amount of code, but you can only have a
>>>>> single function implementation with longjmp/coroutine, or having a
>>>>> saparate thread per command but that would mean taking locks for
>>>>> anything not trivial, which avoids the no-change-to-implementation. Is
>>>>> this what you have in mind?
>>>>
>>>> It would not be opaque to the implementer.  But it would avoid
>>>> introducing new commands and events, instead we have a unified mechanism
>>>> to signal completion.
>>>
>>> Ok.  We have a async mechanism today: .mhandler.cmd_async = ...
>>>
>>> I know it has its problems like no cancelation and is deprecated and
>>> all.  But still: how about using it as interim until QAPI-based async
>>> monitor support is ready?  We could unbreak qxl screendumps without
>>> having to introduce a new (but temporary!) screendump_async command +
>>> completion event.
>>
>> There are a few problems here, but the blocking one is that a command
>> can't go from sync to async. This is an incompatible change.
>>
>> If you mind adding the temporary command and if this issue is so rare
>> that none can reproduce it, then I'd suggest to wait for 1.2.
>>
>
> There are two options really:
>   1. revert the patches that changed qxl screendump to save the ppm
>   before (possibly) updating the framebuffer.
>   2. introduce a new command that is really async
>
>   The third option, what Gerd proposes, doesn't break the blocking chain
>   going from the A, the dual purpose spice client and libvirt client,
>   through libvirt, qemu, spice and back to A.
>
> If no one can reproduce the block then it would seem 1 makes sense.

So let's start with a reproducible test case that demonstrates the problem 
before we start introducing new commands then if there's doubt about the nature 
of the problem.

Regards,

Anthony Liguori

>
> But 1 serves a second purpose, which is to allow the guest to do io
> exits while the server thread is processing the area update request
> issued for the screendump. So it makes sense regardless.
>
> =>  Option 2.

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

[Luiz Capitulino]

On Tue, 06 Mar 2012 07:51:29 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 03/06/2012 07:16 AM, Alon Levy wrote:
> > On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote:
> >> On Tue, 06 Mar 2012 08:36:34 +0100
> >> Gerd Hoffmann<kraxel@redhat.com>  wrote:
> >>
> >>>    Hi,
> >>>
> >>>>> How would the parallel execution facility be opaque to the implementer?
> >>>>> screendump returns, screendump_async needs to pass a closure. You can
> >>>>> automatically generate any amount of code, but you can only have a
> >>>>> single function implementation with longjmp/coroutine, or having a
> >>>>> saparate thread per command but that would mean taking locks for
> >>>>> anything not trivial, which avoids the no-change-to-implementation. Is
> >>>>> this what you have in mind?
> >>>>
> >>>> It would not be opaque to the implementer.  But it would avoid
> >>>> introducing new commands and events, instead we have a unified mechanism
> >>>> to signal completion.
> >>>
> >>> Ok.  We have a async mechanism today: .mhandler.cmd_async = ...
> >>>
> >>> I know it has its problems like no cancelation and is deprecated and
> >>> all.  But still: how about using it as interim until QAPI-based async
> >>> monitor support is ready?  We could unbreak qxl screendumps without
> >>> having to introduce a new (but temporary!) screendump_async command +
> >>> completion event.
> >>
> >> There are a few problems here, but the blocking one is that a command
> >> can't go from sync to async. This is an incompatible change.
> >>
> >> If you mind adding the temporary command and if this issue is so rare
> >> that none can reproduce it, then I'd suggest to wait for 1.2.
> >>
> >
> > There are two options really:
> >   1. revert the patches that changed qxl screendump to save the ppm
> >   before (possibly) updating the framebuffer.
> >   2. introduce a new command that is really async
> >
> >   The third option, what Gerd proposes, doesn't break the blocking chain
> >   going from the A, the dual purpose spice client and libvirt client,
> >   through libvirt, qemu, spice and back to A.
> >
> > If no one can reproduce the block then it would seem 1 makes sense.
> 
> So let's start with a reproducible test case that demonstrates the problem 
> before we start introducing new commands then if there's doubt about the nature 
> of the problem.

Completely agree, I was going to suggest that too.

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

[Alon Levy]

On Tue, Mar 06, 2012 at 10:53:42AM -0300, Luiz Capitulino wrote:
> On Tue, 06 Mar 2012 07:51:29 -0600
> Anthony Liguori <anthony@codemonkey.ws> wrote:
> 
> > On 03/06/2012 07:16 AM, Alon Levy wrote:
> > > On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote:
> > >> On Tue, 06 Mar 2012 08:36:34 +0100
> > >> Gerd Hoffmann<kraxel@redhat.com>  wrote:
> > >>
> > >>>    Hi,
> > >>>
> > >>>>> How would the parallel execution facility be opaque to the implementer?
> > >>>>> screendump returns, screendump_async needs to pass a closure. You can
> > >>>>> automatically generate any amount of code, but you can only have a
> > >>>>> single function implementation with longjmp/coroutine, or having a
> > >>>>> saparate thread per command but that would mean taking locks for
> > >>>>> anything not trivial, which avoids the no-change-to-implementation. Is
> > >>>>> this what you have in mind?
> > >>>>
> > >>>> It would not be opaque to the implementer.  But it would avoid
> > >>>> introducing new commands and events, instead we have a unified mechanism
> > >>>> to signal completion.
> > >>>
> > >>> Ok.  We have a async mechanism today: .mhandler.cmd_async = ...
> > >>>
> > >>> I know it has its problems like no cancelation and is deprecated and
> > >>> all.  But still: how about using it as interim until QAPI-based async
> > >>> monitor support is ready?  We could unbreak qxl screendumps without
> > >>> having to introduce a new (but temporary!) screendump_async command +
> > >>> completion event.
> > >>
> > >> There are a few problems here, but the blocking one is that a command
> > >> can't go from sync to async. This is an incompatible change.
> > >>
> > >> If you mind adding the temporary command and if this issue is so rare
> > >> that none can reproduce it, then I'd suggest to wait for 1.2.
> > >>
> > >
> > > There are two options really:
> > >   1. revert the patches that changed qxl screendump to save the ppm
> > >   before (possibly) updating the framebuffer.
> > >   2. introduce a new command that is really async
> > >
> > >   The third option, what Gerd proposes, doesn't break the blocking chain
> > >   going from the A, the dual purpose spice client and libvirt client,
> > >   through libvirt, qemu, spice and back to A.
> > >
> > > If no one can reproduce the block then it would seem 1 makes sense.
> > 
> > So let's start with a reproducible test case that demonstrates the problem 
> > before we start introducing new commands then if there's doubt about the nature 
> > of the problem.
> 
> Completely agree, I was going to suggest that too.
> 

So cutting off a part of the email is a good way to win arguments? cool
trick. I agree a reproducer is a good idea, but as I mentioned in the
cut part, doing the update area without keeping the monitor waiting
improves performance of the guest by letting it do io exits. Why is that
a bad thing?

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

[Anthony Liguori]

On 03/06/2012 08:23 AM, Alon Levy wrote:
> On Tue, Mar 06, 2012 at 10:53:42AM -0300, Luiz Capitulino wrote:
>
> So cutting off a part of the email is a good way to win arguments? cool
> trick.

It doesn't work as well if you acknowledge that was the motivation ;-) (j/k)

> I agree a reproducer is a good idea, but as I mentioned in the
> cut part, doing the update area without keeping the monitor waiting
> improves performance of the guest by letting it do io exits. Why is that
> a bad thing?

You say, "improves performance", but can you quantify that?

And why is screendump on the performance critical path in the first place? 
Having a small test case that demonstrates the problem (be it functional or 
performance) will help ground this discussion in concrete terms.

I think we need to step back and reexamine what the problem we're trying to 
solve is.

Regards,

Anthony Liguori

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

[Alon Levy]

On Tue, Mar 06, 2012 at 07:51:29AM -0600, Anthony Liguori wrote:
> On 03/06/2012 07:16 AM, Alon Levy wrote:
> >On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote:
> >>On Tue, 06 Mar 2012 08:36:34 +0100
> >>Gerd Hoffmann<kraxel@redhat.com>  wrote:
> >>
> >>>   Hi,
> >>>
> >>>>>How would the parallel execution facility be opaque to the implementer?
> >>>>>screendump returns, screendump_async needs to pass a closure. You can
> >>>>>automatically generate any amount of code, but you can only have a
> >>>>>single function implementation with longjmp/coroutine, or having a
> >>>>>saparate thread per command but that would mean taking locks for
> >>>>>anything not trivial, which avoids the no-change-to-implementation. Is
> >>>>>this what you have in mind?
> >>>>
> >>>>It would not be opaque to the implementer.  But it would avoid
> >>>>introducing new commands and events, instead we have a unified mechanism
> >>>>to signal completion.
> >>>
> >>>Ok.  We have a async mechanism today: .mhandler.cmd_async = ...
> >>>
> >>>I know it has its problems like no cancelation and is deprecated and
> >>>all.  But still: how about using it as interim until QAPI-based async
> >>>monitor support is ready?  We could unbreak qxl screendumps without
> >>>having to introduce a new (but temporary!) screendump_async command +
> >>>completion event.
> >>
> >>There are a few problems here, but the blocking one is that a command
> >>can't go from sync to async. This is an incompatible change.
> >>
> >>If you mind adding the temporary command and if this issue is so rare
> >>that none can reproduce it, then I'd suggest to wait for 1.2.
> >>
> >
> >There are two options really:
> >  1. revert the patches that changed qxl screendump to save the ppm
> >  before (possibly) updating the framebuffer.
> >  2. introduce a new command that is really async
> >
> >  The third option, what Gerd proposes, doesn't break the blocking chain
> >  going from the A, the dual purpose spice client and libvirt client,
> >  through libvirt, qemu, spice and back to A.
> >
> >If no one can reproduce the block then it would seem 1 makes sense.
> 
> So let's start with a reproducible test case that demonstrates the
> problem before we start introducing new commands then if there's
> doubt about the nature of the problem.

s/the problem/different problem/:

 NB: To get this hang I had to disable update_area's from the guest, just
     because otherwise there is a very small window between suspending the
     client and qemu waiting on the same stack trace below issued from the
     guest update_area io out, instead of from the screendump monitor command.

 spice client, qemu, libvirt, virsh screenshot.

 libvirt controlled qemu with qxl device and spice connection.
 qemu ... -vga qxl -spice disable-ticketing,port=5900...
 spicec -h localhost -p 5900
 [boot until qxl driver is loaded, and guest is doing something (Running
 toms 2d benchmark works), suspend spicec]
 virsh screenshot <vm-name> /tmp/dump.ppm

virsh will hang at:
 #1 virCondWait
 #2 qemuMonitorSend
 #3 qemuMonitorJSONCommandWithFd
 #4 qemuMonitorJSONCommand
 #5 qemuMonitorJSONScreendump
 
qemu is hung at:
 main thread:
  #0 read
  #1 read_safe
  #2 dispatcher_send_message
  #3 red_dispatcher_update_area
  #4 qxl_worker_update_area
  #5 qxl_spice_update_area
  #6 qxl_render_update
  #7 qxl_hw_screen_dump
  #8 vga_hw_screen_dump

 spice worker thread:
  #0 nanosleep
  #1 usleep
  #2 flush_display_commands
  #3 handle_dev_update
  #4 dispatcher_handle_single_read
  #5 dispatcher_handle_recv_read
  #6 handle_dev_input
  #7 red_worker_main
  #8 start_thread
  #9 clone

So the two problems here are:
 virsh screenshot is hung if the client is hung / not responsive
 qemu guest thread will hang the first time it does a vmexit if the
 client is hung / not responsive.

Like Gerd mentioned previously, this issue should be addressed in spice
but is hard to correct there, hence the async io patches that landed
long ago, and hence the async update_area done for screendump as well
that we are discussing reverting.

So if I have you convinced it should not be reverted, we are back to
needing a asynchronous command for screendump. And Luiz said changing
the sync command to async is not an option (though I didn't understand
why), so we are left with a new, temporary, command.

Any objections?
> 
> Regards,
> 
> Anthony Liguori
> 
> >
> >But 1 serves a second purpose, which is to allow the guest to do io
> >exits while the server thread is processing the area update request
> >issued for the screendump. So it makes sense regardless.
> >
> >=>  Option 2.
> 
> 

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

[Alon Levy]

On Tue, Mar 06, 2012 at 05:56:49PM +0200, Alon Levy wrote:
> On Tue, Mar 06, 2012 at 07:51:29AM -0600, Anthony Liguori wrote:
> > On 03/06/2012 07:16 AM, Alon Levy wrote:
> > >On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote:
> > >>On Tue, 06 Mar 2012 08:36:34 +0100
> > >>Gerd Hoffmann<kraxel@redhat.com>  wrote:
> > >>
> > >>>   Hi,
> > >>>
> > >>>>>How would the parallel execution facility be opaque to the implementer?
> > >>>>>screendump returns, screendump_async needs to pass a closure. You can
> > >>>>>automatically generate any amount of code, but you can only have a
> > >>>>>single function implementation with longjmp/coroutine, or having a
> > >>>>>saparate thread per command but that would mean taking locks for
> > >>>>>anything not trivial, which avoids the no-change-to-implementation. Is
> > >>>>>this what you have in mind?
> > >>>>
> > >>>>It would not be opaque to the implementer.  But it would avoid
> > >>>>introducing new commands and events, instead we have a unified mechanism
> > >>>>to signal completion.
> > >>>
> > >>>Ok.  We have a async mechanism today: .mhandler.cmd_async = ...
> > >>>
> > >>>I know it has its problems like no cancelation and is deprecated and
> > >>>all.  But still: how about using it as interim until QAPI-based async
> > >>>monitor support is ready?  We could unbreak qxl screendumps without
> > >>>having to introduce a new (but temporary!) screendump_async command +
> > >>>completion event.
> > >>
> > >>There are a few problems here, but the blocking one is that a command
> > >>can't go from sync to async. This is an incompatible change.
> > >>
> > >>If you mind adding the temporary command and if this issue is so rare
> > >>that none can reproduce it, then I'd suggest to wait for 1.2.
> > >>
> > >
> > >There are two options really:
> > >  1. revert the patches that changed qxl screendump to save the ppm
> > >  before (possibly) updating the framebuffer.
> > >  2. introduce a new command that is really async
> > >
> > >  The third option, what Gerd proposes, doesn't break the blocking chain
> > >  going from the A, the dual purpose spice client and libvirt client,
> > >  through libvirt, qemu, spice and back to A.
> > >
> > >If no one can reproduce the block then it would seem 1 makes sense.
> > 
> > So let's start with a reproducible test case that demonstrates the
> > problem before we start introducing new commands then if there's
> > doubt about the nature of the problem.
> 
> s/the problem/different problem/:
> 
>  NB: To get this hang I had to disable update_area's from the guest, just
>      because otherwise there is a very small window between suspending the
>      client and qemu waiting on the same stack trace below issued from the
>      guest update_area io out, instead of from the screendump monitor command.

Explanation to the note: 'disable update_area' - I meant disable the
implementation, i.e. change the ioport_write switch to return without
actually rendering anything. It causes artifacts of course, but if you
know how the vm looks by heart it's good enough to reproduce. Nothing
changed in the guest.

> 
>  spice client, qemu, libvirt, virsh screenshot.
> 
>  libvirt controlled qemu with qxl device and spice connection.
>  qemu ... -vga qxl -spice disable-ticketing,port=5900...
>  spicec -h localhost -p 5900
>  [boot until qxl driver is loaded, and guest is doing something (Running
>  toms 2d benchmark works), suspend spicec]
>  virsh screenshot <vm-name> /tmp/dump.ppm
> 
> virsh will hang at:
>  #1 virCondWait
>  #2 qemuMonitorSend
>  #3 qemuMonitorJSONCommandWithFd
>  #4 qemuMonitorJSONCommand
>  #5 qemuMonitorJSONScreendump
>  
> qemu is hung at:
>  main thread:
>   #0 read
>   #1 read_safe
>   #2 dispatcher_send_message
>   #3 red_dispatcher_update_area
>   #4 qxl_worker_update_area
>   #5 qxl_spice_update_area
>   #6 qxl_render_update
>   #7 qxl_hw_screen_dump
>   #8 vga_hw_screen_dump
> 
>  spice worker thread:
>   #0 nanosleep
>   #1 usleep
>   #2 flush_display_commands
>   #3 handle_dev_update
>   #4 dispatcher_handle_single_read
>   #5 dispatcher_handle_recv_read
>   #6 handle_dev_input
>   #7 red_worker_main
>   #8 start_thread
>   #9 clone
> 
> So the two problems here are:
>  virsh screenshot is hung if the client is hung / not responsive
>  qemu guest thread will hang the first time it does a vmexit if the
>  client is hung / not responsive.
> 
> Like Gerd mentioned previously, this issue should be addressed in spice
> but is hard to correct there, hence the async io patches that landed
> long ago, and hence the async update_area done for screendump as well
> that we are discussing reverting.
> 
> So if I have you convinced it should not be reverted, we are back to
> needing a asynchronous command for screendump. And Luiz said changing
> the sync command to async is not an option (though I didn't understand
> why), so we are left with a new, temporary, command.
> 
> Any objections?
> > 
> > Regards,
> > 
> > Anthony Liguori
> > 
> > >
> > >But 1 serves a second purpose, which is to allow the guest to do io
> > >exits while the server thread is processing the area update request
> > >issued for the screendump. So it makes sense regardless.
> > >
> > >=>  Option 2.
> > 
> > 
> 

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

[Anthony Liguori]

On 03/06/2012 09:56 AM, Alon Levy wrote:
> On Tue, Mar 06, 2012 at 07:51:29AM -0600, Anthony Liguori wrote:
>> On 03/06/2012 07:16 AM, Alon Levy wrote:
>>> On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote:
>>>> On Tue, 06 Mar 2012 08:36:34 +0100
>>>> Gerd Hoffmann<kraxel@redhat.com>   wrote:
>>>>
>>>>>    Hi,
>>>>>
>>>>>>> How would the parallel execution facility be opaque to the implementer?
>>>>>>> screendump returns, screendump_async needs to pass a closure. You can
>>>>>>> automatically generate any amount of code, but you can only have a
>>>>>>> single function implementation with longjmp/coroutine, or having a
>>>>>>> saparate thread per command but that would mean taking locks for
>>>>>>> anything not trivial, which avoids the no-change-to-implementation. Is
>>>>>>> this what you have in mind?
>>>>>>
>>>>>> It would not be opaque to the implementer.  But it would avoid
>>>>>> introducing new commands and events, instead we have a unified mechanism
>>>>>> to signal completion.
>>>>>
>>>>> Ok.  We have a async mechanism today: .mhandler.cmd_async = ...
>>>>>
>>>>> I know it has its problems like no cancelation and is deprecated and
>>>>> all.  But still: how about using it as interim until QAPI-based async
>>>>> monitor support is ready?  We could unbreak qxl screendumps without
>>>>> having to introduce a new (but temporary!) screendump_async command +
>>>>> completion event.
>>>>
>>>> There are a few problems here, but the blocking one is that a command
>>>> can't go from sync to async. This is an incompatible change.
>>>>
>>>> If you mind adding the temporary command and if this issue is so rare
>>>> that none can reproduce it, then I'd suggest to wait for 1.2.
>>>>
>>>
>>> There are two options really:
>>>   1. revert the patches that changed qxl screendump to save the ppm
>>>   before (possibly) updating the framebuffer.
>>>   2. introduce a new command that is really async
>>>
>>>   The third option, what Gerd proposes, doesn't break the blocking chain
>>>   going from the A, the dual purpose spice client and libvirt client,
>>>   through libvirt, qemu, spice and back to A.
>>>
>>> If no one can reproduce the block then it would seem 1 makes sense.
>>
>> So let's start with a reproducible test case that demonstrates the
>> problem before we start introducing new commands then if there's
>> doubt about the nature of the problem.
>
> s/the problem/different problem/:
>
>   NB: To get this hang I had to disable update_area's from the guest, just
>       because otherwise there is a very small window between suspending the
>       client and qemu waiting on the same stack trace below issued from the
>       guest update_area io out, instead of from the screendump monitor command.
>
>   spice client, qemu, libvirt, virsh screenshot.
>
>   libvirt controlled qemu with qxl device and spice connection.
>   qemu ... -vga qxl -spice disable-ticketing,port=5900...
>   spicec -h localhost -p 5900
>   [boot until qxl driver is loaded, and guest is doing something (Running
>   toms 2d benchmark works), suspend spicec]
>   virsh screenshot<vm-name>  /tmp/dump.ppm
>
> virsh will hang at:
>   #1 virCondWait
>   #2 qemuMonitorSend
>   #3 qemuMonitorJSONCommandWithFd
>   #4 qemuMonitorJSONCommand
>   #5 qemuMonitorJSONScreendump
>
> qemu is hung at:
>   main thread:
>    #0 read

Is qxl doing a blocking read?  If so, that's a bug in qxl.  You cannot do a 
blocking read while holding qemu_mutex.

Regards,

Anthony Liguori

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

[Alon Levy]

On Tue, Mar 06, 2012 at 10:16:42AM -0600, Anthony Liguori wrote:
> On 03/06/2012 09:56 AM, Alon Levy wrote:
> >On Tue, Mar 06, 2012 at 07:51:29AM -0600, Anthony Liguori wrote:
> >>On 03/06/2012 07:16 AM, Alon Levy wrote:
> >>>On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote:
> >>>>On Tue, 06 Mar 2012 08:36:34 +0100
> >>>>Gerd Hoffmann<kraxel@redhat.com>   wrote:
> >>>>
> >>>>>   Hi,
> >>>>>
> >>>>>>>How would the parallel execution facility be opaque to the implementer?
> >>>>>>>screendump returns, screendump_async needs to pass a closure. You can
> >>>>>>>automatically generate any amount of code, but you can only have a
> >>>>>>>single function implementation with longjmp/coroutine, or having a
> >>>>>>>saparate thread per command but that would mean taking locks for
> >>>>>>>anything not trivial, which avoids the no-change-to-implementation. Is
> >>>>>>>this what you have in mind?
> >>>>>>
> >>>>>>It would not be opaque to the implementer.  But it would avoid
> >>>>>>introducing new commands and events, instead we have a unified mechanism
> >>>>>>to signal completion.
> >>>>>
> >>>>>Ok.  We have a async mechanism today: .mhandler.cmd_async = ...
> >>>>>
> >>>>>I know it has its problems like no cancelation and is deprecated and
> >>>>>all.  But still: how about using it as interim until QAPI-based async
> >>>>>monitor support is ready?  We could unbreak qxl screendumps without
> >>>>>having to introduce a new (but temporary!) screendump_async command +
> >>>>>completion event.
> >>>>
> >>>>There are a few problems here, but the blocking one is that a command
> >>>>can't go from sync to async. This is an incompatible change.
> >>>>
> >>>>If you mind adding the temporary command and if this issue is so rare
> >>>>that none can reproduce it, then I'd suggest to wait for 1.2.
> >>>>
> >>>
> >>>There are two options really:
> >>>  1. revert the patches that changed qxl screendump to save the ppm
> >>>  before (possibly) updating the framebuffer.
> >>>  2. introduce a new command that is really async
> >>>
> >>>  The third option, what Gerd proposes, doesn't break the blocking chain
> >>>  going from the A, the dual purpose spice client and libvirt client,
> >>>  through libvirt, qemu, spice and back to A.
> >>>
> >>>If no one can reproduce the block then it would seem 1 makes sense.
> >>
> >>So let's start with a reproducible test case that demonstrates the
> >>problem before we start introducing new commands then if there's
> >>doubt about the nature of the problem.
> >
> >s/the problem/different problem/:
> >
> >  NB: To get this hang I had to disable update_area's from the guest, just
> >      because otherwise there is a very small window between suspending the
> >      client and qemu waiting on the same stack trace below issued from the
> >      guest update_area io out, instead of from the screendump monitor command.
> >
> >  spice client, qemu, libvirt, virsh screenshot.
> >
> >  libvirt controlled qemu with qxl device and spice connection.
> >  qemu ... -vga qxl -spice disable-ticketing,port=5900...
> >  spicec -h localhost -p 5900
> >  [boot until qxl driver is loaded, and guest is doing something (Running
> >  toms 2d benchmark works), suspend spicec]
> >  virsh screenshot<vm-name>  /tmp/dump.ppm
> >
> >virsh will hang at:
> >  #1 virCondWait
> >  #2 qemuMonitorSend
> >  #3 qemuMonitorJSONCommandWithFd
> >  #4 qemuMonitorJSONCommand
> >  #5 qemuMonitorJSONScreendump
> >
> >qemu is hung at:
> >  main thread:
> >   #0 read
> 
> Is qxl doing a blocking read?  If so, that's a bug in qxl.  You
> cannot do a blocking read while holding qemu_mutex.

What are you saying, that that read should be fixed? then I guess it's
good that patches fixing it have landed. That stack was prior to "qxl:
make qxl_render_update async", 81fb6f1504fb9ef71f2382f44af34756668296e8
.

> 
> Regards,
> 
> Anthony Liguori
> 

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

[Anthony Liguori]

On 03/06/2012 10:26 AM, Alon Levy wrote:
> On Tue, Mar 06, 2012 at 10:16:42AM -0600, Anthony Liguori wrote:
>> On 03/06/2012 09:56 AM, Alon Levy wrote:
>>> On Tue, Mar 06, 2012 at 07:51:29AM -0600, Anthony Liguori wrote:
>>>> On 03/06/2012 07:16 AM, Alon Levy wrote:
>>>>> On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote:
>>>>>> On Tue, 06 Mar 2012 08:36:34 +0100
>>>>>> Gerd Hoffmann<kraxel@redhat.com>    wrote:
>>>>>>
>>>>>>>    Hi,
>>>>>>>
>>>>>>>>> How would the parallel execution facility be opaque to the implementer?
>>>>>>>>> screendump returns, screendump_async needs to pass a closure. You can
>>>>>>>>> automatically generate any amount of code, but you can only have a
>>>>>>>>> single function implementation with longjmp/coroutine, or having a
>>>>>>>>> saparate thread per command but that would mean taking locks for
>>>>>>>>> anything not trivial, which avoids the no-change-to-implementation. Is
>>>>>>>>> this what you have in mind?
>>>>>>>>
>>>>>>>> It would not be opaque to the implementer.  But it would avoid
>>>>>>>> introducing new commands and events, instead we have a unified mechanism
>>>>>>>> to signal completion.
>>>>>>>
>>>>>>> Ok.  We have a async mechanism today: .mhandler.cmd_async = ...
>>>>>>>
>>>>>>> I know it has its problems like no cancelation and is deprecated and
>>>>>>> all.  But still: how about using it as interim until QAPI-based async
>>>>>>> monitor support is ready?  We could unbreak qxl screendumps without
>>>>>>> having to introduce a new (but temporary!) screendump_async command +
>>>>>>> completion event.
>>>>>>
>>>>>> There are a few problems here, but the blocking one is that a command
>>>>>> can't go from sync to async. This is an incompatible change.
>>>>>>
>>>>>> If you mind adding the temporary command and if this issue is so rare
>>>>>> that none can reproduce it, then I'd suggest to wait for 1.2.
>>>>>>
>>>>>
>>>>> There are two options really:
>>>>>   1. revert the patches that changed qxl screendump to save the ppm
>>>>>   before (possibly) updating the framebuffer.
>>>>>   2. introduce a new command that is really async
>>>>>
>>>>>   The third option, what Gerd proposes, doesn't break the blocking chain
>>>>>   going from the A, the dual purpose spice client and libvirt client,
>>>>>   through libvirt, qemu, spice and back to A.
>>>>>
>>>>> If no one can reproduce the block then it would seem 1 makes sense.
>>>>
>>>> So let's start with a reproducible test case that demonstrates the
>>>> problem before we start introducing new commands then if there's
>>>> doubt about the nature of the problem.
>>>
>>> s/the problem/different problem/:
>>>
>>>   NB: To get this hang I had to disable update_area's from the guest, just
>>>       because otherwise there is a very small window between suspending the
>>>       client and qemu waiting on the same stack trace below issued from the
>>>       guest update_area io out, instead of from the screendump monitor command.
>>>
>>>   spice client, qemu, libvirt, virsh screenshot.
>>>
>>>   libvirt controlled qemu with qxl device and spice connection.
>>>   qemu ... -vga qxl -spice disable-ticketing,port=5900...
>>>   spicec -h localhost -p 5900
>>>   [boot until qxl driver is loaded, and guest is doing something (Running
>>>   toms 2d benchmark works), suspend spicec]
>>>   virsh screenshot<vm-name>   /tmp/dump.ppm
>>>
>>> virsh will hang at:
>>>   #1 virCondWait
>>>   #2 qemuMonitorSend
>>>   #3 qemuMonitorJSONCommandWithFd
>>>   #4 qemuMonitorJSONCommand
>>>   #5 qemuMonitorJSONScreendump
>>>
>>> qemu is hung at:
>>>   main thread:
>>>    #0 read
>>
>> Is qxl doing a blocking read?  If so, that's a bug in qxl.  You
>> cannot do a blocking read while holding qemu_mutex.
>
> What are you saying, that that read should be fixed? then I guess it's
> good that patches fixing it have landed. That stack was prior to "qxl:
> make qxl_render_update async", 81fb6f1504fb9ef71f2382f44af34756668296e8

I'm sorry, I'm thoroughly confused by this thread.

Can you start a new thread with a clear explanation of the problem you're trying 
to solve or at least point me to an existing note?

Regards,

Anthony Liguori

> .
>
>>
>> Regards,
>>
>> Anthony Liguori
>>

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

[Gerd Hoffmann]

  Hi,

>> qemu is hung at:
>>   main thread:
>>    #0 read
> 
> Is qxl doing a blocking read?  If so, that's a bug in qxl.

It used to do that, with the latest spice pull it is gone[1].  And this
fix is exactly what broke screendump.

Spice does lazy rendering on the server side to avoid burning cpu for
nothing, because in the common case there is nothing to render.  Thats
why there is no up-to-date displaysurface we can just write out when the
screendump command comes in.

What qxl screendump used to do is this:

   (1) ask spice server to render the screen
   (2) blocking read, waiting for spice spice server finish    <- BUG
   (3) write out screendump

What qxl screendump does now is:

   (1) ask spice server to render the screen
   (2) write out screendump from outdated displaysurface       <- BUG
   (3) spice server finished, callback comes in, arm BH
   (4) bottom half handler updates displaysurface

What we like to do instead is this:

   (1) ask spice server to render the screen
   (2) spice server finished, callback comes in, arm BH
   (3) bottom half handler updates displaysurface
       and writes the screendump
   (4) signal screendump monitor command is finished.

See our problem now?

cheers,
  Gerd

[1] With the exception of guest with old qxl drivers which is pretty
    much unfixable due to way the old guest <-> qxl interface is
    designed.