[Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Gerd Hoffmann]

  Hi,

This patch qom-ifies the QemuConsoles and adds an new, optional
parameter to the screendump command to specify the device you
want dump from.

Question for the QOM guys: where in the tree should the QemuConsoles
show up?  I can't find anything in the tree doing this.  I've placed
them below /backend.  See patch #2.  Ok?  Better suggestions?

Question for the libvirt guys:  Is it ok for libvirt to just extend the
existing screendump command?  Can libvirt figure there is a new
(optional) parameter?  See patch #5.

Gerd Hoffmann (5):
  console: qom-ify QemuConsole
  console: Hook QemuConsoles into qom tree
  console: add device link to QemuConsoles
  console: add qemu_console_lookup_by_device
  console: extend screendump monitor cmd

 hmp-commands.hx              |    6 ++--
 hmp.c                        |    3 +-
 hw/arm/musicpal.c            |    2 +-
 hw/display/blizzard.c        |    2 +-
 hw/display/cirrus_vga.c      |    4 +--
 hw/display/exynos4210_fimd.c |    2 +-
 hw/display/g364fb.c          |    2 +-
 hw/display/jazz_led.c        |    2 +-
 hw/display/milkymist-vgafb.c |    2 +-
 hw/display/omap_lcdc.c       |    2 +-
 hw/display/pl110.c           |    2 +-
 hw/display/pxa2xx_lcd.c      |    2 +-
 hw/display/qxl.c             |    4 +--
 hw/display/sm501.c           |    2 +-
 hw/display/ssd0303.c         |    2 +-
 hw/display/ssd0323.c         |    2 +-
 hw/display/tc6393xb.c        |    2 +-
 hw/display/tcx.c             |    4 +--
 hw/display/vga-isa-mm.c      |    2 +-
 hw/display/vga-isa.c         |    2 +-
 hw/display/vga-pci.c         |    2 +-
 hw/display/vmware_vga.c      |    7 ++--
 hw/unicore32/puv3.c          |    2 +-
 include/ui/console.h         |   19 ++++++++++-
 qapi-schema.json             |    4 ++-
 qmp-commands.hx              |    3 +-
 ui/console.c                 |   76 ++++++++++++++++++++++++++++++++++++++++--
 27 files changed, 128 insertions(+), 36 deletions(-)

-- 
1.7.9.7

[Qemu-devel] [PATCH 1/5] console: qom-ify QemuConsole

[Gerd Hoffmann]

Just the minimal bits to turn QemuConsoles into Objects.

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

diff --git a/include/ui/console.h b/include/ui/console.h
index e591d74..c8a274d 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -2,6 +2,7 @@
 #define CONSOLE_H
 
 #include "ui/qemu-pixman.h"
+#include "qom/object.h"
 #include "qapi/qmp/qdict.h"
 #include "qemu/notify.h"
 #include "monitor/monitor.h"
@@ -106,6 +107,20 @@ void kbd_put_keysym(int keysym);
 
 /* consoles */
 
+#define TYPE_QEMU_CONSOLE "qemu-console"
+#define QEMU_CONSOLE(obj) \
+    OBJECT_CHECK(QemuConsole, (obj), TYPE_QEMU_CONSOLE)
+#define QEMU_CONSOLE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(QemuConsoleClass, (obj), TYPE_QEMU_CONSOLE)
+#define QEMU_CONSOLE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(QemuConsoleClass, (klass), TYPE_QEMU_CONSOLE)
+
+typedef struct QemuConsoleClass QemuConsoleClass;
+
+struct QemuConsoleClass {
+    ObjectClass parent_class;
+};
+
 #define QEMU_BIG_ENDIAN_FLAG    0x01
 #define QEMU_ALLOCATED_FLAG     0x02
 
diff --git a/ui/console.c b/ui/console.c
index 4f9219e..e9f3080 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -113,6 +113,8 @@ typedef enum {
 } console_type_t;
 
 struct QemuConsole {
+    Object parent;
+
     int index;
     console_type_t console_type;
     DisplayState *ds;
@@ -1197,12 +1199,14 @@ static void text_console_update(void *opaque, console_ch_t *chardata)
 
 static QemuConsole *new_console(DisplayState *ds, console_type_t console_type)
 {
+    Object *obj;
     QemuConsole *s;
     int i;
 
     if (nb_consoles >= MAX_CONSOLES)
         return NULL;
-    s = g_malloc0(sizeof(QemuConsole));
+    obj = object_new(TYPE_QEMU_CONSOLE);
+    s = QEMU_CONSOLE(obj);
     if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
         (console_type == GRAPHIC_CONSOLE))) {
         active_console = s;
@@ -1920,8 +1924,17 @@ static void qemu_chr_parse_vc(QemuOpts *opts, ChardevBackend *backend,
     }
 }
 
+static const TypeInfo qemu_console_info = {
+    .name = TYPE_QEMU_CONSOLE,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(QemuConsole),
+    .class_size = sizeof(QemuConsoleClass),
+};
+
+
 static void register_types(void)
 {
+    type_register_static(&qemu_console_info);
     register_char_driver_qapi("vc", CHARDEV_BACKEND_KIND_VC,
                               qemu_chr_parse_vc);
 }
-- 
1.7.9.7

[Qemu-devel] [PATCH 2/5] console: Hook QemuConsoles into qom tree

[Gerd Hoffmann]

Put them named "console[$index]" below "/backend", so you can
list & inspect them via QMP.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/console.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/ui/console.c b/ui/console.c
index e9f3080..b92bde3 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1541,6 +1541,8 @@ static DisplayState *get_alloc_displaystate(void)
  */
 DisplayState *init_displaystate(void)
 {
+    Error *local_err = NULL;
+    gchar *name;
     int i;
 
     if (!display_state) {
@@ -1552,6 +1554,14 @@ DisplayState *init_displaystate(void)
             consoles[i]->ds == NULL) {
             text_console_do_init(consoles[i]->chr, display_state);
         }
+
+        /* Hook up into the qom tree here (not in new_console()), once
+         * all QemuConsoles are created and the order / numbering
+         * doesn't change any more */
+        name = g_strdup_printf("console[%d]", i);
+        object_property_add_child(container_get(object_get_root(), "/backend"),
+                                  name, OBJECT(consoles[i]), &local_err);
+        g_free(name);
     }
 
     return display_state;
-- 
1.7.9.7

[Qemu-devel] [PATCH 3/5] console: add device link to QemuConsoles

[Gerd Hoffmann]

So it is possible to figure which qemu console displays which device.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/arm/musicpal.c            |    2 +-
 hw/display/blizzard.c        |    2 +-
 hw/display/cirrus_vga.c      |    4 ++--
 hw/display/exynos4210_fimd.c |    2 +-
 hw/display/g364fb.c          |    2 +-
 hw/display/jazz_led.c        |    2 +-
 hw/display/milkymist-vgafb.c |    2 +-
 hw/display/omap_lcdc.c       |    2 +-
 hw/display/pl110.c           |    2 +-
 hw/display/pxa2xx_lcd.c      |    2 +-
 hw/display/qxl.c             |    4 ++--
 hw/display/sm501.c           |    2 +-
 hw/display/ssd0303.c         |    2 +-
 hw/display/ssd0323.c         |    2 +-
 hw/display/tc6393xb.c        |    2 +-
 hw/display/tcx.c             |    4 ++--
 hw/display/vga-isa-mm.c      |    2 +-
 hw/display/vga-isa.c         |    2 +-
 hw/display/vga-pci.c         |    2 +-
 hw/display/vmware_vga.c      |    7 ++++---
 hw/unicore32/puv3.c          |    2 +-
 include/ui/console.h         |    3 ++-
 ui/console.c                 |   15 ++++++++++++++-
 23 files changed, 43 insertions(+), 28 deletions(-)

diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 31586c6..f7b6de5 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -616,7 +616,7 @@ static int musicpal_lcd_init(SysBusDevice *dev)
                           "musicpal-lcd", MP_LCD_SIZE);
     sysbus_init_mmio(dev, &s->iomem);
 
-    s->con = graphic_console_init(&musicpal_gfx_ops, s);
+    s->con = graphic_console_init(DEVICE(dev), &musicpal_gfx_ops, s);
     qemu_console_resize(s->con, 128*3, 64*3);
 
     qdev_init_gpio_in(&dev->qdev, musicpal_lcd_gpio_brigthness_in, 3);
diff --git a/hw/display/blizzard.c b/hw/display/blizzard.c
index 1ca3355..4a466c8 100644
--- a/hw/display/blizzard.c
+++ b/hw/display/blizzard.c
@@ -956,7 +956,7 @@ void *s1d13745_init(qemu_irq gpio_int)
 
     s->fb = g_malloc(0x180000);
 
-    s->con = graphic_console_init(&blizzard_ops, s);
+    s->con = graphic_console_init(NULL, &blizzard_ops, s);
     surface = qemu_console_surface(s->con);
 
     switch (surface_bits_per_pixel(surface)) {
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index db232af..6e47956 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -2910,7 +2910,7 @@ static int vga_initfn(ISADevice *dev)
     vga_common_init(s);
     cirrus_init_common(&d->cirrus_vga, CIRRUS_ID_CLGD5430, 0,
                        isa_address_space(dev), isa_address_space_io(dev));
-    s->con = graphic_console_init(s->hw_ops, s);
+    s->con = graphic_console_init(DEVICE(dev), s->hw_ops, s);
     rom_add_vga(VGABIOS_CIRRUS_FILENAME);
     /* XXX ISA-LFB support */
     /* FIXME not qdev yet */
@@ -2957,7 +2957,7 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
      vga_common_init(&s->vga);
      cirrus_init_common(s, device_id, 1, pci_address_space(dev),
                         pci_address_space_io(dev));
-     s->vga.con = graphic_console_init(s->vga.hw_ops, &s->vga);
+     s->vga.con = graphic_console_init(DEVICE(dev), s->vga.hw_ops, &s->vga);
 
      /* setup PCI */
 
diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c
index e6e7b27..6cb5016 100644
--- a/hw/display/exynos4210_fimd.c
+++ b/hw/display/exynos4210_fimd.c
@@ -1905,7 +1905,7 @@ static int exynos4210_fimd_init(SysBusDevice *dev)
     memory_region_init_io(&s->iomem, &exynos4210_fimd_mmio_ops, s,
             "exynos4210.fimd", FIMD_REGS_SIZE);
     sysbus_init_mmio(dev, &s->iomem);
-    s->console = graphic_console_init(&exynos4210_fimd_ops, s);
+    s->console = graphic_console_init(DEVICE(dev), &exynos4210_fimd_ops, s);
 
     return 0;
 }
diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
index 03810e9..2a4047e 100644
--- a/hw/display/g364fb.c
+++ b/hw/display/g364fb.c
@@ -484,7 +484,7 @@ static void g364fb_init(DeviceState *dev, G364State *s)
 {
     s->vram = g_malloc0(s->vram_size);
 
-    s->con = graphic_console_init(&g364fb_ops, s);
+    s->con = graphic_console_init(dev, &g364fb_ops, 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/display/jazz_led.c b/hw/display/jazz_led.c
index 6306d8c..52035fc 100644
--- a/hw/display/jazz_led.c
+++ b/hw/display/jazz_led.c
@@ -267,7 +267,7 @@ static int jazz_led_init(SysBusDevice *dev)
     memory_region_init_io(&s->iomem, &led_ops, s, "led", 1);
     sysbus_init_mmio(dev, &s->iomem);
 
-    s->con = graphic_console_init(&jazz_led_ops, s);
+    s->con = graphic_console_init(DEVICE(dev), &jazz_led_ops, s);
 
     return 0;
 }
diff --git a/hw/display/milkymist-vgafb.c b/hw/display/milkymist-vgafb.c
index 716997c..3828296 100644
--- a/hw/display/milkymist-vgafb.c
+++ b/hw/display/milkymist-vgafb.c
@@ -283,7 +283,7 @@ static int milkymist_vgafb_init(SysBusDevice *dev)
             "milkymist-vgafb", R_MAX * 4);
     sysbus_init_mmio(dev, &s->regs_region);
 
-    s->con = graphic_console_init(&vgafb_ops, s);
+    s->con = graphic_console_init(DEVICE(dev), &vgafb_ops, s);
 
     return 0;
 }
diff --git a/hw/display/omap_lcdc.c b/hw/display/omap_lcdc.c
index e4a5595..fb72ebe 100644
--- a/hw/display/omap_lcdc.c
+++ b/hw/display/omap_lcdc.c
@@ -406,7 +406,7 @@ struct omap_lcd_panel_s *omap_lcdc_init(MemoryRegion *sysmem,
     memory_region_init_io(&s->iomem, &omap_lcdc_ops, s, "omap.lcdc", 0x100);
     memory_region_add_subregion(sysmem, base, &s->iomem);
 
-    s->con = graphic_console_init(&omap_ops, s);
+    s->con = graphic_console_init(NULL, &omap_ops, s);
 
     return s;
 }
diff --git a/hw/display/pl110.c b/hw/display/pl110.c
index d232431..f259955 100644
--- a/hw/display/pl110.c
+++ b/hw/display/pl110.c
@@ -457,7 +457,7 @@ static int pl110_init(SysBusDevice *dev)
     sysbus_init_mmio(dev, &s->iomem);
     sysbus_init_irq(dev, &s->irq);
     qdev_init_gpio_in(&s->busdev.qdev, pl110_mux_ctrl_set, 1);
-    s->con = graphic_console_init(&pl110_gfx_ops, s);
+    s->con = graphic_console_init(DEVICE(dev), &pl110_gfx_ops, s);
     return 0;
 }
 
diff --git a/hw/display/pxa2xx_lcd.c b/hw/display/pxa2xx_lcd.c
index 12d9cd2..b63a3ea 100644
--- a/hw/display/pxa2xx_lcd.c
+++ b/hw/display/pxa2xx_lcd.c
@@ -1013,7 +1013,7 @@ PXA2xxLCDState *pxa2xx_lcdc_init(MemoryRegion *sysmem,
                           "pxa2xx-lcd-controller", 0x00100000);
     memory_region_add_subregion(sysmem, base, &s->iomem);
 
-    s->con = graphic_console_init(&pxa2xx_ops, s);
+    s->con = graphic_console_init(NULL, &pxa2xx_ops, s);
     surface = qemu_console_surface(s->con);
 
     switch (surface_bits_per_pixel(surface)) {
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index e679830..f8bd7ff 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2069,7 +2069,7 @@ static int qxl_init_primary(PCIDevice *dev)
     portio_list_init(qxl_vga_port_list, qxl_vga_portio_list, vga, "vga");
     portio_list_add(qxl_vga_port_list, pci_address_space_io(dev), 0x3b0);
 
-    vga->con = graphic_console_init(&qxl_ops, qxl);
+    vga->con = graphic_console_init(DEVICE(dev), &qxl_ops, qxl);
     qemu_spice_display_init_common(&qxl->ssd);
 
     rc = qxl_init_common(qxl);
@@ -2094,7 +2094,7 @@ static int qxl_init_secondary(PCIDevice *dev)
     memory_region_init_ram(&qxl->vga.vram, "qxl.vgavram", qxl->vga.vram_size);
     vmstate_register_ram(&qxl->vga.vram, &qxl->pci.qdev);
     qxl->vga.vram_ptr = memory_region_get_ram_ptr(&qxl->vga.vram);
-    qxl->vga.con = graphic_console_init(&qxl_ops, qxl);
+    qxl->vga.con = graphic_console_init(DEVICE(dev), &qxl_ops, qxl);
 
     return qxl_init_common(qxl);
 }
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index f0e6d70..2a0951a 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1449,5 +1449,5 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base,
     }
 
     /* create qemu graphic console */
-    s->con = graphic_console_init(&sm501_ops, s);
+    s->con = graphic_console_init(DEVICE(dev), &sm501_ops, s);
 }
diff --git a/hw/display/ssd0303.c b/hw/display/ssd0303.c
index 3d7ebbe..beea5bf 100644
--- a/hw/display/ssd0303.c
+++ b/hw/display/ssd0303.c
@@ -293,7 +293,7 @@ static int ssd0303_init(I2CSlave *i2c)
 {
     ssd0303_state *s = FROM_I2C_SLAVE(ssd0303_state, i2c);
 
-    s->con = graphic_console_init(&ssd0303_ops, s);
+    s->con = graphic_console_init(DEVICE(i2c), &ssd0303_ops, s);
     qemu_console_resize(s->con, 96 * MAGNIFY, 16 * MAGNIFY);
     return 0;
 }
diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c
index 45e8dc1..c3231c6 100644
--- a/hw/display/ssd0323.c
+++ b/hw/display/ssd0323.c
@@ -342,7 +342,7 @@ static int ssd0323_init(SSISlave *dev)
 
     s->col_end = 63;
     s->row_end = 79;
-    s->con = graphic_console_init(&ssd0323_ops, s);
+    s->con = graphic_console_init(DEVICE(dev), &ssd0323_ops, s);
     qemu_console_resize(s->con, 128 * MAGNIFY, 64 * MAGNIFY);
 
     qdev_init_gpio_in(&dev->qdev, ssd0323_cd, 1);
diff --git a/hw/display/tc6393xb.c b/hw/display/tc6393xb.c
index b5b255c..0cb87bc 100644
--- a/hw/display/tc6393xb.c
+++ b/hw/display/tc6393xb.c
@@ -587,7 +587,7 @@ TC6393xbState *tc6393xb_init(MemoryRegion *sysmem, uint32_t base, qemu_irq irq)
     memory_region_add_subregion(sysmem, base + 0x100000, &s->vram);
     s->scr_width = 480;
     s->scr_height = 640;
-    s->con = graphic_console_init(&tc6393xb_gfx_ops, s);
+    s->con = graphic_console_init(NULL, &tc6393xb_gfx_ops, s);
 
     return s;
 }
diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 77c7191..b295b5d 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -572,14 +572,14 @@ static int tcx_init1(SysBusDevice *dev)
                                  &s->vram_mem, vram_offset, size);
         sysbus_init_mmio(dev, &s->vram_cplane);
 
-        s->con = graphic_console_init(&tcx24_ops, s);
+        s->con = graphic_console_init(DEVICE(dev), &tcx24_ops, s);
     } else {
         /* THC 8 bit (dummy) */
         memory_region_init_io(&s->thc8, &dummy_ops, s, "tcx.thc8",
                               TCX_THC_NREGS_8);
         sysbus_init_mmio(dev, &s->thc8);
 
-        s->con = graphic_console_init(&tcx_ops, s);
+        s->con = graphic_console_init(DEVICE(dev), &tcx_ops, s);
     }
 
     qemu_console_resize(s->con, s->width, s->height);
diff --git a/hw/display/vga-isa-mm.c b/hw/display/vga-isa-mm.c
index 2da08a1..ceeb92f 100644
--- a/hw/display/vga-isa-mm.c
+++ b/hw/display/vga-isa-mm.c
@@ -135,7 +135,7 @@ int isa_vga_mm_init(hwaddr vram_base,
     vga_common_init(&s->vga);
     vga_mm_init(s, vram_base, ctrl_base, it_shift, address_space);
 
-    s->vga.con = graphic_console_init(s->vga.hw_ops, s);
+    s->vga.con = graphic_console_init(NULL, s->vga.hw_ops, s);
 
     vga_init_vbe(&s->vga, address_space);
     return 0;
diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index d2c548e..2b3cc9b 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -62,7 +62,7 @@ static int vga_initfn(ISADevice *dev)
                                         isa_mem_base + 0x000a0000,
                                         vga_io_memory, 1);
     memory_region_set_coalescing(vga_io_memory);
-    s->con = graphic_console_init(s->hw_ops, s);
+    s->con = graphic_console_init(DEVICE(dev), s->hw_ops, s);
 
     vga_init_vbe(s, isa_address_space(dev));
     /* ROM BIOS */
diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
index dc73f28..cea8db7 100644
--- a/hw/display/vga-pci.c
+++ b/hw/display/vga-pci.c
@@ -150,7 +150,7 @@ static int pci_std_vga_initfn(PCIDevice *dev)
     vga_common_init(s);
     vga_init(s, pci_address_space(dev), pci_address_space_io(dev), true);
 
-    s->con = graphic_console_init(s->hw_ops, s);
+    s->con = graphic_console_init(DEVICE(dev), s->hw_ops, 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/display/vmware_vga.c b/hw/display/vmware_vga.c
index 263bf09..fd3569d 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1185,13 +1185,13 @@ static const GraphicHwOps vmsvga_ops = {
     .text_update = vmsvga_text_update,
 };
 
-static void vmsvga_init(struct vmsvga_state_s *s,
+static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s,
                         MemoryRegion *address_space, MemoryRegion *io)
 {
     s->scratch_size = SVGA_SCRATCH_SIZE;
     s->scratch = g_malloc(s->scratch_size * 4);
 
-    s->vga.con = graphic_console_init(&vmsvga_ops, s);
+    s->vga.con = graphic_console_init(dev, &vmsvga_ops, s);
 
     s->fifo_size = SVGA_FIFO_SIZE;
     memory_region_init_ram(&s->fifo_ram, "vmsvga.fifo", s->fifo_size);
@@ -1258,7 +1258,8 @@ static int pci_vmsvga_initfn(PCIDevice *dev)
     memory_region_set_flush_coalesced(&s->io_bar);
     pci_register_bar(&s->card, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->io_bar);
 
-    vmsvga_init(&s->chip, pci_address_space(dev), pci_address_space_io(dev));
+    vmsvga_init(DEVICE(dev), &s->chip,
+                pci_address_space(dev), pci_address_space_io(dev));
 
     pci_register_bar(&s->card, 1, PCI_BASE_ADDRESS_MEM_PREFETCH,
                      &s->chip.vga.vram);
diff --git a/hw/unicore32/puv3.c b/hw/unicore32/puv3.c
index f8d32bc..56d1afa 100644
--- a/hw/unicore32/puv3.c
+++ b/hw/unicore32/puv3.c
@@ -94,7 +94,7 @@ static void puv3_load_kernel(const char *kernel_filename)
     }
 
     /* cheat curses that we have a graphic console, only under ocd console */
-    graphic_console_init(&no_ops, NULL);
+    graphic_console_init(NULL, &no_ops, NULL);
 }
 
 static void puv3_init(QEMUMachineInitArgs *args)
diff --git a/include/ui/console.h b/include/ui/console.h
index c8a274d..22670d8 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -291,7 +291,8 @@ typedef struct GraphicHwOps {
     void (*update_interval)(void *opaque, uint64_t interval);
 } GraphicHwOps;
 
-QemuConsole *graphic_console_init(const GraphicHwOps *ops,
+QemuConsole *graphic_console_init(DeviceState *dev,
+                                  const GraphicHwOps *ops,
                                   void *opaque);
 
 void graphic_hw_update(QemuConsole *con);
diff --git a/ui/console.c b/ui/console.c
index b92bde3..21f762b 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -23,6 +23,7 @@
  */
 #include "qemu-common.h"
 #include "ui/console.h"
+#include "hw/qdev-core.h"
 #include "qemu/timer.h"
 #include "qmp-commands.h"
 #include "sysemu/char.h"
@@ -122,6 +123,7 @@ struct QemuConsole {
     int dcls;
 
     /* Graphic console state.  */
+    Object *device;
     const GraphicHwOps *hw_ops;
     void *hw;
 
@@ -1199,14 +1201,19 @@ static void text_console_update(void *opaque, console_ch_t *chardata)
 
 static QemuConsole *new_console(DisplayState *ds, console_type_t console_type)
 {
+    Error *local_err = NULL;
     Object *obj;
     QemuConsole *s;
     int i;
 
     if (nb_consoles >= MAX_CONSOLES)
         return NULL;
+
     obj = object_new(TYPE_QEMU_CONSOLE);
     s = QEMU_CONSOLE(obj);
+    object_property_add_link(obj, "device", TYPE_DEVICE,
+                             (Object **)&s->device, &local_err);
+
     if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
         (console_type == GRAPHIC_CONSOLE))) {
         active_console = s;
@@ -1567,9 +1574,11 @@ DisplayState *init_displaystate(void)
     return display_state;
 }
 
-QemuConsole *graphic_console_init(const GraphicHwOps *hw_ops,
+QemuConsole *graphic_console_init(DeviceState *dev,
+                                  const GraphicHwOps *hw_ops,
                                   void *opaque)
 {
+    Error *local_err = NULL;
     int width = 640;
     int height = 480;
     QemuConsole *s;
@@ -1580,6 +1589,10 @@ QemuConsole *graphic_console_init(const GraphicHwOps *hw_ops,
     s = new_console(ds, GRAPHIC_CONSOLE);
     s->hw_ops = hw_ops;
     s->hw = opaque;
+    if (dev) {
+        object_property_set_link(OBJECT(s), OBJECT(dev),
+                                 "device", &local_err);
+    }
 
     s->surface = qemu_create_displaysurface(width, height);
     return s;
-- 
1.7.9.7

[Qemu-devel] [PATCH 4/5] console: add qemu_console_lookup_by_device

[Gerd Hoffmann]

Look up the QemuConsole for a given device, using the new link.

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

diff --git a/include/ui/console.h b/include/ui/console.h
index 22670d8..c74e791 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -300,6 +300,7 @@ void graphic_hw_invalidate(QemuConsole *con);
 void graphic_hw_text_update(QemuConsole *con, console_ch_t *chardata);
 
 QemuConsole *qemu_console_lookup_by_index(unsigned int index);
+QemuConsole *qemu_console_lookup_by_device(DeviceState *dev);
 bool qemu_console_is_visible(QemuConsole *con);
 bool qemu_console_is_graphic(QemuConsole *con);
 bool qemu_console_is_fixedsize(QemuConsole *con);
diff --git a/ui/console.c b/ui/console.c
index 21f762b..7c3f2f9 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1606,6 +1606,25 @@ QemuConsole *qemu_console_lookup_by_index(unsigned int index)
     return consoles[index];
 }
 
+QemuConsole *qemu_console_lookup_by_device(DeviceState *dev)
+{
+    Error *local_err = NULL;
+    Object *obj;
+    int i;
+
+    for (i = 0; i < nb_consoles; i++) {
+        if (!consoles[i]) {
+            continue;
+        }
+        obj = object_property_get_link(OBJECT(consoles[i]),
+                                       "device", &local_err);
+        if (DEVICE(obj) == dev) {
+            return consoles[i];
+        }
+    }
+    return NULL;
+}
+
 bool qemu_console_is_visible(QemuConsole *con)
 {
     return (con == active_console) || (con->dcls > 0);
-- 
1.7.9.7

[Qemu-devel] [PATCH 5/5] console: extend screendump monitor cmd

[Gerd Hoffmann]

Add an optional device parameter to the screendump command.

https://bugzilla.redhat.com/show_bug.cgi?id=903910

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hmp-commands.hx  |    6 +++---
 hmp.c            |    3 ++-
 qapi-schema.json |    4 +++-
 qmp-commands.hx  |    3 ++-
 ui/console.c     |   17 ++++++++++++++++-
 5 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index df44906..c6ca7c7 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -235,9 +235,9 @@ ETEXI
 
     {
         .name       = "screendump",
-        .args_type  = "filename:F",
-        .params     = "filename",
-        .help       = "save screen into PPM image 'filename'",
+        .args_type  = "filename:F,device:B?",
+        .params     = "filename [device]",
+        .help       = "save screen from device into PPM image 'filename'",
         .mhandler.cmd = hmp_screen_dump,
     },
 
diff --git a/hmp.c b/hmp.c
index 4fb76ec..a29e241 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1322,9 +1322,10 @@ err_out:
 void hmp_screen_dump(Monitor *mon, const QDict *qdict)
 {
     const char *filename = qdict_get_str(qdict, "filename");
+    const char *device = qdict_get_try_str(qdict, "device");
     Error *err = NULL;
 
-    qmp_screendump(filename, &err);
+    qmp_screendump(filename, device != NULL, device, &err);
     hmp_handle_error(mon, &err);
 }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 751d3c2..40f3b49 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3096,12 +3096,14 @@
 # Write a PPM of the VGA screen to a file.
 #
 # @filename: the path of a new PPM file to store the image
+# @device: #optional device to take the screenshot from
 #
 # Returns: Nothing on success
 #
 # Since: 0.14.0
 ##
-{ 'command': 'screendump', 'data': {'filename': 'str'} }
+{ 'command': 'screendump', 'data': {'filename': 'str',
+                                    '*device' : 'str'} }
 
 ##
 # @nbd-server-start:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 4d65422..32642ef 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -145,7 +145,7 @@ EQMP
 
     {
         .name       = "screendump",
-        .args_type  = "filename:F",
+        .args_type  = "filename:F,device:B?",
         .mhandler.cmd_new = qmp_marshal_input_screendump,
     },
 
@@ -158,6 +158,7 @@ Save screen into PPM image.
 Arguments:
 
 - "filename": file path (json-string)
+- "device": video device (json-string, optional)
 
 Example:
 
diff --git a/ui/console.c b/ui/console.c
index 7c3f2f9..6d49ba2 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -310,10 +310,25 @@ write_err:
     goto out;
 }
 
-void qmp_screendump(const char *filename, Error **errp)
+void qmp_screendump(const char *filename,
+                    bool has_device, const char *device, Error **errp)
 {
     QemuConsole *con = qemu_console_lookup_by_index(0);
     DisplaySurface *surface;
+    DeviceState *dev;
+
+    if (has_device) {
+        dev = qdev_find_recursive(sysbus_get_default(), device);
+        if (NULL == dev) {
+            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+            return;
+        }
+        con = qemu_console_lookup_by_device(dev);
+        if (NULL == con) {
+            error_setg(errp, "There is no QemuConsole linked to %s", device);
+            return;
+        }
+    }
 
     if (con == NULL) {
         error_setg(errp, "There is no QemuConsole I can screendump from.");
-- 
1.7.9.7

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Eric Blake]

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

On 04/18/2013 03:01 AM, Gerd Hoffmann wrote:
>   Hi,
> 
> This patch qom-ifies the QemuConsoles and adds an new, optional
> parameter to the screendump command to specify the device you
> want dump from.
> 
> Question for the QOM guys: where in the tree should the QemuConsoles
> show up?  I can't find anything in the tree doing this.  I've placed
> them below /backend.  See patch #2.  Ok?  Better suggestions?
> 
> Question for the libvirt guys:  Is it ok for libvirt to just extend the
> existing screendump command?  Can libvirt figure there is a new
> (optional) parameter?  See patch #5.

The existing libvirt API is:

char *
virDomainScreenshot(virDomainPtr domain,
                    virStreamPtr stream,
                    unsigned int screen,
                    unsigned int flags)

which uses the screen argument to distinguish both between devices and
heads on a given device:

 * The screen ID is the sequential number of screen. In case of multiple
 * graphics cards, heads are enumerated before devices, e.g. having
 * two graphics cards, both with four heads, screen ID 5 addresses
 * the second head on the second card.

So yes, I think libvirt will be able to drive the new command by knowing
how many heads appear per device, then passing in the appropriate named
device to the QMP command.  And yes, I'll review patch 5 regarding
interface design.

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


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

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Luiz Capitulino]

On Thu, 18 Apr 2013 06:10:19 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 04/18/2013 03:01 AM, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > This patch qom-ifies the QemuConsoles and adds an new, optional
> > parameter to the screendump command to specify the device you
> > want dump from.
> > 
> > Question for the QOM guys: where in the tree should the QemuConsoles
> > show up?  I can't find anything in the tree doing this.  I've placed
> > them below /backend.  See patch #2.  Ok?  Better suggestions?
> > 
> > Question for the libvirt guys:  Is it ok for libvirt to just extend the
> > existing screendump command?  Can libvirt figure there is a new
> > (optional) parameter?  See patch #5.
> 
> The existing libvirt API is:
> 
> char *
> virDomainScreenshot(virDomainPtr domain,
>                     virStreamPtr stream,
>                     unsigned int screen,
>                     unsigned int flags)
> 
> which uses the screen argument to distinguish both between devices and
> heads on a given device:
> 
>  * The screen ID is the sequential number of screen. In case of multiple
>  * graphics cards, heads are enumerated before devices, e.g. having
>  * two graphics cards, both with four heads, screen ID 5 addresses
>  * the second head on the second card.
> 
> So yes, I think libvirt will be able to drive the new command by knowing
> how many heads appear per device, then passing in the appropriate named
> device to the QMP command.  And yes, I'll review patch 5 regarding
> interface design.

We can extend screendump on HMP, but the general rule for QMP is to add a
new command instead so that clients don't have to play tricks like Eric is
suggesting :)

Is there any big issue with adding a new command?

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Eric Blake]

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

On 04/18/2013 09:21 AM, Luiz Capitulino wrote:
>>> Question for the libvirt guys:  Is it ok for libvirt to just extend the
>>> existing screendump command?  Can libvirt figure there is a new
>>> (optional) parameter?  See patch #5.
>>

>> So yes, I think libvirt will be able to drive the new command by knowing
>> how many heads appear per device, then passing in the appropriate named
>> device to the QMP command.  And yes, I'll review patch 5 regarding
>> interface design.
> 
> We can extend screendump on HMP, but the general rule for QMP is to add a
> new command instead so that clients don't have to play tricks like Eric is
> suggesting :)

The problem at hand is that your proposal in patch 5:

-{ 'command': 'screendump', 'data': {'filename': 'str'} }
+{ 'command': 'screendump', 'data': {'filename': 'str',
+                                    '*device' : 'str'} }

still doesn't support the case of dumping just one head of a multi-head
device.  Libvirt's API is already adequately flexible to cover an
arbitrary head of an arbitrary device, once mapped down to QMP, so the
question at hand now is whether to map it down to QMP by adding optional
parameters to the existing command or adding a new command.

> 
> Is there any big issue with adding a new command?

I haven't yet mentioned any tricks, but now that you bring it up, there
are two options:

Option 1:
reuse the same QMP command, but add optional parameters (ability to
specify device in this patch, but libvirt would also want the ability to
specify which head of a multi-head device).
Libvirt always calls screendump, and omits the optional parameters if
the user asked libvirt for screen 0.  If user asks libvirt for screen 1,
libvirt passes the optional parameter, and if qemu is too old, qemu
gives an error about invalid argument, which libvirt then feeds back to
the user as a 'screen 1 not supported'.

Option 2:
existing 'screendump' command can ONLY dump the primary head of the
primary device, and a new QMP command is added that supports head and
device selection.  Libvirt uses query-commands to determine whether new
command has been backported; if so, it uses the new command, if not, it
gives the user a nice error unless screen 0 was selected.

Option 2 is probably slightly nicer for guaranteeing a sane error
message back to the user, but option 1 (the approach of this series)
still seems workable.

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


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

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Gerd Hoffmann]

On 04/18/13 17:34, Eric Blake wrote:
> On 04/18/2013 09:21 AM, Luiz Capitulino wrote:
>>>> Question for the libvirt guys:  Is it ok for libvirt to just extend the
>>>> existing screendump command?  Can libvirt figure there is a new
>>>> (optional) parameter?  See patch #5.
>>>
> 
>>> So yes, I think libvirt will be able to drive the new command by knowing
>>> how many heads appear per device, then passing in the appropriate named
>>> device to the QMP command.  And yes, I'll review patch 5 regarding
>>> interface design.
>>
>> We can extend screendump on HMP, but the general rule for QMP is to add a
>> new command instead so that clients don't have to play tricks like Eric is
>> suggesting :)

Ahem.  It didn't sound like libvirt plays tricks there, to me it simply
looked like an description of how libvirt manages devices + heads and
the confirmation that libvirt indeed know which device it wants a
screenshot from.

We also have to care to not mix up two things:  #1 is whenever we'll go
extent screendump or add screendump2 (see sub-thread started by markus
reply).  #2 the actual design of the new/extended command.

> The problem at hand is that your proposal in patch 5:
> 
> -{ 'command': 'screendump', 'data': {'filename': 'str'} }
> +{ 'command': 'screendump', 'data': {'filename': 'str',
> +                                    '*device' : 'str'} }
> 
> still doesn't support the case of dumping just one head of a multi-head
> device.

qxl is the only device with multihead support, and it works by defining
a large framebuffer and defining the heads as rectangles within this
framebuffer:

   +-------------------------------------------+
   | framebuffer                               |
   |    +-------------+   +----------+         |
   |    | head 1      |   | head 2   |         |
   |    |             |   |          |         |
   |    +-------------+   +----------+         |
   +-------------------------------------------+

(in practice you wouldn't have unused space around the heads of course,
but it's easier to draw this way ;)

So a recent spice client will get the head configuration info, then open
two windows, one for each head.  A old spice client without multihead
support will create a single window covering the whole framebuffer
instead.  Asking qemu to screendump a multihead qxl device will likewise
write out a dump of the whole framebuffer, i.e. the dump will have *all*
heads.

> device selection.  Libvirt uses query-commands to determine whether new

So query-commands doesn't list the arguments supported by a command,
only the commands themself.

> Option 2 is probably slightly nicer for guaranteeing a sane error
> message back to the user, but option 1 (the approach of this series)
> still seems workable.

Sane error messaging is good, I'd still prefer Option 1 though, to get
both we'll need a new query-arguments command I guess ...

cheers,
  Gerd

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
> This patch qom-ifies the QemuConsoles and adds an new, optional
> parameter to the screendump command to specify the device you
> want dump from.
>
> Question for the QOM guys: where in the tree should the QemuConsoles
> show up?  I can't find anything in the tree doing this.  I've placed
> them below /backend.  See patch #2.  Ok?  Better suggestions?
>
> Question for the libvirt guys:  Is it ok for libvirt to just extend the
> existing screendump command?  Can libvirt figure there is a new
> (optional) parameter?  See patch #5.

Nope, QMP can't do that.  I argued for such capabilities, but the
"create a new command" philosophy prevailed.

Go forth and multiply commands!  And have fun picking command names that
aren't fugly.

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Eric Blake]

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

On 04/19/2013 02:37 AM, Markus Armbruster wrote:
>> Question for the libvirt guys:  Is it ok for libvirt to just extend the
>> existing screendump command?  Can libvirt figure there is a new
>> (optional) parameter?  See patch #5.
> 
> Nope, QMP can't do that.  I argued for such capabilities, but the
> "create a new command" philosophy prevailed.
> 
> Go forth and multiply commands!  And have fun picking command names that
> aren't fugly.

Adding optional parameters to an existing command has been done before;
drive-mirror gained 'granularity' and 'buf-size' in 1.4 even though the
command existed in 1.3.

What would make it possible to avoid an explosion of new commands is a
way to introspect WHICH options are available for a given command.  We
don't have that yet, but if we add a command that returns the JSON
representation of what the command expects, THAT would make it
acceptable to add optional arguments to an existing command in a way
that libvirt could query to see whether the qemu it is talking to
supports the optional arguments.

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


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

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Gerd Hoffmann]

On 04/19/13 10:37, Markus Armbruster wrote:
> Gerd Hoffmann <kraxel@redhat.com> writes:
> 
>> Question for the libvirt guys:  Is it ok for libvirt to just extend the
>> existing screendump command?  Can libvirt figure there is a new
>> (optional) parameter?  See patch #5.
> 
> Nope, QMP can't do that.  I argued for such capabilities, but the
> "create a new command" philosophy prevailed.
> 
> Go forth and multiply commands!  And have fun picking command names that
> aren't fugly.

Oh joy.  Lets just enumerate things & use "screendump2" ...

cheers,
  Gerd

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Paolo Bonzini]

Il 22/04/2013 08:55, Gerd Hoffmann ha scritto:
>>> >> Question for the libvirt guys:  Is it ok for libvirt to just extend the
>>> >> existing screendump command?  Can libvirt figure there is a new
>>> >> (optional) parameter?  See patch #5.
>> > 
>> > Nope, QMP can't do that.  I argued for such capabilities, but the
>> > "create a new command" philosophy prevailed.
>> > 
>> > Go forth and multiply commands!  And have fun picking command names that
>> > aren't fugly.
> Oh joy.  Lets just enumerate things & use "screendump2" ...

QMP can't do that _yet_.

Let's fix it instead...

Paolo

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Luiz Capitulino]

On Mon, 22 Apr 2013 11:37:15 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 22/04/2013 08:55, Gerd Hoffmann ha scritto:
> >>> >> Question for the libvirt guys:  Is it ok for libvirt to just extend the
> >>> >> existing screendump command?  Can libvirt figure there is a new
> >>> >> (optional) parameter?  See patch #5.
> >> > 
> >> > Nope, QMP can't do that.  I argued for such capabilities, but the
> >> > "create a new command" philosophy prevailed.
> >> > 
> >> > Go forth and multiply commands!  And have fun picking command names that
> >> > aren't fugly.
> > Oh joy.  Lets just enumerate things & use "screendump2" ...
> 
> QMP can't do that _yet_.
> 
> Let's fix it instead...

The point is that we have chosen not to do so a while ago. In a nutshell,
Anthony thinks that we should have the same compatibility contract of
a C API.

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Paolo Bonzini]

Il 22/04/2013 14:50, Luiz Capitulino ha scritto:
> On Mon, 22 Apr 2013 11:37:15 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 22/04/2013 08:55, Gerd Hoffmann ha scritto:
>>>>>>> Question for the libvirt guys:  Is it ok for libvirt to just extend the
>>>>>>> existing screendump command?  Can libvirt figure there is a new
>>>>>>> (optional) parameter?  See patch #5.
>>>>>
>>>>> Nope, QMP can't do that.  I argued for such capabilities, but the
>>>>> "create a new command" philosophy prevailed.
>>>>>
>>>>> Go forth and multiply commands!  And have fun picking command names that
>>>>> aren't fugly.
>>> Oh joy.  Lets just enumerate things & use "screendump2" ...
>>
>> QMP can't do that _yet_.
>>
>> Let's fix it instead...
> 
> The point is that we have chosen not to do so a while ago. In a nutshell,
> Anthony thinks that we should have the same compatibility contract of
> a C API.

We've been adding fields to types since 0.15, sometimes in the middle of
a struct (since 1.2).  If the C API is a requirement, it should also be
a requirement for structs.  But there are plenty of ways (some nicer,
some uglier) to have different API versions in C.

For example, I think the QIDL patch had ways to annotate each parameter
independently.  You can annotate each argument with the "first version
this appeared in" and complicate the C API generator to generate
multiple C functions for the same command.

It is then the downstream's responsibility not to backport extra
arguments without a full-blown rebase to a newer version (the same as
for C libraries).

Paolo

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 22/04/2013 14:50, Luiz Capitulino ha scritto:
>> On Mon, 22 Apr 2013 11:37:15 +0200
>> Paolo Bonzini <pbonzini@redhat.com> wrote:
>> 
>>> Il 22/04/2013 08:55, Gerd Hoffmann ha scritto:
>>>>>>>> Question for the libvirt guys:  Is it ok for libvirt to just extend the
>>>>>>>> existing screendump command?  Can libvirt figure there is a new
>>>>>>>> (optional) parameter?  See patch #5.
>>>>>>
>>>>>> Nope, QMP can't do that.  I argued for such capabilities, but the
>>>>>> "create a new command" philosophy prevailed.
>>>>>>
>>>>>> Go forth and multiply commands!  And have fun picking command names that
>>>>>> aren't fugly.
>>>> Oh joy.  Lets just enumerate things & use "screendump2" ...
>>>
>>> QMP can't do that _yet_.
>>>
>>> Let's fix it instead...
>> 
>> The point is that we have chosen not to do so a while ago. In a nutshell,
>> Anthony thinks that we should have the same compatibility contract of
>> a C API.
>
> We've been adding fields to types since 0.15, sometimes in the middle of
> a struct (since 1.2).

You can safely add fields to the end of a struct.  If you think about it
in terms of a shipped qmp.h file, it would be:

struct CharInfo
{
};

CharInfo *qmp_query_char(QMPSession *session, Error **errp);

But adding parameters to functions breaks the ABI and there's no way
around unless you make all parameters passed as a single structure.

> If the C API is a requirement, it should also be
> a requirement for structs.  But there are plenty of ways (some nicer,
> some uglier) to have different API versions in C.
>
> For example, I think the QIDL patch had ways to annotate each parameter
> independently.  You can annotate each argument with the "first version
> this appeared in" and complicate the C API generator to generate
> multiple C functions for the same command.
>
> It is then the downstream's responsibility not to backport extra
> arguments without a full-blown rebase to a newer version (the same as
> for C libraries).

Well this is all well and good in abstract, in practice, we want a new
screendump command anyway.

It'd be *much* nicer to return the screenshot data via the QMP session
instead of writing it to a file.  So let's take the opportunity to fix
the command.

We can also introduce a "format" parameter to allow specifying formats
othe than PPM.

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Paolo Bonzini]

Il 22/04/2013 18:49, Anthony Liguori ha scritto:
>> We've been adding fields to types since 0.15, sometimes in the middle of
>> a struct (since 1.2).
> 
> You can safely add fields to the end of a struct. 

For QEMU->user structs it is.  For user->QEMU structs you need to add a
sizeof() at the beginning, or ensure that everything is heap-allocated
(and zero-initialized).

At that point you could also use structs to pass arguments to the
functions (in the C client API) that execute a QMP command.  That's
similar to having keyword arguments in C.

> Well this is all well and good in abstract, in practice, we want a new
> screendump command anyway.
> 
> It'd be *much* nicer to return the screenshot data via the QMP session
> instead of writing it to a file.  So let's take the opportunity to fix
> the command.

That's debatable... the "nicest" way could also be to pass a pipe fd and
retrieve the dump from that fd.  That's quite easy to do with fdsets.
The choice is between implementing SCM_RIGHTS sendfd and a base64 decoder.

> We can also introduce a "format" parameter to allow specifying formats
> othe than PPM.

True, but I'm not sure we want to go there.  We'd need to add support
for options like JPG quality factor etc.

Paolo

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Eric Blake]

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

On 04/22/2013 11:03 AM, Paolo Bonzini wrote:
>> It'd be *much* nicer to return the screenshot data via the QMP session
>> instead of writing it to a file.  So let's take the opportunity to fix
>> the command.

That's a lot of data to be encoding into JSON.

> 
> That's debatable... the "nicest" way could also be to pass a pipe fd and
> retrieve the dump from that fd.  That's quite easy to do with fdsets.
> The choice is between implementing SCM_RIGHTS sendfd and a base64 decoder.

If the existing filename can already be used with fdsets, then libvirt
can already achieve pass to pipe - and pass to pipe is indeed the nicest
way to then get alternate formats without having to decode lots of JSON,
since there are conversion utilities that can take ppm on input and
output other formats without having to teach qemu those other output
formats.

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


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

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Gerd Hoffmann]

On 04/22/13 19:23, Eric Blake wrote:
> On 04/22/2013 11:03 AM, Paolo Bonzini wrote:
>>> It'd be *much* nicer to return the screenshot data via the QMP session
>>> instead of writing it to a file.  So let's take the opportunity to fix
>>> the command.
> 
> That's a lot of data to be encoding into JSON.
> 
>>
>> That's debatable... the "nicest" way could also be to pass a pipe fd and
>> retrieve the dump from that fd.  That's quite easy to do with fdsets.
>> The choice is between implementing SCM_RIGHTS sendfd and a base64 decoder.
> 
> If the existing filename can already be used with fdsets,

It can't.  Easily fixable though.  Question is how libvirt then can
figure fdsets for screendumps do work ...

cheers,
  Gerd

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 22/04/2013 18:49, Anthony Liguori ha scritto:
>>> We've been adding fields to types since 0.15, sometimes in the middle of
>>> a struct (since 1.2).
>> 
>> You can safely add fields to the end of a struct. 
>
> For QEMU->user structs it is.  For user->QEMU structs you need to add a
> sizeof() at the beginning, or ensure that everything is heap-allocated
> (and zero-initialized).

Think library generated from qapi-schema.json.  We want this library to
have a backwards compatible CABI.

There are a couple ways to deal with adding to the end.  You could do it
kernel-style and zero pad structures.  Another option is to have a flags
fields as the first member and use that to indicate optional
parameters.  A 64-bit flags value would allow 64 optional parameters
which should keep us comfortable for quite a while.

> At that point you could also use structs to pass arguments to the
> functions (in the C client API) that execute a QMP command.  That's
> similar to having keyword arguments in C.

Ack.

>> Well this is all well and good in abstract, in practice, we want a new
>> screendump command anyway.
>> 
>> It'd be *much* nicer to return the screenshot data via the QMP session
>> instead of writing it to a file.  So let's take the opportunity to fix
>> the command.
>
> That's debatable... the "nicest" way could also be to pass a pipe fd and
> retrieve the dump from that fd.  That's quite easy to do with fdsets.
> The choice is between implementing SCM_RIGHTS sendfd and a base64
> decoder.

Granted, base64 increases the size by a 66% but I don't think it's a
huge issue.

>> We can also introduce a "format" parameter to allow specifying formats
>> othe than PPM.
>
> True, but I'm not sure we want to go there.  We'd need to add support
> for options like JPG quality factor etc.

PNG would be extremely handy and would go a long way to eliminating the
concern about size.  We already link against libpng too.

You can imagine an interface like:

{ "type": "Blob",
  "data": { "format": "DataFormat", "data": "str" } }

...

{ "union": "ImageOptions",
  "data": { "ppm": "PPMOptions", 
            "png": "PNGOptions" } }

{ "command": "display-get-screenshot",
  "data": { "id": "str", "*ImageOptions": "options",
                           "*format": "DataFormat" },
  "returns": "Blob" }

I think it's worth implementing.  A local screenshot I have is 2.3Mb as
a PPM but only 320k as a PNG.

base64 encoded the PNG is 428k which is still significantly smaller than
the PPM.

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Gerd Hoffmann]

  Hi,

>> True, but I'm not sure we want to go there.  We'd need to add support
>> for options like JPG quality factor etc.
> 
> PNG would be extremely handy and would go a long way to eliminating the
> concern about size.  We already link against libpng too.

vnc uses libpng when available, but it isn't mandatory today.

So we have the choice to (a) make it a hard dependency or (b) make png
support optional.  (b) pretty much implies that nobody will use it
because they can't depend on it being available, so it's largely pointless.

> You can imagine an interface like:
> 
> { "type": "Blob",
>   "data": { "format": "DataFormat", "data": "str" } }
> 
> ...
> 
> { "union": "ImageOptions",
>   "data": { "ppm": "PPMOptions", 
>             "png": "PNGOptions" } }
> 
> { "command": "display-get-screenshot",
>   "data": { "id": "str", "*ImageOptions": "options",
>                            "*format": "DataFormat" },
>   "returns": "Blob" }

> I think it's worth implementing.  A local screenshot I have is 2.3Mb as
> a PPM but only 320k as a PNG.

I don't think it is useful to send image files as base64 over qmp.

We should either send the image file to a file (or filehandle via
fdset).  This would be very simliar to the current screendump command,
and libvirt can easily wind up a pipe to tools like cjpeg or just ask
qemu to dump into a file.

Or we'll go send the raw pixel data over qmp, maybe with optional
compression, so the receiving side doesn't has to bother with decoding
some image file format, like this:

{ "type" : "DisplayContent",
  "data  : { "width"       : int,
             "height"      : int,
             "data"        : str }

cheers,
  Gerd

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Luiz Capitulino]

On Thu, 25 Apr 2013 20:55:40 -0000
Anthony Liguori <aliguori@us.ibm.com> wrote:

> Applied.  Thanks.

Really? This contains the screendump extension we agreed on not doing.

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Eric Blake]

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

On 04/25/2013 03:17 PM, Luiz Capitulino wrote:
> On Thu, 25 Apr 2013 20:55:40 -0000
> Anthony Liguori <aliguori@us.ibm.com> wrote:
> 
>> Applied.  Thanks.
> 
> Really? This contains the screendump extension we agreed on not doing.

I tend to agree that we should revert the QMP change; without a way to
introspect whether the '*device' parameter exists, libvirt can't use it,
and we already agreed that we really need something better anyways with
regards to fd passing, image format selection, the ability to limit to
one head at a time, and so forth.  I'd rather defer it all to 1.6 when
we can get the entire design right, than to have 1.5 expose something
that can't be reliably used and which might change based on designing
the rest of the improvements.

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


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

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Anthony Liguori]

Eric Blake <eblake@redhat.com> writes:

> On 04/25/2013 03:17 PM, Luiz Capitulino wrote:
>> On Thu, 25 Apr 2013 20:55:40 -0000
>> Anthony Liguori <aliguori@us.ibm.com> wrote:
>> 
>>> Applied.  Thanks.
>> 
>> Really? This contains the screendump extension we agreed on not doing.
>
> I tend to agree that we should revert the QMP change;

Again, broken notification.  This wasn't merged.  Sorry for the noise.

Regards,

Anthony Liguori

> without a way to
> introspect whether the '*device' parameter exists, libvirt can't use it,
> and we already agreed that we really need something better anyways with
> regards to fd passing, image format selection, the ability to limit to
> one head at a time, and so forth.  I'd rather defer it all to 1.6 when
> we can get the entire design right, than to have 1.5 expose something
> that can't be reliably used and which might change based on designing
> the rest of the improvements.
>
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Anthony Liguori]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 25 Apr 2013 20:55:40 -0000
> Anthony Liguori <aliguori@us.ibm.com> wrote:
>
>> Applied.  Thanks.
>
> Really? This contains the screendump extension we agreed on not doing.

Nope, just a bug in my notify script.

Somehow was triggered by merging Gerd's pull request.  I'll investigate.

Sorry for the noise.  screendump is still exactly as it was before.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command

[Luiz Capitulino]

On Thu, 25 Apr 2013 17:19:15 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Thu, 25 Apr 2013 20:55:40 -0000
> > Anthony Liguori <aliguori@us.ibm.com> wrote:
> >
> >> Applied.  Thanks.
> >
> > Really? This contains the screendump extension we agreed on not doing.
> 
> Nope, just a bug in my notify script.
> 
> Somehow was triggered by merging Gerd's pull request.  I'll investigate.
> 
> Sorry for the noise.  screendump is still exactly as it was before.

The way we love it :)

Thanks.