[PATCH v2 00/18] Error handling fixes

[PATCH v2 00/18] Error handling fixes

[Markus Armbruster]

v2:
* Old PATCH 01-03 have been merged for 4.2
* PATCH 05-15: Commit message rephrased [David], R-bys kept

Cc: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Corey Minyard <cminyard@mvista.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Halil Pasic <pasic@linux.ibm.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Michael Roth <mdroth@linux.vnet.ibm.com>

Markus Armbruster (18):
  crypto: Fix certificate file error handling crash bug
  crypto: Fix typo in QCryptoTLSSession's <example> comment
  io: Fix Error usage in a comment <example>
  tests: Clean up initialization of Error *err variables
  exec: Fix file_ram_alloc() error API violations
  hw/acpi: Fix legacy CPU plug error API violations
  hw/core: Fix fit_load_fdt() error handling violations
  hw/ipmi: Fix realize() error API violations
  qga: Fix guest-get-fsinfo error API violations
  memory-device: Fix memory pre-plug error API violations
  s390x/event-facility: Fix realize() error API violations
  s390x/cpumodel: Fix feature property error API violations
  s390x/cpumodel: Fix realize() error API violations
  s390x/cpumodel: Fix query-cpu-model-FOO error API violations
  s390x/cpumodel: Fix query-cpu-definitions error API violations
  error: Clean up unusual names of Error * variables
  hw/intc/s390: Simplify error handling in kvm_s390_flic_realize()
  tests-blockjob: Use error_free_or_abort()

 include/crypto/tlssession.h         |  2 +-
 include/io/task.h                   |  2 +-
 crypto/tlscredsx509.c               |  2 +-
 exec.c                              |  6 +-
 hw/acpi/cpu_hotplug.c               | 10 +--
 hw/core/loader-fit.c                | 15 ++---
 hw/intc/s390_flic_kvm.c             | 16 +++--
 hw/ipmi/isa_ipmi_bt.c               |  7 ++-
 hw/ipmi/isa_ipmi_kcs.c              |  7 ++-
 hw/ipmi/pci_ipmi_bt.c               |  6 +-
 hw/ipmi/pci_ipmi_kcs.c              |  6 +-
 hw/mem/memory-device.c              |  6 +-
 hw/ppc/spapr_pci.c                  | 16 ++---
 hw/ppc/spapr_pci_nvlink2.c          | 10 +--
 hw/s390x/event-facility.c           |  6 +-
 qga/commands-posix.c                |  6 +-
 target/s390x/cpu_models.c           | 98 +++++++++++++++++------------
 tests/test-blockjob.c               | 15 +++--
 tests/test-qobject-output-visitor.c |  8 +--
 tests/test-string-output-visitor.c  |  4 +-
 20 files changed, 139 insertions(+), 109 deletions(-)

-- 
2.21.0

[PATCH v2 01/18] crypto: Fix certificate file error handling crash bug

[Markus Armbruster]

qcrypto_tls_creds_load_cert() passes uninitialized GError *gerr by
reference to g_file_get_contents().  When g_file_get_contents() fails,
it'll try to set a GError.  Unless @gerr is null by dumb luck, this
logs a ERROR_OVERWRITTEN_WARNING warning message and leaves @gerr
unchanged.  qcrypto_tls_creds_load_cert() then dereferences the
uninitialized @gerr.

Fix by initializing @gerr properly.

Fixes: 9a2fd4347c40321f5cbb4ab4220e759fcbf87d03
Cc: "Daniel P. Berrangé" <berrange@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 crypto/tlscredsx509.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 01fc304e5d..53a4368f49 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -380,7 +380,7 @@ qcrypto_tls_creds_load_cert(QCryptoTLSCredsX509 *creds,
     gnutls_x509_crt_t cert = NULL;
     g_autofree char *buf = NULL;
     gsize buflen;
-    GError *gerr;
+    GError *gerr = NULL;
     int ret = -1;
     int err;
 
-- 
2.21.0

[PATCH v2 02/18] crypto: Fix typo in QCryptoTLSSession's <example> comment

[Markus Armbruster]

Cc: "Daniel P. Berrangé" <berrange@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/crypto/tlssession.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index e01e1a9dc2..15b9cef086 100644
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -56,7 +56,7 @@
  *
  * static int mysock_run_tls(int sockfd,
  *                           QCryptoTLSCreds *creds,
- *                           Error *errp)
+ *                           Error **errp)
  * {
  *    QCryptoTLSSession *sess;
  *
-- 
2.21.0

[PATCH v2 03/18] io: Fix Error usage in a comment <example>

[Markus Armbruster]

Cc: "Daniel P. Berrangé" <berrange@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/io/task.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/io/task.h b/include/io/task.h
index 5cb9faf9f2..1abbfb8b65 100644
--- a/include/io/task.h
+++ b/include/io/task.h
@@ -119,7 +119,7 @@ typedef void (*QIOTaskWorker)(QIOTask *task,
  *   gboolean myobject_operation_timer(gpointer opaque)
  *   {
  *      QIOTask *task = QIO_TASK(opaque);
- *      Error *err;*
+ *      Error *err = NULL;
  *
  *      ...check something important...
  *       if (err) {
-- 
2.21.0

[PATCH v2 04/18] tests: Clean up initialization of Error *err variables

[Markus Armbruster]

Declaring a local Error *err without initializer looks suspicious.
Fuse the declaration with the initialization to avoid that.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-qobject-output-visitor.c | 8 ++++----
 tests/test-string-output-visitor.c  | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c
index 3e993e5ba8..d7761ebf84 100644
--- a/tests/test-qobject-output-visitor.c
+++ b/tests/test-qobject-output-visitor.c
@@ -145,10 +145,10 @@ static void test_visitor_out_enum_errors(TestOutputVisitorData *data,
                                          const void *unused)
 {
     EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
-    Error *err;
 
     for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-        err = NULL;
+        Error *err = NULL;
+
         visit_type_EnumOne(data->ov, "unused", &bad_values[i], &err);
         error_free_or_abort(&err);
         visitor_reset(data);
@@ -240,11 +240,11 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
     EnumOne bad_values[] = { ENUM_ONE__MAX, -1 };
     UserDefOne u = {0};
     UserDefOne *pu = &u;
-    Error *err;
     int i;
 
     for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-        err = NULL;
+        Error *err = NULL;
+
         u.has_enum1 = true;
         u.enum1 = bad_values[i];
         visit_type_UserDefOne(data->ov, "unused", &pu, &err);
diff --git a/tests/test-string-output-visitor.c b/tests/test-string-output-visitor.c
index 02766c0f65..1be1540767 100644
--- a/tests/test-string-output-visitor.c
+++ b/tests/test-string-output-visitor.c
@@ -207,10 +207,10 @@ static void test_visitor_out_enum_errors(TestOutputVisitorData *data,
                                          const void *unused)
 {
     EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
-    Error *err;
 
     for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-        err = NULL;
+        Error *err = NULL;
+
         visit_type_EnumOne(data->ov, "unused", &bad_values[i], &err);
         error_free_or_abort(&err);
     }
-- 
2.21.0

[PATCH v2 05/18] exec: Fix file_ram_alloc() error API violations

[Markus Armbruster]

When os_mem_prealloc() fails, file_ram_alloc() calls qemu_ram_munmap()
and returns null.  Except it doesn't when its @errp argument is null,
because it checks for failure with (errp && *errp).  Introduced in
commit 056b68af77 "fix qemu exit on memory hotplug when allocation
fails at prealloc time".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 exec.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index ffdb518535..45695a5f2d 100644
--- a/exec.c
+++ b/exec.c
@@ -1841,6 +1841,7 @@ static void *file_ram_alloc(RAMBlock *block,
                             bool truncate,
                             Error **errp)
 {
+    Error *err = NULL;
     MachineState *ms = MACHINE(qdev_get_machine());
     void *area;
 
@@ -1898,8 +1899,9 @@ static void *file_ram_alloc(RAMBlock *block,
     }
 
     if (mem_prealloc) {
-        os_mem_prealloc(fd, area, memory, ms->smp.cpus, errp);
-        if (errp && *errp) {
+        os_mem_prealloc(fd, area, memory, ms->smp.cpus, &err);
+        if (err) {
+            error_propagate(errp, err);
             qemu_ram_munmap(fd, area, memory);
             return NULL;
         }
-- 
2.21.0

[PATCH v2 06/18] hw/acpi: Fix legacy CPU plug error API violations

[Markus Armbruster]

legacy_acpi_cpu_plug_cb() dereferences @errp when
acpi_set_cpu_present_bit() fails.  That's wrong; see the big comment
in error.h.  Introduced in commit cc43364de7 "acpi/cpu-hotplug:
introduce helper function to keep bit setting in one place".

No caller actually passes null, and acpi_set_cpu_present_bit() can't
actually fail.

Fix anyway: drop acpi_set_cpu_present_bit()'s @errp parameter.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/cpu_hotplug.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index 3ac2045a95..9c3bcc84de 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -55,8 +55,7 @@ static const MemoryRegionOps AcpiCpuHotplug_ops = {
     },
 };
 
-static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu,
-                                     Error **errp)
+static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu)
 {
     CPUClass *k = CPU_GET_CLASS(cpu);
     int64_t cpu_id;
@@ -74,10 +73,7 @@ static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu,
 void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
                              AcpiCpuHotplug *g, DeviceState *dev, Error **errp)
 {
-    acpi_set_cpu_present_bit(g, CPU(dev), errp);
-    if (*errp != NULL) {
-        return;
-    }
+    acpi_set_cpu_present_bit(g, CPU(dev));
     acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
 }
 
@@ -92,7 +88,7 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
     gpe_cpu->device = owner;
 
     CPU_FOREACH(cpu) {
-        acpi_set_cpu_present_bit(gpe_cpu, cpu, &error_abort);
+        acpi_set_cpu_present_bit(gpe_cpu, cpu);
     }
 }
 
-- 
2.21.0

[PATCH v2 07/18] hw/core: Fix fit_load_fdt() error handling violations

[Markus Armbruster]

fit_load_fdt() passes @errp to fit_image_addr(), then recovers from
ENOENT failures.  Passing @errp is wrong, because it works only as
long as @errp is neither @error_fatal nor @error_abort.  Messed up in
commit 3eb99edb48 "loader-fit: Wean off error_printf()".

No caller actually passes such values.

Fix anyway: splice in a local Error *err, and error_propagate().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/core/loader-fit.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
index 953b16bc82..c465921b8f 100644
--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -178,11 +178,12 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
                         int cfg, void *opaque, const void *match_data,
                         hwaddr kernel_end, Error **errp)
 {
+    Error *err = NULL;
     const char *name;
     const void *data;
     const void *load_data;
     hwaddr load_addr;
-    int img_off, err;
+    int img_off;
     size_t sz;
     int ret;
 
@@ -197,13 +198,13 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
         return -EINVAL;
     }
 
-    err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
-    if (err == -ENOENT) {
+    ret = fit_image_addr(itb, img_off, "load", &load_addr, &err);
+    if (ret == -ENOENT) {
         load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
-        error_free(*errp);
-    } else if (err) {
-        error_prepend(errp, "unable to read FDT load address from FIT: ");
-        ret = err;
+        error_free(err);
+    } else if (ret) {
+        error_propagate_prepend(errp, err,
+                                "unable to read FDT load address from FIT: ");
         goto out;
     }
 
-- 
2.21.0

[PATCH v2 08/18] hw/ipmi: Fix realize() error API violations

[Markus Armbruster]

isa_ipmi_bt_realize(), ipmi_isa_realize(), pci_ipmi_bt_realize(), and
pci_ipmi_kcs_realize() dereference @errp when IPMIInterfaceClass
method init() fails.  That's wrong; see the big comment in error.h.
Introduced in commit 0719029c47 "ipmi: Add an ISA KCS low-level
interface", then imitated in commit a9b74079cb "ipmi: Add a BT
low-level interface" and commit 12f983c6aa "ipmi: Add PCI IPMI
interfaces".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: Corey Minyard <cminyard@mvista.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ipmi/isa_ipmi_bt.c  | 7 +++++--
 hw/ipmi/isa_ipmi_kcs.c | 7 +++++--
 hw/ipmi/pci_ipmi_bt.c  | 6 ++++--
 hw/ipmi/pci_ipmi_kcs.c | 6 ++++--
 4 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index 9a87ffd3f0..9fba5ed383 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -70,6 +70,7 @@ static void isa_ipmi_bt_lower_irq(IPMIBT *ib)
 
 static void isa_ipmi_bt_realize(DeviceState *dev, Error **errp)
 {
+    Error *err = NULL;
     ISADevice *isadev = ISA_DEVICE(dev);
     ISAIPMIBTDevice *iib = ISA_IPMI_BT(dev);
     IPMIInterface *ii = IPMI_INTERFACE(dev);
@@ -85,9 +86,11 @@ static void isa_ipmi_bt_realize(DeviceState *dev, Error **errp)
     iib->bt.bmc->intf = ii;
     iib->bt.opaque = iib;
 
-    iic->init(ii, 0, errp);
-    if (*errp)
+    iic->init(ii, 0, &err);
+    if (err) {
+        error_propagate(errp, err);
         return;
+    }
 
     if (iib->isairq > 0) {
         isa_init_irq(isadev, &iib->irq, iib->isairq);
diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
index ca3ea36a3f..cc6bd817f2 100644
--- a/hw/ipmi/isa_ipmi_kcs.c
+++ b/hw/ipmi/isa_ipmi_kcs.c
@@ -69,6 +69,7 @@ static void isa_ipmi_kcs_lower_irq(IPMIKCS *ik)
 
 static void ipmi_isa_realize(DeviceState *dev, Error **errp)
 {
+    Error *err = NULL;
     ISADevice *isadev = ISA_DEVICE(dev);
     ISAIPMIKCSDevice *iik = ISA_IPMI_KCS(dev);
     IPMIInterface *ii = IPMI_INTERFACE(dev);
@@ -84,9 +85,11 @@ static void ipmi_isa_realize(DeviceState *dev, Error **errp)
     iik->kcs.bmc->intf = ii;
     iik->kcs.opaque = iik;
 
-    iic->init(ii, 0, errp);
-    if (*errp)
+    iic->init(ii, 0, &err);
+    if (err) {
+        error_propagate(errp, err);
         return;
+    }
 
     if (iik->isairq > 0) {
         isa_init_irq(isadev, &iik->irq, iik->isairq);
diff --git a/hw/ipmi/pci_ipmi_bt.c b/hw/ipmi/pci_ipmi_bt.c
index 6ed925a665..ba9cf016b5 100644
--- a/hw/ipmi/pci_ipmi_bt.c
+++ b/hw/ipmi/pci_ipmi_bt.c
@@ -54,6 +54,7 @@ static void pci_ipmi_lower_irq(IPMIBT *ik)
 
 static void pci_ipmi_bt_realize(PCIDevice *pd, Error **errp)
 {
+    Error *err = NULL;
     PCIIPMIBTDevice *pik = PCI_IPMI_BT(pd);
     IPMIInterface *ii = IPMI_INTERFACE(pd);
     IPMIInterfaceClass *iic = IPMI_INTERFACE_GET_CLASS(ii);
@@ -74,8 +75,9 @@ static void pci_ipmi_bt_realize(PCIDevice *pd, Error **errp)
     pik->bt.raise_irq = pci_ipmi_raise_irq;
     pik->bt.lower_irq = pci_ipmi_lower_irq;
 
-    iic->init(ii, 8, errp);
-    if (*errp) {
+    iic->init(ii, 8, &err);
+    if (err) {
+        error_propagate(errp, err);
         return;
     }
     pci_register_bar(pd, 0, PCI_BASE_ADDRESS_SPACE_IO, &pik->bt.io);
diff --git a/hw/ipmi/pci_ipmi_kcs.c b/hw/ipmi/pci_ipmi_kcs.c
index eeba63baa4..99f46152f4 100644
--- a/hw/ipmi/pci_ipmi_kcs.c
+++ b/hw/ipmi/pci_ipmi_kcs.c
@@ -54,6 +54,7 @@ static void pci_ipmi_lower_irq(IPMIKCS *ik)
 
 static void pci_ipmi_kcs_realize(PCIDevice *pd, Error **errp)
 {
+    Error *err = NULL;
     PCIIPMIKCSDevice *pik = PCI_IPMI_KCS(pd);
     IPMIInterface *ii = IPMI_INTERFACE(pd);
     IPMIInterfaceClass *iic = IPMI_INTERFACE_GET_CLASS(ii);
@@ -74,8 +75,9 @@ static void pci_ipmi_kcs_realize(PCIDevice *pd, Error **errp)
     pik->kcs.raise_irq = pci_ipmi_raise_irq;
     pik->kcs.lower_irq = pci_ipmi_lower_irq;
 
-    iic->init(ii, 8, errp);
-    if (*errp) {
+    iic->init(ii, 8, &err);
+    if (err) {
+        error_propagate(errp, err);
         return;
     }
     pci_register_bar(pd, 0, PCI_BASE_ADDRESS_SPACE_IO, &pik->kcs.io);
-- 
2.21.0

[PATCH v2 09/18] qga: Fix guest-get-fsinfo error API violations

[Markus Armbruster]

build_guest_fsinfo_for_virtual_device() dereferences @errp when
build_guest_fsinfo_for_device() fails.  That's wrong; see the big
comment in error.h.  Introduced in commit 46d4c5723e "qga: Add
guest-get-fsinfo command".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/commands-posix.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1c1a165dae..0be527ccb8 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1049,6 +1049,7 @@ static void build_guest_fsinfo_for_virtual_device(char const *syspath,
                                                   GuestFilesystemInfo *fs,
                                                   Error **errp)
 {
+    Error *err = NULL;
     DIR *dir;
     char *dirpath;
     struct dirent *entry;
@@ -1078,10 +1079,11 @@ static void build_guest_fsinfo_for_virtual_device(char const *syspath,
 
             g_debug(" slave device '%s'", entry->d_name);
             path = g_strdup_printf("%s/slaves/%s", syspath, entry->d_name);
-            build_guest_fsinfo_for_device(path, fs, errp);
+            build_guest_fsinfo_for_device(path, fs, &err);
             g_free(path);
 
-            if (*errp) {
+            if (err) {
+                error_propagate(errp, err);
                 break;
             }
         }
-- 
2.21.0

[PATCH v2 10/18] memory-device: Fix memory pre-plug error API violations

[Markus Armbruster]

memory_device_get_free_addr() dereferences @errp when
memory_device_check_addable() fails.  That's wrong; see the big
comment in error.h.  Introduced in commit 1b6d6af21b "pc-dimm: factor
out capacity and slot checks into MemoryDevice".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: David Hildenbrand <david@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index aef148c1d7..4bc9cf0917 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -99,6 +99,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                                             uint64_t align, uint64_t size,
                                             Error **errp)
 {
+    Error *err = NULL;
     GSList *list = NULL, *item;
     Range as, new = range_empty;
 
@@ -123,8 +124,9 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
         return 0;
     }
 
-    memory_device_check_addable(ms, size, errp);
-    if (*errp) {
+    memory_device_check_addable(ms, size, &err);
+    if (err) {
+        error_propagate(errp, err);
         return 0;
     }
 
-- 
2.21.0

[PATCH v2 11/18] s390x/event-facility: Fix realize() error API violations

[Markus Armbruster]

sclp_events_bus_realize() dereferences @errp when
object_property_set_bool() fails.  That's wrong; see the big comment
in error.h.  Introduced in commit f6102c329c "s390/sclp: rework sclp
event facility initialization + device realization".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/event-facility.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 66205697ae..cdcf9154c4 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -339,14 +339,16 @@ out:
 
 static void sclp_events_bus_realize(BusState *bus, Error **errp)
 {
+    Error *err = NULL;
     BusChild *kid;
 
     /* TODO: recursive realization has to be done in common code */
     QTAILQ_FOREACH(kid, &bus->children, sibling) {
         DeviceState *dev = kid->child;
 
-        object_property_set_bool(OBJECT(dev), true, "realized", errp);
-        if (*errp) {
+        object_property_set_bool(OBJECT(dev), true, "realized", &err);
+        if (errp) {
+            error_propagate(errp, err);
             return;
         }
     }
-- 
2.21.0

[PATCH v2 12/18] s390x/cpumodel: Fix feature property error API violations

[Markus Armbruster]

s390x-cpu property setters set_feature() and set_feature_group()
dereference @errp when the visitor fails.  That's wrong; see the big
comment in error.h.  Introduced in commit 0754f60429 "s390x/cpumodel:
expose features and feature groups as properties".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu_models.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 7e92fb2e15..6a29fd3ab1 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -987,6 +987,7 @@ static void get_feature(Object *obj, Visitor *v, const char *name,
 static void set_feature(Object *obj, Visitor *v, const char *name,
                         void *opaque, Error **errp)
 {
+    Error *err = NULL;
     S390Feat feat = (S390Feat) opaque;
     DeviceState *dev = DEVICE(obj);
     S390CPU *cpu = S390_CPU(obj);
@@ -1002,8 +1003,9 @@ static void set_feature(Object *obj, Visitor *v, const char *name,
         return;
     }
 
-    visit_type_bool(v, name, &value, errp);
-    if (*errp) {
+    visit_type_bool(v, name, &value, &err);
+    if (err) {
+        error_propagate(errp, err);
         return;
     }
     if (value) {
@@ -1043,6 +1045,7 @@ static void get_feature_group(Object *obj, Visitor *v, const char *name,
 static void set_feature_group(Object *obj, Visitor *v, const char *name,
                               void *opaque, Error **errp)
 {
+    Error *err = NULL;
     S390FeatGroup group = (S390FeatGroup) opaque;
     const S390FeatGroupDef *def = s390_feat_group_def(group);
     DeviceState *dev = DEVICE(obj);
@@ -1059,8 +1062,9 @@ static void set_feature_group(Object *obj, Visitor *v, const char *name,
         return;
     }
 
-    visit_type_bool(v, name, &value, errp);
-    if (*errp) {
+    visit_type_bool(v, name, &value, &err);
+    if (err) {
+        error_propagate(errp, err);
         return;
     }
     if (value) {
-- 
2.21.0

[PATCH v2 13/18] s390x/cpumodel: Fix realize() error API violations

[Markus Armbruster]

get_max_cpu_model() dereferences @errp when
kvm_s390_get_host_cpu_model() fails, apply_cpu_model() dereferences it
when kvm_s390_apply_cpu_model() fails, and s390_realize_cpu_model()
dereferences it when get_max_cpu_model() or check_compatibility()
fail.  That's wrong; see the big comment in error.h.  All three
introduced in commit 80560137cf "s390x/cpumodel: check and apply the
CPU model".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu_models.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 6a29fd3ab1..c702e34a26 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -870,6 +870,7 @@ static void check_compatibility(const S390CPUModel *max_model,
 
 static S390CPUModel *get_max_cpu_model(Error **errp)
 {
+    Error *err = NULL;
     static S390CPUModel max_model;
     static bool cached;
 
@@ -878,22 +879,24 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
     }
 
     if (kvm_enabled()) {
-        kvm_s390_get_host_cpu_model(&max_model, errp);
+        kvm_s390_get_host_cpu_model(&max_model, &err);
     } else {
         max_model.def = s390_find_cpu_def(QEMU_MAX_CPU_TYPE, QEMU_MAX_CPU_GEN,
                                           QEMU_MAX_CPU_EC_GA, NULL);
         bitmap_copy(max_model.features, qemu_max_cpu_feat, S390_FEAT_MAX);
-   }
-    if (!*errp) {
-        cached = true;
-        return &max_model;
     }
-    return NULL;
+    if (err) {
+        error_propagate(errp, err);
+        return NULL;
+    }
+    cached = true;
+    return &max_model;
 }
 
 static inline void apply_cpu_model(const S390CPUModel *model, Error **errp)
 {
 #ifndef CONFIG_USER_ONLY
+    Error *err = NULL;
     static S390CPUModel applied_model;
     static bool applied;
 
@@ -909,20 +912,23 @@ static inline void apply_cpu_model(const S390CPUModel *model, Error **errp)
     }
 
     if (kvm_enabled()) {
-        kvm_s390_apply_cpu_model(model, errp);
+        kvm_s390_apply_cpu_model(model, &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
     }
 
-    if (!*errp) {
-        applied = true;
-        if (model) {
-            applied_model = *model;
-        }
+    applied = true;
+    if (model) {
+        applied_model = *model;
     }
 #endif
 }
 
 void s390_realize_cpu_model(CPUState *cs, Error **errp)
 {
+    Error *err = NULL;
     S390CPUClass *xcc = S390_CPU_GET_CLASS(cs);
     S390CPU *cpu = S390_CPU(cs);
     const S390CPUModel *max_model;
@@ -939,7 +945,7 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
     }
 
     max_model = get_max_cpu_model(errp);
-    if (*errp) {
+    if (!max_model) {
         error_prepend(errp, "CPU models are not available: ");
         return;
     }
@@ -951,8 +957,9 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
     cpu->model->cpu_ver = max_model->cpu_ver;
 
     check_consistency(cpu->model);
-    check_compatibility(max_model, cpu->model, errp);
-    if (*errp) {
+    check_compatibility(max_model, cpu->model, &err);
+    if (err) {
+        error_propagate(errp, err);
         return;
     }
 
-- 
2.21.0

[PATCH v2 14/18] s390x/cpumodel: Fix query-cpu-model-FOO error API violations

[Markus Armbruster]

cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
dereferences @errp when the visitor or the QOM setter fails.  That's
wrong; see the big comment in error.h.  Introduced in commit
137974cea3 's390x/cpumodel: implement QMP interface
"query-cpu-model-expansion"'.

Its three callers have the same issue.  Introduced in commit
4e82ef0502 's390x/cpumodel: implement QMP interface
"query-cpu-model-comparison"' and commit f1a47d08ef 's390x/cpumodel:
implement QMP interface "query-cpu-model-baseline"'.

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu_models.c | 43 ++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index c702e34a26..3ed301b5e5 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -477,6 +477,7 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
 static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
                                 Error **errp)
 {
+    Error *err = NULL;
     const QDict *qdict = NULL;
     const QDictEntry *e;
     Visitor *visitor;
@@ -513,24 +514,26 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
 
     if (qdict) {
         visitor = qobject_input_visitor_new(info->props);
-        visit_start_struct(visitor, NULL, NULL, 0, errp);
-        if (*errp) {
+        visit_start_struct(visitor, NULL, NULL, 0, &err);
+        if (err) {
+            error_propagate(errp, err);
             visit_free(visitor);
             object_unref(obj);
             return;
         }
         for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
-            object_property_set(obj, visitor, e->key, errp);
-            if (*errp) {
+            object_property_set(obj, visitor, e->key, &err);
+            if (err) {
                 break;
             }
         }
-        if (!*errp) {
+        if (!err) {
             visit_check_struct(visitor, errp);
         }
         visit_end_struct(visitor, NULL);
         visit_free(visitor);
-        if (*errp) {
+        if (err) {
+            error_propagate(errp, err);
             object_unref(obj);
             return;
         }
@@ -595,13 +598,15 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
                                                       CpuModelInfo *model,
                                                       Error **errp)
 {
+    Error *err = NULL;
     CpuModelExpansionInfo *expansion_info = NULL;
     S390CPUModel s390_model;
     bool delta_changes = false;
 
     /* convert it to our internal representation */
-    cpu_model_from_info(&s390_model, model, errp);
-    if (*errp) {
+    cpu_model_from_info(&s390_model, model, &err);
+    if (err) {
+        error_propagate(errp, err);
         return NULL;
     }
 
@@ -634,18 +639,21 @@ CpuModelCompareInfo *qmp_query_cpu_model_comparison(CpuModelInfo *infoa,
                                                      CpuModelInfo *infob,
                                                      Error **errp)
 {
+    Error *err = NULL;
     CpuModelCompareResult feat_result, gen_result;
     CpuModelCompareInfo *compare_info;
     S390FeatBitmap missing, added;
     S390CPUModel modela, modelb;
 
     /* convert both models to our internal representation */
-    cpu_model_from_info(&modela, infoa, errp);
-    if (*errp) {
+    cpu_model_from_info(&modela, infoa, &err);
+    if (err) {
+        error_propagate(errp, err);
         return NULL;
     }
-    cpu_model_from_info(&modelb, infob, errp);
-    if (*errp) {
+    cpu_model_from_info(&modelb, infob, &err);
+    if (err) {
+        error_propagate(errp, err);
         return NULL;
     }
     compare_info = g_new0(CpuModelCompareInfo, 1);
@@ -707,6 +715,7 @@ CpuModelBaselineInfo *qmp_query_cpu_model_baseline(CpuModelInfo *infoa,
                                                     CpuModelInfo *infob,
                                                     Error **errp)
 {
+    Error *err = NULL;
     CpuModelBaselineInfo *baseline_info;
     S390CPUModel modela, modelb, model;
     uint16_t cpu_type;
@@ -714,13 +723,15 @@ CpuModelBaselineInfo *qmp_query_cpu_model_baseline(CpuModelInfo *infoa,
     uint8_t max_gen;
 
     /* convert both models to our internal representation */
-    cpu_model_from_info(&modela, infoa, errp);
-    if (*errp) {
+    cpu_model_from_info(&modela, infoa, &err);
+    if (err) {
+        error_propagate(errp, err);
         return NULL;
     }
 
-    cpu_model_from_info(&modelb, infob, errp);
-    if (*errp) {
+    cpu_model_from_info(&modelb, infob, &err);
+    if (err) {
+        error_propagate(errp, err);
         return NULL;
     }
 
-- 
2.21.0

[PATCH v2 15/18] s390x/cpumodel: Fix query-cpu-definitions error API violations

[Markus Armbruster]

qmp_query_cpu_definitions() passes @errp to get_max_cpu_model(), then
frees any error it gets back.  This effectively ignores errors.
Dereferencing @errp is wrong; see the big comment in error.h.  Passing
@errp is also wrong, because it works only as long as @errp is neither
@error_fatal nor @error_abort.  Introduced in commit 38cba1f4d8
"s390x: return unavailable features via query-cpu-definitions".

No caller actually passes such @errp values.

Fix anyway: simply pass NULL to get_max_cpu_model().

Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu_models.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 3ed301b5e5..547bab8ac3 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -462,11 +462,7 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
         .list = NULL,
     };
 
-    list_data.model = get_max_cpu_model(errp);
-    if (*errp) {
-        error_free(*errp);
-        *errp = NULL;
-    }
+    list_data.model = get_max_cpu_model(NULL);
 
     object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false,
                          &list_data);
-- 
2.21.0

[PATCH v2 16/18] error: Clean up unusual names of Error * variables

[Markus Armbruster]

Local Error * variables are conventionally named @err or @local_err,
and Error ** parameters @errp.  Naming local variables like parameters
is confusing.  Clean that up.

Naming parameters like local variables is also confusing.  Left for
another day.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/intc/s390_flic_kvm.c    | 10 +++++-----
 hw/ppc/spapr_pci.c         | 16 ++++++++--------
 hw/ppc/spapr_pci_nvlink2.c | 10 +++++-----
 tests/test-blockjob.c      | 16 ++++++++--------
 4 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index c9ee80eaae..30d50c2369 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -582,10 +582,10 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
     struct kvm_create_device cd = {0};
     struct kvm_device_attr test_attr = {0};
     int ret;
-    Error *errp_local = NULL;
+    Error *err = NULL;
 
-    KVM_S390_FLIC_GET_CLASS(dev)->parent_realize(dev, &errp_local);
-    if (errp_local) {
+    KVM_S390_FLIC_GET_CLASS(dev)->parent_realize(dev, &err);
+    if (err) {
         goto fail;
     }
     flic_state->fd = -1;
@@ -593,7 +593,7 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
     cd.type = KVM_DEV_TYPE_FLIC;
     ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd);
     if (ret < 0) {
-        error_setg_errno(&errp_local, errno, "Creating the KVM device failed");
+        error_setg_errno(&err, errno, "Creating the KVM device failed");
         trace_flic_create_device(errno);
         goto fail;
     }
@@ -605,7 +605,7 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
                                             KVM_HAS_DEVICE_ATTR, test_attr);
     return;
 fail:
-    error_propagate(errp, errp_local);
+    error_propagate(errp, err);
 }
 
 static void kvm_s390_flic_reset(DeviceState *dev)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index f6fbcf99ed..723373de73 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2042,13 +2042,13 @@ void spapr_phb_dma_reset(SpaprPhbState *sphb)
 static void spapr_phb_reset(DeviceState *qdev)
 {
     SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(qdev);
-    Error *errp = NULL;
+    Error *err = NULL;
 
     spapr_phb_dma_reset(sphb);
     spapr_phb_nvgpu_free(sphb);
-    spapr_phb_nvgpu_setup(sphb, &errp);
-    if (errp) {
-        error_report_err(errp);
+    spapr_phb_nvgpu_setup(sphb, &err);
+    if (err) {
+        error_report_err(err);
     }
 
     /* Reset the IOMMU state */
@@ -2326,7 +2326,7 @@ int spapr_dt_phb(SpaprMachineState *spapr, SpaprPhbState *phb,
                                 cpu_to_be32(phb->numa_node)};
     SpaprTceTable *tcet;
     SpaprDrc *drc;
-    Error *errp = NULL;
+    Error *err = NULL;
 
     /* Start populating the FDT */
     _FDT(bus_off = fdt_add_subnode(fdt, 0, phb->dtbusname));
@@ -2408,9 +2408,9 @@ int spapr_dt_phb(SpaprMachineState *spapr, SpaprPhbState *phb,
         return ret;
     }
 
-    spapr_phb_nvgpu_populate_dt(phb, fdt, bus_off, &errp);
-    if (errp) {
-        error_report_err(errp);
+    spapr_phb_nvgpu_populate_dt(phb, fdt, bus_off, &err);
+    if (err) {
+        error_report_err(err);
     }
     spapr_phb_nvgpu_ram_populate_dt(phb, fdt);
 
diff --git a/hw/ppc/spapr_pci_nvlink2.c b/hw/ppc/spapr_pci_nvlink2.c
index 4aa89ede23..8332d5694e 100644
--- a/hw/ppc/spapr_pci_nvlink2.c
+++ b/hw/ppc/spapr_pci_nvlink2.c
@@ -57,7 +57,7 @@ struct SpaprPhbPciNvGpuConfig {
     uint64_t nv2_atsd_current;
     int num; /* number of non empty (i.e. tgt!=0) entries in slots[] */
     SpaprPhbPciNvGpuSlot slots[NVGPU_MAX_NUM];
-    Error *errp;
+    Error *err;
 };
 
 static SpaprPhbPciNvGpuSlot *
@@ -153,7 +153,7 @@ static void spapr_phb_pci_collect_nvgpu(PCIBus *bus, PCIDevice *pdev,
             spapr_pci_collect_nvnpu(nvgpus, pdev, tgt, MEMORY_REGION(mr_npu),
                                     &local_err);
         }
-        error_propagate(&nvgpus->errp, local_err);
+        error_propagate(&nvgpus->err, local_err);
     }
     if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
          PCI_HEADER_TYPE_BRIDGE)) {
@@ -187,9 +187,9 @@ void spapr_phb_nvgpu_setup(SpaprPhbState *sphb, Error **errp)
     pci_for_each_device(bus, pci_bus_num(bus),
                         spapr_phb_pci_collect_nvgpu, sphb->nvgpus);
 
-    if (sphb->nvgpus->errp) {
-        error_propagate(errp, sphb->nvgpus->errp);
-        sphb->nvgpus->errp = NULL;
+    if (sphb->nvgpus->err) {
+        error_propagate(errp, sphb->nvgpus->err);
+        sphb->nvgpus->err = NULL;
         goto cleanup_exit;
     }
 
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 7844c9ffcb..e670a20617 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -34,13 +34,13 @@ static BlockJob *mk_job(BlockBackend *blk, const char *id,
                         int flags)
 {
     BlockJob *job;
-    Error *errp = NULL;
+    Error *err = NULL;
 
     job = block_job_create(id, drv, NULL, blk_bs(blk),
                            0, BLK_PERM_ALL, 0, flags, block_job_cb,
-                           NULL, &errp);
+                           NULL, &err);
     if (should_succeed) {
-        g_assert_null(errp);
+        g_assert_null(err);
         g_assert_nonnull(job);
         if (id) {
             g_assert_cmpstr(job->job.id, ==, id);
@@ -48,9 +48,9 @@ static BlockJob *mk_job(BlockBackend *blk, const char *id,
             g_assert_cmpstr(job->job.id, ==, blk_name(blk));
         }
     } else {
-        g_assert_nonnull(errp);
+        g_assert_nonnull(err);
         g_assert_null(job);
-        error_free(errp);
+        error_free(err);
     }
 
     return job;
@@ -80,9 +80,9 @@ static BlockBackend *create_blk(const char *name)
     bdrv_unref(bs);
 
     if (name) {
-        Error *errp = NULL;
-        monitor_add_blk(blk, name, &errp);
-        g_assert_null(errp);
+        Error *err = NULL;
+        monitor_add_blk(blk, name, &err);
+        g_assert_null(err);
     }
 
     return blk;
-- 
2.21.0

[PATCH v2 17/18] hw/intc/s390: Simplify error handling in kvm_s390_flic_realize()

[Markus Armbruster]

Cc: Halil Pasic <pasic@linux.ibm.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Acked-by: Halil Pasic <pasic@linux.ibm.com>
---
 hw/intc/s390_flic_kvm.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index 30d50c2369..dddd33ea61 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -586,16 +586,17 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
 
     KVM_S390_FLIC_GET_CLASS(dev)->parent_realize(dev, &err);
     if (err) {
-        goto fail;
+        error_propagate(errp, err);
+        return;
     }
     flic_state->fd = -1;
 
     cd.type = KVM_DEV_TYPE_FLIC;
     ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd);
     if (ret < 0) {
-        error_setg_errno(&err, errno, "Creating the KVM device failed");
+        error_setg_errno(errp, errno, "Creating the KVM device failed");
         trace_flic_create_device(errno);
-        goto fail;
+        return;
     }
     flic_state->fd = cd.fd;
 
@@ -603,9 +604,6 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
     test_attr.group = KVM_DEV_FLIC_CLEAR_IO_IRQ;
     flic_state->clear_io_supported = !ioctl(flic_state->fd,
                                             KVM_HAS_DEVICE_ATTR, test_attr);
-    return;
-fail:
-    error_propagate(errp, err);
 }
 
 static void kvm_s390_flic_reset(DeviceState *dev)
-- 
2.21.0

[PATCH v2 18/18] tests-blockjob: Use error_free_or_abort()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/test-blockjob.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index e670a20617..4eeb184caf 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -48,9 +48,8 @@ static BlockJob *mk_job(BlockBackend *blk, const char *id,
             g_assert_cmpstr(job->job.id, ==, blk_name(blk));
         }
     } else {
-        g_assert_nonnull(err);
+        error_free_or_abort(&err);
         g_assert_null(job);
-        error_free(err);
     }
 
     return job;
-- 
2.21.0

Re: [PATCH v2 00/18] Error handling fixes

[David Hildenbrand]

On 04.12.19 10:36, Markus Armbruster wrote:
> v2:
> * Old PATCH 01-03 have been merged for 4.2
> * PATCH 05-15: Commit message rephrased [David], R-bys kept

Thanks!


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 01/18] crypto: Fix certificate file error handling crash bug

[Daniel P. Berrangé]

On Wed, Dec 04, 2019 at 10:36:08AM +0100, Markus Armbruster wrote:
> qcrypto_tls_creds_load_cert() passes uninitialized GError *gerr by
> reference to g_file_get_contents().  When g_file_get_contents() fails,
> it'll try to set a GError.  Unless @gerr is null by dumb luck, this
> logs a ERROR_OVERWRITTEN_WARNING warning message and leaves @gerr
> unchanged.  qcrypto_tls_creds_load_cert() then dereferences the
> uninitialized @gerr.
> 
> Fix by initializing @gerr properly.
> 
> Fixes: 9a2fd4347c40321f5cbb4ab4220e759fcbf87d03
> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  crypto/tlscredsx509.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 02/18] crypto: Fix typo in QCryptoTLSSession's <example> comment

[Daniel P. Berrangé]

On Wed, Dec 04, 2019 at 10:36:09AM +0100, Markus Armbruster wrote:
> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/crypto/tlssession.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 03/18] io: Fix Error usage in a comment <example>

[Daniel P. Berrangé]

On Wed, Dec 04, 2019 at 10:36:10AM +0100, Markus Armbruster wrote:
> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/io/task.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 00/18] Error handling fixes

[Markus Armbruster]

Neglected to mention: I'm fine with taking care of getting the whole
series merged.  Maintainers, feel free to take "your" parts through your
tree regardless.  Whatever is left after a while I'll take through mine.

Re: [PATCH v2 04/18] tests: Clean up initialization of Error *err variables

[Eric Blake]

On 12/4/19 3:36 AM, Markus Armbruster wrote:
> Declaring a local Error *err without initializer looks suspicious.
> Fuse the declaration with the initialization to avoid that.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/test-qobject-output-visitor.c | 8 ++++----
>   tests/test-string-output-visitor.c  | 4 ++--
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 05/18] exec: Fix file_ram_alloc() error API violations

[Philippe Mathieu-Daudé]

On 12/4/19 10:36 AM, Markus Armbruster wrote:
> When os_mem_prealloc() fails, file_ram_alloc() calls qemu_ram_munmap()
> and returns null.  Except it doesn't when its @errp argument is null,
> because it checks for failure with (errp && *errp).  Introduced in
> commit 056b68af77 "fix qemu exit on memory hotplug when allocation
> fails at prealloc time".
> 
> No caller actually passes null.
> 
> Fix anyway: splice in a local Error *err, and error_propagate().
> 
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   exec.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index ffdb518535..45695a5f2d 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1841,6 +1841,7 @@ static void *file_ram_alloc(RAMBlock *block,
>                               bool truncate,
>                               Error **errp)
>   {
> +    Error *err = NULL;
>       MachineState *ms = MACHINE(qdev_get_machine());
>       void *area;
>   
> @@ -1898,8 +1899,9 @@ static void *file_ram_alloc(RAMBlock *block,
>       }
>   
>       if (mem_prealloc) {
> -        os_mem_prealloc(fd, area, memory, ms->smp.cpus, errp);
> -        if (errp && *errp) {
> +        os_mem_prealloc(fd, area, memory, ms->smp.cpus, &err);
> +        if (err) {
> +            error_propagate(errp, err);
>               qemu_ram_munmap(fd, area, memory);
>               return NULL;
>           }
> 

Re: [PATCH v2 04/18] tests: Clean up initialization of Error *err variables

[Philippe Mathieu-Daudé]

On 12/4/19 10:36 AM, Markus Armbruster wrote:
> Declaring a local Error *err without initializer looks suspicious.
> Fuse the declaration with the initialization to avoid that.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/test-qobject-output-visitor.c | 8 ++++----
>   tests/test-string-output-visitor.c  | 4 ++--
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c
> index 3e993e5ba8..d7761ebf84 100644
> --- a/tests/test-qobject-output-visitor.c
> +++ b/tests/test-qobject-output-visitor.c
> @@ -145,10 +145,10 @@ static void test_visitor_out_enum_errors(TestOutputVisitorData *data,
>                                            const void *unused)
>   {
>       EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
> -    Error *err;
>   
>       for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
> -        err = NULL;
> +        Error *err = NULL;
> +

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>           visit_type_EnumOne(data->ov, "unused", &bad_values[i], &err);
>           error_free_or_abort(&err);
>           visitor_reset(data);
> @@ -240,11 +240,11 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
>       EnumOne bad_values[] = { ENUM_ONE__MAX, -1 };
>       UserDefOne u = {0};
>       UserDefOne *pu = &u;
> -    Error *err;
>       int i;
>   
>       for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
> -        err = NULL;
> +        Error *err = NULL;
> +
>           u.has_enum1 = true;
>           u.enum1 = bad_values[i];
>           visit_type_UserDefOne(data->ov, "unused", &pu, &err);
> diff --git a/tests/test-string-output-visitor.c b/tests/test-string-output-visitor.c
> index 02766c0f65..1be1540767 100644
> --- a/tests/test-string-output-visitor.c
> +++ b/tests/test-string-output-visitor.c
> @@ -207,10 +207,10 @@ static void test_visitor_out_enum_errors(TestOutputVisitorData *data,
>                                            const void *unused)
>   {
>       EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
> -    Error *err;
>   
>       for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
> -        err = NULL;
> +        Error *err = NULL;
> +
>           visit_type_EnumOne(data->ov, "unused", &bad_values[i], &err);
>           error_free_or_abort(&err);
>       }
> 

Re: [PATCH v2 07/18] hw/core: Fix fit_load_fdt() error handling violations

[Philippe Mathieu-Daudé]

On 12/4/19 10:36 AM, Markus Armbruster wrote:
> fit_load_fdt() passes @errp to fit_image_addr(), then recovers from
> ENOENT failures.  Passing @errp is wrong, because it works only as
> long as @errp is neither @error_fatal nor @error_abort.  Messed up in
> commit 3eb99edb48 "loader-fit: Wean off error_printf()".
> 
> No caller actually passes such values.
> 
> Fix anyway: splice in a local Error *err, and error_propagate().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/core/loader-fit.c | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
> index 953b16bc82..c465921b8f 100644
> --- a/hw/core/loader-fit.c
> +++ b/hw/core/loader-fit.c
> @@ -178,11 +178,12 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>                           int cfg, void *opaque, const void *match_data,
>                           hwaddr kernel_end, Error **errp)
>   {
> +    Error *err = NULL;
>       const char *name;
>       const void *data;
>       const void *load_data;
>       hwaddr load_addr;
> -    int img_off, err;
> +    int img_off;
>       size_t sz;
>       int ret;
>   
> @@ -197,13 +198,13 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>           return -EINVAL;
>       }
>   
> -    err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
> -    if (err == -ENOENT) {
> +    ret = fit_image_addr(itb, img_off, "load", &load_addr, &err);
> +    if (ret == -ENOENT) {
>           load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
> -        error_free(*errp);
> -    } else if (err) {
> -        error_prepend(errp, "unable to read FDT load address from FIT: ");
> -        ret = err;
> +        error_free(err);
> +    } else if (ret) {
> +        error_propagate_prepend(errp, err,
> +                                "unable to read FDT load address from FIT: ");
>           goto out;
>       }
>   

Cleaner.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 09/18] qga: Fix guest-get-fsinfo error API violations

[Philippe Mathieu-Daudé]

On 12/4/19 10:36 AM, Markus Armbruster wrote:
> build_guest_fsinfo_for_virtual_device() dereferences @errp when
> build_guest_fsinfo_for_device() fails.  That's wrong; see the big
> comment in error.h.  Introduced in commit 46d4c5723e "qga: Add
> guest-get-fsinfo command".
> 
> No caller actually passes null.
> 
> Fix anyway: splice in a local Error *err, and error_propagate().
> 
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qga/commands-posix.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 1c1a165dae..0be527ccb8 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1049,6 +1049,7 @@ static void build_guest_fsinfo_for_virtual_device(char const *syspath,
>                                                     GuestFilesystemInfo *fs,
>                                                     Error **errp)
>   {
> +    Error *err = NULL;
>       DIR *dir;
>       char *dirpath;
>       struct dirent *entry;
> @@ -1078,10 +1079,11 @@ static void build_guest_fsinfo_for_virtual_device(char const *syspath,
>   
>               g_debug(" slave device '%s'", entry->d_name);
>               path = g_strdup_printf("%s/slaves/%s", syspath, entry->d_name);
> -            build_guest_fsinfo_for_device(path, fs, errp);
> +            build_guest_fsinfo_for_device(path, fs, &err);
>               g_free(path);
>   
> -            if (*errp) {
> +            if (err) {
> +                error_propagate(errp, err);
>                   break;
>               }
>           }
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 00/18] Error handling fixes

[Cornelia Huck]

On Wed,  4 Dec 2019 10:36:07 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> v2:
> * Old PATCH 01-03 have been merged for 4.2
> * PATCH 05-15: Commit message rephrased [David], R-bys kept

Thanks, queued patches 11-15 to s390-next.

Patch 17 depends on patch 16, so I'll leave merging that one to you.

> 
> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Corey Minyard <cminyard@mvista.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> Markus Armbruster (18):
>   crypto: Fix certificate file error handling crash bug
>   crypto: Fix typo in QCryptoTLSSession's <example> comment
>   io: Fix Error usage in a comment <example>
>   tests: Clean up initialization of Error *err variables
>   exec: Fix file_ram_alloc() error API violations
>   hw/acpi: Fix legacy CPU plug error API violations
>   hw/core: Fix fit_load_fdt() error handling violations
>   hw/ipmi: Fix realize() error API violations
>   qga: Fix guest-get-fsinfo error API violations
>   memory-device: Fix memory pre-plug error API violations
>   s390x/event-facility: Fix realize() error API violations
>   s390x/cpumodel: Fix feature property error API violations
>   s390x/cpumodel: Fix realize() error API violations
>   s390x/cpumodel: Fix query-cpu-model-FOO error API violations
>   s390x/cpumodel: Fix query-cpu-definitions error API violations
>   error: Clean up unusual names of Error * variables
>   hw/intc/s390: Simplify error handling in kvm_s390_flic_realize()
>   tests-blockjob: Use error_free_or_abort()
> 
>  include/crypto/tlssession.h         |  2 +-
>  include/io/task.h                   |  2 +-
>  crypto/tlscredsx509.c               |  2 +-
>  exec.c                              |  6 +-
>  hw/acpi/cpu_hotplug.c               | 10 +--
>  hw/core/loader-fit.c                | 15 ++---
>  hw/intc/s390_flic_kvm.c             | 16 +++--
>  hw/ipmi/isa_ipmi_bt.c               |  7 ++-
>  hw/ipmi/isa_ipmi_kcs.c              |  7 ++-
>  hw/ipmi/pci_ipmi_bt.c               |  6 +-
>  hw/ipmi/pci_ipmi_kcs.c              |  6 +-
>  hw/mem/memory-device.c              |  6 +-
>  hw/ppc/spapr_pci.c                  | 16 ++---
>  hw/ppc/spapr_pci_nvlink2.c          | 10 +--
>  hw/s390x/event-facility.c           |  6 +-
>  qga/commands-posix.c                |  6 +-
>  target/s390x/cpu_models.c           | 98 +++++++++++++++++------------
>  tests/test-blockjob.c               | 15 +++--
>  tests/test-qobject-output-visitor.c |  8 +--
>  tests/test-string-output-visitor.c  |  4 +-
>  20 files changed, 139 insertions(+), 109 deletions(-)
> 

Re: [PATCH v2 06/18] hw/acpi: Fix legacy CPU plug error API violations

[Michael S. Tsirkin]

On Wed, Dec 04, 2019 at 10:36:13AM +0100, Markus Armbruster wrote:
> legacy_acpi_cpu_plug_cb() dereferences @errp when
> acpi_set_cpu_present_bit() fails.  That's wrong; see the big comment
> in error.h.  Introduced in commit cc43364de7 "acpi/cpu-hotplug:
> introduce helper function to keep bit setting in one place".
> 
> No caller actually passes null, and acpi_set_cpu_present_bit() can't
> actually fail.
> 
> Fix anyway: drop acpi_set_cpu_present_bit()'s @errp parameter.
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Feel free to merge through your tree pls.

> ---
>  hw/acpi/cpu_hotplug.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> index 3ac2045a95..9c3bcc84de 100644
> --- a/hw/acpi/cpu_hotplug.c
> +++ b/hw/acpi/cpu_hotplug.c
> @@ -55,8 +55,7 @@ static const MemoryRegionOps AcpiCpuHotplug_ops = {
>      },
>  };
>  
> -static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu,
> -                                     Error **errp)
> +static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu)
>  {
>      CPUClass *k = CPU_GET_CLASS(cpu);
>      int64_t cpu_id;
> @@ -74,10 +73,7 @@ static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu,
>  void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
>                               AcpiCpuHotplug *g, DeviceState *dev, Error **errp)
>  {
> -    acpi_set_cpu_present_bit(g, CPU(dev), errp);
> -    if (*errp != NULL) {
> -        return;
> -    }
> +    acpi_set_cpu_present_bit(g, CPU(dev));
>      acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
>  }
>  
> @@ -92,7 +88,7 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
>      gpe_cpu->device = owner;
>  
>      CPU_FOREACH(cpu) {
> -        acpi_set_cpu_present_bit(gpe_cpu, cpu, &error_abort);
> +        acpi_set_cpu_present_bit(gpe_cpu, cpu);
>      }
>  }
>  
> -- 
> 2.21.0