[Qemu-devel] [PATCH 00/12] Clean up some hw_error() misuse

[Qemu-devel] [PATCH 00/12] Clean up some hw_error() misuse

[Markus Armbruster]

I intend to take this trough my tree, but I'm certainly fine with
maintainers picking up their parts if that makes their job easier.
Just be clear about it.

Markus Armbruster (12):
  hw: Don't use hw_error() for machine initialization errors
  omap: Don't use hw_error() in device init() methods
  arm_mptimer: Don't use hw_error() in realize() method
  etraxfs_eth: Don't use hw_error() in init() method
  raven: Mark use of hw_error() in realize() FIXME
  hw/arm/virt: Fix property "gic-version" error handling
  sysbus: Don't use hw_error() in machine_init_done_notifiers
  isa: Trivially convert remaining PCI-ISA bridges to realize()
  isa: Clean up error handling around isa_bus_new()
  isa: Clean up inappropriate hw_error()
  audio: Clean up inappropriate and unreachable use of hw_error()
  xen-hvm: Mark inappropriate error handling FIXME

 audio/audio.c             | 11 ++---------
 hw/alpha/dp264.c          | 11 ++++++-----
 hw/alpha/typhoon.c        |  3 ++-
 hw/arm/highbank.c         |  6 ++++--
 hw/arm/virt.c             |  5 ++---
 hw/char/exynos4210_uart.c |  9 ++++++---
 hw/core/platform-bus.c    | 25 ++++++++++++-------------
 hw/gpio/omap_gpio.c       | 19 +++++++++++++++----
 hw/i2c/omap_i2c.c         |  8 ++++++--
 hw/i386/pc_piix.c         |  3 ++-
 hw/intc/omap_intc.c       | 10 +++++++---
 hw/isa/i82378.c           |  5 ++++-
 hw/isa/isa-bus.c          | 15 ++-------------
 hw/isa/lpc_ich9.c         | 11 +++++++----
 hw/isa/piix4.c            |  6 ++++--
 hw/isa/vt82c686.c         |  5 ++++-
 hw/m68k/an5206.c          |  4 +++-
 hw/mips/mips_jazz.c       |  2 +-
 hw/mips/mips_r4k.c        |  2 +-
 hw/net/etraxfs_eth.c      |  4 +++-
 hw/pci-host/piix.c        |  6 ++++--
 hw/pci-host/prep.c        |  6 ++++--
 hw/ppc/mac_newworld.c     | 11 ++++++-----
 hw/ppc/mac_oldworld.c     | 16 +++++++++-------
 hw/ppc/prep.c             | 10 ++++++----
 hw/sparc64/sun4u.c        | 12 ++++++------
 hw/timer/arm_mptimer.c    |  5 +++--
 hw/unicore32/puv3.c       | 10 +++++++---
 include/hw/isa/isa.h      |  2 +-
 xen-hvm.c                 |  7 +++++++
 30 files changed, 146 insertions(+), 103 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 01/12] hw: Don't use hw_error() for machine initialization errors

[Markus Armbruster]

Printing CPU registers is not helpful during machine initialization.
Moreover, these are straightforward configuration or "can get
resources" errors, so dumping core isn't appropriate either.  Replace
hw_error() by error_report(); exit(1).  Matches how we report these
errors in other machine initializations.

Cc: Richard Henderson <rth@twiddle.net>
Cc: qemu-arm@nongnu.org
Cc: qemu-ppc@nongnu.org
Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/alpha/dp264.c          | 11 ++++++-----
 hw/arm/highbank.c         |  6 ++++--
 hw/char/exynos4210_uart.c |  9 ++++++---
 hw/m68k/an5206.c          |  4 +++-
 hw/ppc/mac_newworld.c     | 11 ++++++-----
 hw/ppc/mac_oldworld.c     | 16 +++++++++-------
 hw/ppc/prep.c             | 10 ++++++----
 hw/unicore32/puv3.c       | 10 +++++++---
 8 files changed, 47 insertions(+), 30 deletions(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 27bdaa1..38b85ba 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -11,6 +11,7 @@
 #include "hw/loader.h"
 #include "hw/boards.h"
 #include "alpha_sys.h"
+#include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 #include "hw/timer/mc146818rtc.h"
 #include "hw/ide.h"
@@ -104,14 +105,14 @@ static void clipper_init(MachineState *machine)
     palcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS,
                                 bios_name ? bios_name : "palcode-clipper");
     if (palcode_filename == NULL) {
-        hw_error("no palcode provided\n");
+        error_report("no palcode provided");
         exit(1);
     }
     size = load_elf(palcode_filename, cpu_alpha_superpage_to_phys,
                     NULL, &palcode_entry, &palcode_low, &palcode_high,
                     0, EM_ALPHA, 0);
     if (size < 0) {
-        hw_error("could not load palcode '%s'\n", palcode_filename);
+        error_report("could not load palcode '%s'", palcode_filename);
         exit(1);
     }
     g_free(palcode_filename);
@@ -131,7 +132,7 @@ static void clipper_init(MachineState *machine)
                         NULL, &kernel_entry, &kernel_low, &kernel_high,
                         0, EM_ALPHA, 0);
         if (size < 0) {
-            hw_error("could not load kernel '%s'\n", kernel_filename);
+            error_report("could not load kernel '%s'", kernel_filename);
             exit(1);
         }
 
@@ -148,8 +149,8 @@ static void clipper_init(MachineState *machine)
 
             initrd_size = get_image_size(initrd_filename);
             if (initrd_size < 0) {
-                hw_error("could not load initial ram disk '%s'\n",
-                         initrd_filename);
+                error_report("could not load initial ram disk '%s'",
+                             initrd_filename);
                 exit(1);
             }
 
diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index 85ae69e..1dd5170 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -320,11 +320,13 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
         sysboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
         if (sysboot_filename != NULL) {
             if (load_image_targphys(sysboot_filename, 0xfff88000, 0x8000) < 0) {
-                hw_error("Unable to load %s\n", bios_name);
+                error_report("Unable to load %s", bios_name);
+                exit(1);
             }
             g_free(sysboot_filename);
         } else {
-           hw_error("Unable to find %s\n", bios_name);
+            error_report("Unable to find %s", bios_name);
+            exit(1);
         }
     }
 
diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index 215f962..2736b37 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -20,6 +20,7 @@
  */
 
 #include "hw/sysbus.h"
+#include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/char.h"
 
@@ -595,15 +596,17 @@ DeviceState *exynos4210_uart_create(hwaddr addr,
 
     if (!chr) {
         if (channel >= MAX_SERIAL_PORTS) {
-            hw_error("Only %d serial ports are supported by QEMU.\n",
-                     MAX_SERIAL_PORTS);
+            error_report("Only %d serial ports are supported by QEMU",
+                         MAX_SERIAL_PORTS);
+            exit(1);
         }
         chr = serial_hds[channel];
         if (!chr) {
             snprintf(label, ARRAY_SIZE(label), "%s%d", chr_name, channel);
             chr = qemu_chr_new(label, "null", NULL);
             if (!(chr)) {
-                hw_error("Can't assign serial port to UART%d.\n", channel);
+                error_report("Can't assign serial port to UART%d", channel);
+                exit(1);
             }
         }
     }
diff --git a/hw/m68k/an5206.c b/hw/m68k/an5206.c
index c1dea17..8d9ccaa 100644
--- a/hw/m68k/an5206.c
+++ b/hw/m68k/an5206.c
@@ -12,6 +12,7 @@
 #include "hw/loader.h"
 #include "elf.h"
 #include "exec/address-spaces.h"
+#include "qemu/error-report.h"
 #include "sysemu/qtest.h"
 
 #define KERNEL_LOAD_ADDR 0x10000
@@ -39,7 +40,8 @@ static void an5206_init(MachineState *machine)
     }
     cpu = cpu_m68k_init(cpu_model);
     if (!cpu) {
-        hw_error("Unable to find m68k CPU definition\n");
+        error_report("Unable to find m68k CPU definition");
+        exit(1);
     }
     env = &cpu->env;
 
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 1b9a573..ca3d6a8 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -62,6 +62,7 @@
 #include "hw/ide.h"
 #include "hw/loader.h"
 #include "elf.h"
+#include "qemu/error-report.h"
 #include "sysemu/kvm.h"
 #include "kvm_ppc.h"
 #include "hw/usb.h"
@@ -226,7 +227,7 @@ static void ppc_core99_init(MachineState *machine)
         bios_size = -1;
     }
     if (bios_size < 0 || bios_size > BIOS_SIZE) {
-        hw_error("qemu: could not load PowerPC bios '%s'\n", bios_name);
+        error_report("could not load PowerPC bios '%s'", bios_name);
         exit(1);
     }
 
@@ -252,7 +253,7 @@ static void ppc_core99_init(MachineState *machine)
                                               kernel_base,
                                               ram_size - kernel_base);
         if (kernel_size < 0) {
-            hw_error("qemu: could not load kernel '%s'\n", kernel_filename);
+            error_report("could not load kernel '%s'", kernel_filename);
             exit(1);
         }
         /* load initrd */
@@ -261,8 +262,8 @@ static void ppc_core99_init(MachineState *machine)
             initrd_size = load_image_targphys(initrd_filename, initrd_base,
                                               ram_size - initrd_base);
             if (initrd_size < 0) {
-                hw_error("qemu: could not load initial ram disk '%s'\n",
-                         initrd_filename);
+                error_report("could not load initial ram disk '%s'",
+                             initrd_filename);
                 exit(1);
             }
             cmdline_base = round_page(initrd_base + initrd_size);
@@ -344,7 +345,7 @@ static void ppc_core99_init(MachineState *machine)
             break;
 #endif /* defined(TARGET_PPC64) */
         default:
-            hw_error("Bus model not supported on mac99 machine\n");
+            error_report("Bus model not supported on mac99 machine");
             exit(1);
         }
     }
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 21eaf0e..c3f9fe3 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -38,6 +38,7 @@
 #include "hw/ide.h"
 #include "hw/loader.h"
 #include "elf.h"
+#include "qemu/error-report.h"
 #include "sysemu/kvm.h"
 #include "kvm_ppc.h"
 #include "sysemu/block-backend.h"
@@ -153,7 +154,7 @@ static void ppc_heathrow_init(MachineState *machine)
         bios_size = -1;
     }
     if (bios_size < 0 || bios_size > BIOS_SIZE) {
-        hw_error("qemu: could not load PowerPC bios '%s'\n", bios_name);
+        error_report("could not load PowerPC bios '%s'", bios_name);
         exit(1);
     }
 
@@ -178,8 +179,7 @@ static void ppc_heathrow_init(MachineState *machine)
                                               kernel_base,
                                               ram_size - kernel_base);
         if (kernel_size < 0) {
-            hw_error("qemu: could not load kernel '%s'\n",
-                      kernel_filename);
+            error_report("could not load kernel '%s'", kernel_filename);
             exit(1);
         }
         /* load initrd */
@@ -188,8 +188,8 @@ static void ppc_heathrow_init(MachineState *machine)
             initrd_size = load_image_targphys(initrd_filename, initrd_base,
                                               ram_size - initrd_base);
             if (initrd_size < 0) {
-                hw_error("qemu: could not load initial ram disk '%s'\n",
-                         initrd_filename);
+                error_report("could not load initial ram disk '%s'",
+                             initrd_filename);
                 exit(1);
             }
             cmdline_base = round_page(initrd_base + initrd_size);
@@ -246,7 +246,8 @@ static void ppc_heathrow_init(MachineState *machine)
                 ((qemu_irq *)env->irq_inputs)[PPC6xx_INPUT_INT];
             break;
         default:
-            hw_error("Bus model not supported on OldWorld Mac machine\n");
+            error_report("Bus model not supported on OldWorld Mac machine");
+            exit(1);
         }
     }
 
@@ -259,7 +260,8 @@ static void ppc_heathrow_init(MachineState *machine)
 
     /* init basic PC hardware */
     if (PPC_INPUT(env) != PPC_FLAGS_INPUT_6xx) {
-        hw_error("Only 6xx bus is supported on heathrow machine\n");
+        error_report("Only 6xx bus is supported on heathrow machine");
+        exit(1);
     }
     pic = heathrow_pic_init(&pic_mem, 1, heathrow_irqs);
     pci_bus = pci_grackle_init(0xfec00000, pic,
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 5ad28f7..8f08f07 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -33,6 +33,7 @@
 #include "hw/pci/pci_host.h"
 #include "hw/ppc/ppc.h"
 #include "hw/boards.h"
+#include "qemu/error-report.h"
 #include "qemu/log.h"
 #include "hw/ide.h"
 #include "hw/loader.h"
@@ -532,7 +533,7 @@ static void ppc_prep_init(MachineState *machine)
         kernel_size = load_image_targphys(kernel_filename, kernel_base,
                                           ram_size - kernel_base);
         if (kernel_size < 0) {
-            hw_error("qemu: could not load kernel '%s'\n", kernel_filename);
+            error_report("could not load kernel '%s'", kernel_filename);
             exit(1);
         }
         /* load initrd */
@@ -541,8 +542,8 @@ static void ppc_prep_init(MachineState *machine)
             initrd_size = load_image_targphys(initrd_filename, initrd_base,
                                               ram_size - initrd_base);
             if (initrd_size < 0) {
-                hw_error("qemu: could not load initial ram disk '%s'\n",
-                          initrd_filename);
+                error_report("could not load initial ram disk '%s'",
+                             initrd_filename);
             }
         } else {
             initrd_base = 0;
@@ -569,7 +570,8 @@ static void ppc_prep_init(MachineState *machine)
     }
 
     if (PPC_INPUT(env) != PPC_FLAGS_INPUT_6xx) {
-        hw_error("Only 6xx bus is supported on PREP machine\n");
+        error_report("Only 6xx bus is supported on PREP machine");
+        exit(1);
     }
 
     dev = qdev_create(NULL, "raven-pcihost");
diff --git a/hw/unicore32/puv3.c b/hw/unicore32/puv3.c
index 91117b2..1968202 100644
--- a/hw/unicore32/puv3.c
+++ b/hw/unicore32/puv3.c
@@ -17,6 +17,7 @@
 #include "hw/boards.h"
 #include "hw/loader.h"
 #include "hw/i386/pc.h"
+#include "qemu/error-report.h"
 #include "sysemu/qtest.h"
 
 #undef DEBUG_PUV3
@@ -95,7 +96,8 @@ static void puv3_load_kernel(const char *kernel_filename)
     size = load_image_targphys(kernel_filename, KERNEL_LOAD_ADDR,
             KERNEL_MAX_SIZE);
     if (size < 0) {
-        hw_error("Load kernel error: '%s'\n", kernel_filename);
+        error_report("Load kernel error: '%s'", kernel_filename);
+        exit(1);
     }
 
     /* cheat curses that we have a graphic console, only under ocd console */
@@ -112,7 +114,8 @@ static void puv3_init(MachineState *machine)
     UniCore32CPU *cpu;
 
     if (initrd_filename) {
-        hw_error("Please use kernel built-in initramdisk.\n");
+        error_report("Please use kernel built-in initramdisk");
+        exit(1);
     }
 
     if (!cpu_model) {
@@ -121,7 +124,8 @@ static void puv3_init(MachineState *machine)
 
     cpu = uc32_cpu_init(cpu_model);
     if (!cpu) {
-        hw_error("Unable to find CPU definition\n");
+        error_report("Unable to find CPU definition");
+        exit(1);
     }
     env = &cpu->env;
 
-- 
2.4.3

[Qemu-devel] [PATCH 02/12] omap: Don't use hw_error() in device init() methods

[Markus Armbruster]

Device init() methods aren't supposed to call hw_error(), they should
report the error and fail cleanly.  Do that.

Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/gpio/omap_gpio.c | 19 +++++++++++++++----
 hw/i2c/omap_i2c.c   |  8 ++++++--
 hw/intc/omap_intc.c | 10 +++++++---
 3 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/hw/gpio/omap_gpio.c b/hw/gpio/omap_gpio.c
index 3c53898..1e0654b 100644
--- a/hw/gpio/omap_gpio.c
+++ b/hw/gpio/omap_gpio.c
@@ -21,6 +21,7 @@
 #include "hw/hw.h"
 #include "hw/arm/omap.h"
 #include "hw/sysbus.h"
+#include "qemu/error-report.h"
 
 struct omap_gpio_s {
     qemu_irq irq;
@@ -700,8 +701,17 @@ static int omap2_gpio_init(SysBusDevice *sbd)
     int i;
 
     if (!s->iclk) {
-        hw_error("omap2-gpio: iclk not connected\n");
+        error_report("omap2-gpio: iclk not connected");
+        return -1;
     }
+
+    for (i = 0; i < s->modulecount; i++) {
+        if (!s->fclk[i]) {
+            error_report("omap2-gpio: fclk%d not connected", i);
+            return -1;
+        }
+    }
+
     if (s->mpu_model < omap3430) {
         s->modulecount = (s->mpu_model < omap2430) ? 4 : 5;
         memory_region_init_io(&s->iomem, OBJECT(s), &omap2_gpif_top_ops, s,
@@ -710,15 +720,15 @@ static int omap2_gpio_init(SysBusDevice *sbd)
     } else {
         s->modulecount = 6;
     }
+
     s->modules = g_new0(struct omap2_gpio_s, s->modulecount);
     s->handler = g_new0(qemu_irq, s->modulecount * 32);
     qdev_init_gpio_in(dev, omap2_gpio_set, s->modulecount * 32);
     qdev_init_gpio_out(dev, s->handler, s->modulecount * 32);
+
     for (i = 0; i < s->modulecount; i++) {
         struct omap2_gpio_s *m = &s->modules[i];
-        if (!s->fclk[i]) {
-            hw_error("omap2-gpio: fclk%d not connected\n", i);
-        }
+
         m->revision = (s->mpu_model < omap3430) ? 0x18 : 0x25;
         m->handler = &s->handler[i * 32];
         sysbus_init_irq(sbd, &m->irq[0]); /* mpu irq */
@@ -728,6 +738,7 @@ static int omap2_gpio_init(SysBusDevice *sbd)
                               "omap.gpio-module", 0x1000);
         sysbus_init_mmio(sbd, &m->iomem);
     }
+
     return 0;
 }
 
diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c
index b6f544a..8b0b146 100644
--- a/hw/i2c/omap_i2c.c
+++ b/hw/i2c/omap_i2c.c
@@ -20,6 +20,7 @@
 #include "hw/i2c/i2c.h"
 #include "hw/arm/omap.h"
 #include "hw/sysbus.h"
+#include "qemu/error-report.h"
 
 #define TYPE_OMAP_I2C "omap_i2c"
 #define OMAP_I2C(obj) OBJECT_CHECK(OMAPI2CState, (obj), TYPE_OMAP_I2C)
@@ -449,12 +450,15 @@ static int omap_i2c_init(SysBusDevice *sbd)
     OMAPI2CState *s = OMAP_I2C(dev);
 
     if (!s->fclk) {
-        hw_error("omap_i2c: fclk not connected\n");
+        error_report("omap_i2c: fclk not connected");
+        return -1;
     }
     if (s->revision >= OMAP2_INTR_REV && !s->iclk) {
         /* Note that OMAP1 doesn't have a separate interface clock */
-        hw_error("omap_i2c: iclk not connected\n");
+        error_report("omap_i2c: iclk not connected");
+        return -1;
     }
+
     sysbus_init_irq(sbd, &s->irq);
     sysbus_init_irq(sbd, &s->drq[0]);
     sysbus_init_irq(sbd, &s->drq[1]);
diff --git a/hw/intc/omap_intc.c b/hw/intc/omap_intc.c
index e9b38a3..07b6272 100644
--- a/hw/intc/omap_intc.c
+++ b/hw/intc/omap_intc.c
@@ -20,6 +20,7 @@
 #include "hw/hw.h"
 #include "hw/arm/omap.h"
 #include "hw/sysbus.h"
+#include "qemu/error-report.h"
 
 /* Interrupt Handlers */
 struct omap_intr_handler_bank_s {
@@ -367,7 +368,8 @@ static int omap_intc_init(SysBusDevice *sbd)
     struct omap_intr_handler_s *s = OMAP_INTC(dev);
 
     if (!s->iclk) {
-        hw_error("omap-intc: clk not connected\n");
+        error_report("omap-intc: clk not connected");
+        return -1;
     }
     s->nbanks = 1;
     sysbus_init_irq(sbd, &s->parent_intr[0]);
@@ -608,10 +610,12 @@ static int omap2_intc_init(SysBusDevice *sbd)
     struct omap_intr_handler_s *s = OMAP_INTC(dev);
 
     if (!s->iclk) {
-        hw_error("omap2-intc: iclk not connected\n");
+        error_report("omap2-intc: iclk not connected");
+        return -1;
     }
     if (!s->fclk) {
-        hw_error("omap2-intc: fclk not connected\n");
+        error_report("omap2-intc: fclk not connected");
+        return -1;
     }
     s->level_only = 1;
     s->nbanks = 3;
-- 
2.4.3

[Qemu-devel] [PATCH 03/12] arm_mptimer: Don't use hw_error() in realize() method

[Markus Armbruster]

Device realize() methods aren't supposed to call hw_error(), they
should set an error and fail cleanly.  Do that.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/timer/arm_mptimer.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 3e59c2a..f1a34ec 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -220,8 +220,9 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
     int i;
 
     if (s->num_cpu < 1 || s->num_cpu > ARM_MPTIMER_MAX_CPUS) {
-        hw_error("%s: num-cpu must be between 1 and %d\n",
-                 __func__, ARM_MPTIMER_MAX_CPUS);
+        error_setg(errp, "num-cpu must be between 1 and %d\n",
+                   ARM_MPTIMER_MAX_CPUS);
+        return;
     }
     /* We implement one timer block per CPU, and expose multiple MMIO regions:
      *  * region 0 is "timer for this core"
-- 
2.4.3

[Qemu-devel] [PATCH 04/12] etraxfs_eth: Don't use hw_error() in init() method

[Markus Armbruster]

Device init() methods aren't supposed to call hw_error(), they should
report the error and fail cleanly.  Do that.

Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/net/etraxfs_eth.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/net/etraxfs_eth.c b/hw/net/etraxfs_eth.c
index d600275..b562ac9 100644
--- a/hw/net/etraxfs_eth.c
+++ b/hw/net/etraxfs_eth.c
@@ -26,6 +26,7 @@
 #include "hw/sysbus.h"
 #include "net/net.h"
 #include "hw/cris/etraxfs.h"
+#include "qemu/error-report.h"
 
 #define D(x)
 
@@ -589,7 +590,8 @@ static int fs_eth_init(SysBusDevice *sbd)
     ETRAXFSEthState *s = ETRAX_FS_ETH(dev);
 
     if (!s->dma_out || !s->dma_in) {
-        hw_error("Unconnected ETRAX-FS Ethernet MAC.\n");
+        error_report("Unconnected ETRAX-FS Ethernet MAC");
+        return -1;
     }
 
     s->dma_out->client.push = eth_tx_push;
-- 
2.4.3

[Qemu-devel] [PATCH 05/12] raven: Mark use of hw_error() in realize() FIXME

[Markus Armbruster]

Device realize() methods aren't supposed to call hw_error(), they
should set an error and fail cleanly.  Blindly doing that would be
easy enough, but then realize() would fail without undoing its side
effects.  Just mark it FIXME for now.

Cc: "Andreas Färber" <andreas.faerber@web.de>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/pci-host/prep.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index da88cb3..f434596 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -326,6 +326,7 @@ static void raven_realize(PCIDevice *d, Error **errp)
             }
         }
         if (bios_size < 0 || bios_size > BIOS_SIZE) {
+            /* FIXME should error_setg() */
             hw_error("qemu: could not load bios image '%s'\n", s->bios_name);
         }
         g_free(filename);
@@ -355,8 +356,9 @@ static void raven_class_init(ObjectClass *klass, void *data)
     dc->desc = "PReP Host Bridge - Motorola Raven";
     dc->vmsd = &vmstate_raven;
     /*
-     * PCI-facing part of the host bridge, not usable without the
-     * host-facing part, which can't be device_add'ed, yet.
+     * Reason: PCI-facing part of the host bridge, not usable without
+     * the host-facing part, which can't be device_add'ed, yet.
+     * Reason: realize() method uses hw_error().
      */
     dc->cannot_instantiate_with_device_add_yet = true;
 }
-- 
2.4.3

[Qemu-devel] [PATCH 06/12] hw/arm/virt: Fix property "gic-version" error handling

[Markus Armbruster]

virt_set_gic_version() calls exit(1) when passed an invalid property
value.  Property setters are not supposed to do that.  Screwed up in
commit b92ad39.  Harmless, because the property belongs to a machine.
Set an error object instead.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/virt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9c6792c..2a120dd 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1126,9 +1126,8 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp)
     } else if (!strcmp(value, "host")) {
         vms->gic_version = 0; /* Will probe later */
     } else {
-        error_report("Invalid gic-version option value");
-        error_printf("Allowed gic-version values are: 3, 2, host\n");
-        exit(1);
+        error_setg(errp, "Invalid gic-version value");
+        error_append_hint(errp, "Valid values are: 3, 2, host\n");
     }
 }
 
-- 
2.4.3

[Qemu-devel] [PATCH 07/12] sysbus: Don't use hw_error() in machine_init_done_notifiers

[Markus Armbruster]

platform_bus_map_irq() and platform_bus_map_mmio() use hw_error() to
fail.  They run in machine_init_done_notifiers, via
platform_bus_init_notify() and link_sysbus_device().  Printing CPU
registers is not helpful there.

Replace hw_error() by error_report(); exit(1).  If these are
programming errors, it should be replaced by an assertion instead.

While there, observe that both functions always return 0, and
link_sysbus_device() ignores the return value.  Change them to void.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/core/platform-bus.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c
index 70e0518..aa55d01 100644
--- a/hw/core/platform-bus.c
+++ b/hw/core/platform-bus.c
@@ -21,6 +21,7 @@
 
 #include "hw/platform-bus.h"
 #include "exec/address-spaces.h"
+#include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 
 
@@ -106,31 +107,29 @@ static void plaform_bus_refresh_irqs(PlatformBusDevice *pbus)
     pbus->done_gathering = true;
 }
 
-static int platform_bus_map_irq(PlatformBusDevice *pbus, SysBusDevice *sbdev,
-                                int n)
+static void platform_bus_map_irq(PlatformBusDevice *pbus, SysBusDevice *sbdev,
+                                 int n)
 {
     int max_irqs = pbus->num_irqs;
     int irqn;
 
     if (sysbus_is_irq_connected(sbdev, n)) {
         /* IRQ is already mapped, nothing to do */
-        return 0;
+        return;
     }
 
     irqn = find_first_zero_bit(pbus->used_irqs, max_irqs);
     if (irqn >= max_irqs) {
-        hw_error("Platform Bus: Can not fit IRQ line");
-        return -1;
+        error_report("Platform Bus: Can not fit IRQ line");
+        exit(1);
     }
 
     set_bit(irqn, pbus->used_irqs);
     sysbus_connect_irq(sbdev, n, pbus->irqs[irqn]);
-
-    return 0;
 }
 
-static int platform_bus_map_mmio(PlatformBusDevice *pbus, SysBusDevice *sbdev,
-                                 int n)
+static void platform_bus_map_mmio(PlatformBusDevice *pbus, SysBusDevice *sbdev,
+                                  int n)
 {
     MemoryRegion *sbdev_mr = sysbus_mmio_get_region(sbdev, n);
     uint64_t size = memory_region_size(sbdev_mr);
@@ -140,7 +139,7 @@ static int platform_bus_map_mmio(PlatformBusDevice *pbus, SysBusDevice *sbdev,
 
     if (memory_region_is_mapped(sbdev_mr)) {
         /* Region is already mapped, nothing to do */
-        return 0;
+        return;
     }
 
     /*
@@ -155,13 +154,13 @@ static int platform_bus_map_mmio(PlatformBusDevice *pbus, SysBusDevice *sbdev,
     }
 
     if (!found_region) {
-        hw_error("Platform Bus: Can not fit MMIO region of size %"PRIx64, size);
+        error_report("Platform Bus: Can not fit MMIO region of size %"PRIx64,
+                     size);
+        exit(1);
     }
 
     /* Map the device's region into our Platform Bus MMIO space */
     memory_region_add_subregion(&pbus->mmio, off, sbdev_mr);
-
-    return 0;
 }
 
 /*
-- 
2.4.3

[Qemu-devel] [PATCH 08/12] isa: Trivially convert remaining PCI-ISA bridges to realize()

[Markus Armbruster]

These are "ICH9-LPC" and "ebus".

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/isa/lpc_ich9.c  | 5 ++---
 hw/sparc64/sun4u.c | 6 ++----
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 1ffc803..8e58449 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -602,7 +602,7 @@ static void ich9_lpc_initfn(Object *obj)
     ich9_lpc_add_properties(lpc);
 }
 
-static int ich9_lpc_init(PCIDevice *d)
+static void ich9_lpc_realize(PCIDevice *d, Error **errp)
 {
     ICH9LPCState *lpc = ICH9_LPC_DEVICE(d);
     ISABus *isa_bus;
@@ -628,7 +628,6 @@ static int ich9_lpc_init(PCIDevice *d)
     memory_region_add_subregion_overlap(pci_address_space_io(d),
                                         ICH9_RST_CNT_IOPORT, &lpc->rst_cnt_mem,
                                         1);
-    return 0;
 }
 
 static void ich9_device_plug_cb(HotplugHandler *hotplug_dev,
@@ -706,7 +705,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
 
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     dc->reset = ich9_lpc_reset;
-    k->init = ich9_lpc_init;
+    k->realize = ich9_lpc_realize;
     dc->vmsd = &vmstate_ich9_lpc;
     dc->props = ich9_lpc_properties;
     k->config_write = ich9_lpc_config_write;
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index d6b929c..c37e8b0 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -593,8 +593,7 @@ pci_ebus_init(PCIBus *bus, int devfn, qemu_irq *irqs)
     return isa_bus;
 }
 
-static int
-pci_ebus_init1(PCIDevice *pci_dev)
+static void pci_ebus_realize(PCIDevice *pci_dev, Error **errp)
 {
     EbusState *s = DO_UPCAST(EbusState, pci_dev, pci_dev);
 
@@ -614,14 +613,13 @@ pci_ebus_init1(PCIDevice *pci_dev)
     memory_region_init_alias(&s->bar1, OBJECT(s), "bar1", get_system_io(),
                              0, 0x4000);
     pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->bar1);
-    return 0;
 }
 
 static void ebus_class_init(ObjectClass *klass, void *data)
 {
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->init = pci_ebus_init1;
+    k->realize = pci_ebus_realize;
     k->vendor_id = PCI_VENDOR_ID_SUN;
     k->device_id = PCI_DEVICE_ID_SUN_EBUS;
     k->revision = 0x01;
-- 
2.4.3

[Qemu-devel] [PATCH 09/12] isa: Clean up error handling around isa_bus_new()

[Markus Armbruster]

We can have at most one ISA bus.  If you try to create another one,
isa_bus_new() complains to stderr and returns null.

isa_bus_new() is called in two contexts, machine's init() and device's
realize() methods.  Since complaining to stderr is not proper in the
latter context, convert isa_bus_new() to Error.

Machine's init():

* mips_jazz_init(), called from the init() methods of machines
  "magnum" and "pica"

* mips_r4k_init(), the init() method of machine "mips"

* pc_init1() called from the init() methods of non-q35 PC machines

* typhoon_init(), called from clipper_init(), the init() method of
  machine "clipper"

These callers always create the first ISA bus, hence isa_bus_new()
can't fail.  Simply pass &error_abort.

Device's realize():

* i82378_realize(), of PCI device "i82378"

* ich9_lpc_realize(), of PCI device "ICH9-LPC"

* pci_ebus_realize(), of PCI device "ebus"

* piix3_realize(), of PCI device "pci-piix3", abstract parent of
  "PIIX3" and "PIIX3-xen"

* piix4_realize(), of PCI device "PIIX4"

* vt82c686b_realize(), of PCI device "VT82C686B"

Propagate the error.  Note that these devices are typically created
only by machine init() methods with qdev_init_nofail() or similar.  If
we screwed up and created an ISA bus before that call, we now give up
right away.  Before, we'd hobble on, and typically die in
isa_bus_irqs().  Similar if someone finds a way to hot-plug one of
these critters.

Cc: Richard Henderson <rth@twiddle.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: "Hervé Poussineau" <hpoussin@reactos.org>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/alpha/typhoon.c   | 3 ++-
 hw/i386/pc_piix.c    | 3 ++-
 hw/isa/i82378.c      | 5 ++++-
 hw/isa/isa-bus.c     | 4 ++--
 hw/isa/lpc_ich9.c    | 6 +++++-
 hw/isa/piix4.c       | 6 ++++--
 hw/isa/vt82c686.c    | 5 ++++-
 hw/mips/mips_jazz.c  | 2 +-
 hw/mips/mips_r4k.c   | 2 +-
 hw/pci-host/piix.c   | 6 ++++--
 hw/sparc64/sun4u.c   | 6 ++++--
 include/hw/isa/isa.h | 2 +-
 12 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index 421162e..35dc8a5 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -920,7 +920,8 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
     {
         qemu_irq *isa_irqs;
 
-        *isa_bus = isa_bus_new(NULL, get_system_memory(), &s->pchip.reg_io);
+        *isa_bus = isa_bus_new(NULL, get_system_memory(), &s->pchip.reg_io,
+                               &error_abort);
         isa_irqs = i8259_init(*isa_bus,
                               qemu_allocate_irq(typhoon_set_isa_irq, s, 0));
         isa_bus_irqs(*isa_bus, isa_irqs);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 2e41efe..48fdad4 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -200,7 +200,8 @@ static void pc_init1(MachineState *machine,
     } else {
         pci_bus = NULL;
         i440fx_state = NULL;
-        isa_bus = isa_bus_new(NULL, get_system_memory(), system_io);
+        isa_bus = isa_bus_new(NULL, get_system_memory(), system_io,
+                              &error_abort);
         no_hpet = 1;
     }
     isa_bus_irqs(isa_bus, gsi);
diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
index d4c8306..3793c6f 100644
--- a/hw/isa/i82378.c
+++ b/hw/isa/i82378.c
@@ -75,7 +75,10 @@ static void i82378_realize(PCIDevice *pci, Error **errp)
     pci_config_set_interrupt_pin(pci_conf, 1); /* interrupt pin 0 */
 
     isabus = isa_bus_new(dev, get_system_memory(),
-                         pci_address_space_io(pci));
+                         pci_address_space_io(pci), errp);
+    if (!isabus) {
+        return;
+    }
 
     /* This device has:
        2 82C59 (irq)
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 43e0cd8..af6ffd6 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -44,10 +44,10 @@ static const TypeInfo isa_bus_info = {
 };
 
 ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space,
-                    MemoryRegion *address_space_io)
+                    MemoryRegion *address_space_io, Error **errp)
 {
     if (isabus) {
-        fprintf(stderr, "Can't create a second ISA bus\n");
+        error_setg(errp, "Can't create a second ISA bus");
         return NULL;
     }
     if (!dev) {
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 8e58449..ed9907d 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -607,7 +607,11 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
     ICH9LPCState *lpc = ICH9_LPC_DEVICE(d);
     ISABus *isa_bus;
 
-    isa_bus = isa_bus_new(DEVICE(d), get_system_memory(), get_system_io());
+    isa_bus = isa_bus_new(DEVICE(d), get_system_memory(), get_system_io(),
+                          errp);
+    if (!isa_bus) {
+        return;
+    }
 
     pci_set_long(d->wmask + ICH9_LPC_PMBASE,
                  ICH9_LPC_PMBASE_BASE_ADDRESS_MASK);
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 2c59e91..644cfd9 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -90,8 +90,10 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
 {
     PIIX4State *d = PIIX4_PCI_DEVICE(dev);
 
-    isa_bus_new(DEVICE(d), pci_address_space(dev),
-                pci_address_space_io(dev));
+    if (!isa_bus_new(DEVICE(d), pci_address_space(dev),
+                     pci_address_space_io(dev), errp)) {
+        return;
+    }
     piix4_dev = &d->dev;
     qemu_register_reset(piix4_reset, d);
 }
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 252e1d7..6c2190b 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -440,7 +440,10 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
     int i;
 
     isa_bus = isa_bus_new(DEVICE(d), get_system_memory(),
-                          pci_address_space_io(d));
+                          pci_address_space_io(d), errp);
+    if (!isa_bus) {
+        return;
+    }
 
     pci_conf = d->config;
     pci_config_set_prog_interface(pci_conf, 0x0);
diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
index 1ab885b..1cfbaa6 100644
--- a/hw/mips/mips_jazz.c
+++ b/hw/mips/mips_jazz.c
@@ -219,7 +219,7 @@ static void mips_jazz_init(MachineState *machine,
     memory_region_init(isa_mem, NULL, "isa-mem", 0x01000000);
     memory_region_add_subregion(address_space, 0x90000000, isa_io);
     memory_region_add_subregion(address_space, 0x91000000, isa_mem);
-    isa_bus = isa_bus_new(NULL, isa_mem, isa_io);
+    isa_bus = isa_bus_new(NULL, isa_mem, isa_io, &error_abort);
 
     /* ISA devices */
     i8259 = i8259_init(isa_bus, env->irq[4]);
diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
index af10da1..2d4e038 100644
--- a/hw/mips/mips_r4k.c
+++ b/hw/mips/mips_r4k.c
@@ -272,7 +272,7 @@ void mips_r4k_init(MachineState *machine)
     memory_region_init(isa_mem, NULL, "isa-mem", 0x01000000);
     memory_region_add_subregion(get_system_memory(), 0x14000000, isa_io);
     memory_region_add_subregion(get_system_memory(), 0x10000000, isa_mem);
-    isa_bus = isa_bus_new(NULL, isa_mem, get_system_io());
+    isa_bus = isa_bus_new(NULL, isa_mem, get_system_io(), &error_abort);
 
     /* The PIC is attached to the MIPS CPU INT0 pin */
     i8259 = i8259_init(isa_bus, env->irq[2]);
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 715208b..775b4bd 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -651,8 +651,10 @@ static void piix3_realize(PCIDevice *dev, Error **errp)
 {
     PIIX3State *d = PIIX3_PCI_DEVICE(dev);
 
-    isa_bus_new(DEVICE(d), get_system_memory(),
-                pci_address_space_io(dev));
+    if (!isa_bus_new(DEVICE(d), get_system_memory(),
+                     pci_address_space_io(dev), errp)) {
+        return;
+    }
 
     memory_region_init_io(&d->rcr_mem, OBJECT(dev), &rcr_ops, d,
                           "piix3-reset-control", 1);
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index c37e8b0..6c7596b 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -597,8 +597,10 @@ static void pci_ebus_realize(PCIDevice *pci_dev, Error **errp)
 {
     EbusState *s = DO_UPCAST(EbusState, pci_dev, pci_dev);
 
-    isa_bus_new(DEVICE(pci_dev), get_system_memory(),
-                pci_address_space_io(pci_dev));
+    if (!isa_bus_new(DEVICE(pci_dev), get_system_memory(),
+                     pci_address_space_io(pci_dev), errp)) {
+        return;
+    }
 
     pci_dev->config[0x04] = 0x06; // command = bus master, pci mem
     pci_dev->config[0x05] = 0x00;
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index d758b39..de3cd3d 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -59,7 +59,7 @@ struct ISADevice {
 };
 
 ISABus *isa_bus_new(DeviceState *dev, MemoryRegion *address_space,
-                    MemoryRegion *address_space_io);
+                    MemoryRegion *address_space_io, Error **errp);
 void isa_bus_irqs(ISABus *bus, qemu_irq *irqs);
 qemu_irq isa_get_irq(ISADevice *dev, int isairq);
 void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq);
-- 
2.4.3

[Qemu-devel] [PATCH 10/12] isa: Clean up inappropriate hw_error()

[Markus Armbruster]

isa_bus_irqs(), isa_create() and isa_try_create() call hw_error() when
passed a null bus.  Use of hw_error() has always been questionable,
because these are used only during machine initialization, and
printing CPU registers isn't useful there.

Since the previous commit, passing a null bus is a programming error.
Drop the hw_error() and simply let it crash.

Cc: Richard Henderson <rth@twiddle.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: "Hervé Poussineau" <hpoussin@reactos.org>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/isa/isa-bus.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index af6ffd6..630054c 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -63,9 +63,6 @@ ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space,
 
 void isa_bus_irqs(ISABus *bus, qemu_irq *irqs)
 {
-    if (!bus) {
-        hw_error("Can't set isa irqs with no isa bus present.");
-    }
     bus->irqs = irqs;
 }
 
@@ -137,10 +134,6 @@ ISADevice *isa_create(ISABus *bus, const char *name)
 {
     DeviceState *dev;
 
-    if (!bus) {
-        hw_error("Tried to create isa device %s with no isa bus present.",
-                 name);
-    }
     dev = qdev_create(BUS(bus), name);
     return ISA_DEVICE(dev);
 }
@@ -149,10 +142,6 @@ ISADevice *isa_try_create(ISABus *bus, const char *name)
 {
     DeviceState *dev;
 
-    if (!bus) {
-        hw_error("Tried to create isa device %s with no isa bus present.",
-                 name);
-    }
     dev = qdev_try_create(BUS(bus), name);
     return ISA_DEVICE(dev);
 }
-- 
2.4.3

[Qemu-devel] [PATCH 11/12] audio: Clean up inappropriate and unreachable use of hw_error()

[Markus Armbruster]

audio_init() should not use hw_error(), because dumping CPU registers
is unhelpful there, and aborting is wrong, because it can be called
called from an audio device's realize() method.

The two uses of hw_error() come from commit 0d9acba:

* When qemu_new_timer() fails.  It couldn't fail back then, and it
  can't fail now.  Drop the unreachable error handling.

* When no_audio_driver can't be initialized.  It couldn't fail back
  then, and it can't fail now.  Replace the error handling by an
  assertion.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 audio/audio.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index 5be4b15..9b855ed 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1806,9 +1806,6 @@ static void audio_init (void)
     atexit (audio_atexit);
 
     s->ts = timer_new_ns(QEMU_CLOCK_VIRTUAL, audio_timer, s);
-    if (!s->ts) {
-        hw_error("Could not create audio timer\n");
-    }
 
     audio_process_options ("AUDIO", audio_options);
 
@@ -1859,12 +1856,8 @@ static void audio_init (void)
 
     if (!done) {
         done = !audio_driver_init (s, &no_audio_driver);
-        if (!done) {
-            hw_error("Could not initialize audio subsystem\n");
-        }
-        else {
-            dolog ("warning: Using timer based audio emulation\n");
-        }
+        assert(done);
+        dolog ("warning: Using timer based audio emulation\n");
     }
 
     if (conf.period.hertz <= 0) {
-- 
2.4.3

[Qemu-devel] [PATCH 12/12] xen-hvm: Mark inappropriate error handling FIXME

[Markus Armbruster]

Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: xen-devel@lists.xensource.com
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 xen-hvm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen-hvm.c b/xen-hvm.c
index 3d78a0c..2a93390 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -240,6 +240,7 @@ static void xen_ram_init(PCMachineState *pcms,
 
 void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
 {
+    /* FIXME caller ram_block_add() wants error_setg() on failure */
     unsigned long nr_pfn;
     xen_pfn_t *pfn_list;
     int i;
@@ -1192,6 +1193,12 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
 int xen_hvm_init(PCMachineState *pcms,
                  MemoryRegion **ram_memory)
 {
+    /*
+     * FIXME Returns -1 without cleaning up on some errors (harmless
+     * as long as the caller exit()s on error), dies with hw_error()
+     * on others.  hw_error() isn't approprate here.  Should probably
+     * simply exit() on all errors.
+     */
     int i, rc;
     xen_pfn_t ioreq_pfn;
     xen_pfn_t bufioreq_pfn;
-- 
2.4.3

[PATCH 12/12] xen-hvm: Mark inappropriate error handling FIXME

[Markus Armbruster]

Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: xen-devel@lists.xensource.com
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 xen-hvm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen-hvm.c b/xen-hvm.c
index 3d78a0c..2a93390 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -240,6 +240,7 @@ static void xen_ram_init(PCMachineState *pcms,
 
 void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
 {
+    /* FIXME caller ram_block_add() wants error_setg() on failure */
     unsigned long nr_pfn;
     xen_pfn_t *pfn_list;
     int i;
@@ -1192,6 +1193,12 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
 int xen_hvm_init(PCMachineState *pcms,
                  MemoryRegion **ram_memory)
 {
+    /*
+     * FIXME Returns -1 without cleaning up on some errors (harmless
+     * as long as the caller exit()s on error), dies with hw_error()
+     * on others.  hw_error() isn't approprate here.  Should probably
+     * simply exit() on all errors.
+     */
     int i, rc;
     xen_pfn_t ioreq_pfn;
     xen_pfn_t bufioreq_pfn;
-- 
2.4.3

Re: [Qemu-devel] [PATCH 04/12] etraxfs_eth: Don't use hw_error() in init() method

[Edgar E. Iglesias]

On Thu, Dec 10, 2015 at 11:29:24AM +0100, Markus Armbruster wrote:
> Device init() methods aren't supposed to call hw_error(), they should
> report the error and fail cleanly.  Do that.
> 
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/net/etraxfs_eth.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/etraxfs_eth.c b/hw/net/etraxfs_eth.c
> index d600275..b562ac9 100644
> --- a/hw/net/etraxfs_eth.c
> +++ b/hw/net/etraxfs_eth.c
> @@ -26,6 +26,7 @@
>  #include "hw/sysbus.h"
>  #include "net/net.h"
>  #include "hw/cris/etraxfs.h"
> +#include "qemu/error-report.h"
>  
>  #define D(x)
>  
> @@ -589,7 +590,8 @@ static int fs_eth_init(SysBusDevice *sbd)
>      ETRAXFSEthState *s = ETRAX_FS_ETH(dev);
>  
>      if (!s->dma_out || !s->dma_in) {
> -        hw_error("Unconnected ETRAX-FS Ethernet MAC.\n");
> +        error_report("Unconnected ETRAX-FS Ethernet MAC");
> +        return -1;
>      }
>  
>      s->dma_out->client.push = eth_tx_push;
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [PATCH 03/12] arm_mptimer: Don't use hw_error() in realize() method

[Peter Maydell]

On 10 December 2015 at 10:29, Markus Armbruster <armbru@redhat.com> wrote:
> Device realize() methods aren't supposed to call hw_error(), they
> should set an error and fail cleanly.  Do that.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/timer/arm_mptimer.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index 3e59c2a..f1a34ec 100644
> --- a/hw/timer/arm_mptimer.c
> +++ b/hw/timer/arm_mptimer.c
> @@ -220,8 +220,9 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
>      int i;
>
>      if (s->num_cpu < 1 || s->num_cpu > ARM_MPTIMER_MAX_CPUS) {
> -        hw_error("%s: num-cpu must be between 1 and %d\n",
> -                 __func__, ARM_MPTIMER_MAX_CPUS);
> +        error_setg(errp, "num-cpu must be between 1 and %d\n",
> +                   ARM_MPTIMER_MAX_CPUS);
> +        return;

I think the trailing newline is incorrect for error_setg, right?

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 02/12] omap: Don't use hw_error() in device init() methods

[Peter Maydell]

On 10 December 2015 at 10:29, Markus Armbruster <armbru@redhat.com> wrote:
> Device init() methods aren't supposed to call hw_error(), they should
> report the error and fail cleanly.  Do that.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

These are all really "QEMU bug" error paths -- the only place
that uses omap_i2c is the omap SoC init, you can't create the
device on the command line, and so we'll only get these errors
if there's a bug in a function like omap2420_mpu_init. But
I don't object to the patch in principle.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 01/12] hw: Don't use hw_error() for machine initialization errors

[Peter Maydell]

On 10 December 2015 at 10:29, Markus Armbruster <armbru@redhat.com> wrote:
> Printing CPU registers is not helpful during machine initialization.
> Moreover, these are straightforward configuration or "can get
> resources" errors, so dumping core isn't appropriate either.  Replace
> hw_error() by error_report(); exit(1).  Matches how we report these
> errors in other machine initializations.
>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: qemu-arm@nongnu.org
> Cc: qemu-ppc@nongnu.org
> Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 08/12] isa: Trivially convert remaining PCI-ISA bridges to realize()

[Marcel Apfelbaum]

On 12/10/2015 12:29 PM, Markus Armbruster wrote:
> These are "ICH9-LPC" and "ebus".
>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/isa/lpc_ich9.c  | 5 ++---
>   hw/sparc64/sun4u.c | 6 ++----
>   2 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 1ffc803..8e58449 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -602,7 +602,7 @@ static void ich9_lpc_initfn(Object *obj)
>       ich9_lpc_add_properties(lpc);
>   }
>
> -static int ich9_lpc_init(PCIDevice *d)
> +static void ich9_lpc_realize(PCIDevice *d, Error **errp)
>   {
>       ICH9LPCState *lpc = ICH9_LPC_DEVICE(d);
>       ISABus *isa_bus;
> @@ -628,7 +628,6 @@ static int ich9_lpc_init(PCIDevice *d)
>       memory_region_add_subregion_overlap(pci_address_space_io(d),
>                                           ICH9_RST_CNT_IOPORT, &lpc->rst_cnt_mem,
>                                           1);
> -    return 0;
>   }
>
>   static void ich9_device_plug_cb(HotplugHandler *hotplug_dev,
> @@ -706,7 +705,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
>
>       set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>       dc->reset = ich9_lpc_reset;
> -    k->init = ich9_lpc_init;
> +    k->realize = ich9_lpc_realize;
>       dc->vmsd = &vmstate_ich9_lpc;
>       dc->props = ich9_lpc_properties;
>       k->config_write = ich9_lpc_config_write;
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index d6b929c..c37e8b0 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -593,8 +593,7 @@ pci_ebus_init(PCIBus *bus, int devfn, qemu_irq *irqs)
>       return isa_bus;
>   }
>
> -static int
> -pci_ebus_init1(PCIDevice *pci_dev)
> +static void pci_ebus_realize(PCIDevice *pci_dev, Error **errp)
>   {
>       EbusState *s = DO_UPCAST(EbusState, pci_dev, pci_dev);
>
> @@ -614,14 +613,13 @@ pci_ebus_init1(PCIDevice *pci_dev)
>       memory_region_init_alias(&s->bar1, OBJECT(s), "bar1", get_system_io(),
>                                0, 0x4000);
>       pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->bar1);
> -    return 0;
>   }
>
>   static void ebus_class_init(ObjectClass *klass, void *data)
>   {
>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> -    k->init = pci_ebus_init1;
> +    k->realize = pci_ebus_realize;
>       k->vendor_id = PCI_VENDOR_ID_SUN;
>       k->device_id = PCI_DEVICE_ID_SUN_EBUS;
>       k->revision = 0x01;
>


Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Re: [Qemu-devel] [PATCH 09/12] isa: Clean up error handling around isa_bus_new()

[Marcel Apfelbaum]

On 12/10/2015 12:29 PM, Markus Armbruster wrote:
> We can have at most one ISA bus.  If you try to create another one,
> isa_bus_new() complains to stderr and returns null.
>
> isa_bus_new() is called in two contexts, machine's init() and device's
> realize() methods.  Since complaining to stderr is not proper in the
> latter context, convert isa_bus_new() to Error.
>
> Machine's init():
>
> * mips_jazz_init(), called from the init() methods of machines
>    "magnum" and "pica"
>
> * mips_r4k_init(), the init() method of machine "mips"
>
> * pc_init1() called from the init() methods of non-q35 PC machines
>
> * typhoon_init(), called from clipper_init(), the init() method of
>    machine "clipper"
>
> These callers always create the first ISA bus, hence isa_bus_new()
> can't fail.  Simply pass &error_abort.
>
> Device's realize():
>
> * i82378_realize(), of PCI device "i82378"
>
> * ich9_lpc_realize(), of PCI device "ICH9-LPC"
>
> * pci_ebus_realize(), of PCI device "ebus"
>
> * piix3_realize(), of PCI device "pci-piix3", abstract parent of
>    "PIIX3" and "PIIX3-xen"
>
> * piix4_realize(), of PCI device "PIIX4"
>
> * vt82c686b_realize(), of PCI device "VT82C686B"
>
> Propagate the error.  Note that these devices are typically created
> only by machine init() methods with qdev_init_nofail() or similar.  If
> we screwed up and created an ISA bus before that call, we now give up
> right away.  Before, we'd hobble on, and typically die in
> isa_bus_irqs().  Similar if someone finds a way to hot-plug one of
> these critters.

Makes sense.

>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: "Hervé Poussineau" <hpoussin@reactos.org>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/alpha/typhoon.c   | 3 ++-
>   hw/i386/pc_piix.c    | 3 ++-
>   hw/isa/i82378.c      | 5 ++++-
>   hw/isa/isa-bus.c     | 4 ++--
>   hw/isa/lpc_ich9.c    | 6 +++++-
>   hw/isa/piix4.c       | 6 ++++--
>   hw/isa/vt82c686.c    | 5 ++++-
>   hw/mips/mips_jazz.c  | 2 +-
>   hw/mips/mips_r4k.c   | 2 +-
>   hw/pci-host/piix.c   | 6 ++++--
>   hw/sparc64/sun4u.c   | 6 ++++--
>   include/hw/isa/isa.h | 2 +-
>   12 files changed, 34 insertions(+), 16 deletions(-)
>
> diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
> index 421162e..35dc8a5 100644
> --- a/hw/alpha/typhoon.c
> +++ b/hw/alpha/typhoon.c
> @@ -920,7 +920,8 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
>       {
>           qemu_irq *isa_irqs;
>
> -        *isa_bus = isa_bus_new(NULL, get_system_memory(), &s->pchip.reg_io);
> +        *isa_bus = isa_bus_new(NULL, get_system_memory(), &s->pchip.reg_io,
> +                               &error_abort);
>           isa_irqs = i8259_init(*isa_bus,
>                                 qemu_allocate_irq(typhoon_set_isa_irq, s, 0));
>           isa_bus_irqs(*isa_bus, isa_irqs);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 2e41efe..48fdad4 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -200,7 +200,8 @@ static void pc_init1(MachineState *machine,
>       } else {
>           pci_bus = NULL;
>           i440fx_state = NULL;
> -        isa_bus = isa_bus_new(NULL, get_system_memory(), system_io);
> +        isa_bus = isa_bus_new(NULL, get_system_memory(), system_io,
> +                              &error_abort);
>           no_hpet = 1;
>       }
>       isa_bus_irqs(isa_bus, gsi);
> diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
> index d4c8306..3793c6f 100644
> --- a/hw/isa/i82378.c
> +++ b/hw/isa/i82378.c
> @@ -75,7 +75,10 @@ static void i82378_realize(PCIDevice *pci, Error **errp)
>       pci_config_set_interrupt_pin(pci_conf, 1); /* interrupt pin 0 */
>
>       isabus = isa_bus_new(dev, get_system_memory(),
> -                         pci_address_space_io(pci));
> +                         pci_address_space_io(pci), errp);
> +    if (!isabus) {
> +        return;
> +    }
>
>       /* This device has:
>          2 82C59 (irq)
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index 43e0cd8..af6ffd6 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -44,10 +44,10 @@ static const TypeInfo isa_bus_info = {
>   };
>
>   ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space,
> -                    MemoryRegion *address_space_io)
> +                    MemoryRegion *address_space_io, Error **errp)
>   {
>       if (isabus) {
> -        fprintf(stderr, "Can't create a second ISA bus\n");
> +        error_setg(errp, "Can't create a second ISA bus");
>           return NULL;
>       }
>       if (!dev) {
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 8e58449..ed9907d 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -607,7 +607,11 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
>       ICH9LPCState *lpc = ICH9_LPC_DEVICE(d);
>       ISABus *isa_bus;
>
> -    isa_bus = isa_bus_new(DEVICE(d), get_system_memory(), get_system_io());
> +    isa_bus = isa_bus_new(DEVICE(d), get_system_memory(), get_system_io(),
> +                          errp);
> +    if (!isa_bus) {
> +        return;
> +    }
>
>       pci_set_long(d->wmask + ICH9_LPC_PMBASE,
>                    ICH9_LPC_PMBASE_BASE_ADDRESS_MASK);
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 2c59e91..644cfd9 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -90,8 +90,10 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>   {
>       PIIX4State *d = PIIX4_PCI_DEVICE(dev);
>
> -    isa_bus_new(DEVICE(d), pci_address_space(dev),
> -                pci_address_space_io(dev));
> +    if (!isa_bus_new(DEVICE(d), pci_address_space(dev),
> +                     pci_address_space_io(dev), errp)) {
> +        return;
> +    }
>       piix4_dev = &d->dev;
>       qemu_register_reset(piix4_reset, d);
>   }
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 252e1d7..6c2190b 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -440,7 +440,10 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>       int i;
>
>       isa_bus = isa_bus_new(DEVICE(d), get_system_memory(),
> -                          pci_address_space_io(d));
> +                          pci_address_space_io(d), errp);
> +    if (!isa_bus) {
> +        return;
> +    }
>
>       pci_conf = d->config;
>       pci_config_set_prog_interface(pci_conf, 0x0);
> diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
> index 1ab885b..1cfbaa6 100644
> --- a/hw/mips/mips_jazz.c
> +++ b/hw/mips/mips_jazz.c
> @@ -219,7 +219,7 @@ static void mips_jazz_init(MachineState *machine,
>       memory_region_init(isa_mem, NULL, "isa-mem", 0x01000000);
>       memory_region_add_subregion(address_space, 0x90000000, isa_io);
>       memory_region_add_subregion(address_space, 0x91000000, isa_mem);
> -    isa_bus = isa_bus_new(NULL, isa_mem, isa_io);
> +    isa_bus = isa_bus_new(NULL, isa_mem, isa_io, &error_abort);
>
>       /* ISA devices */
>       i8259 = i8259_init(isa_bus, env->irq[4]);
> diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
> index af10da1..2d4e038 100644
> --- a/hw/mips/mips_r4k.c
> +++ b/hw/mips/mips_r4k.c
> @@ -272,7 +272,7 @@ void mips_r4k_init(MachineState *machine)
>       memory_region_init(isa_mem, NULL, "isa-mem", 0x01000000);
>       memory_region_add_subregion(get_system_memory(), 0x14000000, isa_io);
>       memory_region_add_subregion(get_system_memory(), 0x10000000, isa_mem);
> -    isa_bus = isa_bus_new(NULL, isa_mem, get_system_io());
> +    isa_bus = isa_bus_new(NULL, isa_mem, get_system_io(), &error_abort);
>
>       /* The PIC is attached to the MIPS CPU INT0 pin */
>       i8259 = i8259_init(isa_bus, env->irq[2]);
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 715208b..775b4bd 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -651,8 +651,10 @@ static void piix3_realize(PCIDevice *dev, Error **errp)
>   {
>       PIIX3State *d = PIIX3_PCI_DEVICE(dev);
>
> -    isa_bus_new(DEVICE(d), get_system_memory(),
> -                pci_address_space_io(dev));
> +    if (!isa_bus_new(DEVICE(d), get_system_memory(),
> +                     pci_address_space_io(dev), errp)) {
> +        return;
> +    }
>
>       memory_region_init_io(&d->rcr_mem, OBJECT(dev), &rcr_ops, d,
>                             "piix3-reset-control", 1);
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index c37e8b0..6c7596b 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -597,8 +597,10 @@ static void pci_ebus_realize(PCIDevice *pci_dev, Error **errp)
>   {
>       EbusState *s = DO_UPCAST(EbusState, pci_dev, pci_dev);
>
> -    isa_bus_new(DEVICE(pci_dev), get_system_memory(),
> -                pci_address_space_io(pci_dev));
> +    if (!isa_bus_new(DEVICE(pci_dev), get_system_memory(),
> +                     pci_address_space_io(pci_dev), errp)) {
> +        return;
> +    }
>
>       pci_dev->config[0x04] = 0x06; // command = bus master, pci mem
>       pci_dev->config[0x05] = 0x00;
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index d758b39..de3cd3d 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -59,7 +59,7 @@ struct ISADevice {
>   };
>
>   ISABus *isa_bus_new(DeviceState *dev, MemoryRegion *address_space,
> -                    MemoryRegion *address_space_io);
> +                    MemoryRegion *address_space_io, Error **errp);
>   void isa_bus_irqs(ISABus *bus, qemu_irq *irqs);
>   qemu_irq isa_get_irq(ISADevice *dev, int isairq);
>   void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq);
>

Looks good to me.

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks
Marcel

Re: [Qemu-devel] [PATCH 10/12] isa: Clean up inappropriate hw_error()

[Marcel Apfelbaum]

On 12/10/2015 12:29 PM, Markus Armbruster wrote:
> isa_bus_irqs(), isa_create() and isa_try_create() call hw_error() when
> passed a null bus.  Use of hw_error() has always been questionable,
> because these are used only during machine initialization, and
> printing CPU registers isn't useful there.
>
> Since the previous commit, passing a null bus is a programming error.
> Drop the hw_error() and simply let it crash.

Maybe we can be a little nicer add an assert ? :)

Thanks,
Marcel

>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: "Hervé Poussineau" <hpoussin@reactos.org>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/isa/isa-bus.c | 11 -----------
>   1 file changed, 11 deletions(-)
>
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index af6ffd6..630054c 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -63,9 +63,6 @@ ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space,
>
>   void isa_bus_irqs(ISABus *bus, qemu_irq *irqs)
>   {
> -    if (!bus) {
> -        hw_error("Can't set isa irqs with no isa bus present.");
> -    }
>       bus->irqs = irqs;
>   }
>
> @@ -137,10 +134,6 @@ ISADevice *isa_create(ISABus *bus, const char *name)
>   {
>       DeviceState *dev;
>
> -    if (!bus) {
> -        hw_error("Tried to create isa device %s with no isa bus present.",
> -                 name);
> -    }
>       dev = qdev_create(BUS(bus), name);
>       return ISA_DEVICE(dev);
>   }
> @@ -149,10 +142,6 @@ ISADevice *isa_try_create(ISABus *bus, const char *name)
>   {
>       DeviceState *dev;
>
> -    if (!bus) {
> -        hw_error("Tried to create isa device %s with no isa bus present.",
> -                 name);
> -    }
>       dev = qdev_try_create(BUS(bus), name);
>       return ISA_DEVICE(dev);
>   }
>

Re: [Qemu-devel] [PATCH 02/12] omap: Don't use hw_error() in device init() methods

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 10 December 2015 at 10:29, Markus Armbruster <armbru@redhat.com> wrote:
>> Device init() methods aren't supposed to call hw_error(), they should
>> report the error and fail cleanly.  Do that.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> These are all really "QEMU bug" error paths -- the only place
> that uses omap_i2c is the omap SoC init, you can't create the
> device on the command line, and so we'll only get these errors
> if there's a bug in a function like omap2420_mpu_init. But
> I don't object to the patch in principle.

All callers use qdev_init_nofail(), so this patch merely converts the
hw_error() crash into an &error_abort crash.  Improvement, because now
it crashes closer to where the bug is.  I can spell that out in the
commit message if you like.

> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks!

Re: [Qemu-devel] [PATCH 03/12] arm_mptimer: Don't use hw_error() in realize() method

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 10 December 2015 at 10:29, Markus Armbruster <armbru@redhat.com> wrote:
>> Device realize() methods aren't supposed to call hw_error(), they
>> should set an error and fail cleanly.  Do that.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: qemu-arm@nongnu.org
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/timer/arm_mptimer.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
>> index 3e59c2a..f1a34ec 100644
>> --- a/hw/timer/arm_mptimer.c
>> +++ b/hw/timer/arm_mptimer.c
>> @@ -220,8 +220,9 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
>>      int i;
>>
>>      if (s->num_cpu < 1 || s->num_cpu > ARM_MPTIMER_MAX_CPUS) {
>> -        hw_error("%s: num-cpu must be between 1 and %d\n",
>> -                 __func__, ARM_MPTIMER_MAX_CPUS);
>> +        error_setg(errp, "num-cpu must be between 1 and %d\n",
>> +                   ARM_MPTIMER_MAX_CPUS);
>> +        return;
>
> I think the trailing newline is incorrect for error_setg, right?

I always misse one...  will fix!

> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks!

Re: [Qemu-devel] [PATCH 10/12] isa: Clean up inappropriate hw_error()

[Markus Armbruster]

Marcel Apfelbaum <marcel.apfelbaum@gmail.com> writes:

> On 12/10/2015 12:29 PM, Markus Armbruster wrote:
>> isa_bus_irqs(), isa_create() and isa_try_create() call hw_error() when
>> passed a null bus.  Use of hw_error() has always been questionable,
>> because these are used only during machine initialization, and
>> printing CPU registers isn't useful there.
>>
>> Since the previous commit, passing a null bus is a programming error.
>> Drop the hw_error() and simply let it crash.
>
> Maybe we can be a little nicer add an assert ? :)

assert(p) before dereferencing p only converts one kind of crash into
another one.  I tend to do it only when the assert(p) does double-duty
as useful documentation.  Or perhaps when I think there's a real risk of
running into !p in an environment where core dumps are off[*] and
reproducing the failure with a debugger attached could be hard.

To use these three functions, you need an ISABus *.  How could you end
up with a bad one?

* You forget to create the ISA bus, and the compiler is too confused to
  notice.  You'll pass an unitialized ISABus, and asserting it's not
  null is unlikely to help.

* You create multiple ISA buses (that's the only way creating one can
  fail) *and* forget to check for errors.  If you pull that off, I'd
  expect it to explode even in light testing.

* Your pointer gets corrupted between correct initialization and use.
  Asserting it's not null is unlikely to help.


[*] Switching them off on a development machine forfeits your
developer's license, as far as I'm concerned :)

Re: [Qemu-devel] [PATCH 10/12] isa: Clean up inappropriate hw_error()

[Marcel Apfelbaum]

On 12/10/2015 03:09 PM, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel.apfelbaum@gmail.com> writes:
>
>> On 12/10/2015 12:29 PM, Markus Armbruster wrote:
>>> isa_bus_irqs(), isa_create() and isa_try_create() call hw_error() when
>>> passed a null bus.  Use of hw_error() has always been questionable,
>>> because these are used only during machine initialization, and
>>> printing CPU registers isn't useful there.
>>>
>>> Since the previous commit, passing a null bus is a programming error.
>>> Drop the hw_error() and simply let it crash.
>>
>> Maybe we can be a little nicer add an assert ? :)
>
> assert(p) before dereferencing p only converts one kind of crash into
> another one.  I tend to do it only when the assert(p) does double-duty
> as useful documentation.  Or perhaps when I think there's a real risk of
> running into !p in an environment where core dumps are off[*] and
> reproducing the failure with a debugger attached could be hard.
>
> To use these three functions, you need an ISABus *.  How could you end
> up with a bad one?
>
> * You forget to create the ISA bus, and the compiler is too confused to
>    notice.  You'll pass an unitialized ISABus, and asserting it's not
>    null is unlikely to help.
>
> * You create multiple ISA buses (that's the only way creating one can
>    fail) *and* forget to check for errors.  If you pull that off, I'd
>    expect it to explode even in light testing.

This is the scenario I was referring to.

You are perfectly right that using assert will change a crash into another,
but when I am "succeeding" to crash some code (and I am good at it :)),
if I see a NULL pointer de-reference I am starting to wonder if *is possible*
to have a NULL pointer there and the developer didn't take that into account.
(it couldn't me my bug, right, it got be his :))

However, if I see an assert crash *I know* the developer did not expect a NULL
pointer to be passed at all.

For this specific scenario,  (multiple ISA buses) maybe is such a strange case
that we don't need to bother with an assert.

Thanks for the detailed explanation,
Marcel

>
> * Your pointer gets corrupted between correct initialization and use.
>    Asserting it's not null is unlikely to help.
>
>
> [*] Switching them off on a development machine forfeits your
> developer's license, as far as I'm concerned :)
>

Re: [Qemu-devel] [PATCH 09/12] isa: Clean up error handling around isa_bus_new()

[Hervé Poussineau]

Le 10/12/2015 11:29, Markus Armbruster a écrit :
> We can have at most one ISA bus.  If you try to create another one,
> isa_bus_new() complains to stderr and returns null.
>
> isa_bus_new() is called in two contexts, machine's init() and device's
> realize() methods.  Since complaining to stderr is not proper in the
> latter context, convert isa_bus_new() to Error.
>
> Machine's init():
>
> * mips_jazz_init(), called from the init() methods of machines
>    "magnum" and "pica"
>
> * mips_r4k_init(), the init() method of machine "mips"
>
> * pc_init1() called from the init() methods of non-q35 PC machines
>
> * typhoon_init(), called from clipper_init(), the init() method of
>    machine "clipper"
>
> These callers always create the first ISA bus, hence isa_bus_new()
> can't fail.  Simply pass &error_abort.
>
> Device's realize():
>
> * i82378_realize(), of PCI device "i82378"
>
> * ich9_lpc_realize(), of PCI device "ICH9-LPC"
>
> * pci_ebus_realize(), of PCI device "ebus"
>
> * piix3_realize(), of PCI device "pci-piix3", abstract parent of
>    "PIIX3" and "PIIX3-xen"
>
> * piix4_realize(), of PCI device "PIIX4"
>
> * vt82c686b_realize(), of PCI device "VT82C686B"
>
> Propagate the error.  Note that these devices are typically created
> only by machine init() methods with qdev_init_nofail() or similar.  If
> we screwed up and created an ISA bus before that call, we now give up
> right away.  Before, we'd hobble on, and typically die in
> isa_bus_irqs().  Similar if someone finds a way to hot-plug one of
> these critters.
>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: "Hervé Poussineau" <hpoussin@reactos.org>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Reviewed-by: Hervé Poussineau <hpoussin@reactos.org>

Re: [Qemu-devel] [PATCH 10/12] isa: Clean up inappropriate hw_error()

[Hervé Poussineau]

Le 10/12/2015 11:29, Markus Armbruster a écrit :
> isa_bus_irqs(), isa_create() and isa_try_create() call hw_error() when
> passed a null bus.  Use of hw_error() has always been questionable,
> because these are used only during machine initialization, and
> printing CPU registers isn't useful there.
>
> Since the previous commit, passing a null bus is a programming error.
> Drop the hw_error() and simply let it crash.
>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: "Hervé Poussineau" <hpoussin@reactos.org>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Creating a second ISA bus raises an error in isa_bus_new, so
Reviewed-by: Hervé Poussineau <hpoussin@reactos.org>

Re: [Qemu-devel] [PATCH 01/12] hw: Don't use hw_error() for machine initialization errors

[Thomas Huth]

On 10/12/15 11:29, Markus Armbruster wrote:
> Printing CPU registers is not helpful during machine initialization.
> Moreover, these are straightforward configuration or "can get
> resources" errors, so dumping core isn't appropriate either.  Replace
> hw_error() by error_report(); exit(1).  Matches how we report these
> errors in other machine initializations.
> 
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: qemu-arm@nongnu.org
> Cc: qemu-ppc@nongnu.org
> Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/alpha/dp264.c          | 11 ++++++-----
>  hw/arm/highbank.c         |  6 ++++--
>  hw/char/exynos4210_uart.c |  9 ++++++---
>  hw/m68k/an5206.c          |  4 +++-
>  hw/ppc/mac_newworld.c     | 11 ++++++-----
>  hw/ppc/mac_oldworld.c     | 16 +++++++++-------
>  hw/ppc/prep.c             | 10 ++++++----
>  hw/unicore32/puv3.c       | 10 +++++++---
>  8 files changed, 47 insertions(+), 30 deletions(-)
[...]
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 5ad28f7..8f08f07 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -33,6 +33,7 @@
>  #include "hw/pci/pci_host.h"
>  #include "hw/ppc/ppc.h"
>  #include "hw/boards.h"
> +#include "qemu/error-report.h"
>  #include "qemu/log.h"
>  #include "hw/ide.h"
>  #include "hw/loader.h"
> @@ -532,7 +533,7 @@ static void ppc_prep_init(MachineState *machine)
>          kernel_size = load_image_targphys(kernel_filename, kernel_base,
>                                            ram_size - kernel_base);
>          if (kernel_size < 0) {
> -            hw_error("qemu: could not load kernel '%s'\n", kernel_filename);
> +            error_report("could not load kernel '%s'", kernel_filename);
>              exit(1);
>          }
>          /* load initrd */
> @@ -541,8 +542,8 @@ static void ppc_prep_init(MachineState *machine)
>              initrd_size = load_image_targphys(initrd_filename, initrd_base,
>                                                ram_size - initrd_base);
>              if (initrd_size < 0) {
> -                hw_error("qemu: could not load initial ram disk '%s'\n",
> -                          initrd_filename);
> +                error_report("could not load initial ram disk '%s'",
> +                             initrd_filename);

Shouldn't you add an "exit(1)" here, too?

>              }
>          } else {
>              initrd_base = 0;

Apart from the missing exit(), the patch looks like a good idea to me.

 Thomas

Re: [Qemu-devel] [PATCH 05/12] raven: Mark use of hw_error() in realize() FIXME

[Thomas Huth]

On 10/12/15 11:29, Markus Armbruster wrote:
> Device realize() methods aren't supposed to call hw_error(), they
> should set an error and fail cleanly.  Blindly doing that would be
> easy enough, but then realize() would fail without undoing its side
> effects.  Just mark it FIXME for now.
> 
> Cc: "Andreas Färber" <andreas.faerber@web.de>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/pci-host/prep.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index da88cb3..f434596 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -326,6 +326,7 @@ static void raven_realize(PCIDevice *d, Error **errp)
>              }
>          }
>          if (bios_size < 0 || bios_size > BIOS_SIZE) {
> +            /* FIXME should error_setg() */
>              hw_error("qemu: could not load bios image '%s'\n", s->bios_name);
>          }
>          g_free(filename);
> @@ -355,8 +356,9 @@ static void raven_class_init(ObjectClass *klass, void *data)
>      dc->desc = "PReP Host Bridge - Motorola Raven";
>      dc->vmsd = &vmstate_raven;
>      /*
> -     * PCI-facing part of the host bridge, not usable without the
> -     * host-facing part, which can't be device_add'ed, yet.
> +     * Reason: PCI-facing part of the host bridge, not usable without
> +     * the host-facing part, which can't be device_add'ed, yet.
> +     * Reason: realize() method uses hw_error().
>       */
>      dc->cannot_instantiate_with_device_add_yet = true;
>  }

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 01/12] hw: Don't use hw_error() for machine initialization errors

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 10/12/15 11:29, Markus Armbruster wrote:
>> Printing CPU registers is not helpful during machine initialization.
>> Moreover, these are straightforward configuration or "can get
>> resources" errors, so dumping core isn't appropriate either.  Replace
>> hw_error() by error_report(); exit(1).  Matches how we report these
>> errors in other machine initializations.
>> 
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: qemu-arm@nongnu.org
>> Cc: qemu-ppc@nongnu.org
>> Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/alpha/dp264.c          | 11 ++++++-----
>>  hw/arm/highbank.c         |  6 ++++--
>>  hw/char/exynos4210_uart.c |  9 ++++++---
>>  hw/m68k/an5206.c          |  4 +++-
>>  hw/ppc/mac_newworld.c     | 11 ++++++-----
>>  hw/ppc/mac_oldworld.c     | 16 +++++++++-------
>>  hw/ppc/prep.c             | 10 ++++++----
>>  hw/unicore32/puv3.c       | 10 +++++++---
>>  8 files changed, 47 insertions(+), 30 deletions(-)
> [...]
>> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
>> index 5ad28f7..8f08f07 100644
>> --- a/hw/ppc/prep.c
>> +++ b/hw/ppc/prep.c
>> @@ -33,6 +33,7 @@
>>  #include "hw/pci/pci_host.h"
>>  #include "hw/ppc/ppc.h"
>>  #include "hw/boards.h"
>> +#include "qemu/error-report.h"
>>  #include "qemu/log.h"
>>  #include "hw/ide.h"
>>  #include "hw/loader.h"
>> @@ -532,7 +533,7 @@ static void ppc_prep_init(MachineState *machine)
>>          kernel_size = load_image_targphys(kernel_filename, kernel_base,
>>                                            ram_size - kernel_base);
>>          if (kernel_size < 0) {
>> -            hw_error("qemu: could not load kernel '%s'\n", kernel_filename);
>> +            error_report("could not load kernel '%s'", kernel_filename);
>>              exit(1);
>>          }
>>          /* load initrd */
>> @@ -541,8 +542,8 @@ static void ppc_prep_init(MachineState *machine)
>>              initrd_size = load_image_targphys(initrd_filename, initrd_base,
>>                                                ram_size - initrd_base);
>>              if (initrd_size < 0) {
>> -                hw_error("qemu: could not load initial ram disk '%s'\n",
>> -                          initrd_filename);
>> +                error_report("could not load initial ram disk '%s'",
>> +                             initrd_filename);
>
> Shouldn't you add an "exit(1)" here, too?

Yes.

I rechecked the complete series for additional instances of this
mistake, and found none.

>>              }
>>          } else {
>>              initrd_base = 0;
>
> Apart from the missing exit(), the patch looks like a good idea to me.

Thanks!

Re: [Qemu-devel] [PATCH 11/12] audio: Clean up inappropriate and unreachable use of hw_error()

[Gerd Hoffmann]

On Do, 2015-12-10 at 11:29 +0100, Markus Armbruster wrote:
> audio_init() should not use hw_error(), because dumping CPU registers
> is unhelpful there, and aborting is wrong, because it can be called
> called from an audio device's realize() method.
> 
> The two uses of hw_error() come from commit 0d9acba:
> 
> * When qemu_new_timer() fails.  It couldn't fail back then, and it
>   can't fail now.  Drop the unreachable error handling.
> 
> * When no_audio_driver can't be initialized.  It couldn't fail back
>   then, and it can't fail now.  Replace the error handling by an
>   assertion.
> 
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

> ---
>  audio/audio.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/audio/audio.c b/audio/audio.c
> index 5be4b15..9b855ed 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1806,9 +1806,6 @@ static void audio_init (void)
>      atexit (audio_atexit);
>  
>      s->ts = timer_new_ns(QEMU_CLOCK_VIRTUAL, audio_timer, s);
> -    if (!s->ts) {
> -        hw_error("Could not create audio timer\n");
> -    }
>  
>      audio_process_options ("AUDIO", audio_options);
>  
> @@ -1859,12 +1856,8 @@ static void audio_init (void)
>  
>      if (!done) {
>          done = !audio_driver_init (s, &no_audio_driver);
> -        if (!done) {
> -            hw_error("Could not initialize audio subsystem\n");
> -        }
> -        else {
> -            dolog ("warning: Using timer based audio emulation\n");
> -        }
> +        assert(done);
> +        dolog ("warning: Using timer based audio emulation\n");
>      }
>  
>      if (conf.period.hertz <= 0) {

Re: [Qemu-devel] [PATCH 06/12] hw/arm/virt: Fix property "gic-version" error handling

[Peter Maydell]

On 10 December 2015 at 10:29, Markus Armbruster <armbru@redhat.com> wrote:
> virt_set_gic_version() calls exit(1) when passed an invalid property
> value.  Property setters are not supposed to do that.  Screwed up in
> commit b92ad39.  Harmless, because the property belongs to a machine.
> Set an error object instead.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/arm/virt.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 9c6792c..2a120dd 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1126,9 +1126,8 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp)
>      } else if (!strcmp(value, "host")) {
>          vms->gic_version = 0; /* Will probe later */
>      } else {
> -        error_report("Invalid gic-version option value");
> -        error_printf("Allowed gic-version values are: 3, 2, host\n");
> -        exit(1);
> +        error_setg(errp, "Invalid gic-version value");
> +        error_append_hint(errp, "Valid values are: 3, 2, host\n");
>      }

Should hint strings have trailing newlines?

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 06/12] hw/arm/virt: Fix property "gic-version" error handling

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 10 December 2015 at 10:29, Markus Armbruster <armbru@redhat.com> wrote:
>> virt_set_gic_version() calls exit(1) when passed an invalid property
>> value.  Property setters are not supposed to do that.  Screwed up in
>> commit b92ad39.  Harmless, because the property belongs to a machine.
>> Set an error object instead.
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: qemu-arm@nongnu.org
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/arm/virt.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 9c6792c..2a120dd 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1126,9 +1126,8 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp)
>>      } else if (!strcmp(value, "host")) {
>>          vms->gic_version = 0; /* Will probe later */
>>      } else {
>> -        error_report("Invalid gic-version option value");
>> -        error_printf("Allowed gic-version values are: 3, 2, host\n");
>> -        exit(1);
>> +        error_setg(errp, "Invalid gic-version value");
>> +        error_append_hint(errp, "Valid values are: 3, 2, host\n");
>>      }
>
> Should hint strings have trailing newlines?

Two answers:

1. No, because error_report_err() prints the hint followed by a newline.

2. Yes, because error_appent_hint() accumulates, and requiring its users
to know which call will be the final one is an awkward interface.

I'll change error_report_err() and improve the documentation.

> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks!