[PATCH 00/21] Error handling fixes, may contain 4.2 material

[PATCH 00/21] Error handling fixes, may contain 4.2 material

[Markus Armbruster]

PATCH 2-4 fix crash bugs.  Including them would be a no-brainer at
-rc0.  But we're post -rc3, and even for crash bugs we require a
certain likelihood of users getting bitten.

Jens, please assess impact of PATCH 2's crash bug.

Kevin, please do the same for PATCH 3.

Daniel, please do the same for PATCH 4.

The remainder is definitely not 4.2 material.

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: Jens Freimann <jfreimann@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Nishanth Aravamudan <naravamudan@digitalocean.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>

Markus Armbruster (21):
  net/virtio: Drop useless n->primary_dev not null checks
  net/virtio: Fix failover error handling crash bugs
  block/file-posix: Fix laio_init() error handling crash bug
  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 latent file_ram_alloc() error handling bug
  hw/acpi: Fix latent legacy CPU plug error handling bug
  hw/core: Fix latent fit_load_fdt() error handling bug
  hw/ipmi: Fix latent realize() error handling bugs
  qga: Fix latent guest-get-fsinfo error handling bug
  memory-device: Fix latent memory pre-plug error handling bugs
  s390x/event-facility: Fix latent realize() error handling bug
  s390x/cpu_models: Fix latent feature property error handling bugs
  s390/cpu_modules: Fix latent realize() error handling bugs
  s390x: Fix latent query-cpu-model-FOO error handling bugs
  s390x: Fix latent query-cpu-definitions error handling bug
  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 +-
 block/file-posix.c                  |  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/net/virtio-net.c                 | 27 ++++----
 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 +-
 22 files changed, 154 insertions(+), 123 deletions(-)

-- 
2.21.0

[PATCH 01/21] net/virtio: Drop useless n->primary_dev not null checks

[Markus Armbruster]

virtio_net_handle_migration_primary() returns early when it can't
ensure n->primary_dev is non-null.  Checking it again right after that
early return is redundant.  Drop.

If n->primary_dev is null on entering failover_replug_primary(), @pdev
will become null, and pdev->partially_hotplugged will crash.  Checking
n->primary_dev later is useless.  It can't actually be null, because
its caller virtio_net_handle_migration_primary() ensures it isn't.
Drop the useless check.

Cc: Jens Freimann <jfreimann@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/net/virtio-net.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3c31471026..87088ba374 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2810,11 +2810,6 @@ static bool failover_replug_primary(VirtIONet *n, Error **errp)
             goto out;
         }
     }
-    if (!n->primary_dev) {
-            error_setg(errp, "virtio_net: couldn't find primary device");
-            goto out;
-    }
-
     n->primary_bus = n->primary_dev->parent_bus;
     if (!n->primary_bus) {
         error_setg(errp, "virtio_net: couldn't find primary bus");
@@ -2849,8 +2844,7 @@ static void virtio_net_handle_migration_primary(VirtIONet *n,
         }
     }
 
-    if (migration_in_setup(s) && !should_be_hidden &&
-        n->primary_dev) {
+    if (migration_in_setup(s) && !should_be_hidden) {
         if (failover_unplug_primary(n)) {
             vmstate_unregister(n->primary_dev, qdev_get_vmsd(n->primary_dev),
                     n->primary_dev);
-- 
2.21.0

[PATCH 02/21] net/virtio: Fix failover error handling crash bugs

[Markus Armbruster]

Functions that take an Error ** parameter to pass an error to the
caller expect the parameter to point to null.
failover_replug_primary() violates this precondition in several
places:

* After qemu_opts_from_qdict() failed, *errp is no longer null.
  Passing it to error_setg() is wrong, and will trip the assertion in
  error_setv().  Messed up in commit 150ab54aa6 "net/virtio: fix
  re-plugging of primary device".  Simply drop the error_setg().

* Passing @errp to qemu_opt_set_bool(), hotplug_handler_pre_plug(),
  and hotplug_handler_plug() is wrong.  If one of the first two fails,
  *errp is no longer null.  Risks tripping the same assertion.
  Moreover, continuing after such errors is unsafe.  Messed up in
  commit 9711cd0dfc "net/virtio: add failover support".  Fix by
  handling each error properly.

failover_replug_primary() crashes when passed a null @errp.  Also
messed up in commit 9711cd0dfc.  This bug can't bite as no caller
actually passes null.  Fix it anyway.

Fixes: 9711cd0dfc3fa414f7f64935713c07134ae67971
Fixes: 150ab54aa6934583180f88a2bd540bc6fc4fbff3
Cc: Jens Freimann <jfreimann@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/net/virtio-net.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 87088ba374..db3d7c38e6 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2795,6 +2795,7 @@ static bool failover_unplug_primary(VirtIONet *n)
 
 static bool failover_replug_primary(VirtIONet *n, Error **errp)
 {
+    Error *err = NULL;
     HotplugHandler *hotplug_ctrl;
     PCIDevice *pdev = PCI_DEVICE(n->primary_dev);
 
@@ -2806,27 +2807,33 @@ static bool failover_replug_primary(VirtIONet *n, Error **errp)
                 qemu_find_opts("device"),
                 n->primary_device_dict, errp);
         if (!n->primary_device_opts) {
-            error_setg(errp, "virtio_net: couldn't find primary device opts");
-            goto out;
+            return false;
         }
     }
     n->primary_bus = n->primary_dev->parent_bus;
     if (!n->primary_bus) {
         error_setg(errp, "virtio_net: couldn't find primary bus");
-        goto out;
+        return false;
     }
     qdev_set_parent_bus(n->primary_dev, n->primary_bus);
     n->primary_should_be_hidden = false;
     qemu_opt_set_bool(n->primary_device_opts,
-                      "partially_hotplugged", true, errp);
+                      "partially_hotplugged", true, &err);
+    if (err) {
+        goto out;
+    }
     hotplug_ctrl = qdev_get_hotplug_handler(n->primary_dev);
     if (hotplug_ctrl) {
-        hotplug_handler_pre_plug(hotplug_ctrl, n->primary_dev, errp);
+        hotplug_handler_pre_plug(hotplug_ctrl, n->primary_dev, &err);
+        if (err) {
+            goto out;
+        }
         hotplug_handler_plug(hotplug_ctrl, n->primary_dev, errp);
     }
 
 out:
-    return *errp == NULL;
+    error_propagate(errp, err);
+    return !err;
 }
 
 static void virtio_net_handle_migration_primary(VirtIONet *n,
-- 
2.21.0

[PATCH 03/21] block/file-posix: Fix laio_init() error handling crash bug

[Markus Armbruster]

raw_aio_attach_aio_context() passes uninitialized Error *local_err by
reference to laio_init() via aio_setup_linux_aio().  When laio_init()
fails, it passes it on to error_setg_errno(), tripping error_setv()'s
assertion unless @local_err is null by dumb luck.

Fix by initializing @local_err properly.

Fixes: ed6e2161715c527330f936d44af4c547f25f687e
Cc: Nishanth Aravamudan <naravamudan@digitalocean.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/file-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 1f0f61a02b..1b805bd938 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1973,7 +1973,7 @@ static void raw_aio_attach_aio_context(BlockDriverState *bs,
 #ifdef CONFIG_LINUX_AIO
     BDRVRawState *s = bs->opaque;
     if (s->use_linux_aio) {
-        Error *local_err;
+        Error *local_err = NULL;
         if (!aio_setup_linux_aio(new_context, &local_err)) {
             error_reportf_err(local_err, "Unable to use native AIO, "
                                          "falling back to thread pool: ");
-- 
2.21.0

[PATCH 04/21] 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 05/21] 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 06/21] 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 07/21] 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 08/21] exec: Fix latent file_ram_alloc() error handling bug

[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).  Messed up in
commit 056b68af77 "fix qemu exit on memory hotplug when allocation
fails at prealloc time".

The bug can't bite as no caller actually passes null.  Fix it anyway.

Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Markus Armbruster <armbru@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 09/21] hw/acpi: Fix latent legacy CPU plug error handling bug

[Markus Armbruster]

legacy_acpi_cpu_plug_cb() crashes when acpi_set_cpu_present_bit()
fails and its @errp argument is null.  Messed up in commit cc43364de7
"acpi/cpu-hotplug: introduce helper function to keep bit setting in
one place".

The bug can't bite as no caller actually passes null, and
acpi_set_cpu_present_bit() can't actually fail.  Fix it anyway.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Markus Armbruster <armbru@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 10/21] hw/core: Fix latent fit_load_fdt() error handling bug

[Markus Armbruster]

fit_load_fdt() recovers from fit_image_addr() failing with ENOENT.
Except it doesn't when its @errp argument is &error_fatal or
&error_abort, because it blindly passes @errp to fit_image_addr().
Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()".

The bug can't bite as no caller actually passes &error_fatal or
&error_abort.  Fix it anyway.

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 11/21] hw/ipmi: Fix latent realize() error handling bugs

[Markus Armbruster]

isa_ipmi_bt_realize(), ipmi_isa_realize(), pci_ipmi_bt_realize(), and
pci_ipmi_kcs_realize() crash when IPMIInterfaceClass method init()
fails and their @errp argument is null.  First messed up 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".

The bug can't bite as no caller actually passes null, and none of the
init() methods can actually fail.  Fix it anyway.

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 12/21] qga: Fix latent guest-get-fsinfo error handling bug

[Markus Armbruster]

build_guest_fsinfo_for_virtual_device() crashes when
build_guest_fsinfo_for_device() fails and its @errp argument is null.
Messed up in commit 46d4c5723e "qga: Add guest-get-fsinfo command".

The bug can't bite as no caller actually passes null.  Fix it anyway.

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 13/21] memory-device: Fix latent memory pre-plug error handling bugs

[Markus Armbruster]

memory_device_get_free_addr() crashes when
memory_device_check_addable() fails and its @errp argument is null.
Messed up in commit 1b6d6af21b "pc-dimm: factor out capacity and slot
checks into MemoryDevice".

The bug can't bite as no caller actually passes null.  Fix it anyway.

Cc: David Hildenbrand <david@redhat.com>
Signed-off-by: Markus Armbruster <armbru@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 14/21] s390x/event-facility: Fix latent realize() error handling bug

[Markus Armbruster]

sclp_events_bus_realize() crashes when object_property_set_bool()
fails and its @errp argument is null.  Messed up in commit f6102c329c
"s390/sclp: rework sclp event facility initialization + device
realization".

The bug can't bite as no caller actually passes null.  Fix it anyway.

Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Markus Armbruster <armbru@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 15/21] s390x/cpu_models: Fix latent feature property error handling bugs

[Markus Armbruster]

s390x-cpu property setters set_feature() and set_feature_group() crash
when the visitor fails and its @errp argument is null.  Messed up in
commit 0754f60429 "s390x/cpumodel: expose features and feature groups
as properties".

The bug can't bite as no caller actually passes null.  Fix it anyway.

Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Markus Armbruster <armbru@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 16/21] s390/cpu_modules: Fix latent realize() error handling bugs

[Markus Armbruster]

get_max_cpu_model() crashes when kvm_s390_get_host_cpu_model() fails
and its @errp argument is null.

apply_cpu_model() crashes when kvm_s390_apply_cpu_model() fails and
its @errp argument is null.

s390_realize_cpu_model() crashes when get_max_cpu_model() or
check_compatibility() fail, and its @errp argument is null.

All three messed up in commit 80560137cf "s390x/cpumodel: check and
apply the CPU model".

The bugs can't bite as no caller actually passes null.  Fix them
anyway.

Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Markus Armbruster <armbru@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 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[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
crashes when the visitor or the QOM setter fails, and its @errp
argument is null.  Messed up in commit 137974cea3 's390x/cpumodel:
implement QMP interface "query-cpu-model-expansion"'.

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

The bugs can't bite as no caller actually passes null.  Fix them
anyway.

Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Markus Armbruster <armbru@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 18/21] s390x: Fix latent query-cpu-definitions error handling bug

[Markus Armbruster]

qmp_query_cpu_definitions() tries to ignore get_max_cpu_model()'s
errors.  However, it crashes when its @errp argument is null or
&error_abort, and exit(1)s when it's &error_fatal.  Messed up in
commit 38cba1f4d8 "s390x: return unavailable features via
query-cpu-definitions".

The bug can't bite as no caller actually passes such @errp values.
Fix it anyway.

Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Markus Armbruster <armbru@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 19/21] 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>
---
 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 20/21] 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>
---
 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 21/21] tests-blockjob: Use error_free_or_abort()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@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 19/21] error: Clean up unusual names of Error * variables

[Philippe Mathieu-Daudé]

On 11/30/19 8:42 PM, Markus Armbruster wrote:
> 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>
> ---
>   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(-)

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

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[David Hildenbrand]

> Am 30.11.2019 um 20:42 schrieb Markus Armbruster <armbru@redhat.com>:
> 
> cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
> qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
> crashes when the visitor or the QOM setter fails, and its @errp
> argument is null.  Messed up in commit 137974cea3 's390x/cpumodel:
> implement QMP interface "query-cpu-model-expansion"'.
> 
> Its three callers have the same bug.  Messed up in commit 4e82ef0502
> 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
> and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
> "query-cpu-model-baseline"'.
> 
> The bugs can't bite as no caller actually passes null.  Fix them
> anyway.

https://en.m.wikipedia.org/wiki/Software_bug

  „ A software bug is an error, flaw or fault in a computer program or system that causes it to produce an incorrect or unexpected result, or to behave in unintended ways. „

Please make it clear in the descriptions that these are cleanups and not bugfixes. It might be very confusing for people looking out for real bugs. Also, please change the terminology „messed up“ to „introduced in“ or similar.

(applies to all s390x patches)

Thanks.

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[Aleksandar Markovic]

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

On Saturday, November 30, 2019, David Hildenbrand <dhildenb@redhat.com>
wrote:

>
>
> > Am 30.11.2019 um 20:42 schrieb Markus Armbruster <armbru@redhat.com>:
> >
> > cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
> > qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
> > crashes when the visitor or the QOM setter fails, and its @errp
> > argument is null.  Messed up in commit 137974cea3 's390x/cpumodel:
> > implement QMP interface "query-cpu-model-expansion"'.
> >
> > Its three callers have the same bug.  Messed up in commit 4e82ef0502
> > 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
> > and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
> > "query-cpu-model-baseline"'.
> >
> > The bugs can't bite as no caller actually passes null.  Fix them
> > anyway.
>
> https://en.m.wikipedia.org/wiki/Software_bug
>
>   „ A software bug is an error, flaw or fault in a computer program or
> system that causes it to produce an incorrect or unexpected result, or to
> behave in unintended ways. „
>
> Please make it clear in the descriptions that these are cleanups and not
> bugfixes. It might be very confusing for people looking out for real bugs.


>
Disclaimer: I am not entirely familiar with the code in question, so take
my opinion with reasonablereservation.

It looks that we here deal with latent bugs. As you probably know from
experience, a latent bugs, when they are activated with some ostensibly
unrelated code change, can be much more difficult to diagnose and fix than
regular bugs.

In that light, this change is not a clean up. It is a fix of a latent bugs,
and Markus' aproach to treat it as a bug fix looks right to me. I would
just add a word "latent" or similar, which would even more distance the
patch from "cleanup" meaning.

David, if I understand well, this patch fixes the commit done by you. I
definitely understand this is not a pleasant position, but we all
(definitelly including myself too) should learn to handle such situations
as gracefully as we can.

Yours,
Aleksandar



>
>
>
> Also, please change the terminology „messed up“ to „introduced in“ or
> similar.
>
> (applies to all s390x patches)
>
> Thanks.

[-- Attachment #2: Type: text/html, Size: 3499 bytes --]

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[Aleksandar Markovic]

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

On Sunday, December 1, 2019, Aleksandar Markovic <
aleksandar.m.mail@gmail.com> wrote:

>
>
> On Saturday, November 30, 2019, David Hildenbrand <dhildenb@redhat.com>
> wrote:
>
>>
>>
>> > Am 30.11.2019 um 20:42 schrieb Markus Armbruster <armbru@redhat.com>:
>> >
>> > cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
>> > qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
>> > crashes when the visitor or the QOM setter fails, and its @errp
>> > argument is null.  Messed up in commit 137974cea3 's390x/cpumodel:
>> > implement QMP interface "query-cpu-model-expansion"'.
>> >
>> > Its three callers have the same bug.  Messed up in commit 4e82ef0502
>> > 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
>> > and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
>> > "query-cpu-model-baseline"'.
>> >
>> > The bugs can't bite as no caller actually passes null.  Fix them
>> > anyway.
>>
>> https://en.m.wikipedia.org/wiki/Software_bug
>>
>>   „ A software bug is an error, flaw or fault in a computer program or
>> system that causes it to produce an incorrect or unexpected result, or to
>> behave in unintended ways. „
>>
>> Please make it clear in the descriptions that these are cleanups and not
>> bugfixes. It might be very confusing for people looking out for real bugs.
>
>
>>
> Disclaimer: I am not entirely familiar with the code in question, so take
> my opinion with reasonablereservation.
>
> It looks that we here deal with latent bugs. As you probably know from
> experience, a latent bugs, when they are activated with some ostensibly
> unrelated code change, can be much more difficult to diagnose and fix than
> regular bugs.
>
>
Oops, I didn't even realize that the patch title contains the word
"latent". (I wrote the previous message without that knowledge. For some
strange reason, my email client doesn't display email subject while
replying.)

In this case, I would suggest usage of phrase "latent bug" instead of
"latent error" or so in the message title, to strenghten the point that
this is not a cleanup.

Yours, Aleksandar



> In that light, this change is not a clean up. It is a fix of a latent
> bugs, and Markus' aproach to treat it as a bug fix looks right to me. I
> would just add a word "latent" or similar, which would even more distance
> the patch from "cleanup" meaning.
>
> David, if I understand well, this patch fixes the commit done by you. I
> definitely understand this is not a pleasant position, but we all
> (definitelly including myself too) should learn to handle such situations
> as gracefully as we can.
>
> Yours,
> Aleksandar
>
>
>
>>
>>
>>
>> Also, please change the terminology „messed up“ to „introduced in“ or
>> similar.
>>
>> (applies to all s390x patches)
>>
>> Thanks.
>
>

[-- Attachment #2: Type: text/html, Size: 4458 bytes --]

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[David Hildenbrand]

On 01.12.19 14:46, Aleksandar Markovic wrote:
> 
> 
> On Saturday, November 30, 2019, David Hildenbrand <dhildenb@redhat.com
> <mailto:dhildenb@redhat.com>> wrote:
> 
> 
> 
>     > Am 30.11.2019 um 20:42 schrieb Markus Armbruster
>     <armbru@redhat.com <mailto:armbru@redhat.com>>:
>     >
>     > cpu_model_from_info() is a helper for
>     qmp_query_cpu_model_expansion(),
>     > qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
>     > crashes when the visitor or the QOM setter fails, and its @errp
>     > argument is null.  Messed up in commit 137974cea3 's390x/cpumodel:
>     > implement QMP interface "query-cpu-model-expansion"'.
>     >
>     > Its three callers have the same bug.  Messed up in commit 4e82ef0502
>     > 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
>     > and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
>     > "query-cpu-model-baseline"'.
>     >
>     > The bugs can't bite as no caller actually passes null.  Fix them
>     > anyway.
> 
>     https://en.m.wikipedia.org/wiki/Software_bug
>     <https://en.m.wikipedia.org/wiki/Software_bug>
> 
>       „ A software bug is an error, flaw or fault in a computer program
>     or system that causes it to produce an incorrect or unexpected
>     result, or to behave in unintended ways. „
> 
>     Please make it clear in the descriptions that these are cleanups and
>     not bugfixes. It might be very confusing for people looking out for
>     real bugs.
> 
> 
> 
> Disclaimer: I am not entirely familiar with the code in question, so
> take my opinion with reasonablereservation.
> 
> It looks that we here deal with latent bugs. As you probably know from
> experience, a latent bugs, when they are activated with some ostensibly
> unrelated code change, can be much more difficult to diagnose and fix
> than regular bugs.

"https://economictimes.indiatimes.com/definition/latent-bug

"Definition: An uncovered or unidentified bug which exists in the system
over a period of time is known as the Latent Bug. The bug may persist in
the system in one or more versions of the software."

AFAIK, a latent BUG can be triggered, it simply was never triggered.


Do you think the following code is buggy?

static int get_val(int *ptr)
{
	return *ptr;
}

int main()
{
	int a = 0;

	return get_val(&a);
}

I claim, no, although we could access a NULL pointer if ever reworked.
There is no invalid system state possible.


> 
> In that light, this change is not a clean up. It is a fix of a latent
> bugs, and Markus' aproach to treat it as a bug fix looks right to me. I
> would just add a word "latent" or similar, which would even more
> distance the patch from "cleanup" meaning.

I agree iff there is some way to trigger it. Otherwise, to me it is a
cleanup.If it's a BUG, it deserves proper Fixes tags and some
description how it can be triggered.


-- 
Thanks,

David / dhildenb

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[Aleksandar Markovic]

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

On Sunday, December 1, 2019, Aleksandar Markovic <
aleksandar.m.mail@gmail.com> wrote:

>
>
> On Sunday, December 1, 2019, Aleksandar Markovic <
> aleksandar.m.mail@gmail.com> wrote:
>
>>
>>
>> On Saturday, November 30, 2019, David Hildenbrand <dhildenb@redhat.com>
>> wrote:
>>
>>>
>>>
>>> > Am 30.11.2019 um 20:42 schrieb Markus Armbruster <armbru@redhat.com>:
>>> >
>>> > cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(
>>> ),
>>> > qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
>>> > crashes when the visitor or the QOM setter fails, and its @errp
>>> > argument is null.  Messed up in commit 137974cea3 's390x/cpumodel:
>>> > implement QMP interface "query-cpu-model-expansion"'.
>>> >
>>> > Its three callers have the same bug.  Messed up in commit 4e82ef0502
>>> > 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
>>> > and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
>>> > "query-cpu-model-baseline"'.
>>> >
>>> > The bugs can't bite as no caller actually passes null.  Fix them
>>> > anyway.
>>>
>>> https://en.m.wikipedia.org/wiki/Software_bug
>>>
>>>   „ A software bug is an error, flaw or fault in a computer program or
>>> system that causes it to produce an incorrect or unexpected result, or to
>>> behave in unintended ways. „
>>>
>>> Please make it clear in the descriptions that these are cleanups and not
>>> bugfixes. It might be very confusing for people looking out for real bugs.
>>
>>
>>>
>> Disclaimer: I am not entirely familiar with the code in question, so take
>> my opinion with reasonablereservation.
>>
>> It looks that we here deal with latent bugs. As you probably know from
>> experience, a latent bugs, when they are activated with some ostensibly
>> unrelated code change, can be much more difficult to diagnose and fix than
>> regular bugs.
>>
>>
> Oops, I didn't even realize that the patch title contains the word
> "latent". (I wrote the previous message without that knowledge. For some
> strange reason, my email client doesn't display email subject while
> replying.)
>
> In this case, I would suggest usage of phrase "latent bug" instead of
> "latent error" or so in the message title, to strenghten the point that
> this is not a cleanup.
>
>
Actually, the message title already does use "latent .... bugs". So it is
fine - in my opinion.



> Yours, Aleksandar
>
>
>
>> In that light, this change is not a clean up. It is a fix of a latent
>> bugs, and Markus' aproach to treat it as a bug fix looks right to me. I
>> would just add a word "latent" or similar, which would even more distance
>> the patch from "cleanup" meaning.
>>
>> David, if I understand well, this patch fixes the commit done by you. I
>> definitely understand this is not a pleasant position, but we all
>> (definitelly including myself too) should learn to handle such situations
>> as gracefully as we can.
>>
>> Yours,
>> Aleksandar
>>
>>
>>
>>>
>>>
>>>
>>> Also, please change the terminology „messed up“ to „introduced in“ or
>>> similar.
>>>
>>> (applies to all s390x patches)
>>>
>>> Thanks.
>>
>>

[-- Attachment #2: Type: text/html, Size: 5017 bytes --]

Re: [PATCH 13/21] memory-device: Fix latent memory pre-plug error handling bugs

[David Hildenbrand]

On 30.11.19 20:42, Markus Armbruster wrote:
> memory_device_get_free_addr() crashes when
> memory_device_check_addable() fails and its @errp argument is null.
> Messed up in commit 1b6d6af21b "pc-dimm: factor out capacity and slot
> checks into MemoryDevice".
> 
> The bug can't bite as no caller actually passes null.  Fix it anyway.
> 
> Cc: David Hildenbrand <david@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@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;
>      }

I remember that some time ago, the best practice was to use "local_err",
what is the latest state of that?

I still disagree that these are BUGs or even latent BUGs. If somebody
things these are BUGs and not cleanups, then we should probably have
proper "Fixes: " tags instead.

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCH 16/21] s390/cpu_modules: Fix latent realize() error handling bugs

[David Hildenbrand]

On 30.11.19 20:42, Markus Armbruster wrote:
> get_max_cpu_model() crashes when kvm_s390_get_host_cpu_model() fails
> and its @errp argument is null.
> 
> apply_cpu_model() crashes when kvm_s390_apply_cpu_model() fails and
> its @errp argument is null.
> 
> s390_realize_cpu_model() crashes when get_max_cpu_model() or
> check_compatibility() fail, and its @errp argument is null.
> 
> All three messed up in commit 80560137cf "s390x/cpumodel: check and
> apply the CPU model".
> 
> The bugs can't bite as no caller actually passes null.  Fix them
> anyway.
> 

Subject is wrong, should e.g., start with "s390x/cpumodels". (I am not
aware of CPU modules :) )

[...]

Same comment regarding "local_err" and "BUG".

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 00/21] Error handling fixes, may contain 4.2 material

[Michael S. Tsirkin]

On Sat, Nov 30, 2019 at 08:42:19PM +0100, Markus Armbruster wrote:
> PATCH 2-4 fix crash bugs.  Including them would be a no-brainer at
> -rc0.  But we're post -rc3, and even for crash bugs we require a
> certain likelihood of users getting bitten.
> 
> Jens, please assess impact of PATCH 2's crash bug.
> 
> Kevin, please do the same for PATCH 3.
> 
> Daniel, please do the same for PATCH 4.

virtio things:

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

Jason do you want to pick these?

> The remainder is definitely not 4.2 material.
> 
> 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: Jens Freimann <jfreimann@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Nishanth Aravamudan <naravamudan@digitalocean.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Markus Armbruster (21):
>   net/virtio: Drop useless n->primary_dev not null checks
>   net/virtio: Fix failover error handling crash bugs
>   block/file-posix: Fix laio_init() error handling crash bug
>   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 latent file_ram_alloc() error handling bug
>   hw/acpi: Fix latent legacy CPU plug error handling bug
>   hw/core: Fix latent fit_load_fdt() error handling bug
>   hw/ipmi: Fix latent realize() error handling bugs
>   qga: Fix latent guest-get-fsinfo error handling bug
>   memory-device: Fix latent memory pre-plug error handling bugs
>   s390x/event-facility: Fix latent realize() error handling bug
>   s390x/cpu_models: Fix latent feature property error handling bugs
>   s390/cpu_modules: Fix latent realize() error handling bugs
>   s390x: Fix latent query-cpu-model-FOO error handling bugs
>   s390x: Fix latent query-cpu-definitions error handling bug
>   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 +-
>  block/file-posix.c                  |  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/net/virtio-net.c                 | 27 ++++----
>  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 +-
>  22 files changed, 154 insertions(+), 123 deletions(-)
> 
> -- 
> 2.21.0

Re: [PATCH 11/21] hw/ipmi: Fix latent realize() error handling bugs

[Corey Minyard]

On Sat, Nov 30, 2019 at 08:42:30PM +0100, Markus Armbruster wrote:
> isa_ipmi_bt_realize(), ipmi_isa_realize(), pci_ipmi_bt_realize(), and
> pci_ipmi_kcs_realize() crash when IPMIInterfaceClass method init()
> fails and their @errp argument is null.  First messed up 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".
> 
> The bug can't bite as no caller actually passes null, and none of the
> init() methods can actually fail.  Fix it anyway.

Well, whatever.  It looks correct and is better style.  I've added this
to my tree.

-corey

> 
> 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
> 

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 01.12.19 14:46, Aleksandar Markovic wrote:
>> 
>> 
>> On Saturday, November 30, 2019, David Hildenbrand <dhildenb@redhat.com
>> <mailto:dhildenb@redhat.com>> wrote:
>> 
>> 
>> 
>>     > Am 30.11.2019 um 20:42 schrieb Markus Armbruster
>>     <armbru@redhat.com <mailto:armbru@redhat.com>>:
>>     >
>>     > cpu_model_from_info() is a helper for
>>     qmp_query_cpu_model_expansion(),
>>     > qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
>>     > crashes when the visitor or the QOM setter fails, and its @errp
>>     > argument is null.  Messed up in commit 137974cea3 's390x/cpumodel:
>>     > implement QMP interface "query-cpu-model-expansion"'.
>>     >
>>     > Its three callers have the same bug.  Messed up in commit 4e82ef0502
>>     > 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
>>     > and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
>>     > "query-cpu-model-baseline"'.
>>     >
>>     > The bugs can't bite as no caller actually passes null.  Fix them
>>     > anyway.
>> 
>>     https://en.m.wikipedia.org/wiki/Software_bug
>>     <https://en.m.wikipedia.org/wiki/Software_bug>
>> 
>>       „ A software bug is an error, flaw or fault in a computer program
>>     or system that causes it to produce an incorrect or unexpected
>>     result, or to behave in unintended ways. „
>> 
>>     Please make it clear in the descriptions that these are cleanups and
>>     not bugfixes. It might be very confusing for people looking out for
>>     real bugs.
>> 
>> 
>> 
>> Disclaimer: I am not entirely familiar with the code in question, so
>> take my opinion with reasonablereservation.
>> 
>> It looks that we here deal with latent bugs. As you probably know from
>> experience, a latent bugs, when they are activated with some ostensibly
>> unrelated code change, can be much more difficult to diagnose and fix
>> than regular bugs.
>
> "https://economictimes.indiatimes.com/definition/latent-bug
>
> "Definition: An uncovered or unidentified bug which exists in the system
> over a period of time is known as the Latent Bug. The bug may persist in
> the system in one or more versions of the software."
>
> AFAIK, a latent BUG can be triggered, it simply was never triggered.

First search hit.  Here's my second one:

    Q: What are latent bugs?

    A: These bugs do not cause problems today. However, they are lurking
    just waiting to reveal themselves later.  The Ariane 5 rocket
    failure was caused by a float->int conversion error that lay dormant
    when previous rockets were slower; but the faster Ariane 5 triggered
    the problem.  The Millennium bug is another example of a latent bug
    that came to light when circumstances changed.  Latent bugs are much
    harder to test using conventional testing techniques, and finding
    them requires someone with foresight to ask.

http://www.geekinterview.com/question_details/36689

My point is: common usage of the term is not as clear-cut as your quote
makes it seem.

> Do you think the following code is buggy?
>
> static int get_val(int *ptr)
> {
> 	return *ptr;
> }
>
> int main()
> {
> 	int a = 0;
>
> 	return get_val(&a);
> }
>
> I claim, no, although we could access a NULL pointer if ever reworked.
> There is no invalid system state possible.

get_val() is silent on how it wants to be used.  error.h is anything
but: it spells out how it wantes to be used in quite some detail.  In
particular:

 * Receive an error and pass it on to the caller:
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     if (err) {
 *         handle the error...
 *         error_propagate(errp, err);
 *     }
 * where Error **errp is a parameter, by convention the last one.
 *
 * Do *not* "optimize" this to
 *     foo(arg, errp);
 *     if (*errp) { // WRONG!
 *         handle the error...
 *     }
 * because errp may be NULL!

My patch fixes this exact misuse of the interface.

>> In that light, this change is not a clean up. It is a fix of a latent
>> bugs, and Markus' aproach to treat it as a bug fix looks right to me. I
>> would just add a word "latent" or similar, which would even more
>> distance the patch from "cleanup" meaning.
>
> I agree iff there is some way to trigger it. Otherwise, to me it is a
> cleanup.If it's a BUG, it deserves proper Fixes tags and some
> description how it can be triggered.

Yes, a bug that can bite deserves a reproducer and a formal Fixes: tag.

The thing we're discussing (however we may want to call it) does not
have a reproducer, and I think we're in agreement that it doesn't need a
Fixes: tag.

However, my patch is not cleaning up something that's dirty, it's fixing
something that's unequivocally wrong: a violation of a stated interface
contract.

The violation happens to have no ill effects at this time due to the way
the violating code is being used.

I call that a "latent bug".  git-log has quite a few occurences of
"latent bug", by Richard Henderson, Daniel Berrangé, Paolo, ...

Your point that the commit message should not confuse people looking for
real bugs is well taken.  I think "latent bug" is clear enough, and also
concise.  I'm of course open to better phrasings.

   s390x: Fix currently harmless query-cpu-model-FOO error API violations

feels no clearer to me than

   s390x: Fix latent query-cpu-model-FOO error handling bugs

It's also too long.

I tried.  Your turn :)

Re: [PATCH 16/21] s390/cpu_modules: Fix latent realize() error handling bugs

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 30.11.19 20:42, Markus Armbruster wrote:
>> get_max_cpu_model() crashes when kvm_s390_get_host_cpu_model() fails
>> and its @errp argument is null.
>> 
>> apply_cpu_model() crashes when kvm_s390_apply_cpu_model() fails and
>> its @errp argument is null.
>> 
>> s390_realize_cpu_model() crashes when get_max_cpu_model() or
>> check_compatibility() fail, and its @errp argument is null.
>> 
>> All three messed up in commit 80560137cf "s390x/cpumodel: check and
>> apply the CPU model".
>> 
>> The bugs can't bite as no caller actually passes null.  Fix them
>> anyway.
>> 
>
> Subject is wrong, should e.g., start with "s390x/cpumodels". (I am not
> aware of CPU modules :) )

Of course.

> [...]
>
> Same comment regarding "local_err" and "BUG".
>
> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!

Re: [PATCH 13/21] memory-device: Fix latent memory pre-plug error handling bugs

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 30.11.19 20:42, Markus Armbruster wrote:
>> memory_device_get_free_addr() crashes when
>> memory_device_check_addable() fails and its @errp argument is null.
>> Messed up in commit 1b6d6af21b "pc-dimm: factor out capacity and slot
>> checks into MemoryDevice".
>> 
>> The bug can't bite as no caller actually passes null.  Fix it anyway.
>> 
>> Cc: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@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;
>>      }
>
> I remember that some time ago, the best practice was to use "local_err",
> what is the latest state of that?

Hundreds of local Error * variables are named @local_err, and hundreds
more are named @err.

For what it's worth, the big comment in error.h uses @err, except in one
place where it needs two of them.

> I still disagree that these are BUGs or even latent BUGs. If somebody
> things these are BUGs and not cleanups, then we should probably have
> proper "Fixes: " tags instead.

Let's continue that discussion in the sub-thread where you first raised
this objection.

> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!

Re: [PATCH 08/21] exec: Fix latent file_ram_alloc() error handling bug

[Igor Mammedov]

On Sat, 30 Nov 2019 20:42:27 +0100
Markus Armbruster <armbru@redhat.com> 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).  Messed up in
> commit 056b68af77 "fix qemu exit on memory hotplug when allocation
> fails at prealloc time".
> 
> The bug can't bite as no caller actually passes null.  Fix it anyway.
> 
> 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;
>          }

Re: [PATCH 09/21] hw/acpi: Fix latent legacy CPU plug error handling bug

[Igor Mammedov]

On Sat, 30 Nov 2019 20:42:28 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> legacy_acpi_cpu_plug_cb() crashes when acpi_set_cpu_present_bit()
> fails and its @errp argument is null.  Messed up in commit cc43364de7
> "acpi/cpu-hotplug: introduce helper function to keep bit setting in
> one place".
> 
> The bug can't bite as no caller actually passes null, and
> acpi_set_cpu_present_bit() can't actually fail.  Fix it anyway.
> 
> 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);
>      }
>  }
>  

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[David Hildenbrand]

[...]

> First search hit.  Here's my second one:
> 
>     Q: What are latent bugs?
> 
>     A: These bugs do not cause problems today. However, they are lurking
>     just waiting to reveal themselves later.  The Ariane 5 rocket
>     failure was caused by a float->int conversion error that lay dormant
>     when previous rockets were slower; but the faster Ariane 5 triggered
>     the problem.  The Millennium bug is another example of a latent bug
>     that came to light when circumstances changed.  Latent bugs are much
>     harder to test using conventional testing techniques, and finding
>     them requires someone with foresight to ask.
> 
> http://www.geekinterview.com/question_details/36689

Google search "latent software BUG"

Hit 1: What I posted


Hit 2:
https://www.quora.com/What-are-some-examples-for-a-latent-defect-in-software-testing

"The problems will not cause the damage currently, but wait to reveal
themselves at a later time. ... E.g. February has 28 days. ... These
defects do not cause damage to the system immediately but wait for a
particular event sometime to cause damage and show their presence."

"Mostly, these types of bugs are unexpected outcome of any corner/edge
case scenarios which was executed with some specific set of test data."


Hit 3: https://sqa.stackexchange.com/questions/9170/what-is-a-latent-bug

"Latent bugs are bugs which exist, but have not yet been discovered.

They are bugs waiting to be found."

"In Software Quality Assurance:

Latent defects are the those which arises in the field, and unknown
until they reported by the field staff."


Hit 4:
https://sqa.stackexchange.com/questions/13980/what-is-the-difference-between-gold-bug-and-latent-bug

"A latent bug is a bug which is present in the system from previous
iterations or release (in your scenario Sprint 1). They are either low
priority bugs, which either went undetected or were not reported."


Same at least for Hit 5, 6, 7 (then I got tired ;) )

https://blog.qatestlab.com/2011/10/21/latent-and-masked-software-bugs-what-is-the-difference/
https://www.testing-whiz.com/blog/3-types-of-unusual-software-defects-you-should-not-miss
https://www.360logica.com/blog/latent-defect-hide-n-seek-defect-software-testing/

Which contain perfect examples and descriptions for latent bugs :)

e.g.,

"Let’s imagine that an application is able to print a document either by
laser printer or by dot matrix printer. To reach this, the application
first searches for the laser printer. In this case if it finds a laser
printer (used by default) it uses this one and prints. In case if it
does not find a laser printer, the application searches for dot matrix
printer. And if the application finds a dot matrix printer, it  gives an
error message. This unleashes a latent defect. Therefore this
application will never search for the dot matrix printer. And the
application never got tested for the dot matrix printer. That means the
accurate conditions were never met for the dot matrix printer. This is
what we call a latent software bug."

[...]

> 
>>> In that light, this change is not a clean up. It is a fix of a latent
>>> bugs, and Markus' aproach to treat it as a bug fix looks right to me. I
>>> would just add a word "latent" or similar, which would even more
>>> distance the patch from "cleanup" meaning.
>>
>> I agree iff there is some way to trigger it. Otherwise, to me it is a
>> cleanup.If it's a BUG, it deserves proper Fixes tags and some
>> description how it can be triggered.
> 
> Yes, a bug that can bite deserves a reproducer and a formal Fixes: tag.

A "BUG that cannot be triggered" is an oxymoron. :)

The code might be "error prone", it might "violate API guidelines", it
might "not obey coding practices". If it can't be triggered, it's - by
definition - not a (latent) BUG.

> 
> The thing we're discussing (however we may want to call it) does not
> have a reproducer, and I think we're in agreement that it doesn't need a
> Fixes: tag.
> 
> However, my patch is not cleaning up something that's dirty, it's fixing
> something that's unequivocally wrong: a violation of a stated interface
> contract.

In general, I don't care about these minor details, but if somebody
literally tells the world for all eternity in a handful of patch
descriptions "David messed up the code by introducing a (latent) BUG
(that cannot be triggered)" I get suspicious. Something nicer (IMHO)
would be "Commit X introduced error prone code, let's rework that to
make it more stable in the future and obey the "error API" guidelines. I
hope you see the difference ;)

> 
> Your point that the commit message should not confuse people looking for
> real bugs is well taken.  I think "latent bug" is clear enough, and also
> concise.  I'm of course open to better phrasings.
> 
>    s390x: Fix currently harmless query-cpu-model-FOO error API violations
> 
> feels no clearer to me than
> 
>    s390x: Fix latent query-cpu-model-FOO error handling bugs
> 
> It's also too long.
> 
> I tried.  Your turn :)


s390x: Fix query-cpu-model-FOO error API violations

s390x: Make query-cpu-model-FOO error handling less error prone

s390x: Cleanup error handling in query-cpu-model-FOO

s390x: Rework error handling in query-cpu-model-FOO

...

but, enough time spent on this, feel free to continue with this however
you want.

-- 
Thanks,

David / dhildenb

Re: [PATCH 02/21] net/virtio: Fix failover error handling crash bugs

[Jens Freimann]

On Sat, Nov 30, 2019 at 08:42:21PM +0100, Markus Armbruster wrote:
>Functions that take an Error ** parameter to pass an error to the
>caller expect the parameter to point to null.
>failover_replug_primary() violates this precondition in several
>places:
>
>* After qemu_opts_from_qdict() failed, *errp is no longer null.
>  Passing it to error_setg() is wrong, and will trip the assertion in
>  error_setv().  Messed up in commit 150ab54aa6 "net/virtio: fix
>  re-plugging of primary device".  Simply drop the error_setg().
>
>* Passing @errp to qemu_opt_set_bool(), hotplug_handler_pre_plug(),
>  and hotplug_handler_plug() is wrong.  If one of the first two fails,
>  *errp is no longer null.  Risks tripping the same assertion.
>  Moreover, continuing after such errors is unsafe.  Messed up in
>  commit 9711cd0dfc "net/virtio: add failover support".  Fix by
>  handling each error properly.
>
>failover_replug_primary() crashes when passed a null @errp.  Also
>messed up in commit 9711cd0dfc.  This bug can't bite as no caller
>actually passes null.  Fix it anyway.
>
>Fixes: 9711cd0dfc3fa414f7f64935713c07134ae67971
>Fixes: 150ab54aa6934583180f88a2bd540bc6fc4fbff3
>Cc: Jens Freimann <jfreimann@redhat.com>
>Cc: Michael S. Tsirkin <mst@redhat.com>
>Signed-off-by: Markus Armbruster <armbru@redhat.com>
>---
> hw/net/virtio-net.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>

Thanks Markus!

Reviewed-by: Jens Freimann <jfreimann@redhat.com>

Re: [PATCH 01/21] net/virtio: Drop useless n->primary_dev not null checks

[Jens Freimann]

On Sat, Nov 30, 2019 at 08:42:20PM +0100, Markus Armbruster wrote:
>virtio_net_handle_migration_primary() returns early when it can't
>ensure n->primary_dev is non-null.  Checking it again right after that
>early return is redundant.  Drop.
>
>If n->primary_dev is null on entering failover_replug_primary(), @pdev
>will become null, and pdev->partially_hotplugged will crash.  Checking
>n->primary_dev later is useless.  It can't actually be null, because
>its caller virtio_net_handle_migration_primary() ensures it isn't.
>Drop the useless check.
>
>Cc: Jens Freimann <jfreimann@redhat.com>
>Cc: Michael S. Tsirkin <mst@redhat.com>
>Signed-off-by: Markus Armbruster <armbru@redhat.com>
>---
> hw/net/virtio-net.c | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>

Thanks Markus!

Reviewed-by: Jens Freimann <jfreimann@redhat.com>

regards
Jens

Re: [PATCH 18/21] s390x: Fix latent query-cpu-definitions error handling bug

[David Hildenbrand]

On 30.11.19 20:42, Markus Armbruster wrote:
> qmp_query_cpu_definitions() tries to ignore get_max_cpu_model()'s
> errors.  However, it crashes when its @errp argument is null or
> &error_abort, and exit(1)s when it's &error_fatal.  Messed up in
> commit 38cba1f4d8 "s390x: return unavailable features via
> query-cpu-definitions".

s/crashes .../would crash .../ etc.

Reviewed-by: David Hildenbrand <david@redhat.com>

> 
> The bug can't bite as no caller actually passes such @errp values.
> Fix it anyway.
> 
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@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);
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH 14/21] s390x/event-facility: Fix latent realize() error handling bug

[David Hildenbrand]

On 30.11.19 20:42, Markus Armbruster wrote:
> sclp_events_bus_realize() crashes when object_property_set_bool()
> fails and its @errp argument is null.  Messed up in commit f6102c329c
> "s390/sclp: rework sclp event facility initialization + device
> realization".
> 

s/crashes .../would crash .../ ...

Reviewed-by: David Hildenbrand <david@redhat.com>

> The bug can't bite as no caller actually passes null.  Fix it anyway.
> 
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@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;
>          }
>      }
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH 15/21] s390x/cpu_models: Fix latent feature property error handling bugs

[David Hildenbrand]

On 30.11.19 20:42, Markus Armbruster wrote:
> s390x-cpu property setters set_feature() and set_feature_group() crash
> when the visitor fails and its @errp argument is null.  Messed up in
> commit 0754f60429 "s390x/cpumodel: expose features and feature groups
> as properties".

Same comment as to the other patches :)

I think we usually use "s390x/cpumodels", but that's just nitpicking.

Reviewed-by: David Hildenbrand <david@redhat.com>

> 
> The bug can't bite as no caller actually passes null.  Fix it anyway.
> 
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@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) {
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH 03/21] block/file-posix: Fix laio_init() error handling crash bug

[Stefan Hajnoczi]

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

On Sat, Nov 30, 2019 at 08:42:22PM +0100, Markus Armbruster wrote:
> raw_aio_attach_aio_context() passes uninitialized Error *local_err by
> reference to laio_init() via aio_setup_linux_aio().  When laio_init()
> fails, it passes it on to error_setg_errno(), tripping error_setv()'s
> assertion unless @local_err is null by dumb luck.
> 
> Fix by initializing @local_err properly.
> 
> Fixes: ed6e2161715c527330f936d44af4c547f25f687e
> Cc: Nishanth Aravamudan <naravamudan@digitalocean.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/file-posix.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Worth including in QEMU 4.2.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 00/21] Error handling fixes, may contain 4.2 material

[Daniel P. Berrangé]

On Sat, Nov 30, 2019 at 08:42:19PM +0100, Markus Armbruster wrote:
> PATCH 2-4 fix crash bugs.  Including them would be a no-brainer at
> -rc0.  But we're post -rc3, and even for crash bugs we require a
> certain likelihood of users getting bitten.
> 
> Jens, please assess impact of PATCH 2's crash bug.
> 
> Kevin, please do the same for PATCH 3.
> 
> Daniel, please do the same for PATCH 4.


The code has existed like this since 2.5.0, and its not a security
issue, so I can't see any justification for putting it into 4.2
at the last minute.

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 00/21] Error handling fixes, may contain 4.2 material

[Jens Freimann]

On Sat, Nov 30, 2019 at 08:42:19PM +0100, Markus Armbruster wrote:
>PATCH 2-4 fix crash bugs.  Including them would be a no-brainer at
>-rc0.  But we're post -rc3, and even for crash bugs we require a
>certain likelihood of users getting bitten.
>
>Jens, please assess impact of PATCH 2's crash bug.

Guest can't use it to trigger a qemu crash because the hotplug_handler
called after the pre-plug doesn't do anything with errp.

qemu_opt_set_bool() is also unlikely to trigger the bug because both
reasons for why it would set errp are not true in this path.

I would prefer for this fix to go into 4.2 but if it is the only one
and triggers a large amount of work then don't do it.

regards
Jens

Re: [PATCH 03/21] block/file-posix: Fix laio_init() error handling crash bug

[Kevin Wolf]

Am 30.11.2019 um 20:42 hat Markus Armbruster geschrieben:
> raw_aio_attach_aio_context() passes uninitialized Error *local_err by
> reference to laio_init() via aio_setup_linux_aio().  When laio_init()
> fails, it passes it on to error_setg_errno(), tripping error_setv()'s
> assertion unless @local_err is null by dumb luck.
> 
> Fix by initializing @local_err properly.
> 
> Fixes: ed6e2161715c527330f936d44af4c547f25f687e
> Cc: Nishanth Aravamudan <naravamudan@digitalocean.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[Cornelia Huck]

On Sat, 30 Nov 2019 20:42:36 +0100
Markus Armbruster <armbru@redhat.com> wrote:

I don't really want to restart the discussion :), but what about:

> cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
> qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
> crashes when the visitor or the QOM setter fails, and its @errp
> argument is null. 

"It would crash when the visitor or the QOM setter fails if its @errp
argument were NULL." ?

(Hope I got my conditionals right...)

> Messed up in commit 137974cea3 's390x/cpumodel:

I agree that "Introduced" is a bit nicer than "Messed up".

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

If we agree, I can tweak the various commit messages for the s390x
patches and apply them.

> The bugs can't bite as no caller actually passes null.  Fix them
> anyway.
> 
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  target/s390x/cpu_models.c | 43 ++++++++++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 16 deletions(-)

David, I don't think you gave a R-b for that one yet?

Re: [PATCH 20/21] hw/intc/s390: Simplify error handling in kvm_s390_flic_realize()

[Cornelia Huck]

On Sat, 30 Nov 2019 20:42:39 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> 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>
> ---
>  hw/intc/s390_flic_kvm.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

...someone else wants to take a look before I queue this?

Re: [PATCH 15/21] s390x/cpu_models: Fix latent feature property error handling bugs

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 30.11.19 20:42, Markus Armbruster wrote:
>> s390x-cpu property setters set_feature() and set_feature_group() crash
>> when the visitor fails and its @errp argument is null.  Messed up in
>> commit 0754f60429 "s390x/cpumodel: expose features and feature groups
>> as properties".
>
> Same comment as to the other patches :)
>
> I think we usually use "s390x/cpumodels", but that's just nitpicking.

$ git-log --oneline target/s390x/cpu_models.c | awk '$2 ~ /:$/ { print $2 }' | sort | uniq -c
      1 S390:
      6 qapi:
      1 qemu-common:
      1 qmp:
      2 qobject:
      1 qom:
      1 s390/cpumodel:
      1 s390x/ccw:
     21 s390x/cpumodel:
      1 s390x/cpumodels:
      1 s390x/kvm:
      4 s390x/tcg:
      7 s390x:
      1 target/s390x/cpu_models:
     17 target/s390x:
      1 target:

You're right, except for the plural vs. singular.  I should've browsed
git-log.

> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> [...]
>
>> First search hit.  Here's my second one:
>> 
>>     Q: What are latent bugs?
>> 
>>     A: These bugs do not cause problems today. However, they are lurking
>>     just waiting to reveal themselves later.  The Ariane 5 rocket
>>     failure was caused by a float->int conversion error that lay dormant
>>     when previous rockets were slower; but the faster Ariane 5 triggered
>>     the problem.  The Millennium bug is another example of a latent bug
>>     that came to light when circumstances changed.  Latent bugs are much
>>     harder to test using conventional testing techniques, and finding
>>     them requires someone with foresight to ask.
>> 
>> http://www.geekinterview.com/question_details/36689
>
> Google search "latent software BUG"

I think this argument has run its course.  Let's agree to differ on the
finer meaning and possible uses of "latent", and craft a mutually
agreeable commit message instead.  I'll reply to Cornelia's message.

[...]

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[Markus Armbruster]

Cornelia Huck <cohuck@redhat.com> writes:

> On Sat, 30 Nov 2019 20:42:36 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
> I don't really want to restart the discussion :), but what about:
>
>> cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
>> qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
>> crashes when the visitor or the QOM setter fails, and its @errp
>> argument is null. 
>
> "It would crash when the visitor or the QOM setter fails if its @errp
> argument were NULL." ?
>
> (Hope I got my conditionals right...)

I don't think this is an improvement.

The commit message matches a pattern "what's wrong, since when, impact,
how is it fixed".  The pattern has become habit for me.  Its "what's
wrong" part is strictly local.  The non-local argument comes in only
when we assess impact.

Use of "would" in the what part's conditional signals the condition is
unlikely.  True (it's actually impossible), but distracting (because it
involves the non-local argument I'm not yet ready to make).

Let me try a different phrasing below.

>> Messed up in commit 137974cea3 's390x/cpumodel:
>
> I agree that "Introduced" is a bit nicer than "Messed up".

Works fine for me.  I didn't mean any disrespect --- I'd have to
disrespect myself: the mess corrected by PATCH 10 is mine.

>> implement QMP interface "query-cpu-model-expansion"'.
>> 
>> Its three callers have the same bug.  Messed up in commit 4e82ef0502

Feel free to call it "issue" rather than "bug".  I don't care, but David
might.

>> 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
>> and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
>> "query-cpu-model-baseline"'.
>
> If we agree, I can tweak the various commit messages for the s390x
> patches and apply them.

Tweaking the non-s390x commit messages as well would be nicer, but
requires a respin.

Let's try to craft a mutually agreeable commit message for this patch.
Here's my attempt:

    s390x: Fix query-cpu-model-FOO error API violations

    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.  To fix, 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>

Adapting it to other patches should be straightforward.

>> The bugs can't bite as no caller actually passes null.  Fix them
>> anyway.
>> 
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  target/s390x/cpu_models.c | 43 ++++++++++++++++++++++++---------------
>>  1 file changed, 27 insertions(+), 16 deletions(-)
>
> David, I don't think you gave a R-b for that one yet?

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[Cornelia Huck]

On Tue, 03 Dec 2019 08:49:58 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Cornelia Huck <cohuck@redhat.com> writes:
> 
> > On Sat, 30 Nov 2019 20:42:36 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> > I don't really want to restart the discussion :), but what about:
> >  
> >> cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
> >> qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
> >> crashes when the visitor or the QOM setter fails, and its @errp
> >> argument is null.   
> >
> > "It would crash when the visitor or the QOM setter fails if its @errp
> > argument were NULL." ?
> >
> > (Hope I got my conditionals right...)  
> 
> I don't think this is an improvement.
> 
> The commit message matches a pattern "what's wrong, since when, impact,
> how is it fixed".  The pattern has become habit for me.  Its "what's
> wrong" part is strictly local.  The non-local argument comes in only
> when we assess impact.
> 
> Use of "would" in the what part's conditional signals the condition is
> unlikely.  True (it's actually impossible), but distracting (because it
> involves the non-local argument I'm not yet ready to make).

I think we'll have to agree to disagree here...

> 
> Let me try a different phrasing below.

...but also see below :)

> 
> >> Messed up in commit 137974cea3 's390x/cpumodel:  
> >
> > I agree that "Introduced" is a bit nicer than "Messed up".  
> 
> Works fine for me.  I didn't mean any disrespect --- I'd have to
> disrespect myself: the mess corrected by PATCH 10 is mine.
> 
> >> implement QMP interface "query-cpu-model-expansion"'.
> >> 
> >> Its three callers have the same bug.  Messed up in commit 4e82ef0502  
> 
> Feel free to call it "issue" rather than "bug".  I don't care, but David
> might.
> 
> >> 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
> >> and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
> >> "query-cpu-model-baseline"'.  
> >
> > If we agree, I can tweak the various commit messages for the s390x
> > patches and apply them.  
> 
> Tweaking the non-s390x commit messages as well would be nicer, but
> requires a respin.
> 
> Let's try to craft a mutually agreeable commit message for this patch.
> Here's my attempt:
> 
>     s390x: Fix query-cpu-model-FOO error API violations
> 
>     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.  To fix, 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>

That sounds good to me.

> 
> Adapting it to other patches should be straightforward.

Ok, so how to proceed? I'm happy to tweak the commit messages for
s390x, but that is bound to get messy.

> 
> >> The bugs can't bite as no caller actually passes null.  Fix them
> >> anyway.
> >> 
> >> Cc: David Hildenbrand <david@redhat.com>
> >> Cc: Cornelia Huck <cohuck@redhat.com>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  target/s390x/cpu_models.c | 43 ++++++++++++++++++++++++---------------
> >>  1 file changed, 27 insertions(+), 16 deletions(-)  
> >
> > David, I don't think you gave a R-b for that one yet?  

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[David Hildenbrand]

>>     s390x: Fix query-cpu-model-FOO error API violations
>>
>>     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.  To fix, 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>
> 
> That sounds good to me.

Yes, something I would have enjoyed reading from the first sentence.

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 20/21] hw/intc/s390: Simplify error handling in kvm_s390_flic_realize()

[Halil Pasic]

On Mon, 2 Dec 2019 17:40:07 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Sat, 30 Nov 2019 20:42:39 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
> 
> > 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>
> > ---
> >  hw/intc/s390_flic_kvm.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
> ...someone else wants to take a look before I queue this?
> 

I guess it is a matter of taste.

Acked-by: Halil Pasic <pasic@linux.ibm.com>

The only difference I can see is if the **errp argument where
to already contain an error (*errp). I guess the old code would
just keep the first error, and discard the next one, while error_setv()
does an assert(*errp == NULL).

I assume calling with *errp != NULL is not a valid scenario. But then, I
can't say I understand the use-case behind this discard the newer error
behavior of error_propagate().

Regards,
Halil

Re: [PATCH 13/21] memory-device: Fix latent memory pre-plug error handling bugs

[Eric Blake]

On 12/1/19 11:07 PM, Markus Armbruster wrote:

>>>   {
>>> +    Error *err = NULL;

>> I remember that some time ago, the best practice was to use "local_err",
>> what is the latest state of that?
> 
> Hundreds of local Error * variables are named @local_err, and hundreds
> more are named @err.
> 
> For what it's worth, the big comment in error.h uses @err, except in one
> place where it needs two of them.

What's more, if we go through with Vladimir's Coccinelle cleanup to use 
ERRP_AUTO_PROPAGATE, then we don't have either name to worry about (both 
'&local_err' and '&err' are replaced by 'errp').

> 
>> I still disagree that these are BUGs or even latent BUGs. If somebody
>> things these are BUGs and not cleanups, then we should probably have
>> proper "Fixes: " tags instead.
> 
> Let's continue that discussion in the sub-thread where you first raised
> this objection.

One benefit of fixing the style (whether or not you count it as a bug 
fix) is that the Coccinelle script for updating to a new style is more 
likely to apply correctly.

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

Re: [PATCH 21/21] tests-blockjob: Use error_free_or_abort()

[Eric Blake]

On 11/30/19 1:42 PM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/test-blockjob.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 

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

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

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

Re: [PATCH 20/21] hw/intc/s390: Simplify error handling in kvm_s390_flic_realize()

[Markus Armbruster]

Halil Pasic <pasic@linux.ibm.com> writes:

> On Mon, 2 Dec 2019 17:40:07 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Sat, 30 Nov 2019 20:42:39 +0100
>> Markus Armbruster <armbru@redhat.com> wrote:
>> 
>> > 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>
>> > ---
>> >  hw/intc/s390_flic_kvm.c | 10 ++++------
>> >  1 file changed, 4 insertions(+), 6 deletions(-)
>> 
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> 
>> ...someone else wants to take a look before I queue this?
>> 
>
> I guess it is a matter of taste.
>
> Acked-by: Halil Pasic <pasic@linux.ibm.com>

Thanks!

> The only difference I can see is if the **errp argument where
> to already contain an error (*errp). I guess the old code would
> just keep the first error, and discard the next one, while error_setv()
> does an assert(*errp == NULL).

Correct.

> I assume calling with *errp != NULL is not a valid scenario.

Correct again.  On function entry, @errp must either be null,
@error_fatal, @error_abort, or point to null.

>                                                              But then, I
> can't say I understand the use-case behind this discard the newer error
> behavior of error_propagate().

The documented[1] use case is "receive and accumulate multiple errors
(first one wins)".  See the big comment in include/qapi/error.h.

For what it's worth, GError explicitly disallows such accumulation: "The
error variable dest points to must be NULL"[2].  If you do it anyway, it
logs a warning nobody reads[3], then throws away the newer error.


[1] It's "ex post facto" documentation, like much of the Error API
documentation.

[2] https://developer.gnome.org/glib/stable/glib-Error-Reporting.html#g-propagate-error

[3] First order approximation based on the amount of crap supposedly
stable applications log.

Re: [PATCH 00/21] Error handling fixes, may contain 4.2 material

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Sat, Nov 30, 2019 at 08:42:19PM +0100, Markus Armbruster wrote:
>> PATCH 2-4 fix crash bugs.  Including them would be a no-brainer at
>> -rc0.  But we're post -rc3, and even for crash bugs we require a
>> certain likelihood of users getting bitten.
>> 
>> Jens, please assess impact of PATCH 2's crash bug.
>> 
>> Kevin, please do the same for PATCH 3.
>> 
>> Daniel, please do the same for PATCH 4.
>
> virtio things:
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

In my haste to get this into -rc4, I lost your r-bys.  Sorry about that!

> Jason do you want to pick these?

Merged in commit 39032981fa851d25fb27527f25f046fed800e585.

Re: [PATCH 04/21] crypto: Fix certificate file error handling crash bug

[Vladimir Sementsov-Ogievskiy]

30.11.2019 22:42, 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(-)
> 
> 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;
>   
> 

I didn't check the logic in commit message (and I don't know how GError works),
but initializing local pointer to NULL never hurts:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 05/21] crypto: Fix typo in QCryptoTLSSession's <example> comment

[Vladimir Sementsov-Ogievskiy]

30.11.2019 22:42, 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(-)
> 
> 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;
>    *
> 


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 06/21] io: Fix Error usage in a comment <example>

[Vladimir Sementsov-Ogievskiy]

30.11.2019 22:42, 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(-)
> 
> 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) {
> 

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

(I also think you can safely s/Fix Error usage/Fix typo/ in subject)

-- 
Best regards,
Vladimir

Re: [PATCH 07/21] tests: Clean up initialization of Error *err variables

[Vladimir Sementsov-Ogievskiy]

30.11.2019 22:42, 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>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

Re: [PATCH 10/21] hw/core: Fix latent fit_load_fdt() error handling bug

[Vladimir Sementsov-Ogievskiy]

30.11.2019 22:42, Markus Armbruster wrote:
> fit_load_fdt() recovers from fit_image_addr() failing with ENOENT.
> Except it doesn't when its @errp argument is &error_fatal or
> &error_abort, because it blindly passes @errp to fit_image_addr().
> Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()".
> 
> The bug can't bite as no caller actually passes &error_fatal or
> &error_abort.  Fix it anyway.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Hmm, actually it makes my
"[PATCH v7 01/21] hw/core/loader-fit: fix freeing errp in fit_load_fdt"
unnecessary. If you want you may drop my 01 (as it covers less problems),
and in this case you may note in your cover letter, that
errp = NULL is broken here too (may be nobady pass it?),
and normal errp is handled wrong, as *errp doesn't set to NULL after
error_free(*errp) (still, the only caller rely on return value more than on
err, so the problem can't be triggered with current code)

> ---
>   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;
>       }
>   
> 

So much attention to that function :)

I'd also propose the following:

diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
index c465921b8f..2c9efeef7a 100644
--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -180,8 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
  {
      Error *err = NULL;
      const char *name;
-    const void *data;
-    const void *load_data;
+    void *data;
+    void *load_data;
      hwaddr load_addr;
      int img_off;
      size_t sz;
@@ -218,9 +218,9 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,

      ret = 0;
  out:
-    g_free((void *) data);
+    g_free(data);
      if (data != load_data) {
-        g_free((void *) load_data);
+        g_free(load_data);
      }
      return ret;
  }


Or, even better:

--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -180,7 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
  {
      Error *err = NULL;
      const char *name;
-    const void *data;
+    g_autofree void *data = NULL;
+    g_autofree void *fdt_filter_data = NULL;
      const void *load_data;
      hwaddr load_addr;
      int img_off;
@@ -202,27 +203,23 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
      if (ret == -ENOENT) {
          load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
          error_free(err);
+        return 0;
      } else if (ret) {
          error_propagate_prepend(errp, err,
                                  "unable to read FDT load address from FIT: ");
-        goto out;
+        return ret;
      }

      if (ldr->fdt_filter) {
-        load_data = ldr->fdt_filter(opaque, data, match_data, &load_addr);
+        load_data = fdt_filter_data =
+                ldr->fdt_filter(opaque, data, match_data, &load_addr);
      }

      load_addr = ldr->addr_to_phys(opaque, load_addr);
      sz = fdt_totalsize(load_data);
      rom_add_blob_fixed(name, load_data, sz, load_addr);

-    ret = 0;
-out:
-    g_free((void *) data);
-    if (data != load_data) {
-        g_free((void *) load_data);
-    }
-    return ret;
+    return 0;
  }




-- 
Best regards,
Vladimir

Re: [PATCH 12/21] qga: Fix latent guest-get-fsinfo error handling bug

[Vladimir Sementsov-Ogievskiy]

30.11.2019 22:42, Markus Armbruster wrote:
> build_guest_fsinfo_for_virtual_device() crashes when
> build_guest_fsinfo_for_device() fails and its @errp argument is null.
> Messed up in commit 46d4c5723e "qga: Add guest-get-fsinfo command".
> 
> The bug can't bite as no caller actually passes null.  Fix it anyway.
> 
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Actually, all such bugs should be fixed by my auto-generated series..
And, if fixing by hand, it may be better to teach this function to return
int, than propagation is not needed.

> ---
>   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;
>               }
>           }
> 


-- 
Best regards,
Vladimir

Re: [PATCH 06/21] io: Fix Error usage in a comment <example>

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 30.11.2019 22:42, 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(-)
>> 
>> 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) {
>> 
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> (I also think you can safely s/Fix Error usage/Fix typo/ in subject)

I'd say it's both: the trailing * is clearly a typo, but the missing
initializer is asking Murphy for random crashes at inopportune times.
Examples better set *good* examples :)

Thanks!

Re: [PATCH 10/21] hw/core: Fix latent fit_load_fdt() error handling bug

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 30.11.2019 22:42, Markus Armbruster wrote:
>> fit_load_fdt() recovers from fit_image_addr() failing with ENOENT.
>> Except it doesn't when its @errp argument is &error_fatal or
>> &error_abort, because it blindly passes @errp to fit_image_addr().
>> Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()".
>> 
>> The bug can't bite as no caller actually passes &error_fatal or
>> &error_abort.  Fix it anyway.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Hmm, actually it makes my
> "[PATCH v7 01/21] hw/core/loader-fit: fix freeing errp in fit_load_fdt"
> unnecessary. If you want you may drop my 01 (as it covers less problems),

Yes.

> and in this case you may note in your cover letter, that
> errp = NULL is broken here too (may be nobady pass it?),

You're right, null @errp is wrong, too.

> and normal errp is handled wrong, as *errp doesn't set to NULL after
> error_free(*errp)

Yes, that's wrong, too.  fit_load_fdt() itself doesn't use *errp after
freeing it, but it sets a trap for its callers.  

>                   (still, the only caller rely on return value more than on
> err, so the problem can't be triggered with current code)

True.

New commit message (based on v2's):

    hw/core: Fix fit_load_fdt() error API violations

    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.  Error
    recovery dereferences @errp.  That's also wrong; see the big comment
    in error.h.  Error recovery can leave *errp pointing to a freed
    Error object.  Wrong, it must be null on success.  Messed up in
    commit 3eb99edb48 "loader-fit: Wean off error_printf()".

    No caller actually passes such values, or uses *errp on success.

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

    Signed-off-by: Markus Armbruster <armbru@redhat.com>

Okay?

>
>> ---
>>   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;
>>       }
>>   
>> 
>
> So much attention to that function :)
>
> I'd also propose the following:
>
> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
> index c465921b8f..2c9efeef7a 100644
> --- a/hw/core/loader-fit.c
> +++ b/hw/core/loader-fit.c
> @@ -180,8 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>   {
>       Error *err = NULL;
>       const char *name;
> -    const void *data;
> -    const void *load_data;
> +    void *data;
> +    void *load_data;
>       hwaddr load_addr;
>       int img_off;
>       size_t sz;
> @@ -218,9 +218,9 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>
>       ret = 0;
>   out:
> -    g_free((void *) data);
> +    g_free(data);
>       if (data != load_data) {
> -        g_free((void *) load_data);
> +        g_free(load_data);
>       }
>       return ret;
>   }
>
>
> Or, even better:
>
> --- a/hw/core/loader-fit.c
> +++ b/hw/core/loader-fit.c
> @@ -180,7 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>   {
>       Error *err = NULL;
>       const char *name;
> -    const void *data;
> +    g_autofree void *data = NULL;
> +    g_autofree void *fdt_filter_data = NULL;
>       const void *load_data;
>       hwaddr load_addr;
>       int img_off;
> @@ -202,27 +203,23 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>       if (ret == -ENOENT) {
>           load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>           error_free(err);
> +        return 0;
>       } else if (ret) {
>           error_propagate_prepend(errp, err,
>                                   "unable to read FDT load address from FIT: ");
> -        goto out;
> +        return ret;
>       }
>
>       if (ldr->fdt_filter) {
> -        load_data = ldr->fdt_filter(opaque, data, match_data, &load_addr);
> +        load_data = fdt_filter_data =
> +                ldr->fdt_filter(opaque, data, match_data, &load_addr);
>       }
>
>       load_addr = ldr->addr_to_phys(opaque, load_addr);
>       sz = fdt_totalsize(load_data);
>       rom_add_blob_fixed(name, load_data, sz, load_addr);
>
> -    ret = 0;
> -out:
> -    g_free((void *) data);
> -    if (data != load_data) {
> -        g_free((void *) load_data);
> -    }
> -    return ret;
> +    return 0;
>   }

Looks like a sensible separate cleanup patch to me.  Care to post it?

Thanks!

Re: [PATCH 12/21] qga: Fix latent guest-get-fsinfo error handling bug

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 30.11.2019 22:42, Markus Armbruster wrote:
>> build_guest_fsinfo_for_virtual_device() crashes when
>> build_guest_fsinfo_for_device() fails and its @errp argument is null.
>> Messed up in commit 46d4c5723e "qga: Add guest-get-fsinfo command".
>> 
>> The bug can't bite as no caller actually passes null.  Fix it anyway.
>> 
>> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Actually, all such bugs should be fixed by my auto-generated series..

I see.  I didn't consider that.

One advantage of my manual fixing is a clearer record of the flaws in
git.  It should also keep your work simpler, which is always a good idea
for massive, mechanical patches.

> And, if fixing by hand, it may be better to teach this function to return
> int, than propagation is not needed.

I went for the minimal fix.

I believe returning something useful is better.  It also matches the
GError conventions.  Deviating from them in this point was a mistake.

Touching up functions to return something useful by hand is a lot of
work, though: functions have many returns, and we have many functions in
need of this touch-up.  Some clever Coccinellery might be able to pull
it off.  I haven't tried.

However, your clever "auto propagation" Coccinellery makes such a change
less compelling, because it hides the propagation.

Thanks!

Re: [PATCH 10/21] hw/core: Fix latent fit_load_fdt() error handling bug

[Vladimir Sementsov-Ogievskiy]

06.12.2019 10:46, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 30.11.2019 22:42, Markus Armbruster wrote:
>>> fit_load_fdt() recovers from fit_image_addr() failing with ENOENT.
>>> Except it doesn't when its @errp argument is &error_fatal or
>>> &error_abort, because it blindly passes @errp to fit_image_addr().
>>> Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()".
>>>
>>> The bug can't bite as no caller actually passes &error_fatal or
>>> &error_abort.  Fix it anyway.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> Hmm, actually it makes my
>> "[PATCH v7 01/21] hw/core/loader-fit: fix freeing errp in fit_load_fdt"
>> unnecessary. If you want you may drop my 01 (as it covers less problems),
> 
> Yes.
> 
>> and in this case you may note in your cover letter, that
>> errp = NULL is broken here too (may be nobady pass it?),
> 
> You're right, null @errp is wrong, too.
> 
>> and normal errp is handled wrong, as *errp doesn't set to NULL after
>> error_free(*errp)
> 
> Yes, that's wrong, too.  fit_load_fdt() itself doesn't use *errp after
> freeing it, but it sets a trap for its callers.
> 
>>                    (still, the only caller rely on return value more than on
>> err, so the problem can't be triggered with current code)
> 
> True.
> 
> New commit message (based on v2's):
> 
>      hw/core: Fix fit_load_fdt() error API violations
> 
>      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.  Error
>      recovery dereferences @errp.  That's also wrong; see the big comment
>      in error.h.  Error recovery can leave *errp pointing to a freed
>      Error object.  Wrong, it must be null on success.  Messed up in
>      commit 3eb99edb48 "loader-fit: Wean off error_printf()".
> 
>      No caller actually passes such values, or uses *errp on success.
> 
>      Fix anyway: splice in a local Error *err, and error_propagate().
> 
>      Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> Okay?

Yes) also add
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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;
>>>        }
>>>    
>>>
>>
>> So much attention to that function :)
>>
>> I'd also propose the following:
>>
>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
>> index c465921b8f..2c9efeef7a 100644
>> --- a/hw/core/loader-fit.c
>> +++ b/hw/core/loader-fit.c
>> @@ -180,8 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>    {
>>        Error *err = NULL;
>>        const char *name;
>> -    const void *data;
>> -    const void *load_data;
>> +    void *data;
>> +    void *load_data;
>>        hwaddr load_addr;
>>        int img_off;
>>        size_t sz;
>> @@ -218,9 +218,9 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>
>>        ret = 0;
>>    out:
>> -    g_free((void *) data);
>> +    g_free(data);
>>        if (data != load_data) {
>> -        g_free((void *) load_data);
>> +        g_free(load_data);
>>        }
>>        return ret;
>>    }
>>
>>
>> Or, even better:
>>
>> --- a/hw/core/loader-fit.c
>> +++ b/hw/core/loader-fit.c
>> @@ -180,7 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>    {
>>        Error *err = NULL;
>>        const char *name;
>> -    const void *data;
>> +    g_autofree void *data = NULL;
>> +    g_autofree void *fdt_filter_data = NULL;
>>        const void *load_data;
>>        hwaddr load_addr;
>>        int img_off;
>> @@ -202,27 +203,23 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>        if (ret == -ENOENT) {
>>            load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>>            error_free(err);
>> +        return 0;
>>        } else if (ret) {
>>            error_propagate_prepend(errp, err,
>>                                    "unable to read FDT load address from FIT: ");
>> -        goto out;
>> +        return ret;
>>        }
>>
>>        if (ldr->fdt_filter) {
>> -        load_data = ldr->fdt_filter(opaque, data, match_data, &load_addr);
>> +        load_data = fdt_filter_data =
>> +                ldr->fdt_filter(opaque, data, match_data, &load_addr);
>>        }
>>
>>        load_addr = ldr->addr_to_phys(opaque, load_addr);
>>        sz = fdt_totalsize(load_data);
>>        rom_add_blob_fixed(name, load_data, sz, load_addr);
>>
>> -    ret = 0;
>> -out:
>> -    g_free((void *) data);
>> -    if (data != load_data) {
>> -        g_free((void *) load_data);
>> -    }
>> -    return ret;
>> +    return 0;
>>    }
> 
> Looks like a sensible separate cleanup patch to me.  Care to post it?
> 

Yes, I'll send



-- 
Best regards,
Vladimir

Re: [PATCH 11/21] hw/ipmi: Fix latent realize() error handling bugs

[Markus Armbruster]

Corey Minyard <cminyard@mvista.com> writes:

> On Sat, Nov 30, 2019 at 08:42:30PM +0100, Markus Armbruster wrote:
>> isa_ipmi_bt_realize(), ipmi_isa_realize(), pci_ipmi_bt_realize(), and
>> pci_ipmi_kcs_realize() crash when IPMIInterfaceClass method init()
>> fails and their @errp argument is null.  First messed up 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".
>> 
>> The bug can't bite as no caller actually passes null, and none of the
>> init() methods can actually fail.  Fix it anyway.
>
> Well, whatever.  It looks correct and is better style.  I've added this
> to my tree.

I've since posted v2 with a revamped commit message, and I'm ready to
post a pull request.  I really want the whole thing committed before the
Christmas break, so Vladimir can base on it more easily.  Options:

* You post a pull request before me.

* Ask me to drop this patch from my pull request, so you can take it
  through your tree at your leisure.

* Post your Reviewed-by or Acked-by for me to include in my pull
  request.

* Do nothing; I'll post my pull request later this week.

Re: [PATCH 11/21] hw/ipmi: Fix latent realize() error handling bugs

[Corey Minyard]

On Mon, Dec 16, 2019 at 10:20:04AM +0100, Markus Armbruster wrote:
> Corey Minyard <cminyard@mvista.com> writes:
> 
> I've since posted v2 with a revamped commit message, and I'm ready to
> post a pull request.  I really want the whole thing committed before the
> Christmas break, so Vladimir can base on it more easily.  Options:
> 
> * You post a pull request before me.
> 
> * Ask me to drop this patch from my pull request, so you can take it
>   through your tree at your leisure.
> 
> * Post your Reviewed-by or Acked-by for me to include in my pull
>   request.
> 
> * Do nothing; I'll post my pull request later this week.
> 

Since you already have it ready, I'll choose the "do nothing" patch
and let you post the pull request.  Thanks.

-corey

Re: [PATCH 11/21] hw/ipmi: Fix latent realize() error handling bugs

[Markus Armbruster]

Corey Minyard <cminyard@mvista.com> writes:

> On Mon, Dec 16, 2019 at 10:20:04AM +0100, Markus Armbruster wrote:
>> Corey Minyard <cminyard@mvista.com> writes:
>> 
>> I've since posted v2 with a revamped commit message, and I'm ready to
>> post a pull request.  I really want the whole thing committed before the
>> Christmas break, so Vladimir can base on it more easily.  Options:
>> 
>> * You post a pull request before me.
>> 
>> * Ask me to drop this patch from my pull request, so you can take it
>>   through your tree at your leisure.
>> 
>> * Post your Reviewed-by or Acked-by for me to include in my pull
>>   request.
>> 
>> * Do nothing; I'll post my pull request later this week.
>> 
>
> Since you already have it ready, I'll choose the "do nothing" patch
> and let you post the pull request.  Thanks.

Okay, sent.  Thanks for your quick reply!

Re: [PATCH 10/21] hw/core: Fix latent fit_load_fdt() error handling bug

[Vladimir Sementsov-Ogievskiy]

06.12.2019 13:53, Vladimir Sementsov-Ogievskiy wrote:
> 06.12.2019 10:46, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> 30.11.2019 22:42, Markus Armbruster wrote:
>>>> fit_load_fdt() recovers from fit_image_addr() failing with ENOENT.
>>>> Except it doesn't when its @errp argument is &error_fatal or
>>>> &error_abort, because it blindly passes @errp to fit_image_addr().
>>>> Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()".
>>>>
>>>> The bug can't bite as no caller actually passes &error_fatal or
>>>> &error_abort.  Fix it anyway.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>> Hmm, actually it makes my
>>> "[PATCH v7 01/21] hw/core/loader-fit: fix freeing errp in fit_load_fdt"
>>> unnecessary. If you want you may drop my 01 (as it covers less problems),
>>
>> Yes.
>>
>>> and in this case you may note in your cover letter, that
>>> errp = NULL is broken here too (may be nobady pass it?),
>>
>> You're right, null @errp is wrong, too.
>>
>>> and normal errp is handled wrong, as *errp doesn't set to NULL after
>>> error_free(*errp)
>>
>> Yes, that's wrong, too.  fit_load_fdt() itself doesn't use *errp after
>> freeing it, but it sets a trap for its callers.
>>
>>>                    (still, the only caller rely on return value more than on
>>> err, so the problem can't be triggered with current code)
>>
>> True.
>>
>> New commit message (based on v2's):
>>
>>      hw/core: Fix fit_load_fdt() error API violations
>>
>>      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.  Error
>>      recovery dereferences @errp.  That's also wrong; see the big comment
>>      in error.h.  Error recovery can leave *errp pointing to a freed
>>      Error object.  Wrong, it must be null on success.  Messed up in
>>      commit 3eb99edb48 "loader-fit: Wean off error_printf()".
>>
>>      No caller actually passes such values, or uses *errp on success.
>>
>>      Fix anyway: splice in a local Error *err, and error_propagate().
>>
>>      Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> Okay?
> 
> Yes) also add
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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;
>>>>        }
>>>>
>>>
>>> So much attention to that function :)
>>>
>>> I'd also propose the following:
>>>
>>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
>>> index c465921b8f..2c9efeef7a 100644
>>> --- a/hw/core/loader-fit.c
>>> +++ b/hw/core/loader-fit.c
>>> @@ -180,8 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>    {
>>>        Error *err = NULL;
>>>        const char *name;
>>> -    const void *data;
>>> -    const void *load_data;
>>> +    void *data;
>>> +    void *load_data;
>>>        hwaddr load_addr;
>>>        int img_off;
>>>        size_t sz;
>>> @@ -218,9 +218,9 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>
>>>        ret = 0;
>>>    out:
>>> -    g_free((void *) data);
>>> +    g_free(data);
>>>        if (data != load_data) {
>>> -        g_free((void *) load_data);
>>> +        g_free(load_data);
>>>        }
>>>        return ret;
>>>    }
>>>
>>>
>>> Or, even better:
>>>
>>> --- a/hw/core/loader-fit.c
>>> +++ b/hw/core/loader-fit.c
>>> @@ -180,7 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>    {
>>>        Error *err = NULL;
>>>        const char *name;
>>> -    const void *data;
>>> +    g_autofree void *data = NULL;
>>> +    g_autofree void *fdt_filter_data = NULL;
>>>        const void *load_data;
>>>        hwaddr load_addr;
>>>        int img_off;
>>> @@ -202,27 +203,23 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>        if (ret == -ENOENT) {
>>>            load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>>>            error_free(err);
>>> +        return 0;
>>>        } else if (ret) {
>>>            error_propagate_prepend(errp, err,
>>>                                    "unable to read FDT load address from FIT: ");
>>> -        goto out;
>>> +        return ret;
>>>        }
>>>
>>>        if (ldr->fdt_filter) {
>>> -        load_data = ldr->fdt_filter(opaque, data, match_data, &load_addr);
>>> +        load_data = fdt_filter_data =
>>> +                ldr->fdt_filter(opaque, data, match_data, &load_addr);
>>>        }
>>>
>>>        load_addr = ldr->addr_to_phys(opaque, load_addr);
>>>        sz = fdt_totalsize(load_data);
>>>        rom_add_blob_fixed(name, load_data, sz, load_addr);
>>>
>>> -    ret = 0;
>>> -out:
>>> -    g_free((void *) data);
>>> -    if (data != load_data) {
>>> -        g_free((void *) load_data);
>>> -    }
>>> -    return ret;
>>> +    return 0;
>>>    }
>>
>> Looks like a sensible separate cleanup patch to me.  Care to post it?
>>
> 
> Yes, I'll send
> 


Hm, it doesn't compile, as fit_load_image_alloc return type is const pointer..
So, I just don't understand the logic of the code (for me it's strange to
free pointer, returned by function, returning const pointer)

And what are the reasons, to make return type of fit_load_image_alloc constant,
I don't see.. IMHO, there are no reasons.
Same thing with .fdt_filter ...

I don't want to dig in, so, sorry, I'll not send :)


-- 
Best regards,
Vladimir

Re: [PATCH 10/21] hw/core: Fix latent fit_load_fdt() error handling bug

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 06.12.2019 13:53, Vladimir Sementsov-Ogievskiy wrote:
>> 06.12.2019 10:46, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
[...]
>>>> So much attention to that function :)
>>>>
>>>> I'd also propose the following:
[...]
>>>
>>> Looks like a sensible separate cleanup patch to me.  Care to post it?
>>>
>> 
>> Yes, I'll send
>> 
>
>
> Hm, it doesn't compile, as fit_load_image_alloc return type is const pointer..
> So, I just don't understand the logic of the code (for me it's strange to
> free pointer, returned by function, returning const pointer)
>
> And what are the reasons, to make return type of fit_load_image_alloc constant,
> I don't see.. IMHO, there are no reasons.
> Same thing with .fdt_filter ...
>
> I don't want to dig in, so, sorry, I'll not send :)

That's okay.