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

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

[Markus Armbruster]

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

v3:
* PATCH 02: Cover a missed instance, fix botched code motion [Thomas]
* PATCH 11: Add some assertions [Marcel, Michael]
v2:
* PATCH 01: Supply missing exit() [Thomas]
* PATCH 02: Improve commit message [Peter]
* PATCH 03: Fix up error message [Peter]
* PATCH 06: New, to unbreak the next patch [Peter]

Markus Armbruster (13):
  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
  error: Don't append a newline when printing the error hint
  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       | 29 +++++++++++++++++++++--------
 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          | 18 +++++-------------
 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             | 11 +++++++----
 hw/sparc64/sun4u.c        | 12 ++++++------
 hw/timer/arm_mptimer.c    |  5 +++--
 hw/unicore32/puv3.c       | 10 +++++++---
 include/hw/isa/isa.h      |  2 +-
 qdev-monitor.c            |  2 ++
 util/error.c              |  2 +-
 util/qemu-option.c        |  4 ++--
 xen-hvm.c                 |  7 +++++++
 33 files changed, 161 insertions(+), 110 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v3 01/13] 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@pond.sub.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Thomas Huth <thuth@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             | 11 +++++++----
 hw/unicore32/puv3.c       | 10 +++++++---
 8 files changed, 48 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 a0a5a06..cb9926e 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -315,11 +315,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..0e102fc 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,9 @@ 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);
+                exit(1);
             }
         } else {
             initrd_base = 0;
@@ -569,7 +571,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 v3 02/13] 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.

The errors are all device misconfiguration.  All callers use
qdev_init_nofail(), so this patch merely converts hw_error() crashes
into &error_abort crashes.  Improvement, because now it crashes closer
to where the misconfiguration bug would be, and a few more bad
examples of hw_error() use are gone.

Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/gpio/omap_gpio.c | 29 +++++++++++++++++++++--------
 hw/i2c/omap_i2c.c   |  8 ++++++--
 hw/intc/omap_intc.c | 10 +++++++---
 3 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/hw/gpio/omap_gpio.c b/hw/gpio/omap_gpio.c
index 3c53898..63d8b42 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;
@@ -682,7 +683,8 @@ static int omap_gpio_init(SysBusDevice *sbd)
     struct omap_gpif_s *s = OMAP1_GPIO(dev);
 
     if (!s->clk) {
-        hw_error("omap-gpio: clk not connected\n");
+        error_report("omap-gpio: clk not connected");
+        return -1;
     }
     qdev_init_gpio_in(dev, omap_gpio_set, 16);
     qdev_init_gpio_out(dev, s->omap1.handler, 16);
@@ -700,25 +702,35 @@ 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;
     }
+
+    s->modulecount = s->mpu_model < omap2430 ? 4
+                   : s->mpu_model < omap3430 ? 5
+                   : 6;
+
+    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,
                               "omap2.gpio", 0x1000);
         sysbus_init_mmio(sbd, &s->iomem);
-    } 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 +740,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 v3 03/13] 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 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..5dfab66 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",
+                   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 v3 04/13] 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@pond.sub.org>
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

[Qemu-devel] [PATCH v3 05/13] 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@pond.sub.org>
Reviewed-by: Thomas Huth <thuth@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 v3 06/13] error: Don't append a newline when printing the error hint

[Markus Armbruster]

Since commit 50b7b00, we have error_append_hint() to conveniently
accumulate Error member @hint.  error_report_err() prints it with a
newline appended.  Consequently, users of error_append_hint() need to
know whether theirs is the final line of the hint to decide whether it
needs a newline.  Not a nice interface.

Change error_report_err() to print just the hint, and the (still few)
users of error_append_hint() to add the required newline.

Cc: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qdev-monitor.c     | 2 ++
 util/error.c       | 2 +-
 util/qemu-option.c | 4 ++--
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index a35098f..30936df 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -304,6 +304,7 @@ static void qbus_list_bus(DeviceState *dev, Error **errp)
         error_append_hint(errp, "%s\"%s\"", sep, child->name);
         sep = ", ";
     }
+    error_append_hint(errp, "\n");
 }
 
 static void qbus_list_dev(BusState *bus, Error **errp)
@@ -321,6 +322,7 @@ static void qbus_list_dev(BusState *bus, Error **errp)
         }
         sep = ", ";
     }
+    error_append_hint(errp, "\n");
 }
 
 static BusState *qbus_find_bus(DeviceState *dev, char *elem)
diff --git a/util/error.c b/util/error.c
index 80c89a2..9b27c45 100644
--- a/util/error.c
+++ b/util/error.c
@@ -204,7 +204,7 @@ void error_report_err(Error *err)
 {
     error_report("%s", error_get_pretty(err));
     if (err->hint) {
-        error_printf_unless_qmp("%s\n", err->hint->str);
+        error_printf_unless_qmp("%s", err->hint->str);
     }
     error_free(err);
 }
diff --git a/util/qemu-option.c b/util/qemu-option.c
index a50ecea..a2d593a 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -206,7 +206,7 @@ void parse_option_size(const char *name, const char *value,
         default:
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a size");
             error_append_hint(errp, "You may use k, M, G or T suffixes for "
-                    "kilobytes, megabytes, gigabytes and terabytes.");
+                    "kilobytes, megabytes, gigabytes and terabytes.\n");
             return;
         }
     } else {
@@ -647,7 +647,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "id",
                        "an identifier");
             error_append_hint(errp, "Identifiers consist of letters, digits, "
-                              "'-', '.', '_', starting with a letter.");
+                              "'-', '.', '_', starting with a letter.\n");
             return NULL;
         }
         opts = qemu_opts_find(list, id);
-- 
2.4.3

[Qemu-devel] [PATCH v3 07/13] 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 v3 08/13] 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>
Reviewed-by: Thomas Huth <thuth@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 v3 09/13] 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@pond.sub.org>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@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 v3 10/13] 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@pond.sub.org>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Hervé Poussineau <hpoussin@reactos.org>
Reviewed-by: Michael S. Tsirkin <mst@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 v3 11/13] 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@pond.sub.org>
Reviewed-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/isa/isa-bus.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index af6ffd6..a24d32c 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -63,9 +63,7 @@ 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.");
-    }
+    assert(bus);
     bus->irqs = irqs;
 }
 
@@ -137,10 +135,7 @@ 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);
-    }
+    assert(bus);
     dev = qdev_create(BUS(bus), name);
     return ISA_DEVICE(dev);
 }
@@ -149,10 +144,7 @@ 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);
-    }
+    assert(bus);
     dev = qdev_try_create(BUS(bus), name);
     return ISA_DEVICE(dev);
 }
-- 
2.4.3

[Qemu-devel] [PATCH v3 12/13] 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@pond.sub.org>
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) {
-- 
2.4.3

[Qemu-devel] [PATCH v3 13/13] 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 v3 13/13] 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 v3 07/13] hw/arm/virt: Fix property "gic-version" error handling

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> 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 be "Valid values are 3, 2, host.\n" for consistency with similar
hints elsewhere.

Re: [Qemu-devel] [Xen-devel] [PATCH v3 13/13] xen-hvm: Mark inappropriate error handling FIXME

[Stefano Stabellini]

On Thu, 17 Dec 2015, Markus Armbruster wrote:
> 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()

Could you please stop the comment here and just replace hw_error() with
either return -1 or exit(1) within xen_hvm_init?


> +     * 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
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [Xen-devel] [PATCH v3 13/13] xen-hvm: Mark inappropriate error handling FIXME

[Stefano Stabellini]

On Thu, 17 Dec 2015, Markus Armbruster wrote:
> 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()

Could you please stop the comment here and just replace hw_error() with
either return -1 or exit(1) within xen_hvm_init?


> +     * 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
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [Qemu-devel] [Xen-devel] [PATCH v3 13/13] xen-hvm: Mark inappropriate error handling FIXME

[Markus Armbruster]

Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:

> On Thu, 17 Dec 2015, Markus Armbruster wrote:
>> 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()
>
> Could you please stop the comment here and just replace hw_error() with
> either return -1 or exit(1) within xen_hvm_init?

If compile-testing is okay, I can either

(1) replace hw_error() by return -1, or

(2) replace both hw_error() and return -1 by exit(1), and make
xen_hvm_init() return void.

Your pick?

>
>> +     * 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;

Re: [Xen-devel] [PATCH v3 13/13] xen-hvm: Mark inappropriate error handling FIXME

[Markus Armbruster]

Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:

> On Thu, 17 Dec 2015, Markus Armbruster wrote:
>> 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()
>
> Could you please stop the comment here and just replace hw_error() with
> either return -1 or exit(1) within xen_hvm_init?

If compile-testing is okay, I can either

(1) replace hw_error() by return -1, or

(2) replace both hw_error() and return -1 by exit(1), and make
xen_hvm_init() return void.

Your pick?

>
>> +     * 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;

Re: [Qemu-devel] [Xen-devel] [PATCH v3 13/13] xen-hvm: Mark inappropriate error handling FIXME

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
>
>> On Thu, 17 Dec 2015, Markus Armbruster wrote:
>>> 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()
>>
>> Could you please stop the comment here and just replace hw_error() with
>> either return -1 or exit(1) within xen_hvm_init?
>
> If compile-testing is okay, I can either
>
> (1) replace hw_error() by return -1, or
>
> (2) replace both hw_error() and return -1 by exit(1), and make
> xen_hvm_init() return void.
>
> Your pick?

If you don't mind, I'll do (2) as a follow-up patch.  (2) because I like
it better, and follow-up patch because I'd prefer not to delay my error
pull request any longer.

Re: [Xen-devel] [PATCH v3 13/13] xen-hvm: Mark inappropriate error handling FIXME

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
>
>> On Thu, 17 Dec 2015, Markus Armbruster wrote:
>>> 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()
>>
>> Could you please stop the comment here and just replace hw_error() with
>> either return -1 or exit(1) within xen_hvm_init?
>
> If compile-testing is okay, I can either
>
> (1) replace hw_error() by return -1, or
>
> (2) replace both hw_error() and return -1 by exit(1), and make
> xen_hvm_init() return void.
>
> Your pick?

If you don't mind, I'll do (2) as a follow-up patch.  (2) because I like
it better, and follow-up patch because I'd prefer not to delay my error
pull request any longer.

Re: [Qemu-devel] [Xen-devel] [PATCH v3 13/13] xen-hvm: Mark inappropriate error handling FIXME

[Stefano Stabellini]

On Wed, 13 Jan 2016, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
> >
> >> On Thu, 17 Dec 2015, Markus Armbruster wrote:
> >>> 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()
> >>
> >> Could you please stop the comment here and just replace hw_error() with
> >> either return -1 or exit(1) within xen_hvm_init?
> >
> > If compile-testing is okay, I can either
> >
> > (1) replace hw_error() by return -1, or
> >
> > (2) replace both hw_error() and return -1 by exit(1), and make
> > xen_hvm_init() return void.
> >
> > Your pick?
> 
> If you don't mind, I'll do (2) as a follow-up patch.  (2) because I like
> it better, and follow-up patch because I'd prefer not to delay my error
> pull request any longer.

That's fine. Sorry, I lost your previous email.

Re: [Xen-devel] [PATCH v3 13/13] xen-hvm: Mark inappropriate error handling FIXME

[Stefano Stabellini]

On Wed, 13 Jan 2016, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
> >
> >> On Thu, 17 Dec 2015, Markus Armbruster wrote:
> >>> 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()
> >>
> >> Could you please stop the comment here and just replace hw_error() with
> >> either return -1 or exit(1) within xen_hvm_init?
> >
> > If compile-testing is okay, I can either
> >
> > (1) replace hw_error() by return -1, or
> >
> > (2) replace both hw_error() and return -1 by exit(1), and make
> > xen_hvm_init() return void.
> >
> > Your pick?
> 
> If you don't mind, I'll do (2) as a follow-up patch.  (2) because I like
> it better, and follow-up patch because I'd prefer not to delay my error
> pull request any longer.

That's fine. Sorry, I lost your previous email.