* [PATCH 01/11] cryptodev: Fix cryptodev_builtin_cleanup() error API violation
2020-04-20 8:32 [PATCH 00/11] Miscellaneous error handling fixes Markus Armbruster
@ 2020-04-20 8:32 ` Markus Armbruster
2020-04-20 8:32 ` [PATCH 02/11] block/file-posix: Fix check_cache_dropped() error handling Markus Armbruster
` (10 subsequent siblings)
11 siblings, 0 replies; 23+ messages in thread
From: Markus Armbruster @ 2020-04-20 8:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Gonglei
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 02/11] block/file-posix: Fix check_cache_dropped() error handling
2020-04-20 8:32 [PATCH 00/11] Miscellaneous error handling fixes Markus Armbruster
2020-04-20 8:32 ` [PATCH 01/11] cryptodev: Fix cryptodev_builtin_cleanup() error API violation Markus Armbruster
@ 2020-04-20 8:32 ` Markus Armbruster
2020-04-20 15:05 ` Eric Blake
2020-04-20 8:32 ` [PATCH 03/11] cpus: Fix configure_icount() error API violation Markus Armbruster
` (9 subsequent siblings)
11 siblings, 1 reply; 23+ messages in thread
From: Markus Armbruster @ 2020-04-20 8:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 02/11] block/file-posix: Fix check_cache_dropped() error handling
2020-04-20 8:32 ` [PATCH 02/11] block/file-posix: Fix check_cache_dropped() error handling Markus Armbruster
@ 2020-04-20 15:05 ` Eric Blake
0 siblings, 0 replies; 23+ messages in thread
From: Eric Blake @ 2020-04-20 15:05 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 03/11] cpus: Fix configure_icount() error API violation
2020-04-20 8:32 [PATCH 00/11] Miscellaneous error handling fixes Markus Armbruster
2020-04-20 8:32 ` [PATCH 01/11] cryptodev: Fix cryptodev_builtin_cleanup() error API violation Markus Armbruster
2020-04-20 8:32 ` [PATCH 02/11] block/file-posix: Fix check_cache_dropped() error handling Markus Armbruster
@ 2020-04-20 8:32 ` Markus Armbruster
2020-04-20 8:32 ` [PATCH 04/11] cpus: Proper range-checking for -icount shift=N Markus Armbruster
` (8 subsequent siblings)
11 siblings, 0 replies; 23+ messages in thread
From: Markus Armbruster @ 2020-04-20 8:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 04/11] cpus: Proper range-checking for -icount shift=N
2020-04-20 8:32 [PATCH 00/11] Miscellaneous error handling fixes Markus Armbruster
` (2 preceding siblings ...)
2020-04-20 8:32 ` [PATCH 03/11] cpus: Fix configure_icount() error API violation Markus Armbruster
@ 2020-04-20 8:32 ` Markus Armbruster
2020-04-20 8:32 ` [PATCH 05/11] arm/virt: Fix virt_machine_device_plug_cb() error API violation Markus Armbruster
` (7 subsequent siblings)
11 siblings, 0 replies; 23+ messages in thread
From: Markus Armbruster @ 2020-04-20 8:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 05/11] arm/virt: Fix virt_machine_device_plug_cb() error API violation
2020-04-20 8:32 [PATCH 00/11] Miscellaneous error handling fixes Markus Armbruster
` (3 preceding siblings ...)
2020-04-20 8:32 ` [PATCH 04/11] cpus: Proper range-checking for -icount shift=N Markus Armbruster
@ 2020-04-20 8:32 ` Markus Armbruster
2020-04-20 8:52 ` Philippe Mathieu-Daudé
2020-04-20 8:32 ` [PATCH 06/11] fdc: Fix fallback=auto error handling Markus Armbruster
` (6 subsequent siblings)
11 siblings, 1 reply; 23+ messages in thread
From: Markus Armbruster @ 2020-04-20 8:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, qemu-arm
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 05/11] arm/virt: Fix virt_machine_device_plug_cb() error API violation
2020-04-20 8:32 ` [PATCH 05/11] arm/virt: Fix virt_machine_device_plug_cb() error API violation Markus Armbruster
@ 2020-04-20 8:52 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-20 8:52 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Peter Maydell, qemu-arm
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);
> }
> }
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 06/11] fdc: Fix fallback=auto error handling
2020-04-20 8:32 [PATCH 00/11] Miscellaneous error handling fixes Markus Armbruster
` (4 preceding siblings ...)
2020-04-20 8:32 ` [PATCH 05/11] arm/virt: Fix virt_machine_device_plug_cb() error API violation Markus Armbruster
@ 2020-04-20 8:32 ` Markus Armbruster
2020-04-20 8:54 ` Philippe Mathieu-Daudé
2020-04-20 8:32 ` [PATCH 07/11] bochs-display: Fix vgamem=SIZE " Markus Armbruster
` (5 subsequent siblings)
11 siblings, 1 reply; 23+ messages in thread
From: Markus Armbruster @ 2020-04-20 8:32 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 06/11] fdc: Fix fallback=auto error handling
2020-04-20 8:32 ` [PATCH 06/11] fdc: Fix fallback=auto error handling Markus Armbruster
@ 2020-04-20 8:54 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-20 8:54 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: John Snow
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 07/11] bochs-display: Fix vgamem=SIZE error handling
2020-04-20 8:32 [PATCH 00/11] Miscellaneous error handling fixes Markus Armbruster
` (5 preceding siblings ...)
2020-04-20 8:32 ` [PATCH 06/11] fdc: Fix fallback=auto error handling Markus Armbruster
@ 2020-04-20 8:32 ` Markus Armbruster
2020-04-20 8:54 ` Philippe Mathieu-Daudé
2020-04-20 8:32 ` [PATCH 08/11] virtio-net: Fix duplex=... and speed=... " Markus Armbruster
` (4 subsequent siblings)
11 siblings, 1 reply; 23+ messages in thread
From: Markus Armbruster @ 2020-04-20 8:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 07/11] bochs-display: Fix vgamem=SIZE error handling
2020-04-20 8:32 ` [PATCH 07/11] bochs-display: Fix vgamem=SIZE " Markus Armbruster
@ 2020-04-20 8:54 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-20 8:54 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Gerd Hoffmann
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 08/11] virtio-net: Fix duplex=... and speed=... error handling
2020-04-20 8:32 [PATCH 00/11] Miscellaneous error handling fixes Markus Armbruster
` (6 preceding siblings ...)
2020-04-20 8:32 ` [PATCH 07/11] bochs-display: Fix vgamem=SIZE " Markus Armbruster
@ 2020-04-20 8:32 ` Markus Armbruster
2020-04-20 9:04 ` Philippe Mathieu-Daudé
2020-04-20 8:32 ` [PATCH 09/11] xen/pt: Fix flawed conversion to realize() Markus Armbruster
` (3 subsequent siblings)
11 siblings, 1 reply; 23+ messages in thread
From: Markus Armbruster @ 2020-04-20 8:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Jason Wang, Michael S. Tsirkin
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 08/11] virtio-net: Fix duplex=... and speed=... error handling
2020-04-20 8:32 ` [PATCH 08/11] virtio-net: Fix duplex=... and speed=... " Markus Armbruster
@ 2020-04-20 9:04 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-20 9:04 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Jason Wang, Michael S. Tsirkin
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 09/11] xen/pt: Fix flawed conversion to realize()
2020-04-20 8:32 [PATCH 00/11] Miscellaneous error handling fixes Markus Armbruster
` (7 preceding siblings ...)
2020-04-20 8:32 ` [PATCH 08/11] virtio-net: Fix duplex=... and speed=... " Markus Armbruster
@ 2020-04-20 8:32 ` Markus Armbruster
2020-04-20 8:58 ` Paul Durrant
2020-04-20 8:32 ` [PATCH 10/11] io: Fix qio_channel_socket_close() error handling Markus Armbruster
` (2 subsequent siblings)
11 siblings, 1 reply; 23+ messages in thread
From: Markus Armbruster @ 2020-04-20 8:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Anthony Perard, xen-devel, Stefano Stabellini, Paul Durrant
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* RE: [PATCH 09/11] xen/pt: Fix flawed conversion to realize()
2020-04-20 8:32 ` [PATCH 09/11] xen/pt: Fix flawed conversion to realize() Markus Armbruster
@ 2020-04-20 8:58 ` Paul Durrant
0 siblings, 0 replies; 23+ messages in thread
From: Paul Durrant @ 2020-04-20 8:58 UTC (permalink / raw)
To: 'Markus Armbruster', qemu-devel
Cc: 'Anthony Perard', xen-devel, 'Stefano Stabellini'
> -----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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 10/11] io: Fix qio_channel_socket_close() error handling
2020-04-20 8:32 [PATCH 00/11] Miscellaneous error handling fixes Markus Armbruster
` (8 preceding siblings ...)
2020-04-20 8:32 ` [PATCH 09/11] xen/pt: Fix flawed conversion to realize() Markus Armbruster
@ 2020-04-20 8:32 ` Markus Armbruster
2020-04-20 9:34 ` Daniel P. Berrangé
2020-04-20 8:32 ` [PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint() " Markus Armbruster
2020-04-20 12:57 ` [PATCH 00/11] Miscellaneous error handling fixes no-reply
11 siblings, 1 reply; 23+ messages in thread
From: Markus Armbruster @ 2020-04-20 8:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel P . Berrangé
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 10/11] io: Fix qio_channel_socket_close() error handling
2020-04-20 8:32 ` [PATCH 10/11] io: Fix qio_channel_socket_close() error handling Markus Armbruster
@ 2020-04-20 9:34 ` Daniel P. Berrangé
0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrangé @ 2020-04-20 9:34 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
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 :|
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling
2020-04-20 8:32 [PATCH 00/11] Miscellaneous error handling fixes Markus Armbruster
` (9 preceding siblings ...)
2020-04-20 8:32 ` [PATCH 10/11] io: Fix qio_channel_socket_close() error handling Markus Armbruster
@ 2020-04-20 8:32 ` Markus Armbruster
2020-04-20 8:48 ` Zhanghailiang
` (2 more replies)
2020-04-20 12:57 ` [PATCH 00/11] Miscellaneous error handling fixes no-reply
11 siblings, 3 replies; 23+ messages in thread
From: Markus Armbruster @ 2020-04-20 8:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhang Chen, zhanghailiang
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* RE: [PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling
2020-04-20 8:32 ` [PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint() " Markus Armbruster
@ 2020-04-20 8:48 ` Zhanghailiang
2020-04-20 8:56 ` Philippe Mathieu-Daudé
2020-04-21 0:24 ` Zhang, Chen
2 siblings, 0 replies; 23+ messages in thread
From: Zhanghailiang @ 2020-04-20 8:48 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel@nongnu.org; +Cc: Zhang Chen
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling
2020-04-20 8:32 ` [PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint() " Markus Armbruster
2020-04-20 8:48 ` Zhanghailiang
@ 2020-04-20 8:56 ` Philippe Mathieu-Daudé
2020-04-21 0:24 ` Zhang, Chen
2 siblings, 0 replies; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-20 8:56 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Zhang Chen, zhanghailiang
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>
^ permalink raw reply [flat|nested] 23+ messages in thread* RE: [PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint() error handling
2020-04-20 8:32 ` [PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint() " Markus Armbruster
2020-04-20 8:48 ` Zhanghailiang
2020-04-20 8:56 ` Philippe Mathieu-Daudé
@ 2020-04-21 0:24 ` Zhang, Chen
2 siblings, 0 replies; 23+ messages in thread
From: Zhang, Chen @ 2020-04-21 0:24 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel@nongnu.org; +Cc: zhanghailiang
> -----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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 00/11] Miscellaneous error handling fixes
2020-04-20 8:32 [PATCH 00/11] Miscellaneous error handling fixes Markus Armbruster
` (10 preceding siblings ...)
2020-04-20 8:32 ` [PATCH 11/11] migration/colo: Fix qmp_xen_colo_do_checkpoint() " Markus Armbruster
@ 2020-04-20 12:57 ` no-reply
11 siblings, 0 replies; 23+ messages in thread
From: no-reply @ 2020-04-20 12:57 UTC (permalink / raw)
To: armbru; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 23+ messages in thread