[PATCH 00/11] Miscellaneous error handling fixes

[PATCH 00/11] Miscellaneous error handling fixes

[Markus Armbruster]

Maintainers decide whether any of these are serious enough to still go
into 5.0.  I doubt it.

Markus Armbruster (11):
  cryptodev: Fix cryptodev_builtin_cleanup() error API violation
  block/file-posix: Fix check_cache_dropped() error handling
  cpus: Fix configure_icount() error API violation
  cpus: Proper range-checking for -icount shift=N
  arm/virt: Fix virt_machine_device_plug_cb() error API violation
  fdc: Fix fallback=auto error handling
  bochs-display: Fix vgamem=SIZE error handling
  virtio-net: Fix duplex=... and speed=... error handling
  xen/pt: Fix flawed conversion to realize()
  io: Fix qio_channel_socket_close() error handling
  migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling

 backends/cryptodev-builtin.c | 10 ++-----
 block/file-posix.c           |  5 +++-
 cpus.c                       | 52 +++++++++++++++++++++---------------
 hw/arm/virt.c                |  4 +--
 hw/block/fdc.c               |  1 +
 hw/display/bochs-display.c   |  6 +++--
 hw/net/virtio-net.c          |  5 +++-
 hw/xen/xen_pt.c              | 12 ++++-----
 io/channel-socket.c          |  5 ++--
 migration/colo.c             |  8 +++++-
 10 files changed, 63 insertions(+), 45 deletions(-)

-- 
2.21.1

[PATCH 01/11] cryptodev: Fix cryptodev_builtin_cleanup() error API violation

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

cryptodev_builtin_cleanup() passes @errp to
cryptodev_builtin_sym_close_session() in a loop.  Harmless, because
cryptodev_builtin_sym_close_session() can't actually fail.  Fix it
anyway.

Cc: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 backends/cryptodev-builtin.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c
index c8ae3b9742..14316333fe 100644
--- a/backends/cryptodev-builtin.c
+++ b/backends/cryptodev-builtin.c
@@ -282,12 +282,7 @@ static int cryptodev_builtin_sym_close_session(
     CryptoDevBackendBuiltin *builtin =
                       CRYPTODEV_BACKEND_BUILTIN(backend);
 
-    if (session_id >= MAX_NUM_SESSIONS ||
-              builtin->sessions[session_id] == NULL) {
-        error_setg(errp, "Cannot find a valid session id: %" PRIu64 "",
-                      session_id);
-        return -1;
-    }
+    assert(session_id < MAX_NUM_SESSIONS && builtin->sessions[session_id]);
 
     qcrypto_cipher_free(builtin->sessions[session_id]->cipher);
     g_free(builtin->sessions[session_id]);
@@ -356,8 +351,7 @@ static void cryptodev_builtin_cleanup(
 
     for (i = 0; i < MAX_NUM_SESSIONS; i++) {
         if (builtin->sessions[i] != NULL) {
-            cryptodev_builtin_sym_close_session(
-                    backend, i, 0, errp);
+            cryptodev_builtin_sym_close_session(backend, i, 0, &error_abort);
         }
     }
 
-- 
2.21.1

[PATCH 02/11] block/file-posix: Fix check_cache_dropped() error handling

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

check_cache_dropped() calls error_setg() in a loop.  It fails to break
the loop in one instance.  If a subsequent iteration error_setg()s
again, it trips error_setv()'s assertion.

Fix it to break the loop.

Fixes: 31be8a2a97ecba7d31a82932286489cac318e9e9
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/file-posix.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 7e19bbff5f..094e3b0212 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2691,10 +2691,13 @@ static void check_cache_dropped(BlockDriverState *bs, Error **errp)
         vec_end = DIV_ROUND_UP(length, page_size);
         for (i = 0; i < vec_end; i++) {
             if (vec[i] & 0x1) {
-                error_setg(errp, "page cache still in use!");
                 break;
             }
         }
+        if (i < vec_end) {
+            error_setg(errp, "page cache still in use!");
+            break;
+        }
     }
 
     if (window) {
-- 
2.21.1

[PATCH 03/11] cpus: Fix configure_icount() error API violation

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

configure_icount() is wrong that way.  Harmless, because its @errp is
always &error_abort or &error_fatal.

Just as wrong (and just as harmless): when it fails, it can still
update global state.

Fix all that.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 cpus.c | 51 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/cpus.c b/cpus.c
index ef441bdf62..1b542b37f9 100644
--- a/cpus.c
+++ b/cpus.c
@@ -797,40 +797,49 @@ void cpu_ticks_init(void)
 
 void configure_icount(QemuOpts *opts, Error **errp)
 {
-    const char *option;
+    const char *option = qemu_opt_get(opts, "shift");
+    bool sleep = qemu_opt_get_bool(opts, "sleep", true);
+    bool align = qemu_opt_get_bool(opts, "align", false);
+    long time_shift = -1;
     char *rem_str = NULL;
 
-    option = qemu_opt_get(opts, "shift");
-    if (!option) {
-        if (qemu_opt_get(opts, "align") != NULL) {
-            error_setg(errp, "Please specify shift option when using align");
-        }
+    if (!option && qemu_opt_get(opts, "align")) {
+        error_setg(errp, "Please specify shift option when using align");
         return;
     }
 
-    icount_sleep = qemu_opt_get_bool(opts, "sleep", true);
+    if (align && !sleep) {
+        error_setg(errp, "align=on and sleep=off are incompatible");
+        return;
+    }
+
+    if (strcmp(option, "auto") != 0) {
+        errno = 0;
+        time_shift = strtol(option, &rem_str, 0);
+        if (errno != 0 || *rem_str != '\0' || !strlen(option)) {
+            error_setg(errp, "icount: Invalid shift value");
+            return;
+        }
+    } else if (icount_align_option) {
+        error_setg(errp, "shift=auto and align=on are incompatible");
+        return;
+    } else if (!icount_sleep) {
+        error_setg(errp, "shift=auto and sleep=off are incompatible");
+        return;
+    }
+
+    icount_sleep = sleep;
     if (icount_sleep) {
         timers_state.icount_warp_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
                                          icount_timer_cb, NULL);
     }
 
-    icount_align_option = qemu_opt_get_bool(opts, "align", false);
+    icount_align_option = align;
 
-    if (icount_align_option && !icount_sleep) {
-        error_setg(errp, "align=on and sleep=off are incompatible");
-    }
-    if (strcmp(option, "auto") != 0) {
-        errno = 0;
-        timers_state.icount_time_shift = strtol(option, &rem_str, 0);
-        if (errno != 0 || *rem_str != '\0' || !strlen(option)) {
-            error_setg(errp, "icount: Invalid shift value");
-        }
+    if (time_shift >= 0) {
+        timers_state.icount_time_shift = time_shift;
         use_icount = 1;
         return;
-    } else if (icount_align_option) {
-        error_setg(errp, "shift=auto and align=on are incompatible");
-    } else if (!icount_sleep) {
-        error_setg(errp, "shift=auto and sleep=off are incompatible");
     }
 
     use_icount = 2;
-- 
2.21.1

[PATCH 04/11] cpus: Proper range-checking for -icount shift=N

[Markus Armbruster]

timers_state.icount_time_shift must be in [0,63] to avoid undefined
behavior when shifting by it, e.g. in cpu_icount_to_ns().
icount_adjust() clamps it to [0,MAX_ICOUNT_SHIFT], with
MAX_ICOUNT_SHIFT = 10.  configure_icount() doesn't.  Fix that.

Fixes: a8bfac37085c3372366d722f131a7e18d664ee4d
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 cpus.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/cpus.c b/cpus.c
index 1b542b37f9..5670c96bcf 100644
--- a/cpus.c
+++ b/cpus.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/config-file.h"
+#include "qemu/cutils.h"
 #include "migration/vmstate.h"
 #include "monitor/monitor.h"
 #include "qapi/error.h"
@@ -801,7 +802,6 @@ void configure_icount(QemuOpts *opts, Error **errp)
     bool sleep = qemu_opt_get_bool(opts, "sleep", true);
     bool align = qemu_opt_get_bool(opts, "align", false);
     long time_shift = -1;
-    char *rem_str = NULL;
 
     if (!option && qemu_opt_get(opts, "align")) {
         error_setg(errp, "Please specify shift option when using align");
@@ -814,9 +814,8 @@ void configure_icount(QemuOpts *opts, Error **errp)
     }
 
     if (strcmp(option, "auto") != 0) {
-        errno = 0;
-        time_shift = strtol(option, &rem_str, 0);
-        if (errno != 0 || *rem_str != '\0' || !strlen(option)) {
+        if (qemu_strtol(option, NULL, 0, &time_shift) < 0
+            || time_shift < 0 || time_shift > MAX_ICOUNT_SHIFT) {
             error_setg(errp, "icount: Invalid shift value");
             return;
         }
-- 
2.21.1

[PATCH 05/11] arm/virt: Fix virt_machine_device_plug_cb() error API violation

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

virt_machine_device_plug_cb() passes @errp to
cryptodev_builtin_sym_close_session() in a loop.  Harmless, because
cryptodev_builtin_sym_close_session() can't actually fail.  Fix by
dropping its Error ** parameter.

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

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7dc96abf72..cca5316256 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1186,7 +1186,7 @@ static void create_smmu(const VirtMachineState *vms,
     g_free(node);
 }
 
-static void create_virtio_iommu_dt_bindings(VirtMachineState *vms, Error **errp)
+static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
 {
     const char compat[] = "virtio,pci-iommu";
     uint16_t bdf = vms->virtio_iommu_bdf;
@@ -2118,7 +2118,7 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
 
         vms->iommu = VIRT_IOMMU_VIRTIO;
         vms->virtio_iommu_bdf = pci_get_bdf(pdev);
-        create_virtio_iommu_dt_bindings(vms, errp);
+        create_virtio_iommu_dt_bindings(vms);
     }
 }
 
-- 
2.21.1

[PATCH 06/11] fdc: Fix fallback=auto error handling

[Markus Armbruster]

fdctrl_realize_common() rejects fallback=auto.  Used by devices
"isa-fdc", "sysbus-fdc", "SUNW,fdtwo".  The error handling is broken:

    $ qemu-system-x86_64 -nodefaults -device isa-fdc,fallback=auto,driveA=fd0 -drive if=none,id=fd0
    **
    ERROR:/work/armbru/qemu/hw/block/fdc.c:434:pick_drive_type: assertion failed: (drv->drive != FLOPPY_DRIVE_TYPE_AUTO)
    Aborted (core dumped)

Cause: fdctrl_realize_common() neglects to bail out after setting the
error.  Fix that.

Fixes: a73275dd6fc3bfda33165bebc28e0c33c20cb0a0
Cc: John Snow <jsnow@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/block/fdc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 33bc9e2f92..9628cc171e 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2615,6 +2615,7 @@ static void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl,
 
     if (fdctrl->fallback == FLOPPY_DRIVE_TYPE_AUTO) {
         error_setg(errp, "Cannot choose a fallback FDrive type of 'auto'");
+        return;
     }
 
     /* Fill 'command_to_handler' lookup table */
-- 
2.21.1

[PATCH 07/11] bochs-display: Fix vgamem=SIZE error handling

[Markus Armbruster]

bochs_display_realize() rejects out-of-range vgamem.  The error
handling is broken:

    $ qemu-system-x86_64 -S -display none -monitor stdio
    QEMU 4.2.93 monitor - type 'help' for more information
    (qemu) device_add bochs-display,vgamem=1
    Error: bochs-display: video memory too small
    (qemu) device_add bochs-display,vgamem=1
    RAMBlock "0000:00:04.0/bochs-display-vram" already registered, abort!
    Aborted (core dumped)

Cause: bochs_display_realize() neglects to bail out after setting the
error.  Fix that.

Fixes: 765c94290863eef1fc4a67819d452cc13b7854a1
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/display/bochs-display.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
index 70eb619ef4..e763a0a72d 100644
--- a/hw/display/bochs-display.c
+++ b/hw/display/bochs-display.c
@@ -267,16 +267,18 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp)
     Object *obj = OBJECT(dev);
     int ret;
 
-    s->con = graphic_console_init(DEVICE(dev), 0, &bochs_display_gfx_ops, s);
-
     if (s->vgamem < 4 * MiB) {
         error_setg(errp, "bochs-display: video memory too small");
+        return;
     }
     if (s->vgamem > 256 * MiB) {
         error_setg(errp, "bochs-display: video memory too big");
+        return;
     }
     s->vgamem = pow2ceil(s->vgamem);
 
+    s->con = graphic_console_init(DEVICE(dev), 0, &bochs_display_gfx_ops, s);
+
     memory_region_init_ram(&s->vram, obj, "bochs-display-vram", s->vgamem,
                            &error_fatal);
     memory_region_init_io(&s->vbe, obj, &bochs_display_vbe_ops, s,
-- 
2.21.1

[PATCH 08/11] virtio-net: Fix duplex=... and speed=... error handling

[Markus Armbruster]

virtio_net_device_realize() rejects invalid duplex and speed values.
The error handling is broken:

    $ ../qemu/bld-sani/x86_64-softmmu/qemu-system-x86_64 -S -display none -monitor stdio
    QEMU 4.2.93 monitor - type 'help' for more information
    (qemu) device_add virtio-net,duplex=x
    Error: 'duplex' must be 'half' or 'full'
    (qemu) c
    =================================================================
    ==15654==ERROR: AddressSanitizer: heap-use-after-free on address 0x62e000014590 at pc 0x560b75c8dc13 bp 0x7fffdf1a6950 sp 0x7fffdf1a6940
    READ of size 8 at 0x62e000014590 thread T0
	#0 0x560b75c8dc12 in object_dynamic_cast_assert /work/armbru/qemu/qom/object.c:826
	#1 0x560b74c38ac0 in virtio_vmstate_change /work/armbru/qemu/hw/virtio/virtio.c:3210
	#2 0x560b74d9765e in vm_state_notify /work/armbru/qemu/softmmu/vl.c:1271
	#3 0x560b7494ba72 in vm_prepare_start /work/armbru/qemu/cpus.c:2156
	#4 0x560b7494bacd in vm_start /work/armbru/qemu/cpus.c:2162
	#5 0x560b75a7d890 in qmp_cont /work/armbru/qemu/monitor/qmp-cmds.c:160
	#6 0x560b75a8d70a in hmp_cont /work/armbru/qemu/monitor/hmp-cmds.c:1043
	#7 0x560b75a799f2 in handle_hmp_command /work/armbru/qemu/monitor/hmp.c:1082
    [...]

    0x62e000014590 is located 33168 bytes inside of 42288-byte region [0x62e00000c400,0x62e000016930)
    freed by thread T1 here:
	#0 0x7feadd39491f in __interceptor_free (/lib64/libasan.so.5+0x10d91f)
	#1 0x7feadcebcd7c in g_free (/lib64/libglib-2.0.so.0+0x55d7c)
	#2 0x560b75c8fd40 in object_unref /work/armbru/qemu/qom/object.c:1128
	#3 0x560b7498a625 in memory_region_unref /work/armbru/qemu/memory.c:1762
	#4 0x560b74999fa4 in do_address_space_destroy /work/armbru/qemu/memory.c:2788
	#5 0x560b762362fc in call_rcu_thread /work/armbru/qemu/util/rcu.c:283
	#6 0x560b761c8884 in qemu_thread_start /work/armbru/qemu/util/qemu-thread-posix.c:519
	#7 0x7fead9be34bf in start_thread (/lib64/libpthread.so.0+0x84bf)

    previously allocated by thread T0 here:
	#0 0x7feadd394d18 in __interceptor_malloc (/lib64/libasan.so.5+0x10dd18)
	#1 0x7feadcebcc88 in g_malloc (/lib64/libglib-2.0.so.0+0x55c88)
	#2 0x560b75c8cf8a in object_new /work/armbru/qemu/qom/object.c:699
	#3 0x560b75010ad9 in qdev_device_add /work/armbru/qemu/qdev-monitor.c:654
	#4 0x560b750120c2 in qmp_device_add /work/armbru/qemu/qdev-monitor.c:805
	#5 0x560b75012c1b in hmp_device_add /work/armbru/qemu/qdev-monitor.c:905
    [...]
    ==15654==ABORTING

Cause: virtio_net_device_realize() neglects to bail out after setting
the error.  Fix that.

Fixes: 9473939ed7addcaaeb8fde5c093918fb7fa0919c
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/net/virtio-net.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index a46e3b37a7..b52ff4ab63 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2947,6 +2947,7 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
             n->net_conf.duplex = DUPLEX_FULL;
         } else {
             error_setg(errp, "'duplex' must be 'half' or 'full'");
+            return;
         }
         n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
     } else {
@@ -2955,7 +2956,9 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
 
     if (n->net_conf.speed < SPEED_UNKNOWN) {
         error_setg(errp, "'speed' must be between 0 and INT_MAX");
-    } else if (n->net_conf.speed >= 0) {
+        return;
+    }
+    if (n->net_conf.speed >= 0) {
         n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
     }
 
-- 
2.21.1

[PATCH 09/11] xen/pt: Fix flawed conversion to realize()

[Markus Armbruster]

The conversion of xen_pt_initfn() to xen_pt_realize() blindly replaced
XEN_PT_ERR() by error_setg().  Several error conditions that did not
fail xen_pt_initfn() now fail xen_pt_realize().  Unsurprisingly, the
cleanup on these errors looks highly suspicious.

Revert the inappropriate replacements.

Fixes: 5a11d0f7549e24a10e178a9dc8ff5e698031d9a6
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Paul Durrant <paul@xen.org>
Cc: xen-devel@lists.xenproject.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/xen/xen_pt.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index b91082cb8b..81d5ad8da7 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -858,8 +858,8 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
 
     rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq);
     if (rc < 0) {
-        error_setg_errno(errp, errno, "Mapping machine irq %u to"
-                         " pirq %i failed", machine_irq, pirq);
+        XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (err: %d)\n",
+                   machine_irq, pirq, errno);
 
         /* Disable PCI intx assertion (turn on bit10 of devctl) */
         cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -880,8 +880,8 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
                                        PCI_SLOT(d->devfn),
                                        e_intx);
         if (rc < 0) {
-            error_setg_errno(errp, errno, "Binding of interrupt %u failed",
-                             e_intx);
+            XEN_PT_ERR(d, "Binding of interrupt %i failed! (err: %d)\n",
+                       e_intx, errno);
 
             /* Disable PCI intx assertion (turn on bit10 of devctl) */
             cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -889,8 +889,8 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
 
             if (xen_pt_mapped_machine_irq[machine_irq] == 0) {
                 if (xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq)) {
-                    error_setg_errno(errp, errno, "Unmapping of machine"
-                            " interrupt %u failed", machine_irq);
+                    XEN_PT_ERR(d, "Unmapping of machine interrupt %i failed!"
+                               " (err: %d)\n", machine_irq, errno);
                 }
             }
             s->machine_irq = 0;
-- 
2.21.1

[PATCH 10/11] io: Fix qio_channel_socket_close() error handling

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

qio_channel_socket_close() passes @errp first to
socket_listen_cleanup(), and then, if closesocket() fails, to
error_setg_errno().  If socket_listen_cleanup() failed, this will trip
the assertion in error_setv().

Fix by ignoring a second error.

Fixes: 73564c407caedf992a1c688b5fea776a8b56ba2a
Cc: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 io/channel-socket.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index b74f5b92a0..e1b4667087 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -704,6 +704,7 @@ qio_channel_socket_close(QIOChannel *ioc,
 {
     QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
     int rc = 0;
+    Error *err = NULL;
 
     if (sioc->fd != -1) {
 #ifdef WIN32
@@ -715,8 +716,8 @@ qio_channel_socket_close(QIOChannel *ioc,
 
         if (closesocket(sioc->fd) < 0) {
             sioc->fd = -1;
-            error_setg_errno(errp, errno,
-                             "Unable to close socket");
+            error_setg_errno(&err, errno, "Unable to close socket");
+            error_propagate(errp, err);
             return -1;
         }
         sioc->fd = -1;
-- 
2.21.1

[PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

qmp_xen_colo_do_checkpoint() passes @errp first to
replication_do_checkpoint_all(), and then to
colo_notify_filters_event().  If both fail, this will trip the
assertion in error_setv().

Similar code in secondary_vm_do_failover() calls
colo_notify_filters_event() only after replication_do_checkpoint_all()
succeeded.  Do the same here.

Fixes: 0e8818f023616677416840d6ddc880db8de3c967
Cc: Zhang Chen <chen.zhang@intel.com>
Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/colo.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/migration/colo.c b/migration/colo.c
index a54ac84f41..1b3493729b 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -263,7 +263,13 @@ ReplicationStatus *qmp_query_xen_replication_status(Error **errp)
 
 void qmp_xen_colo_do_checkpoint(Error **errp)
 {
-    replication_do_checkpoint_all(errp);
+    Error *err = NULL;
+
+    replication_do_checkpoint_all(&err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
     /* Notify all filters of all NIC to do checkpoint */
     colo_notify_filters_event(COLO_EVENT_CHECKPOINT, errp);
 }
-- 
2.21.1

RE: [PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling

[Zhanghailiang]

Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

> -----Original Message-----
> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Monday, April 20, 2020 4:33 PM
> To: qemu-devel@nongnu.org
> Cc: Zhang Chen <chen.zhang@intel.com>; Zhanghailiang
> <zhang.zhanghailiang@huawei.com>
> Subject: [PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint()
> error handling
> 
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> call.
> 
> qmp_xen_colo_do_checkpoint() passes @errp first to
> replication_do_checkpoint_all(), and then to
> colo_notify_filters_event().  If both fail, this will trip the
> assertion in error_setv().
> 
> Similar code in secondary_vm_do_failover() calls
> colo_notify_filters_event() only after replication_do_checkpoint_all()
> succeeded.  Do the same here.
> 
> Fixes: 0e8818f023616677416840d6ddc880db8de3c967
> Cc: Zhang Chen <chen.zhang@intel.com>
> Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  migration/colo.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c
> index a54ac84f41..1b3493729b 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -263,7 +263,13 @@ ReplicationStatus
> *qmp_query_xen_replication_status(Error **errp)
> 
>  void qmp_xen_colo_do_checkpoint(Error **errp)
>  {
> -    replication_do_checkpoint_all(errp);
> +    Error *err = NULL;
> +
> +    replication_do_checkpoint_all(&err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
>      /* Notify all filters of all NIC to do checkpoint */
>      colo_notify_filters_event(COLO_EVENT_CHECKPOINT, errp);
>  }
> --
> 2.21.1

Re: [PATCH 05/11] arm/virt: Fix virt_machine_device_plug_cb() error API violation

[Philippe Mathieu-Daudé]

On 4/20/20 10:32 AM, Markus Armbruster wrote:
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> call.
> 
> virt_machine_device_plug_cb() passes @errp to
> cryptodev_builtin_sym_close_session() in a loop.  Harmless, because
> cryptodev_builtin_sym_close_session() can't actually fail.  Fix by
> dropping its Error ** parameter.
> 
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

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

> ---
>   hw/arm/virt.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 7dc96abf72..cca5316256 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1186,7 +1186,7 @@ static void create_smmu(const VirtMachineState *vms,
>       g_free(node);
>   }
>   
> -static void create_virtio_iommu_dt_bindings(VirtMachineState *vms, Error **errp)
> +static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
>   {
>       const char compat[] = "virtio,pci-iommu";
>       uint16_t bdf = vms->virtio_iommu_bdf;
> @@ -2118,7 +2118,7 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>   
>           vms->iommu = VIRT_IOMMU_VIRTIO;
>           vms->virtio_iommu_bdf = pci_get_bdf(pdev);
> -        create_virtio_iommu_dt_bindings(vms, errp);
> +        create_virtio_iommu_dt_bindings(vms);
>       }
>   }
>   
> 

Re: [PATCH 06/11] fdc: Fix fallback=auto error handling

[Philippe Mathieu-Daudé]

On 4/20/20 10:32 AM, Markus Armbruster wrote:
> fdctrl_realize_common() rejects fallback=auto.  Used by devices
> "isa-fdc", "sysbus-fdc", "SUNW,fdtwo".  The error handling is broken:
> 
>      $ qemu-system-x86_64 -nodefaults -device isa-fdc,fallback=auto,driveA=fd0 -drive if=none,id=fd0
>      **
>      ERROR:/work/armbru/qemu/hw/block/fdc.c:434:pick_drive_type: assertion failed: (drv->drive != FLOPPY_DRIVE_TYPE_AUTO)
>      Aborted (core dumped)
> 
> Cause: fdctrl_realize_common() neglects to bail out after setting the
> error.  Fix that.
> 
> Fixes: a73275dd6fc3bfda33165bebc28e0c33c20cb0a0
> Cc: John Snow <jsnow@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/block/fdc.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 33bc9e2f92..9628cc171e 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2615,6 +2615,7 @@ static void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl,
>   
>       if (fdctrl->fallback == FLOPPY_DRIVE_TYPE_AUTO) {
>           error_setg(errp, "Cannot choose a fallback FDrive type of 'auto'");
> +        return;
>       }
>   
>       /* Fill 'command_to_handler' lookup table */
> 

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

Re: [PATCH 07/11] bochs-display: Fix vgamem=SIZE error handling

[Philippe Mathieu-Daudé]

On 4/20/20 10:32 AM, Markus Armbruster wrote:
> bochs_display_realize() rejects out-of-range vgamem.  The error
> handling is broken:
> 
>      $ qemu-system-x86_64 -S -display none -monitor stdio
>      QEMU 4.2.93 monitor - type 'help' for more information
>      (qemu) device_add bochs-display,vgamem=1
>      Error: bochs-display: video memory too small
>      (qemu) device_add bochs-display,vgamem=1
>      RAMBlock "0000:00:04.0/bochs-display-vram" already registered, abort!
>      Aborted (core dumped)
> 
> Cause: bochs_display_realize() neglects to bail out after setting the
> error.  Fix that.
> 
> Fixes: 765c94290863eef1fc4a67819d452cc13b7854a1
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/display/bochs-display.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
> index 70eb619ef4..e763a0a72d 100644
> --- a/hw/display/bochs-display.c
> +++ b/hw/display/bochs-display.c
> @@ -267,16 +267,18 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp)
>       Object *obj = OBJECT(dev);
>       int ret;
>   
> -    s->con = graphic_console_init(DEVICE(dev), 0, &bochs_display_gfx_ops, s);
> -
>       if (s->vgamem < 4 * MiB) {
>           error_setg(errp, "bochs-display: video memory too small");
> +        return;
>       }
>       if (s->vgamem > 256 * MiB) {
>           error_setg(errp, "bochs-display: video memory too big");
> +        return;
>       }
>       s->vgamem = pow2ceil(s->vgamem);
>   
> +    s->con = graphic_console_init(DEVICE(dev), 0, &bochs_display_gfx_ops, s);
> +
>       memory_region_init_ram(&s->vram, obj, "bochs-display-vram", s->vgamem,
>                              &error_fatal);
>       memory_region_init_io(&s->vbe, obj, &bochs_display_vbe_ops, s,
> 

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

Re: [PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling

[Philippe Mathieu-Daudé]

On 4/20/20 10:32 AM, Markus Armbruster wrote:
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> call.
> 
> qmp_xen_colo_do_checkpoint() passes @errp first to
> replication_do_checkpoint_all(), and then to
> colo_notify_filters_event().  If both fail, this will trip the
> assertion in error_setv().
> 
> Similar code in secondary_vm_do_failover() calls
> colo_notify_filters_event() only after replication_do_checkpoint_all()
> succeeded.  Do the same here.
> 
> Fixes: 0e8818f023616677416840d6ddc880db8de3c967
> Cc: Zhang Chen <chen.zhang@intel.com>
> Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   migration/colo.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c
> index a54ac84f41..1b3493729b 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -263,7 +263,13 @@ ReplicationStatus *qmp_query_xen_replication_status(Error **errp)
>   
>   void qmp_xen_colo_do_checkpoint(Error **errp)
>   {
> -    replication_do_checkpoint_all(errp);
> +    Error *err = NULL;
> +
> +    replication_do_checkpoint_all(&err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
>       /* Notify all filters of all NIC to do checkpoint */
>       colo_notify_filters_event(COLO_EVENT_CHECKPOINT, errp);
>   }
> 

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

RE: [PATCH 09/11] xen/pt: Fix flawed conversion to realize()

[Paul Durrant]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: 20 April 2020 09:33
> To: qemu-devel@nongnu.org
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Anthony Perard <anthony.perard@citrix.com>; Paul
> Durrant <paul@xen.org>; xen-devel@lists.xenproject.org
> Subject: [PATCH 09/11] xen/pt: Fix flawed conversion to realize()
> 
> The conversion of xen_pt_initfn() to xen_pt_realize() blindly replaced
> XEN_PT_ERR() by error_setg().  Several error conditions that did not
> fail xen_pt_initfn() now fail xen_pt_realize().  Unsurprisingly, the
> cleanup on these errors looks highly suspicious.
> 
> Revert the inappropriate replacements.
> 
> Fixes: 5a11d0f7549e24a10e178a9dc8ff5e698031d9a6
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Paul Durrant <paul@xen.org>

Re: [PATCH 08/11] virtio-net: Fix duplex=... and speed=... error handling

[Philippe Mathieu-Daudé]

On 4/20/20 10:32 AM, Markus Armbruster wrote:
> virtio_net_device_realize() rejects invalid duplex and speed values.
> The error handling is broken:
> 
>      $ ../qemu/bld-sani/x86_64-softmmu/qemu-system-x86_64 -S -display none -monitor stdio
>      QEMU 4.2.93 monitor - type 'help' for more information
>      (qemu) device_add virtio-net,duplex=x
>      Error: 'duplex' must be 'half' or 'full'
>      (qemu) c
>      =================================================================
>      ==15654==ERROR: AddressSanitizer: heap-use-after-free on address 0x62e000014590 at pc 0x560b75c8dc13 bp 0x7fffdf1a6950 sp 0x7fffdf1a6940
>      READ of size 8 at 0x62e000014590 thread T0
> 	#0 0x560b75c8dc12 in object_dynamic_cast_assert /work/armbru/qemu/qom/object.c:826
> 	#1 0x560b74c38ac0 in virtio_vmstate_change /work/armbru/qemu/hw/virtio/virtio.c:3210
> 	#2 0x560b74d9765e in vm_state_notify /work/armbru/qemu/softmmu/vl.c:1271
> 	#3 0x560b7494ba72 in vm_prepare_start /work/armbru/qemu/cpus.c:2156
> 	#4 0x560b7494bacd in vm_start /work/armbru/qemu/cpus.c:2162
> 	#5 0x560b75a7d890 in qmp_cont /work/armbru/qemu/monitor/qmp-cmds.c:160
> 	#6 0x560b75a8d70a in hmp_cont /work/armbru/qemu/monitor/hmp-cmds.c:1043
> 	#7 0x560b75a799f2 in handle_hmp_command /work/armbru/qemu/monitor/hmp.c:1082
>      [...]
> 
>      0x62e000014590 is located 33168 bytes inside of 42288-byte region [0x62e00000c400,0x62e000016930)
>      freed by thread T1 here:
> 	#0 0x7feadd39491f in __interceptor_free (/lib64/libasan.so.5+0x10d91f)
> 	#1 0x7feadcebcd7c in g_free (/lib64/libglib-2.0.so.0+0x55d7c)
> 	#2 0x560b75c8fd40 in object_unref /work/armbru/qemu/qom/object.c:1128
> 	#3 0x560b7498a625 in memory_region_unref /work/armbru/qemu/memory.c:1762
> 	#4 0x560b74999fa4 in do_address_space_destroy /work/armbru/qemu/memory.c:2788
> 	#5 0x560b762362fc in call_rcu_thread /work/armbru/qemu/util/rcu.c:283
> 	#6 0x560b761c8884 in qemu_thread_start /work/armbru/qemu/util/qemu-thread-posix.c:519
> 	#7 0x7fead9be34bf in start_thread (/lib64/libpthread.so.0+0x84bf)
> 
>      previously allocated by thread T0 here:
> 	#0 0x7feadd394d18 in __interceptor_malloc (/lib64/libasan.so.5+0x10dd18)
> 	#1 0x7feadcebcc88 in g_malloc (/lib64/libglib-2.0.so.0+0x55c88)
> 	#2 0x560b75c8cf8a in object_new /work/armbru/qemu/qom/object.c:699
> 	#3 0x560b75010ad9 in qdev_device_add /work/armbru/qemu/qdev-monitor.c:654
> 	#4 0x560b750120c2 in qmp_device_add /work/armbru/qemu/qdev-monitor.c:805
> 	#5 0x560b75012c1b in hmp_device_add /work/armbru/qemu/qdev-monitor.c:905
>      [...]
>      ==15654==ABORTING
> 
> Cause: virtio_net_device_realize() neglects to bail out after setting
> the error.  Fix that.
> 
> Fixes: 9473939ed7addcaaeb8fde5c093918fb7fa0919c
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/net/virtio-net.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a46e3b37a7..b52ff4ab63 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2947,6 +2947,7 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>               n->net_conf.duplex = DUPLEX_FULL;
>           } else {
>               error_setg(errp, "'duplex' must be 'half' or 'full'");
> +            return;
>           }
>           n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
>       } else {
> @@ -2955,7 +2956,9 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>   
>       if (n->net_conf.speed < SPEED_UNKNOWN) {
>           error_setg(errp, "'speed' must be between 0 and INT_MAX");
> -    } else if (n->net_conf.speed >= 0) {
> +        return;
> +    }
> +    if (n->net_conf.speed >= 0) {
>           n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
>       }
>   
> 

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

Re: [PATCH 10/11] io: Fix qio_channel_socket_close() error handling

[Daniel P. Berrangé]

On Mon, Apr 20, 2020 at 10:32:35AM +0200, Markus Armbruster wrote:
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> call.
> 
> qio_channel_socket_close() passes @errp first to
> socket_listen_cleanup(), and then, if closesocket() fails, to
> error_setg_errno().  If socket_listen_cleanup() failed, this will trip
> the assertion in error_setv().
> 
> Fix by ignoring a second error.
> 
> Fixes: 73564c407caedf992a1c688b5fea776a8b56ba2a
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  io/channel-socket.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

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


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

Re: [PATCH 00/11] Miscellaneous error handling fixes

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200420083236.19309-1-armbru@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH 00/11] Miscellaneous error handling fixes
Message-id: 20200420083236.19309-1-armbru@redhat.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
942181b migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling
a8c2f29 io: Fix qio_channel_socket_close() error handling
cedb56e xen/pt: Fix flawed conversion to realize()
7173a0e virtio-net: Fix duplex=... and speed=... error handling
9c7afc3 bochs-display: Fix vgamem=SIZE error handling
f3ad7d4 fdc: Fix fallback=auto error handling
e484e15 arm/virt: Fix virt_machine_device_plug_cb() error API violation
1c54f10 cpus: Proper range-checking for -icount shift=N
0a69dc4 cpus: Fix configure_icount() error API violation
bafe5f5 block/file-posix: Fix check_cache_dropped() error handling
a690fa1 cryptodev: Fix cryptodev_builtin_cleanup() error API violation

=== OUTPUT BEGIN ===
1/11 Checking commit a690fa1dc425 (cryptodev: Fix cryptodev_builtin_cleanup() error API violation)
2/11 Checking commit bafe5f5e9888 (block/file-posix: Fix check_cache_dropped() error handling)
3/11 Checking commit 0a69dc45007c (cpus: Fix configure_icount() error API violation)
ERROR: consider using qemu_strtol in preference to strtol
#58: FILE: cpus.c:818:
+        time_shift = strtol(option, &rem_str, 0);

total: 1 errors, 0 warnings, 70 lines checked

Patch 3/11 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/11 Checking commit 1c54f102aed3 (cpus: Proper range-checking for -icount shift=N)
5/11 Checking commit e484e1536ed7 (arm/virt: Fix virt_machine_device_plug_cb() error API violation)
6/11 Checking commit f3ad7d444a66 (fdc: Fix fallback=auto error handling)
7/11 Checking commit 9c7afc3c57a4 (bochs-display: Fix vgamem=SIZE error handling)
8/11 Checking commit 7173a0e399fd (virtio-net: Fix duplex=... and speed=... error handling)
9/11 Checking commit cedb56ec3da3 (xen/pt: Fix flawed conversion to realize())
10/11 Checking commit a8c2f294abb2 (io: Fix qio_channel_socket_close() error handling)
11/11 Checking commit 942181b72505 (migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200420083236.19309-1-armbru@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 02/11] block/file-posix: Fix check_cache_dropped() error handling

[Eric Blake]

On 4/20/20 3:32 AM, Markus Armbruster wrote:
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> call.
> 
> check_cache_dropped() calls error_setg() in a loop.  It fails to break
> the loop in one instance.  If a subsequent iteration error_setg()s
> again, it trips error_setv()'s assertion.
> 
> Fix it to break the loop.
> 
> Fixes: 31be8a2a97ecba7d31a82932286489cac318e9e9
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block/file-posix.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)

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

Doesn't seem to be a fresh regression in 5.0 (present since 3.0), so no 
need to rush it in before 5.1.

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

RE: [PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling

[Zhang, Chen]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Monday, April 20, 2020 4:33 PM
> To: qemu-devel@nongnu.org
> Cc: Zhang, Chen <chen.zhang@intel.com>; zhanghailiang
> <zhang.zhanghailiang@huawei.com>
> Subject: [PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint()
> error handling
> 
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the latter kind
> twice without clearing it in between is wrong: if the first call sets an error, it
> no longer points to NULL for the second call.
> 
> qmp_xen_colo_do_checkpoint() passes @errp first to
> replication_do_checkpoint_all(), and then to colo_notify_filters_event().  If
> both fail, this will trip the assertion in error_setv().
> 
> Similar code in secondary_vm_do_failover() calls
> colo_notify_filters_event() only after replication_do_checkpoint_all()
> succeeded.  Do the same here.
> 
> Fixes: 0e8818f023616677416840d6ddc880db8de3c967
> Cc: Zhang Chen <chen.zhang@intel.com>
> Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Looks good for me.

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Zhang Chen

> ---
>  migration/colo.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c index a54ac84f41..1b3493729b
> 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -263,7 +263,13 @@ ReplicationStatus
> *qmp_query_xen_replication_status(Error **errp)
> 
>  void qmp_xen_colo_do_checkpoint(Error **errp)  {
> -    replication_do_checkpoint_all(errp);
> +    Error *err = NULL;
> +
> +    replication_do_checkpoint_all(&err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
>      /* Notify all filters of all NIC to do checkpoint */
>      colo_notify_filters_event(COLO_EVENT_CHECKPOINT, errp);  }
> --
> 2.21.1