[PATCH 0/3] Fix crash when adding a second ISA VGA device

[PATCH 0/3] Fix crash when adding a second ISA VGA device

[Thomas Huth]

QEMU currently abort()s if the user tries to add a second ISA VGA
device, for example:

$ ./qemu-system-x86_64 -device isa-vga -device isa-vga
RAMBlock "vga.vram" already registered, abort!
Aborted (core dumped)
$ ./qemu-system-x86_64 -device isa-cirrus-vga -device isa-cirrus-vga
RAMBlock "vga.vram" already registered, abort!
Aborted (core dumped)

Such a crash should never happen just because of giving bad parameters
at the command line, we should give a proper error message instead
and exit gracefully.

Note: There have been previous attempts to fix this problem, but the
first committed solution had bad side effects and got reverted
(https://gitlab.com/qemu-project/qemu/-/issues/733). There was another
idea to fix it by QOM'ifying the related devices (see the commits around
23f6e3b11be74abae), but after having another close look at the problem,
I think this doesn't work either: For getting unique names in the
vmstate_register_ram() function, the devices need to return unique names
from the qdev_get_dev_path() function, and those ISA VGA devices don't
support that there (unlike PCI, ISA devices don't have a slot id ...
they could be distinguished by their I/O port base address, but all the
ISA VGA cards currently use the same address there, so that doesn't
work either). ==> So the very original idea of checking for the availability
of the "vga.vram" memory region still seems the only usable approach to
me right now. While the original patch by Jose R. Ziviani only fixed the
issue for the isa-vga device, I'm taking a more general approach now by
adding the fix in the vga_common_init() function, so that it works for
the isa-cirrus-vga device, too.

Thomas Huth (3):
  hw/display/cirrus_vga: Clean up indentation in
    pci_cirrus_vga_realize()
  hw/display: Allow vga_common_init() to return errors
  hw/display/vga: Report a proper error when adding a 2nd ISA VGA

 hw/display/ati.c            |  7 ++++-
 hw/display/cirrus_vga.c     | 62 ++++++++++++++++++++-----------------
 hw/display/cirrus_vga_isa.c |  7 ++++-
 hw/display/qxl.c            |  6 +++-
 hw/display/vga-isa.c        |  9 +++++-
 hw/display/vga-mmio.c       |  8 ++++-
 hw/display/vga-pci.c        | 15 +++++++--
 hw/display/vga.c            | 15 +++++++--
 hw/display/vga_int.h        |  2 +-
 hw/display/virtio-vga.c     |  7 ++++-
 hw/display/vmware_vga.c     |  2 +-
 11 files changed, 100 insertions(+), 40 deletions(-)

-- 
2.27.0

[PATCH 1/3] hw/display/cirrus_vga: Clean up indentation in pci_cirrus_vga_realize()

[Thomas Huth]

Most of the code in this function had been indented with 5 spaces instead
of 4. Since 4 is our preferred style, remove one space in the bad lines here.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/display/cirrus_vga.c | 57 +++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index fdca6ca659..7da1be3e12 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -2940,27 +2940,28 @@ void cirrus_init_common(CirrusVGAState *s, Object *owner,
 
 static void pci_cirrus_vga_realize(PCIDevice *dev, Error **errp)
 {
-     PCICirrusVGAState *d = PCI_CIRRUS_VGA(dev);
-     CirrusVGAState *s = &d->cirrus_vga;
-     PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
-     int16_t device_id = pc->device_id;
-
-     /* follow real hardware, cirrus card emulated has 4 MB video memory.
-       Also accept 8 MB/16 MB for backward compatibility. */
-     if (s->vga.vram_size_mb != 4 && s->vga.vram_size_mb != 8 &&
-         s->vga.vram_size_mb != 16) {
-         error_setg(errp, "Invalid cirrus_vga ram size '%u'",
-                    s->vga.vram_size_mb);
-         return;
-     }
-     /* setup VGA */
-     vga_common_init(&s->vga, OBJECT(dev));
-     cirrus_init_common(s, OBJECT(dev), device_id, 1, pci_address_space(dev),
-                        pci_address_space_io(dev));
-     s->vga.con = graphic_console_init(DEVICE(dev), 0, s->vga.hw_ops, &s->vga);
-
-     /* setup PCI */
+    PCICirrusVGAState *d = PCI_CIRRUS_VGA(dev);
+    CirrusVGAState *s = &d->cirrus_vga;
+    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
+    int16_t device_id = pc->device_id;
+
+    /*
+     * Follow real hardware, cirrus card emulated has 4 MB video memory.
+     * Also accept 8 MB/16 MB for backward compatibility.
+     */
+    if (s->vga.vram_size_mb != 4 && s->vga.vram_size_mb != 8 &&
+        s->vga.vram_size_mb != 16) {
+        error_setg(errp, "Invalid cirrus_vga ram size '%u'",
+                   s->vga.vram_size_mb);
+        return;
+    }
+    /* setup VGA */
+    vga_common_init(&s->vga, OBJECT(dev));
+    cirrus_init_common(s, OBJECT(dev), device_id, 1, pci_address_space(dev),
+                       pci_address_space_io(dev));
+    s->vga.con = graphic_console_init(DEVICE(dev), 0, s->vga.hw_ops, &s->vga);
 
+    /* setup PCI */
     memory_region_init(&s->pci_bar, OBJECT(dev), "cirrus-pci-bar0", 0x2000000);
 
     /* XXX: add byte swapping apertures */
@@ -2968,14 +2969,14 @@ static void pci_cirrus_vga_realize(PCIDevice *dev, Error **errp)
     memory_region_add_subregion(&s->pci_bar, 0x1000000,
                                 &s->cirrus_linear_bitblt_io);
 
-     /* setup memory space */
-     /* memory #0 LFB */
-     /* memory #1 memory-mapped I/O */
-     /* XXX: s->vga.vram_size must be a power of two */
-     pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->pci_bar);
-     if (device_id == CIRRUS_ID_CLGD5446) {
-         pci_register_bar(&d->dev, 1, 0, &s->cirrus_mmio_io);
-     }
+    /* setup memory space */
+    /* memory #0 LFB */
+    /* memory #1 memory-mapped I/O */
+    /* XXX: s->vga.vram_size must be a power of two */
+    pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->pci_bar);
+    if (device_id == CIRRUS_ID_CLGD5446) {
+        pci_register_bar(&d->dev, 1, 0, &s->cirrus_mmio_io);
+    }
 }
 
 static Property pci_vga_cirrus_properties[] = {
-- 
2.27.0

[PATCH 2/3] hw/display: Allow vga_common_init() to return errors

[Thomas Huth]

The vga_common_init() function currently cannot report errors to its
caller. But in the following patch, we'd need this possibility, so
let's change it to take an "Error **" as parameter for this.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/display/ati.c            |  7 ++++++-
 hw/display/cirrus_vga.c     |  7 ++++++-
 hw/display/cirrus_vga_isa.c |  7 ++++++-
 hw/display/qxl.c            |  6 +++++-
 hw/display/vga-isa.c        |  9 ++++++++-
 hw/display/vga-mmio.c       |  8 +++++++-
 hw/display/vga-pci.c        | 15 +++++++++++++--
 hw/display/vga.c            |  9 +++++++--
 hw/display/vga_int.h        |  2 +-
 hw/display/virtio-vga.c     |  7 ++++++-
 hw/display/vmware_vga.c     |  2 +-
 11 files changed, 66 insertions(+), 13 deletions(-)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index 31f22754dc..d2def7a4d3 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -926,6 +926,7 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
 {
     ATIVGAState *s = ATI_VGA(dev);
     VGACommonState *vga = &s->vga;
+    Error *local_err = NULL;
 
     if (s->model) {
         int i;
@@ -955,7 +956,11 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
     }
 
     /* init vga bits */
-    vga_common_init(vga, OBJECT(s));
+    vga_common_init(vga, OBJECT(s), &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
     vga_init(vga, OBJECT(s), pci_address_space(dev),
              pci_address_space_io(dev), true);
     vga->con = graphic_console_init(DEVICE(s), 0, s->vga.hw_ops, &s->vga);
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 7da1be3e12..037f842322 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -2944,6 +2944,7 @@ static void pci_cirrus_vga_realize(PCIDevice *dev, Error **errp)
     CirrusVGAState *s = &d->cirrus_vga;
     PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
     int16_t device_id = pc->device_id;
+    Error *local_err = NULL;
 
     /*
      * Follow real hardware, cirrus card emulated has 4 MB video memory.
@@ -2956,7 +2957,11 @@ static void pci_cirrus_vga_realize(PCIDevice *dev, Error **errp)
         return;
     }
     /* setup VGA */
-    vga_common_init(&s->vga, OBJECT(dev));
+    vga_common_init(&s->vga, OBJECT(dev), &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
     cirrus_init_common(s, OBJECT(dev), device_id, 1, pci_address_space(dev),
                        pci_address_space_io(dev));
     s->vga.con = graphic_console_init(DEVICE(dev), 0, s->vga.hw_ops, &s->vga);
diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
index 4f6fb1af3b..8ac017d0ce 100644
--- a/hw/display/cirrus_vga_isa.c
+++ b/hw/display/cirrus_vga_isa.c
@@ -46,6 +46,7 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev, Error **errp)
     ISADevice *isadev = ISA_DEVICE(dev);
     ISACirrusVGAState *d = ISA_CIRRUS_VGA(dev);
     VGACommonState *s = &d->cirrus_vga.vga;
+    Error *local_err = NULL;
 
     /* follow real hardware, cirrus card emulated has 4 MB video memory.
        Also accept 8 MB/16 MB for backward compatibility. */
@@ -56,7 +57,11 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev, Error **errp)
         return;
     }
     s->global_vmstate = true;
-    vga_common_init(s, OBJECT(dev));
+    vga_common_init(s, OBJECT(dev), &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
     cirrus_init_common(&d->cirrus_vga, OBJECT(dev), CIRRUS_ID_CLGD5430, 0,
                        isa_address_space(isadev),
                        isa_address_space_io(isadev));
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 1f9ad31943..adbdbcaeb6 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2215,7 +2215,11 @@ static void qxl_realize_primary(PCIDevice *dev, Error **errp)
     qxl_init_ramsize(qxl);
     vga->vbe_size = qxl->vgamem_size;
     vga->vram_size_mb = qxl->vga.vram_size / MiB;
-    vga_common_init(vga, OBJECT(dev));
+    vga_common_init(vga, OBJECT(dev), &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
     vga_init(vga, OBJECT(dev),
              pci_address_space(dev), pci_address_space_io(dev), false);
     portio_list_init(&qxl->vga_port_list, OBJECT(dev), qxl_vga_portio_list,
diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index 90851e730b..ba0f679a2d 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -25,6 +25,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/isa/isa.h"
 #include "vga_int.h"
 #include "ui/pixel_ops.h"
@@ -60,9 +61,15 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
     VGACommonState *s = &d->state;
     MemoryRegion *vga_io_memory;
     const MemoryRegionPortio *vga_ports, *vbe_ports;
+    Error *local_err = NULL;
 
     s->global_vmstate = true;
-    vga_common_init(s, OBJECT(dev));
+    vga_common_init(s, OBJECT(dev), &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
     s->legacy_address_space = isa_address_space(isadev);
     vga_io_memory = vga_init_io(s, OBJECT(dev), &vga_ports, &vbe_ports);
     isa_register_portio_list(isadev, &d->portio_vga,
diff --git a/hw/display/vga-mmio.c b/hw/display/vga-mmio.c
index 4969368081..2c1b7d9973 100644
--- a/hw/display/vga-mmio.c
+++ b/hw/display/vga-mmio.c
@@ -88,6 +88,7 @@ static void vga_mmio_realizefn(DeviceState *dev, Error **errp)
 {
     VGAMmioState *s = VGA_MMIO(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    Error *local_err = NULL;
 
     memory_region_init_io(&s->iomem, OBJECT(dev), &vga_mm_ctrl_ops, s,
                           "vga-mmio", 0x100000);
@@ -102,7 +103,12 @@ static void vga_mmio_realizefn(DeviceState *dev, Error **errp)
 
     s->vga.bank_offset = 0;
     s->vga.global_vmstate = true;
-    vga_common_init(&s->vga, OBJECT(dev));
+    vga_common_init(&s->vga, OBJECT(dev), &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
     sysbus_init_mmio(sbd, &s->vga.vram);
     s->vga.con = graphic_console_init(dev, 0, s->vga.hw_ops, &s->vga);
 }
diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
index 62fb5c38c1..0ce159fc12 100644
--- a/hw/display/vga-pci.c
+++ b/hw/display/vga-pci.c
@@ -25,6 +25,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/pci/pci.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
@@ -235,11 +236,16 @@ static void pci_std_vga_realize(PCIDevice *dev, Error **errp)
 {
     PCIVGAState *d = PCI_VGA(dev);
     VGACommonState *s = &d->vga;
+    Error *local_err = NULL;
     bool qext = false;
     bool edid = false;
 
     /* vga + console init */
-    vga_common_init(s, OBJECT(dev));
+    vga_common_init(s, OBJECT(dev), &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
     vga_init(s, OBJECT(dev), pci_address_space(dev), pci_address_space_io(dev),
              true);
 
@@ -271,11 +277,16 @@ static void pci_secondary_vga_realize(PCIDevice *dev, Error **errp)
 {
     PCIVGAState *d = PCI_VGA(dev);
     VGACommonState *s = &d->vga;
+    Error *local_err = NULL;
     bool qext = false;
     bool edid = false;
 
     /* vga + console init */
-    vga_common_init(s, OBJECT(dev));
+    vga_common_init(s, OBJECT(dev), &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
     s->con = graphic_console_init(DEVICE(dev), 0, s->hw_ops, s);
 
     /* mmio bar */
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 9d1f66af40..7fc6ab7e4f 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2168,9 +2168,10 @@ static inline uint32_t uint_clamp(uint32_t val, uint32_t vmin, uint32_t vmax)
     return val;
 }
 
-void vga_common_init(VGACommonState *s, Object *obj)
+void vga_common_init(VGACommonState *s, Object *obj, Error **errp)
 {
     int i, j, v, b;
+    Error *local_err = NULL;
 
     for(i = 0;i < 256; i++) {
         v = 0;
@@ -2206,7 +2207,11 @@ void vga_common_init(VGACommonState *s, Object *obj)
 
     s->is_vbe_vmstate = 1;
     memory_region_init_ram_nomigrate(&s->vram, obj, "vga.vram", s->vram_size,
-                           &error_fatal);
+                                     &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
     vmstate_register_ram(&s->vram, s->global_vmstate ? NULL : DEVICE(obj));
     xen_register_framebuffer(&s->vram);
     s->vram_ptr = memory_region_get_ram_ptr(&s->vram);
diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index 847e784ca6..3e8892df28 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -156,7 +156,7 @@ static inline int c6_to_8(int v)
     return (v << 2) | (b << 1) | b;
 }
 
-void vga_common_init(VGACommonState *s, Object *obj);
+void vga_common_init(VGACommonState *s, Object *obj, Error **errp);
 void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
               MemoryRegion *address_space_io, bool init_vga_ports);
 MemoryRegion *vga_init_io(VGACommonState *s, Object *obj,
diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index 5a2f7a4540..4276c49b93 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -103,12 +103,17 @@ static void virtio_vga_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     VirtIOVGABase *vvga = VIRTIO_VGA_BASE(vpci_dev);
     VirtIOGPUBase *g = vvga->vgpu;
     VGACommonState *vga = &vvga->vga;
+    Error *local_err = NULL;
     uint32_t offset;
     int i;
 
     /* init vga compat bits */
     vga->vram_size_mb = 8;
-    vga_common_init(vga, OBJECT(vpci_dev));
+    vga_common_init(vga, OBJECT(vpci_dev), &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
     vga_init(vga, OBJECT(vpci_dev), pci_address_space(&vpci_dev->pci_dev),
              pci_address_space_io(&vpci_dev->pci_dev), true);
     pci_register_bar(&vpci_dev->pci_dev, 0,
diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 0cc43a1f15..98c83474ad 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1254,7 +1254,7 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s,
                            &error_fatal);
     s->fifo_ptr = memory_region_get_ram_ptr(&s->fifo_ram);
 
-    vga_common_init(&s->vga, OBJECT(dev));
+    vga_common_init(&s->vga, OBJECT(dev), &error_fatal);
     vga_init(&s->vga, OBJECT(dev), address_space, io, true);
     vmstate_register(NULL, 0, &vmstate_vga_common, &s->vga);
     s->new_depth = 32;
-- 
2.27.0

[PATCH 3/3] hw/display/vga: Report a proper error when adding a 2nd ISA VGA

[Thomas Huth]

QEMU currently abort()s if the user tries to add a second ISA VGA
device, for example:

$ ./qemu-system-x86_64 -device isa-vga -device isa-vga
RAMBlock "vga.vram" already registered, abort!
Aborted (core dumped)
$ ./qemu-system-x86_64 -device isa-cirrus-vga -device isa-cirrus-vga
RAMBlock "vga.vram" already registered, abort!
Aborted (core dumped)
$ ./qemu-system-mips64el -M pica61 -device isa-vga
RAMBlock "vga.vram" already registered, abort!
Aborted (core dumped)

Such a crash should never happen just because of giving bad parameters
at the command line. Let's return a proper error message instead.
(The idea is based on an original patch by Jose R. Ziviani for the
isa-vga device, but this now fixes it for the isa-cirrus-vga device, too)

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/44
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/display/vga.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 7fc6ab7e4f..1dca3dd7c0 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2206,6 +2206,12 @@ void vga_common_init(VGACommonState *s, Object *obj, Error **errp)
     s->vbe_size_mask = s->vbe_size - 1;
 
     s->is_vbe_vmstate = 1;
+
+    if (s->global_vmstate && qemu_ram_block_by_name("vga.vram")) {
+        error_setg(errp, "A global VGA device has already been registered");
+        return;
+    }
+
     memory_region_init_ram_nomigrate(&s->vram, obj, "vga.vram", s->vram_size,
                                      &local_err);
     if (local_err) {
-- 
2.27.0

Re: [PATCH 2/3] hw/display: Allow vga_common_init() to return errors

[Philippe Mathieu-Daudé]

On 16/3/22 14:24, Thomas Huth wrote:
> The vga_common_init() function currently cannot report errors to its
> caller. But in the following patch, we'd need this possibility, so
> let's change it to take an "Error **" as parameter for this.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/display/ati.c            |  7 ++++++-
>   hw/display/cirrus_vga.c     |  7 ++++++-
>   hw/display/cirrus_vga_isa.c |  7 ++++++-
>   hw/display/qxl.c            |  6 +++++-
>   hw/display/vga-isa.c        |  9 ++++++++-
>   hw/display/vga-mmio.c       |  8 +++++++-
>   hw/display/vga-pci.c        | 15 +++++++++++++--
>   hw/display/vga.c            |  9 +++++++--
>   hw/display/vga_int.h        |  2 +-
>   hw/display/virtio-vga.c     |  7 ++++++-
>   hw/display/vmware_vga.c     |  2 +-
>   11 files changed, 66 insertions(+), 13 deletions(-)

Please setup scripts/git.orderfile :)

> diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
> index 847e784ca6..3e8892df28 100644
> --- a/hw/display/vga_int.h
> +++ b/hw/display/vga_int.h
> @@ -156,7 +156,7 @@ static inline int c6_to_8(int v)
>       return (v << 2) | (b << 1) | b;
>   }
>   
> -void vga_common_init(VGACommonState *s, Object *obj);
> +void vga_common_init(VGACommonState *s, Object *obj, Error **errp);

Can we also return a boolean value? IIUC Markus recommended to check
a boolean return value rather than Error* handle.

Re: [PATCH 1/3] hw/display/cirrus_vga: Clean up indentation in pci_cirrus_vga_realize()

[Philippe Mathieu-Daudé]

On 16/3/22 14:24, Thomas Huth wrote:
> Most of the code in this function had been indented with 5 spaces instead
> of 4. Since 4 is our preferred style, remove one space in the bad lines here.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/display/cirrus_vga.c | 57 +++++++++++++++++++++--------------------
>   1 file changed, 29 insertions(+), 28 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 2/3] hw/display: Allow vga_common_init() to return errors

[Thomas Huth]

On 16/03/2022 14.32, Philippe Mathieu-Daudé wrote:
> On 16/3/22 14:24, Thomas Huth wrote:
>> The vga_common_init() function currently cannot report errors to its
>> caller. But in the following patch, we'd need this possibility, so
>> let's change it to take an "Error **" as parameter for this.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   hw/display/ati.c            |  7 ++++++-
>>   hw/display/cirrus_vga.c     |  7 ++++++-
>>   hw/display/cirrus_vga_isa.c |  7 ++++++-
>>   hw/display/qxl.c            |  6 +++++-
>>   hw/display/vga-isa.c        |  9 ++++++++-
>>   hw/display/vga-mmio.c       |  8 +++++++-
>>   hw/display/vga-pci.c        | 15 +++++++++++++--
>>   hw/display/vga.c            |  9 +++++++--
>>   hw/display/vga_int.h        |  2 +-
>>   hw/display/virtio-vga.c     |  7 ++++++-
>>   hw/display/vmware_vga.c     |  2 +-
>>   11 files changed, 66 insertions(+), 13 deletions(-)
> 
> Please setup scripts/git.orderfile :)
> 
>> diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
>> index 847e784ca6..3e8892df28 100644
>> --- a/hw/display/vga_int.h
>> +++ b/hw/display/vga_int.h
>> @@ -156,7 +156,7 @@ static inline int c6_to_8(int v)
>>       return (v << 2) | (b << 1) | b;
>>   }
>> -void vga_common_init(VGACommonState *s, Object *obj);
>> +void vga_common_init(VGACommonState *s, Object *obj, Error **errp);
> 
> Can we also return a boolean value? IIUC Markus recommended to check
> a boolean return value rather than Error* handle.

Really? A very quick grep shows something different:

$ grep -r ^void.*Error include/ | wc -l
94
$ grep -r ^bool.*Error include/ | wc -l
46

I also can't see that recommendation in docs/devel/style.rst. I think you 
either got that wrong, or the coding style needs an update first.

  Thomas

Re: [PATCH 2/3] hw/display: Allow vga_common_init() to return errors

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 16/03/2022 14.32, Philippe Mathieu-Daudé wrote:
>> On 16/3/22 14:24, Thomas Huth wrote:
>>> The vga_common_init() function currently cannot report errors to its
>>> caller. But in the following patch, we'd need this possibility, so
>>> let's change it to take an "Error **" as parameter for this.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   hw/display/ati.c            |  7 ++++++-
>>>   hw/display/cirrus_vga.c     |  7 ++++++-
>>>   hw/display/cirrus_vga_isa.c |  7 ++++++-
>>>   hw/display/qxl.c            |  6 +++++-
>>>   hw/display/vga-isa.c        |  9 ++++++++-
>>>   hw/display/vga-mmio.c       |  8 +++++++-
>>>   hw/display/vga-pci.c        | 15 +++++++++++++--
>>>   hw/display/vga.c            |  9 +++++++--
>>>   hw/display/vga_int.h        |  2 +-
>>>   hw/display/virtio-vga.c     |  7 ++++++-
>>>   hw/display/vmware_vga.c     |  2 +-
>>>   11 files changed, 66 insertions(+), 13 deletions(-)
>>
>> Please setup scripts/git.orderfile :)
>> 
>>> diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
>>> index 847e784ca6..3e8892df28 100644
>>> --- a/hw/display/vga_int.h
>>> +++ b/hw/display/vga_int.h
>>> @@ -156,7 +156,7 @@ static inline int c6_to_8(int v)
>>>       return (v << 2) | (b << 1) | b;
>>>   }
>>> -void vga_common_init(VGACommonState *s, Object *obj);
>>> +void vga_common_init(VGACommonState *s, Object *obj, Error **errp);
>> 
>> Can we also return a boolean value? IIUC Markus recommended to check
>> a boolean return value rather than Error* handle.
>
> Really? A very quick grep shows something different:
>
> $ grep -r ^void.*Error include/ | wc -l
> 94
> $ grep -r ^bool.*Error include/ | wc -l
> 46

Historical reasons.  We deviated from GLib here only to find out that
the deviation leads to awkward code.  I flipped the guidance in commit
e3fe3988d7 "error: Document Error API usage rules" (2020-07-10).  A lot
of old code remains.

> I also can't see that recommendation in docs/devel/style.rst. I think you 
> either got that wrong, or the coding style needs an update first.

It's in include/qapi/error.h:

/*
 * Error reporting system loosely patterned after Glib's GError.
 *
 * = Rules =

[...]

 * - Whenever practical, also return a value that indicates success /
 *   failure.  This can make the error checking more concise, and can
 *   avoid useless error object creation and destruction.  Note that
 *   we still have many functions returning void.  We recommend
 *   • bool-valued functions return true on success / false on failure,
 *   • pointer-valued functions return non-null / null pointer, and
 *   • integer-valued functions return non-negative / negative.

[...]

 * Call a function, receive an error from it, and pass it to the caller
 * - when the function returns a value that indicates failure, say
 *   false:
 *     if (!foo(arg, errp)) {
 *         handle the error...
 *     }
 * - when it does not, say because it is a void function:
 *     ERRP_GUARD();
 *     foo(arg, errp);
 *     if (*errp) {
 *         handle the error...
 *     }
 * More on ERRP_GUARD() below.

[...]

 * Receive an error, and handle it locally
 * - when the function returns a value that indicates failure, say
 *   false:
 *     Error *err = NULL;
 *     if (!foo(arg, &err)) {
 *         handle the error...
 *     }
 * - when it does not, say because it is a void function:
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     if (err) {
 *         handle the error...
 *     }

Note that error.h's big comment I abbreviated here has some 200
non-blank lines.  It's too long and detailed for style.rst (which has
some 500 non-blank lines).  Instead style.rst points to error.h under
QEMU Specific Idioms / Error handling and reporting / Propagating
errors.

Re: [PATCH 2/3] hw/display: Allow vga_common_init() to return errors

[Thomas Huth]

On 16/03/2022 15.16, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 16/03/2022 14.32, Philippe Mathieu-Daudé wrote:
>>> On 16/3/22 14:24, Thomas Huth wrote:
>>>> The vga_common_init() function currently cannot report errors to its
>>>> caller. But in the following patch, we'd need this possibility, so
>>>> let's change it to take an "Error **" as parameter for this.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>    hw/display/ati.c            |  7 ++++++-
>>>>    hw/display/cirrus_vga.c     |  7 ++++++-
>>>>    hw/display/cirrus_vga_isa.c |  7 ++++++-
>>>>    hw/display/qxl.c            |  6 +++++-
>>>>    hw/display/vga-isa.c        |  9 ++++++++-
>>>>    hw/display/vga-mmio.c       |  8 +++++++-
>>>>    hw/display/vga-pci.c        | 15 +++++++++++++--
>>>>    hw/display/vga.c            |  9 +++++++--
>>>>    hw/display/vga_int.h        |  2 +-
>>>>    hw/display/virtio-vga.c     |  7 ++++++-
>>>>    hw/display/vmware_vga.c     |  2 +-
>>>>    11 files changed, 66 insertions(+), 13 deletions(-)
>>>
>>> Please setup scripts/git.orderfile :)
>>>
>>>> diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
>>>> index 847e784ca6..3e8892df28 100644
>>>> --- a/hw/display/vga_int.h
>>>> +++ b/hw/display/vga_int.h
>>>> @@ -156,7 +156,7 @@ static inline int c6_to_8(int v)
>>>>        return (v << 2) | (b << 1) | b;
>>>>    }
>>>> -void vga_common_init(VGACommonState *s, Object *obj);
>>>> +void vga_common_init(VGACommonState *s, Object *obj, Error **errp);
>>>
>>> Can we also return a boolean value? IIUC Markus recommended to check
>>> a boolean return value rather than Error* handle.
>>
>> Really? A very quick grep shows something different:
>>
>> $ grep -r ^void.*Error include/ | wc -l
>> 94
>> $ grep -r ^bool.*Error include/ | wc -l
>> 46
> 
> Historical reasons.  We deviated from GLib here only to find out that
> the deviation leads to awkward code.  I flipped the guidance in commit
> e3fe3988d7 "error: Document Error API usage rules" (2020-07-10).  A lot
> of old code remains.

Hmm, you should add some BiteSizeTasks to our issue tracker then to get this 
fixed, otherwise people like me will copy-n-paste the bad code examples that 
are all over the place!

  Thomas

Re: [PATCH 2/3] hw/display: Allow vga_common_init() to return errors

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 16/03/2022 15.16, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> On 16/03/2022 14.32, Philippe Mathieu-Daudé wrote:
>>>> On 16/3/22 14:24, Thomas Huth wrote:
>>>>> The vga_common_init() function currently cannot report errors to its
>>>>> caller. But in the following patch, we'd need this possibility, so
>>>>> let's change it to take an "Error **" as parameter for this.
>>>>>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>    hw/display/ati.c            |  7 ++++++-
>>>>>    hw/display/cirrus_vga.c     |  7 ++++++-
>>>>>    hw/display/cirrus_vga_isa.c |  7 ++++++-
>>>>>    hw/display/qxl.c            |  6 +++++-
>>>>>    hw/display/vga-isa.c        |  9 ++++++++-
>>>>>    hw/display/vga-mmio.c       |  8 +++++++-
>>>>>    hw/display/vga-pci.c        | 15 +++++++++++++--
>>>>>    hw/display/vga.c            |  9 +++++++--
>>>>>    hw/display/vga_int.h        |  2 +-
>>>>>    hw/display/virtio-vga.c     |  7 ++++++-
>>>>>    hw/display/vmware_vga.c     |  2 +-
>>>>>    11 files changed, 66 insertions(+), 13 deletions(-)
>>>>
>>>> Please setup scripts/git.orderfile :)
>>>>
>>>>> diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
>>>>> index 847e784ca6..3e8892df28 100644
>>>>> --- a/hw/display/vga_int.h
>>>>> +++ b/hw/display/vga_int.h
>>>>> @@ -156,7 +156,7 @@ static inline int c6_to_8(int v)
>>>>>        return (v << 2) | (b << 1) | b;
>>>>>    }
>>>>> -void vga_common_init(VGACommonState *s, Object *obj);
>>>>> +void vga_common_init(VGACommonState *s, Object *obj, Error **errp);
>>>>
>>>> Can we also return a boolean value? IIUC Markus recommended to check
>>>> a boolean return value rather than Error* handle.
>>>
>>> Really? A very quick grep shows something different:
>>>
>>> $ grep -r ^void.*Error include/ | wc -l
>>> 94
>>> $ grep -r ^bool.*Error include/ | wc -l
>>> 46
>>
>> Historical reasons.  We deviated from GLib here only to find out that
>> the deviation leads to awkward code.  I flipped the guidance in commit
>> e3fe3988d7 "error: Document Error API usage rules" (2020-07-10).  A lot
>> of old code remains.
>
> Hmm, you should add some BiteSizeTasks to our issue tracker then to
> get this fixed, otherwise people like me will copy-n-paste the bad
> code examples that are all over the place!

I'm not sure the issue tracker is a good fit here.  An issue tracker
works best when you use it to track units of work that can be completed
in one go.  An issue then tracks progress of its unit of work.

This isn't a unit, it's more like a class of units.

I added an item to https://wiki.qemu.org/ToDo/CodeTransitions for now.

Re: [PATCH 2/3] hw/display: Allow vga_common_init() to return errors

[Thomas Huth]

On 17/03/2022 08.40, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 16/03/2022 15.16, Markus Armbruster wrote:
>>> Thomas Huth <thuth@redhat.com> writes:
>>>
>>>> On 16/03/2022 14.32, Philippe Mathieu-Daudé wrote:
>>>>> On 16/3/22 14:24, Thomas Huth wrote:
>>>>>> The vga_common_init() function currently cannot report errors to its
>>>>>> caller. But in the following patch, we'd need this possibility, so
>>>>>> let's change it to take an "Error **" as parameter for this.
>>>>>>
>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>> ---
>>>>>>     hw/display/ati.c            |  7 ++++++-
>>>>>>     hw/display/cirrus_vga.c     |  7 ++++++-
>>>>>>     hw/display/cirrus_vga_isa.c |  7 ++++++-
>>>>>>     hw/display/qxl.c            |  6 +++++-
>>>>>>     hw/display/vga-isa.c        |  9 ++++++++-
>>>>>>     hw/display/vga-mmio.c       |  8 +++++++-
>>>>>>     hw/display/vga-pci.c        | 15 +++++++++++++--
>>>>>>     hw/display/vga.c            |  9 +++++++--
>>>>>>     hw/display/vga_int.h        |  2 +-
>>>>>>     hw/display/virtio-vga.c     |  7 ++++++-
>>>>>>     hw/display/vmware_vga.c     |  2 +-
>>>>>>     11 files changed, 66 insertions(+), 13 deletions(-)
>>>>>
>>>>> Please setup scripts/git.orderfile :)
>>>>>
>>>>>> diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
>>>>>> index 847e784ca6..3e8892df28 100644
>>>>>> --- a/hw/display/vga_int.h
>>>>>> +++ b/hw/display/vga_int.h
>>>>>> @@ -156,7 +156,7 @@ static inline int c6_to_8(int v)
>>>>>>         return (v << 2) | (b << 1) | b;
>>>>>>     }
>>>>>> -void vga_common_init(VGACommonState *s, Object *obj);
>>>>>> +void vga_common_init(VGACommonState *s, Object *obj, Error **errp);
>>>>>
>>>>> Can we also return a boolean value? IIUC Markus recommended to check
>>>>> a boolean return value rather than Error* handle.
>>>>
>>>> Really? A very quick grep shows something different:
>>>>
>>>> $ grep -r ^void.*Error include/ | wc -l
>>>> 94
>>>> $ grep -r ^bool.*Error include/ | wc -l
>>>> 46
>>>
>>> Historical reasons.  We deviated from GLib here only to find out that
>>> the deviation leads to awkward code.  I flipped the guidance in commit
>>> e3fe3988d7 "error: Document Error API usage rules" (2020-07-10).  A lot
>>> of old code remains.
>>
>> Hmm, you should add some BiteSizeTasks to our issue tracker then to
>> get this fixed, otherwise people like me will copy-n-paste the bad
>> code examples that are all over the place!
> 
> I'm not sure the issue tracker is a good fit here.  An issue tracker
> works best when you use it to track units of work that can be completed
> in one go.  An issue then tracks progress of its unit of work.

That's why I wrote "*some* BiteSizeTasks", not "one BitSizeTask" ... of 
course you've got to break it down for unexperienced new developers first, 
e.g. by subsystem. I did that for example for the indentation clean up tasks:

  https://gitlab.com/qemu-project/qemu/-/issues/376
  https://gitlab.com/qemu-project/qemu/-/issues/371
  https://gitlab.com/qemu-project/qemu/-/issues/372

> This isn't a unit, it's more like a class of units.
> 
> I added an item to https://wiki.qemu.org/ToDo/CodeTransitions for now.

Thanks, but I doubt that this will help much - the description is really 
terse, and for anybody who is not involved in this email thread here, I 
guess it's hard for them to figure out the related parts in the 
include/qapi/error.h on their own ... so if you really want somebody else to 
work on this topic, I think you have to elaborate there a little bit (e.g. 
by giving an example in-place).

  Thomas

Re: [PATCH 2/3] hw/display: Allow vga_common_init() to return errors

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 17/03/2022 08.40, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> On 16/03/2022 15.16, Markus Armbruster wrote:
>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>
>>>>> On 16/03/2022 14.32, Philippe Mathieu-Daudé wrote:
>>>>>> On 16/3/22 14:24, Thomas Huth wrote:
>>>>>>> The vga_common_init() function currently cannot report errors to its
>>>>>>> caller. But in the following patch, we'd need this possibility, so
>>>>>>> let's change it to take an "Error **" as parameter for this.
>>>>>>>
>>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>

[...]

>>>> Historical reasons.  We deviated from GLib here only to find out that
>>>> the deviation leads to awkward code.  I flipped the guidance in commit
>>>> e3fe3988d7 "error: Document Error API usage rules" (2020-07-10).  A lot
>>>> of old code remains.
>>>
>>> Hmm, you should add some BiteSizeTasks to our issue tracker then to
>>> get this fixed, otherwise people like me will copy-n-paste the bad
>>> code examples that are all over the place!
>>
>> I'm not sure the issue tracker is a good fit here.  An issue tracker
>> works best when you use it to track units of work that can be completed
>> in one go.  An issue then tracks progress of its unit of work.
>
> That's why I wrote "*some* BiteSizeTasks", not "one BitSizeTask"
> ... of course you've got to break it down for unexperienced new
> developers first, e.g. by subsystem. I did that for example for the
> indentation clean up tasks:
>
>  https://gitlab.com/qemu-project/qemu/-/issues/376
>  https://gitlab.com/qemu-project/qemu/-/issues/371
>  https://gitlab.com/qemu-project/qemu/-/issues/372

Okay, I get you now.

When I flipped the guidance, I also updated a substantial amount of code
to honor the rule:

    Subject: [PATCH v4 00/45] Less clumsy error checking
    Date: Tue,  7 Jul 2020 18:05:28 +0200

    [...]

    This series adopts the GError rule (in writing), and updates a
    substantial amount of code to honor the rule.  Cuts the number of
    error_propagate() calls nearly by half.  The diffstat speaks for
    itself.

    https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg02580.html

The "substantial amount of code" were a few subsystems with many, many
callers.  What remains, as far as I can tell, is many functions with few
callers each.

If I run into a bite-sized conversion task I'd like a volunteer to
tackle, I should file an issue for it.

But collecting the everything to be converted, then partitioning it into
sane parts (*many* parts), then creating an issue for each, is not going
to happen.  Nobody got time for that.

>> This isn't a unit, it's more like a class of units.
>> I added an item to https://wiki.qemu.org/ToDo/CodeTransitions for
>> now.
>
> Thanks, but I doubt that this will help much - the description is
> really terse, and for anybody who is not involved in this email thread
> here, I guess it's hard for them to figure out the related parts in
> the include/qapi/error.h on their own ... so if you really want
> somebody else to work on this topic, I think you have to elaborate
> there a little bit (e.g. by giving an example in-place).

"as explained in include/qapi/error.h" points to the full explanation,
which includes examples.