* [PATCH 00/22] Error handling fixes & cleanups
@ 2020-06-22 10:42 Markus Armbruster
2020-06-22 10:42 ` [PATCH 01/22] net/virtio: Fix failover_replug_primary() return value regression Markus Armbruster
` (21 more replies)
0 siblings, 22 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel
This series addresses a number of error handling issues I found while
working on error handling improvements. It's based on my "[PATCH v2
00/16] Crazy shit around -global (pardon my french)".
Based-on: <20200622094227.1271650-1-armbru@redhat.com>
Markus Armbruster (22):
net/virtio: Fix failover_replug_primary() return value regression
pci: Delete useless error_propagate()
Clean up some calls to ignore Error objects the right way
tests: Use &error_abort where appropriate
tests: Use error_free_or_abort() where appropriate
usb/dev-mtp: Fix Error double free after inotify failure
spapr: Plug minor memory leak in spapr_machine_init()
qga: Plug unlikely memory leak in guest-set-memory-blocks
sd/milkymist-memcard: Plug minor memory leak in realize
test-util-filemonitor: Plug unlikely memory leak
vnc: Plug minor memory leak in vnc_display_open()
tests/qom-proplist: Delete a superfluous error_free()
aspeed: Clean up roundabout error propagation
qdev: Drop qbus_set_bus_hotplug_handler() parameter @errp
qdev: Drop qbus_set_hotplug_handler() parameter @errp
hw: Fix error API violation around object_property_set_link()
hw/arm: Drop useless object_property_set_link() error handling
riscv/sifive_u: Fix sifive_u_soc_realize() error API violations
riscv_hart: Fix riscv_harts_realize() error API violations
mips/cps: Fix mips_cps_realize() error API violations
x86: Fix x86_cpu_new() error API violations
amd_iommu: Fix amdvi_realize() error API violation
include/hw/qdev-core.h | 5 +-
chardev/char-socket.c | 6 +-
hw/9pfs/9p.c | 6 +-
hw/acpi/pcihp.c | 3 +-
hw/acpi/piix4.c | 2 +-
hw/arm/armsse.c | 53 ++++----------
hw/arm/armv7m.c | 7 +-
hw/arm/aspeed_ast2600.c | 30 +++-----
hw/arm/aspeed_soc.c | 24 +++----
hw/arm/nrf51_soc.c | 6 +-
hw/arm/virt.c | 4 +-
hw/char/virtio-serial-bus.c | 4 +-
hw/core/bus.c | 8 +--
hw/display/virtio-gpu-pci.c | 2 +-
hw/display/virtio-vga.c | 2 +-
hw/dma/sparc32_dma.c | 6 +-
hw/dma/xilinx_axidma.c | 12 +---
hw/i386/amd_iommu.c | 6 +-
hw/i386/x86.c | 12 +---
hw/mips/cps.c | 35 +++++----
hw/misc/macio/macio.c | 3 +-
hw/net/virtio-net.c | 2 +-
hw/net/xilinx_axienet.c | 12 +---
hw/pci/pci.c | 3 -
hw/pci/pcie.c | 2 +-
hw/pci/shpc.c | 2 +-
hw/ppc/spapr.c | 4 +-
hw/ppc/spapr_drc.c | 4 +-
hw/ppc/spapr_pci.c | 4 +-
hw/riscv/riscv_hart.c | 14 ++--
hw/riscv/sifive_u.c | 8 ++-
hw/s390x/ap-bridge.c | 2 +-
hw/s390x/css-bridge.c | 2 +-
hw/s390x/s390-pci-bus.c | 14 +---
hw/scsi/scsi-bus.c | 2 +-
hw/scsi/virtio-scsi.c | 4 +-
hw/scsi/vmw_pvscsi.c | 2 +-
hw/sd/milkymist-memcard.c | 5 +-
hw/usb/bus.c | 2 +-
hw/usb/dev-mtp.c | 2 -
hw/usb/dev-smartcard-reader.c | 2 +-
hw/virtio/virtio-iommu-pci.c | 2 +-
hw/xen/xen-bus.c | 2 +-
hw/xen/xen-legacy-backend.c | 2 +-
qga/commands-posix.c | 1 +
tests/check-block-qdict.c | 24 ++-----
tests/check-qobject.c | 5 +-
tests/check-qom-proplist.c | 8 +--
tests/test-base64.c | 3 +-
tests/test-bdrv-graph-mod.c | 4 +-
tests/test-block-iothread.c | 3 +-
tests/test-crypto-cipher.c | 8 +--
tests/test-io-task.c | 4 +-
tests/test-logging.c | 12 +---
tests/test-qemu-opts.c | 22 ++----
tests/test-replication.c | 109 +++++++++--------------------
tests/test-string-input-visitor.c | 33 +++------
tests/test-string-output-visitor.c | 16 ++---
tests/test-util-filemonitor.c | 1 +
ui/vnc.c | 6 +-
60 files changed, 198 insertions(+), 395 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 01/22] net/virtio: Fix failover_replug_primary() return value regression
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 21:25 ` Jens Freimann
2020-06-22 10:42 ` [PATCH 02/22] pci: Delete useless error_propagate() Markus Armbruster
` (20 subsequent siblings)
21 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Jens Freimann, qemu-stable, Michael S . Tsirkin
Commit 150ab54aa6 "net/virtio: fix re-plugging of primary device"
fixed failover_replug_primary() to return false on failure. Commit
5a0948d36c "net/virtio: Fix failover error handling crash bugs" broke
it again for hotplug_handler_plug() failure. Unbreak it.
Commit 5a0948d36c4cbc1c5534afac6fee99de55245d12
Fixes: 5a0948d36c4cbc1c5534afac6fee99de55245d12
Cc: Jens Freimann <jfreimann@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/net/virtio-net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index aff67a92df..9bb5578e5d 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3129,7 +3129,7 @@ static bool failover_replug_primary(VirtIONet *n, Error **errp)
if (err) {
goto out;
}
- hotplug_handler_plug(hotplug_ctrl, n->primary_dev, errp);
+ hotplug_handler_plug(hotplug_ctrl, n->primary_dev, &err);
}
out:
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 02/22] pci: Delete useless error_propagate()
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
2020-06-22 10:42 ` [PATCH 01/22] net/virtio: Fix failover_replug_primary() return value regression Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 21:26 ` Jens Freimann
2020-06-22 10:42 ` [PATCH 03/22] Clean up some calls to ignore Error objects the right way Markus Armbruster
` (19 subsequent siblings)
21 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Jens Freimann, Michael S . Tsirkin
Cc: Jens Freimann <jfreimann@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/pci/pci.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b22dedc88c..de0fae10ab 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2123,7 +2123,6 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
if (!pci_bus_is_express(pci_get_bus(pci_dev))) {
error_setg(errp, "failover primary device must be on "
"PCIExpress bus");
- error_propagate(errp, local_err);
pci_qdev_unrealize(DEVICE(pci_dev));
return;
}
@@ -2131,7 +2130,6 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
if (class_id != PCI_CLASS_NETWORK_ETHERNET) {
error_setg(errp, "failover primary device is not an "
"Ethernet device");
- error_propagate(errp, local_err);
pci_qdev_unrealize(DEVICE(pci_dev));
return;
}
@@ -2141,7 +2139,6 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
} else {
error_setg(errp, "failover: primary device must be in its own "
"PCI slot");
- error_propagate(errp, local_err);
pci_qdev_unrealize(DEVICE(pci_dev));
return;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 03/22] Clean up some calls to ignore Error objects the right way
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
2020-06-22 10:42 ` [PATCH 01/22] net/virtio: Fix failover_replug_primary() return value regression Markus Armbruster
2020-06-22 10:42 ` [PATCH 02/22] pci: Delete useless error_propagate() Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 14:43 ` Greg Kurz
2020-06-22 10:42 ` [PATCH 04/22] tests: Use &error_abort where appropriate Markus Armbruster
` (18 subsequent siblings)
21 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Jerome Forissier, Daniel P . Berrange, Greg Kurz
Receiving the error in a local variable only to free it is less clear
(and also less efficient) than passing NULL. Clean up.
Cc: Daniel P. Berrange <berrange@redhat.com>
Cc: Jerome Forissier <jerome@forissier.org>
CC: Greg Kurz <groug@kaod.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
chardev/char-socket.c | 6 ++----
hw/9pfs/9p.c | 6 ++----
hw/arm/virt.c | 4 +---
hw/ppc/spapr_drc.c | 4 +---
ui/vnc.c | 3 +--
5 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index afebeec5c3..b0cae97960 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -815,22 +815,20 @@ static void tcp_chr_tls_init(Chardev *chr)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
QIOChannelTLS *tioc;
- Error *err = NULL;
gchar *name;
if (s->is_listen) {
tioc = qio_channel_tls_new_server(
s->ioc, s->tls_creds,
s->tls_authz,
- &err);
+ NULL);
} else {
tioc = qio_channel_tls_new_client(
s->ioc, s->tls_creds,
s->addr->u.inet.host,
- &err);
+ NULL);
}
if (tioc == NULL) {
- error_free(err);
tcp_chr_disconnect(chr);
return;
}
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 45a788f6e6..9755fba9a9 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1399,7 +1399,6 @@ static void coroutine_fn v9fs_attach(void *opaque)
size_t offset = 7;
V9fsQID qid;
ssize_t err;
- Error *local_err = NULL;
v9fs_string_init(&uname);
v9fs_string_init(&aname);
@@ -1437,9 +1436,8 @@ static void coroutine_fn v9fs_attach(void *opaque)
error_setg(&s->migration_blocker,
"Migration is disabled when VirtFS export path '%s' is mounted in the guest using mount_tag '%s'",
s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag);
- err = migrate_add_blocker(s->migration_blocker, &local_err);
- if (local_err) {
- error_free(local_err);
+ err = migrate_add_blocker(s->migration_blocker, NULL);
+ if (err < 0) {
error_free(s->migration_blocker);
s->migration_blocker = NULL;
clunk_fid(s, fid);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index caceb1e4a0..29b9d5b2e6 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -217,11 +217,9 @@ static bool cpu_type_valid(const char *cpu)
static void create_kaslr_seed(VirtMachineState *vms, const char *node)
{
- Error *err = NULL;
uint64_t seed;
- if (qemu_guest_getrandom(&seed, sizeof(seed), &err)) {
- error_free(err);
+ if (qemu_guest_getrandom(&seed, sizeof(seed), NULL)) {
return;
}
qemu_fdt_setprop_u64(vms->fdt, node, "kaslr-seed", seed);
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 2689104295..951bcdf2c0 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -1163,16 +1163,14 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
if (!drc->fdt) {
- Error *local_err = NULL;
void *fdt;
int fdt_size;
fdt = create_device_tree(&fdt_size);
if (drck->dt_populate(drc, spapr, fdt, &drc->fdt_start_offset,
- &local_err)) {
+ NULL)) {
g_free(fdt);
- error_free(local_err);
rc = SPAPR_DR_CC_RESPONSE_ERROR;
goto out;
}
diff --git a/ui/vnc.c b/ui/vnc.c
index 12a12714e1..0702a76cce 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -458,9 +458,8 @@ static VncServerInfo2List *qmp_query_server_entry(QIOChannelSocket *ioc,
Error *err = NULL;
SocketAddress *addr;
- addr = qio_channel_socket_get_local_address(ioc, &err);
+ addr = qio_channel_socket_get_local_address(ioc, NULL);
if (!addr) {
- error_free(err);
return prev;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 04/22] tests: Use &error_abort where appropriate
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (2 preceding siblings ...)
2020-06-22 10:42 ` [PATCH 03/22] Clean up some calls to ignore Error objects the right way Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 11:26 ` Thomas Huth
2020-06-22 10:42 ` [PATCH 05/22] tests: Use error_free_or_abort() " Markus Armbruster
` (17 subsequent siblings)
21 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel
Receiving the error in a local variable only to assert there is none
is less clear than passing &error_abort. Clean up.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/check-qobject.c | 5 +-
tests/test-logging.c | 12 +---
tests/test-qemu-opts.c | 22 ++----
tests/test-replication.c | 109 +++++++++--------------------
tests/test-string-input-visitor.c | 33 +++------
tests/test-string-output-visitor.c | 16 ++---
6 files changed, 57 insertions(+), 140 deletions(-)
diff --git a/tests/check-qobject.c b/tests/check-qobject.c
index 593c3a0618..6b6deaeb8b 100644
--- a/tests/check-qobject.c
+++ b/tests/check-qobject.c
@@ -9,6 +9,7 @@
#include "qemu/osdep.h"
#include "block/qdict.h"
+#include "qapi/error.h"
#include "qapi/qmp/qbool.h"
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qlist.h"
@@ -213,7 +214,6 @@ static void qobject_is_equal_list_test(void)
static void qobject_is_equal_dict_test(void)
{
- Error *local_err = NULL;
QDict *dict_0, *dict_1, *dict_cloned;
QDict *dict_different_key, *dict_different_value, *dict_different_null_key;
QDict *dict_longer, *dict_shorter, *dict_nested;
@@ -276,8 +276,7 @@ static void qobject_is_equal_dict_test(void)
dict_different_null_key, dict_longer, dict_shorter,
dict_nested);
- dict_crumpled = qobject_to(QDict, qdict_crumple(dict_1, &local_err));
- g_assert(!local_err);
+ dict_crumpled = qobject_to(QDict, qdict_crumple(dict_1, &error_abort));
check_equal(dict_crumpled, dict_nested);
qdict_flatten(dict_nested);
diff --git a/tests/test-logging.c b/tests/test-logging.c
index 8580b82420..8a1161de1d 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -113,7 +113,6 @@ static void test_logfile_write(gconstpointer data)
QemuLogFile *logfile;
QemuLogFile *logfile2;
gchar const *dir = data;
- Error *err = NULL;
g_autofree gchar *file_path = NULL;
g_autofree gchar *file_path1 = NULL;
FILE *orig_fd;
@@ -132,8 +131,7 @@ static void test_logfile_write(gconstpointer data)
* Test that even if an open file handle is changed,
* our handle remains valid due to RCU.
*/
- qemu_set_log_filename(file_path, &err);
- g_assert(!err);
+ qemu_set_log_filename(file_path, &error_abort);
rcu_read_lock();
logfile = atomic_rcu_read(&qemu_logfile);
orig_fd = logfile->fd;
@@ -142,8 +140,7 @@ static void test_logfile_write(gconstpointer data)
fflush(logfile->fd);
/* Change the logfile and ensure that the handle is still valid. */
- qemu_set_log_filename(file_path1, &err);
- g_assert(!err);
+ qemu_set_log_filename(file_path1, &error_abort);
logfile2 = atomic_rcu_read(&qemu_logfile);
g_assert(logfile->fd == orig_fd);
g_assert(logfile2->fd != logfile->fd);
@@ -156,7 +153,6 @@ static void test_logfile_lock(gconstpointer data)
{
FILE *logfile;
gchar const *dir = data;
- Error *err = NULL;
g_autofree gchar *file_path = NULL;
file_path = g_build_filename(dir, "qemu_test_logfile_lock0.log", NULL);
@@ -166,7 +162,7 @@ static void test_logfile_lock(gconstpointer data)
* that even if an open file handle is closed,
* our handle remains valid for use due to RCU.
*/
- qemu_set_log_filename(file_path, &err);
+ qemu_set_log_filename(file_path, &error_abort);
logfile = qemu_log_lock();
g_assert(logfile);
fprintf(logfile, "%s 1st write to file\n", __func__);
@@ -180,8 +176,6 @@ static void test_logfile_lock(gconstpointer data)
fprintf(logfile, "%s 2nd write to file\n", __func__);
fflush(logfile);
qemu_log_unlock(logfile);
-
- g_assert(!err);
}
/* Remove a directory and all its entries (non-recursive). */
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 2a0f42a09b..297ffe79dd 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -187,7 +187,6 @@ static void test_qemu_opt_get(void)
static void test_qemu_opt_get_bool(void)
{
- Error *err = NULL;
QemuOptsList *list;
QemuOpts *opts;
bool opt;
@@ -210,16 +209,14 @@ static void test_qemu_opt_get_bool(void)
opt = qemu_opt_get_bool(opts, "bool1", false);
g_assert(opt == false);
- qemu_opt_set_bool(opts, "bool1", true, &err);
- g_assert(!err);
+ qemu_opt_set_bool(opts, "bool1", true, &error_abort);
/* now we have set bool1, should know about it */
opt = qemu_opt_get_bool(opts, "bool1", false);
g_assert(opt == true);
/* having reset the value, opt should be the reset one not defval */
- qemu_opt_set_bool(opts, "bool1", false, &err);
- g_assert(!err);
+ qemu_opt_set_bool(opts, "bool1", false, &error_abort);
opt = qemu_opt_get_bool(opts, "bool1", true);
g_assert(opt == false);
@@ -233,7 +230,6 @@ static void test_qemu_opt_get_bool(void)
static void test_qemu_opt_get_number(void)
{
- Error *err = NULL;
QemuOptsList *list;
QemuOpts *opts;
uint64_t opt;
@@ -256,16 +252,14 @@ static void test_qemu_opt_get_number(void)
opt = qemu_opt_get_number(opts, "number1", 5);
g_assert(opt == 5);
- qemu_opt_set_number(opts, "number1", 10, &err);
- g_assert(!err);
+ qemu_opt_set_number(opts, "number1", 10, &error_abort);
/* now we have set number1, should know about it */
opt = qemu_opt_get_number(opts, "number1", 5);
g_assert(opt == 10);
/* having reset it, the returned should be the reset one not defval */
- qemu_opt_set_number(opts, "number1", 15, &err);
- g_assert(!err);
+ qemu_opt_set_number(opts, "number1", 15, &error_abort);
opt = qemu_opt_get_number(opts, "number1", 5);
g_assert(opt == 15);
@@ -367,7 +361,6 @@ static void test_qemu_opt_unset(void)
static void test_qemu_opts_reset(void)
{
- Error *err = NULL;
QemuOptsList *list;
QemuOpts *opts;
uint64_t opt;
@@ -390,8 +383,7 @@ static void test_qemu_opts_reset(void)
opt = qemu_opt_get_number(opts, "number1", 5);
g_assert(opt == 5);
- qemu_opt_set_number(opts, "number1", 10, &err);
- g_assert(!err);
+ qemu_opt_set_number(opts, "number1", 10, &error_abort);
/* now we have set number1, should know about it */
opt = qemu_opt_get_number(opts, "number1", 5);
@@ -406,7 +398,6 @@ static void test_qemu_opts_reset(void)
static void test_qemu_opts_set(void)
{
- Error *err = NULL;
QemuOptsList *list;
QemuOpts *opts;
const char *opt;
@@ -421,8 +412,7 @@ static void test_qemu_opts_set(void)
g_assert(opts == NULL);
/* implicitly create opts and set str3 value */
- qemu_opts_set(list, NULL, "str3", "value", &err);
- g_assert(!err);
+ qemu_opts_set(list, NULL, "str3", "value", &error_abort);
g_assert(!QTAILQ_EMPTY(&list->head));
/* get the just created opts */
diff --git a/tests/test-replication.c b/tests/test-replication.c
index cbc37db2df..e0b03dafc2 100644
--- a/tests/test-replication.c
+++ b/tests/test-replication.c
@@ -139,8 +139,6 @@ static void make_temp(char *template)
static void prepare_imgs(void)
{
- Error *local_err = NULL;
-
make_temp(p_local_disk);
make_temp(s_local_disk);
make_temp(s_active_disk);
@@ -148,19 +146,15 @@ static void prepare_imgs(void)
/* Primary */
bdrv_img_create(p_local_disk, "qcow2", NULL, NULL, NULL, IMG_SIZE,
- BDRV_O_RDWR, true, &local_err);
- g_assert(!local_err);
+ BDRV_O_RDWR, true, &error_abort);
/* Secondary */
bdrv_img_create(s_local_disk, "qcow2", NULL, NULL, NULL, IMG_SIZE,
- BDRV_O_RDWR, true, &local_err);
- g_assert(!local_err);
+ BDRV_O_RDWR, true, &error_abort);
bdrv_img_create(s_active_disk, "qcow2", NULL, NULL, NULL, IMG_SIZE,
- BDRV_O_RDWR, true, &local_err);
- g_assert(!local_err);
+ BDRV_O_RDWR, true, &error_abort);
bdrv_img_create(s_hidden_disk, "qcow2", NULL, NULL, NULL, IMG_SIZE,
- BDRV_O_RDWR, true, &local_err);
- g_assert(!local_err);
+ BDRV_O_RDWR, true, &error_abort);
}
static void cleanup_imgs(void)
@@ -179,7 +173,6 @@ static BlockBackend *start_primary(void)
BlockBackend *blk;
QemuOpts *opts;
QDict *qdict;
- Error *local_err = NULL;
char *cmdline;
cmdline = g_strdup_printf("driver=replication,mode=primary,node-name=xxx,"
@@ -193,12 +186,10 @@ static BlockBackend *start_primary(void)
qdict_set_default_str(qdict, BDRV_OPT_CACHE_DIRECT, "off");
qdict_set_default_str(qdict, BDRV_OPT_CACHE_NO_FLUSH, "off");
- blk = blk_new_open(NULL, NULL, qdict, BDRV_O_RDWR, &local_err);
+ blk = blk_new_open(NULL, NULL, qdict, BDRV_O_RDWR, &error_abort);
g_assert(blk);
- g_assert(!local_err);
- monitor_add_blk(blk, P_ID, &local_err);
- g_assert(!local_err);
+ monitor_add_blk(blk, P_ID, &error_abort);
qemu_opts_del(opts);
@@ -248,12 +239,10 @@ static void test_primary_write(void)
static void test_primary_start(void)
{
BlockBackend *blk = NULL;
- Error *local_err = NULL;
blk = start_primary();
- replication_start_all(REPLICATION_MODE_PRIMARY, &local_err);
- g_assert(!local_err);
+ replication_start_all(REPLICATION_MODE_PRIMARY, &error_abort);
/* read from 0 to IMG_SIZE */
test_blk_read(blk, 0, 0, IMG_SIZE, 0, IMG_SIZE, true);
@@ -266,46 +255,35 @@ static void test_primary_start(void)
static void test_primary_stop(void)
{
- Error *local_err = NULL;
bool failover = true;
start_primary();
- replication_start_all(REPLICATION_MODE_PRIMARY, &local_err);
- g_assert(!local_err);
+ replication_start_all(REPLICATION_MODE_PRIMARY, &error_abort);
- replication_stop_all(failover, &local_err);
- g_assert(!local_err);
+ replication_stop_all(failover, &error_abort);
teardown_primary();
}
static void test_primary_do_checkpoint(void)
{
- Error *local_err = NULL;
-
start_primary();
- replication_start_all(REPLICATION_MODE_PRIMARY, &local_err);
- g_assert(!local_err);
+ replication_start_all(REPLICATION_MODE_PRIMARY, &error_abort);
- replication_do_checkpoint_all(&local_err);
- g_assert(!local_err);
+ replication_do_checkpoint_all(&error_abort);
teardown_primary();
}
static void test_primary_get_error_all(void)
{
- Error *local_err = NULL;
-
start_primary();
- replication_start_all(REPLICATION_MODE_PRIMARY, &local_err);
- g_assert(!local_err);
+ replication_start_all(REPLICATION_MODE_PRIMARY, &error_abort);
- replication_get_error_all(&local_err);
- g_assert(!local_err);
+ replication_get_error_all(&error_abort);
teardown_primary();
}
@@ -316,7 +294,6 @@ static BlockBackend *start_secondary(void)
QDict *qdict;
BlockBackend *blk;
char *cmdline;
- Error *local_err = NULL;
/* add s_local_disk and forge S_LOCAL_DISK_ID */
cmdline = g_strdup_printf("file.filename=%s,driver=qcow2,"
@@ -329,10 +306,9 @@ static BlockBackend *start_secondary(void)
qdict_set_default_str(qdict, BDRV_OPT_CACHE_DIRECT, "off");
qdict_set_default_str(qdict, BDRV_OPT_CACHE_NO_FLUSH, "off");
- blk = blk_new_open(NULL, NULL, qdict, BDRV_O_RDWR, &local_err);
+ blk = blk_new_open(NULL, NULL, qdict, BDRV_O_RDWR, &error_abort);
assert(blk);
- monitor_add_blk(blk, S_LOCAL_DISK_ID, &local_err);
- g_assert(!local_err);
+ monitor_add_blk(blk, S_LOCAL_DISK_ID, &error_abort);
/* format s_local_disk with pattern "0x11" */
test_blk_write(blk, 0x11, 0, IMG_SIZE, false);
@@ -356,10 +332,9 @@ static BlockBackend *start_secondary(void)
qdict_set_default_str(qdict, BDRV_OPT_CACHE_DIRECT, "off");
qdict_set_default_str(qdict, BDRV_OPT_CACHE_NO_FLUSH, "off");
- blk = blk_new_open(NULL, NULL, qdict, BDRV_O_RDWR, &local_err);
+ blk = blk_new_open(NULL, NULL, qdict, BDRV_O_RDWR, &error_abort);
assert(blk);
- monitor_add_blk(blk, S_ID, &local_err);
- g_assert(!local_err);
+ monitor_add_blk(blk, S_ID, &error_abort);
qemu_opts_del(opts);
@@ -420,12 +395,10 @@ static void test_secondary_write(void)
static void test_secondary_start(void)
{
BlockBackend *top_blk, *local_blk;
- Error *local_err = NULL;
bool failover = true;
top_blk = start_secondary();
- replication_start_all(REPLICATION_MODE_SECONDARY, &local_err);
- g_assert(!local_err);
+ replication_start_all(REPLICATION_MODE_SECONDARY, &error_abort);
/* read from s_local_disk (0, IMG_SIZE) */
test_blk_read(top_blk, 0x11, 0, IMG_SIZE, 0, IMG_SIZE, false);
@@ -446,8 +419,7 @@ static void test_secondary_start(void)
0, IMG_SIZE / 2, false);
/* unblock top_bs */
- replication_stop_all(failover, &local_err);
- g_assert(!local_err);
+ replication_stop_all(failover, &error_abort);
teardown_secondary();
}
@@ -456,12 +428,10 @@ static void test_secondary_start(void)
static void test_secondary_stop(void)
{
BlockBackend *top_blk, *local_blk;
- Error *local_err = NULL;
bool failover = true;
top_blk = start_secondary();
- replication_start_all(REPLICATION_MODE_SECONDARY, &local_err);
- g_assert(!local_err);
+ replication_start_all(REPLICATION_MODE_SECONDARY, &error_abort);
/* write 0x22 to s_local_disk (IMG_SIZE / 2, IMG_SIZE) */
local_blk = blk_by_name(S_LOCAL_DISK_ID);
@@ -475,8 +445,7 @@ static void test_secondary_stop(void)
test_blk_write(top_blk, 0x33, 0, IMG_SIZE / 2, false);
/* do active commit */
- replication_stop_all(failover, &local_err);
- g_assert(!local_err);
+ replication_stop_all(failover, &error_abort);
/* read from s_local_disk (0, IMG_SIZE / 2) */
test_blk_read(top_blk, 0x33, 0, IMG_SIZE / 2,
@@ -493,11 +462,9 @@ static void test_secondary_stop(void)
static void test_secondary_continuous_replication(void)
{
BlockBackend *top_blk, *local_blk;
- Error *local_err = NULL;
top_blk = start_secondary();
- replication_start_all(REPLICATION_MODE_SECONDARY, &local_err);
- g_assert(!local_err);
+ replication_start_all(REPLICATION_MODE_SECONDARY, &error_abort);
/* write 0x22 to s_local_disk (IMG_SIZE / 2, IMG_SIZE) */
local_blk = blk_by_name(S_LOCAL_DISK_ID);
@@ -511,22 +478,18 @@ static void test_secondary_continuous_replication(void)
test_blk_write(top_blk, 0x33, 0, IMG_SIZE / 2, false);
/* do failover (active commit) */
- replication_stop_all(true, &local_err);
- g_assert(!local_err);
+ replication_stop_all(true, &error_abort);
/* it should ignore all requests from now on */
/* start after failover */
- replication_start_all(REPLICATION_MODE_PRIMARY, &local_err);
- g_assert(!local_err);
+ replication_start_all(REPLICATION_MODE_PRIMARY, &error_abort);
/* checkpoint */
- replication_do_checkpoint_all(&local_err);
- g_assert(!local_err);
+ replication_do_checkpoint_all(&error_abort);
/* stop */
- replication_stop_all(true, &local_err);
- g_assert(!local_err);
+ replication_stop_all(true, &error_abort);
/* read from s_local_disk (0, IMG_SIZE / 2) */
test_blk_read(top_blk, 0x33, 0, IMG_SIZE / 2,
@@ -543,12 +506,10 @@ static void test_secondary_continuous_replication(void)
static void test_secondary_do_checkpoint(void)
{
BlockBackend *top_blk, *local_blk;
- Error *local_err = NULL;
bool failover = true;
top_blk = start_secondary();
- replication_start_all(REPLICATION_MODE_SECONDARY, &local_err);
- g_assert(!local_err);
+ replication_start_all(REPLICATION_MODE_SECONDARY, &error_abort);
/* write 0x22 to s_local_disk (IMG_SIZE / 2, IMG_SIZE) */
local_blk = blk_by_name(S_LOCAL_DISK_ID);
@@ -559,35 +520,29 @@ static void test_secondary_do_checkpoint(void)
test_blk_read(top_blk, 0x11, IMG_SIZE / 2,
IMG_SIZE / 2, 0, IMG_SIZE, false);
- replication_do_checkpoint_all(&local_err);
- g_assert(!local_err);
+ replication_do_checkpoint_all(&error_abort);
/* after checkpoint, read pattern 0x22 from s_local_disk */
test_blk_read(top_blk, 0x22, IMG_SIZE / 2,
IMG_SIZE / 2, 0, IMG_SIZE, false);
/* unblock top_bs */
- replication_stop_all(failover, &local_err);
- g_assert(!local_err);
+ replication_stop_all(failover, &error_abort);
teardown_secondary();
}
static void test_secondary_get_error_all(void)
{
- Error *local_err = NULL;
bool failover = true;
start_secondary();
- replication_start_all(REPLICATION_MODE_SECONDARY, &local_err);
- g_assert(!local_err);
+ replication_start_all(REPLICATION_MODE_SECONDARY, &error_abort);
- replication_get_error_all(&local_err);
- g_assert(!local_err);
+ replication_get_error_all(&error_abort);
/* unblock top_bs */
- replication_stop_all(failover, &local_err);
- g_assert(!local_err);
+ replication_stop_all(failover, &error_abort);
teardown_secondary();
}
diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 5418e085a4..249faafc9d 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -53,8 +53,7 @@ static void test_visitor_in_int(TestInputVisitorData *data,
v = visitor_input_test_init(data, "-42");
- visit_type_int(v, NULL, &res, &err);
- g_assert(!err);
+ visit_type_int(v, NULL, &res, &error_abort);
g_assert_cmpint(res, ==, value);
v = visitor_input_test_init(data, "not an int");
@@ -327,44 +326,37 @@ static void test_visitor_in_uintList(TestInputVisitorData *data,
static void test_visitor_in_bool(TestInputVisitorData *data,
const void *unused)
{
- Error *err = NULL;
bool res = false;
Visitor *v;
v = visitor_input_test_init(data, "true");
- visit_type_bool(v, NULL, &res, &err);
- g_assert(!err);
+ visit_type_bool(v, NULL, &res, &error_abort);
g_assert_cmpint(res, ==, true);
v = visitor_input_test_init(data, "yes");
- visit_type_bool(v, NULL, &res, &err);
- g_assert(!err);
+ visit_type_bool(v, NULL, &res, &error_abort);
g_assert_cmpint(res, ==, true);
v = visitor_input_test_init(data, "on");
- visit_type_bool(v, NULL, &res, &err);
- g_assert(!err);
+ visit_type_bool(v, NULL, &res, &error_abort);
g_assert_cmpint(res, ==, true);
v = visitor_input_test_init(data, "false");
- visit_type_bool(v, NULL, &res, &err);
- g_assert(!err);
+ visit_type_bool(v, NULL, &res, &error_abort);
g_assert_cmpint(res, ==, false);
v = visitor_input_test_init(data, "no");
- visit_type_bool(v, NULL, &res, &err);
- g_assert(!err);
+ visit_type_bool(v, NULL, &res, &error_abort);
g_assert_cmpint(res, ==, false);
v = visitor_input_test_init(data, "off");
- visit_type_bool(v, NULL, &res, &err);
- g_assert(!err);
+ visit_type_bool(v, NULL, &res, &error_abort);
g_assert_cmpint(res, ==, false);
}
@@ -377,8 +369,7 @@ static void test_visitor_in_number(TestInputVisitorData *data,
v = visitor_input_test_init(data, "3.14");
- visit_type_number(v, NULL, &res, &err);
- g_assert(!err);
+ visit_type_number(v, NULL, &res, &error_abort);
g_assert_cmpfloat(res, ==, value);
/* NaN and infinity has to be rejected */
@@ -399,13 +390,11 @@ static void test_visitor_in_string(TestInputVisitorData *data,
const void *unused)
{
char *res = NULL, *value = (char *) "Q E M U";
- Error *err = NULL;
Visitor *v;
v = visitor_input_test_init(data, value);
- visit_type_str(v, NULL, &res, &err);
- g_assert(!err);
+ visit_type_str(v, NULL, &res, &error_abort);
g_assert_cmpstr(res, ==, value);
g_free(res);
@@ -414,7 +403,6 @@ static void test_visitor_in_string(TestInputVisitorData *data,
static void test_visitor_in_enum(TestInputVisitorData *data,
const void *unused)
{
- Error *err = NULL;
Visitor *v;
EnumOne i;
@@ -423,8 +411,7 @@ static void test_visitor_in_enum(TestInputVisitorData *data,
v = visitor_input_test_init(data, EnumOne_str(i));
- visit_type_EnumOne(v, NULL, &res, &err);
- g_assert(!err);
+ visit_type_EnumOne(v, NULL, &res, &error_abort);
g_assert_cmpint(i, ==, res);
}
}
diff --git a/tests/test-string-output-visitor.c b/tests/test-string-output-visitor.c
index 3bd732222c..9f6581439a 100644
--- a/tests/test-string-output-visitor.c
+++ b/tests/test-string-output-visitor.c
@@ -71,11 +71,9 @@ static void test_visitor_out_int(TestOutputVisitorData *data,
const void *unused)
{
int64_t value = 42;
- Error *err = NULL;
char *str;
- visit_type_int(data->ov, NULL, &value, &err);
- g_assert(!err);
+ visit_type_int(data->ov, NULL, &value, &error_abort);
str = visitor_get(data);
if (data->human) {
@@ -120,12 +118,10 @@ static void test_visitor_out_intList(TestOutputVisitorData *data,
static void test_visitor_out_bool(TestOutputVisitorData *data,
const void *unused)
{
- Error *err = NULL;
bool value = true;
char *str;
- visit_type_bool(data->ov, NULL, &value, &err);
- g_assert(!err);
+ visit_type_bool(data->ov, NULL, &value, &error_abort);
str = visitor_get(data);
g_assert_cmpstr(str, ==, "true");
@@ -135,11 +131,9 @@ static void test_visitor_out_number(TestOutputVisitorData *data,
const void *unused)
{
double value = 3.14;
- Error *err = NULL;
char *str;
- visit_type_number(data->ov, NULL, &value, &err);
- g_assert(!err);
+ visit_type_number(data->ov, NULL, &value, &error_abort);
str = visitor_get(data);
g_assert_cmpstr(str, ==, "3.140000");
@@ -150,11 +144,9 @@ static void test_visitor_out_string(TestOutputVisitorData *data,
{
char *string = (char *) "Q E M U";
const char *string_human = "\"Q E M U\"";
- Error *err = NULL;
char *str;
- visit_type_str(data->ov, NULL, &string, &err);
- g_assert(!err);
+ visit_type_str(data->ov, NULL, &string, &error_abort);
str = visitor_get(data);
if (data->human) {
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 05/22] tests: Use error_free_or_abort() where appropriate
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (3 preceding siblings ...)
2020-06-22 10:42 ` [PATCH 04/22] tests: Use &error_abort where appropriate Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 10:42 ` [PATCH 06/22] usb/dev-mtp: Fix Error double free after inotify failure Markus Armbruster
` (16 subsequent siblings)
21 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel
Replace
g_assert(err != NULL);
error_free(err);
err = NULL;
and variations thereof by
error_free_or_abort(&err);
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/check-block-qdict.c | 24 ++++++------------------
tests/check-qom-proplist.c | 7 ++-----
tests/test-base64.c | 3 +--
tests/test-bdrv-graph-mod.c | 4 +---
tests/test-block-iothread.c | 3 +--
tests/test-crypto-cipher.c | 8 ++------
tests/test-io-task.c | 4 +---
7 files changed, 14 insertions(+), 39 deletions(-)
diff --git a/tests/check-block-qdict.c b/tests/check-block-qdict.c
index 73d3e9f574..5a25825093 100644
--- a/tests/check-block-qdict.c
+++ b/tests/check-block-qdict.c
@@ -610,9 +610,7 @@ static void qdict_rename_keys_test(void)
copy = qdict_clone_shallow(dict);
qdict_rename_keys(copy, renames, &local_err);
- g_assert(local_err != NULL);
- error_free(local_err);
- local_err = NULL;
+ error_free_or_abort(&local_err);
g_assert_cmpstr(qdict_get_str(copy, "abc"), ==, "foo");
g_assert_cmpstr(qdict_get_str(copy, "abcdef"), ==, "bar");
@@ -649,9 +647,7 @@ static void qdict_crumple_test_bad_inputs(void)
qdict_put_str(src, "rule.0.policy", "allow");
g_assert(qdict_crumple(src, &error) == NULL);
- g_assert(error != NULL);
- error_free(error);
- error = NULL;
+ error_free_or_abort(&error);
qobject_unref(src);
src = qdict_new();
@@ -660,9 +656,7 @@ static void qdict_crumple_test_bad_inputs(void)
qdict_put_str(src, "rule.a", "allow");
g_assert(qdict_crumple(src, &error) == NULL);
- g_assert(error != NULL);
- error_free(error);
- error = NULL;
+ error_free_or_abort(&error);
qobject_unref(src);
src = qdict_new();
@@ -673,9 +667,7 @@ static void qdict_crumple_test_bad_inputs(void)
qdict_put_str(src, "rule.b", "allow");
g_assert(qdict_crumple(src, &error) == NULL);
- g_assert(error != NULL);
- error_free(error);
- error = NULL;
+ error_free_or_abort(&error);
qobject_unref(src);
src = qdict_new();
@@ -684,9 +676,7 @@ static void qdict_crumple_test_bad_inputs(void)
qdict_put_str(src, "rule.3", "allow");
g_assert(qdict_crumple(src, &error) == NULL);
- g_assert(error != NULL);
- error_free(error);
- error = NULL;
+ error_free_or_abort(&error);
qobject_unref(src);
src = qdict_new();
@@ -695,9 +685,7 @@ static void qdict_crumple_test_bad_inputs(void)
qdict_put_str(src, "rule.+1", "allow");
g_assert(qdict_crumple(src, &error) == NULL);
- g_assert(error != NULL);
- error_free(error);
- error = NULL;
+ error_free_or_abort(&error);
qobject_unref(src);
}
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 13a824cfae..a44d6e1171 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -494,17 +494,14 @@ static void test_dummy_getenum(void)
"av",
"BadAnimal",
&err);
- g_assert(err != NULL);
- error_free(err);
- err = NULL;
+ error_free_or_abort(&err);
/* A non-enum property name */
val = object_property_get_enum(OBJECT(dobj),
"iv",
"DummyAnimal",
&err);
- g_assert(err != NULL);
- error_free(err);
+ error_free_or_abort(&err);
object_unparent(OBJECT(dobj));
}
diff --git a/tests/test-base64.c b/tests/test-base64.c
index ec122ceba5..a7f722c459 100644
--- a/tests/test-base64.c
+++ b/tests/test-base64.c
@@ -54,10 +54,9 @@ static void test_base64_bad(const char *input,
&len,
&err);
- g_assert(err != NULL);
+ error_free_or_abort(&err);
g_assert(actual == NULL);
g_assert_cmpint(len, ==, 0);
- error_free(err);
}
diff --git a/tests/test-bdrv-graph-mod.c b/tests/test-bdrv-graph-mod.c
index f93f3168b0..8cff13830e 100644
--- a/tests/test-bdrv-graph-mod.c
+++ b/tests/test-bdrv-graph-mod.c
@@ -115,9 +115,7 @@ static void test_update_perm_tree(void)
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, &error_abort);
bdrv_append(filter, bs, &local_err);
-
- g_assert_nonnull(local_err);
- error_free(local_err);
+ error_free_or_abort(&local_err);
blk_unref(root);
}
diff --git a/tests/test-block-iothread.c b/tests/test-block-iothread.c
index a953794be2..3f866a35c6 100644
--- a/tests/test-block-iothread.c
+++ b/tests/test-block-iothread.c
@@ -650,8 +650,7 @@ static void test_propagate_mirror(void)
blk_insert_bs(blk, src, &error_abort);
bdrv_try_set_aio_context(target, ctx, &local_err);
- g_assert(local_err);
- error_free(local_err);
+ error_free_or_abort(&local_err);
g_assert(blk_get_aio_context(blk) == main_ctx);
g_assert(bdrv_get_aio_context(src) == main_ctx);
diff --git a/tests/test-crypto-cipher.c b/tests/test-crypto-cipher.c
index 07fa2fa616..bebba1a4f4 100644
--- a/tests/test-crypto-cipher.c
+++ b/tests/test-crypto-cipher.c
@@ -761,10 +761,7 @@ static void test_cipher_short_plaintext(void)
sizeof(plaintext1),
&err);
g_assert(ret == -1);
- g_assert(err != NULL);
-
- error_free(err);
- err = NULL;
+ error_free_or_abort(&err);
/* Should report an error as plaintext is larger than
* block size, but not a multiple of block size
@@ -775,9 +772,8 @@ static void test_cipher_short_plaintext(void)
sizeof(plaintext2),
&err);
g_assert(ret == -1);
- g_assert(err != NULL);
+ error_free_or_abort(&err);
- error_free(err);
qcrypto_cipher_free(cipher);
}
diff --git a/tests/test-io-task.c b/tests/test-io-task.c
index c8a3813d49..85e7a98da5 100644
--- a/tests/test-io-task.c
+++ b/tests/test-io-task.c
@@ -240,9 +240,7 @@ static void test_task_thread_failure(void)
object_unref(obj);
g_assert(data.source == obj);
- g_assert(data.err != NULL);
-
- error_free(data.err);
+ error_free_or_abort(&data.err);
self = g_thread_self();
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 06/22] usb/dev-mtp: Fix Error double free after inotify failure
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (4 preceding siblings ...)
2020-06-22 10:42 ` [PATCH 05/22] tests: Use error_free_or_abort() " Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 10:42 ` [PATCH 07/22] spapr: Plug minor memory leak in spapr_machine_init() Markus Armbruster
` (15 subsequent siblings)
21 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel P . Berrangé, Gerd Hoffmann, qemu-stable
error_report_err() frees its first argument. Freeing it again is
wrong. Don't.
Fixes: 47287c27d0c367a89f7b2851e23a7f8b2d499dd6
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/usb/dev-mtp.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 168428156b..15a2243101 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -634,7 +634,6 @@ static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
error_reportf_err(err,
"usb-mtp: failed to add watch for %s: ",
o->path);
- error_free(err);
} else {
trace_usb_mtp_file_monitor_event(s->dev.addr, o->path,
"Watch Added");
@@ -1279,7 +1278,6 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
if (err) {
error_reportf_err(err,
"usb-mtp: file monitoring init failed: ");
- error_free(err);
} else {
QTAILQ_INIT(&s->events);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 07/22] spapr: Plug minor memory leak in spapr_machine_init()
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (5 preceding siblings ...)
2020-06-22 10:42 ` [PATCH 06/22] usb/dev-mtp: Fix Error double free after inotify failure Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 14:54 ` Greg Kurz
2020-06-22 10:42 ` [PATCH 08/22] qga: Plug unlikely memory leak in guest-set-memory-blocks Markus Armbruster
` (14 subsequent siblings)
21 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel; +Cc: David Gibson, qemu-ppc
spapr_machine_init() leaks an Error object when
kvmppc_check_papr_resize_hpt() fails and spapr->resize_hpt is
SPAPR_RESIZE_HPT_DISABLED, i.e. when the host doesn't support hash
page table resizing, and the user didn't ask for it. As harmless as
memory leaks can possibly be. Plug it.
Fixes: 30f4b05bd090564181554d0890605eb2c143e4ea
Cc: David Gibson <dgibson@redhat.com>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/ppc/spapr.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bd9345cdac..9bd2183ead 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2731,6 +2731,7 @@ static void spapr_machine_init(MachineState *machine)
error_report_err(resize_hpt_err);
exit(1);
}
+ error_free(resize_hpt_err);
spapr->rma_size = spapr_rma_size(spapr, &error_fatal);
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 08/22] qga: Plug unlikely memory leak in guest-set-memory-blocks
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (6 preceding siblings ...)
2020-06-22 10:42 ` [PATCH 07/22] spapr: Plug minor memory leak in spapr_machine_init() Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-23 8:35 ` Zhanghailiang
2020-06-22 10:42 ` [PATCH 09/22] sd/milkymist-memcard: Plug minor memory leak in realize Markus Armbruster
` (13 subsequent siblings)
21 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, Hailiang Zhang
transfer_memory_block() leaks an Error object when reading file
/sys/devices/system/memory/memory<INDEX>/state fails with errno other
than ENOENT, and @sys2memblk is false, i.e. when the state file exists
but cannot be read (seems quite unlikely), and this is
guest-set-memory-blocks, not guest-get-memory-blocks.
Plug the leak.
Fixes: bd240fca42d5f072fb758a71720d9de9990ac553
Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Cc: Hailiang Zhang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qga/commands-posix.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index ae1348dc8f..cdbeb59dcc 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2421,6 +2421,7 @@ static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
if (sys2memblk) {
error_propagate(errp, local_err);
} else {
+ error_free(local_err);
result->response =
GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_FAILED;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 09/22] sd/milkymist-memcard: Plug minor memory leak in realize
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (7 preceding siblings ...)
2020-06-22 10:42 ` [PATCH 08/22] qga: Plug unlikely memory leak in guest-set-memory-blocks Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 10:42 ` [PATCH 10/22] test-util-filemonitor: Plug unlikely memory leak Markus Armbruster
` (12 subsequent siblings)
21 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Walle, Philippe Mathieu-Daudé
milkymist_memcard_realize() leaks an Error object when realization of
its "sd-card" device fails. Quite harmless, since we only ever
realize this once, in milkymist_init() via milkymist_memcard_create().
Plug the leak.
Fixes: 3d0369ba499866cc6a839f71212d97876500762d
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Michael Walle <michael@walle.cc>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/sd/milkymist-memcard.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index 482e97191e..afdb8aa0c0 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -280,9 +280,8 @@ static void milkymist_memcard_realize(DeviceState *dev, Error **errp)
blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
carddev = qdev_new(TYPE_SD_CARD);
qdev_prop_set_drive(carddev, "drive", blk);
- qdev_realize_and_unref(carddev, BUS(&s->sdbus), &err);
- if (err) {
- error_setg(errp, "failed to init SD card: %s", error_get_pretty(err));
+ if (!qdev_realize_and_unref(carddev, BUS(&s->sdbus), &err)) {
+ error_propagate_prepend(errp, err, "failed to init SD card: %s");
return;
}
s->enabled = blk && blk_is_inserted(blk);
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 10/22] test-util-filemonitor: Plug unlikely memory leak
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (8 preceding siblings ...)
2020-06-22 10:42 ` [PATCH 09/22] sd/milkymist-memcard: Plug minor memory leak in realize Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 10:42 ` [PATCH 11/22] vnc: Plug minor memory leak in vnc_display_open() Markus Armbruster
` (11 subsequent siblings)
21 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel P . Berrangé
test_file_monitor_events() leaks an Error object when
qemu_file_monitor_add_watch() fails, which seems unlikely. Plug it.
Cc: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/test-util-filemonitor.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/test-util-filemonitor.c b/tests/test-util-filemonitor.c
index 45009c69f4..8f0eff3d03 100644
--- a/tests/test-util-filemonitor.c
+++ b/tests/test-util-filemonitor.c
@@ -495,6 +495,7 @@ test_file_monitor_events(void)
if (*op->watchid < 0) {
g_printerr("Unable to add watch %s",
error_get_pretty(local_err));
+ error_free(local_err);
goto cleanup;
}
if (debug) {
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 11/22] vnc: Plug minor memory leak in vnc_display_open()
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (9 preceding siblings ...)
2020-06-22 10:42 ` [PATCH 10/22] test-util-filemonitor: Plug unlikely memory leak Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 10:42 ` [PATCH 12/22] tests/qom-proplist: Delete a superfluous error_free() Markus Armbruster
` (10 subsequent siblings)
21 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel P . Berrange, Gerd Hoffmann
vnc_display_print_local_addr() leaks the Error object when
qio_channel_socket_get_local_address() fails. Seems unlikely. Called
when we create a VNC display with vnc_display_open(). Plug the leak
by passing NULL to ignore the error.
Cc: Daniel P. Berrange <berrange@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
ui/vnc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index 0702a76cce..527ad25124 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3274,13 +3274,12 @@ int vnc_display_pw_expire(const char *id, time_t expires)
static void vnc_display_print_local_addr(VncDisplay *vd)
{
SocketAddress *addr;
- Error *err = NULL;
if (!vd->listener || !vd->listener->nsioc) {
return;
}
- addr = qio_channel_socket_get_local_address(vd->listener->sioc[0], &err);
+ addr = qio_channel_socket_get_local_address(vd->listener->sioc[0], NULL);
if (!addr) {
return;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 12/22] tests/qom-proplist: Delete a superfluous error_free()
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (10 preceding siblings ...)
2020-06-22 10:42 ` [PATCH 11/22] vnc: Plug minor memory leak in vnc_display_open() Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 10:42 ` [PATCH 13/22] aspeed: Clean up roundabout error propagation Markus Armbruster
` (9 subsequent siblings)
21 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/check-qom-proplist.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index a44d6e1171..347a866319 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -421,7 +421,6 @@ static void test_dummy_createcmdl(void)
user_creatable_del("dev0", &err);
g_assert(err == NULL);
- error_free(err);
object_unref(OBJECT(dobj));
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 13/22] aspeed: Clean up roundabout error propagation
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (11 preceding siblings ...)
2020-06-22 10:42 ` [PATCH 12/22] tests/qom-proplist: Delete a superfluous error_free() Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 12:37 ` Cédric Le Goater
2020-06-22 10:42 ` [PATCH 14/22] qdev: Drop qbus_set_bus_hotplug_handler() parameter @errp Markus Armbruster
` (8 subsequent siblings)
21 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Cédric Le Goater
Replace
sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &local_err);
error_propagate(&err, local_err);
if (err) {
error_propagate(errp, err);
return;
}
by
sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &err);
if (err) {
error_propagate(errp, err);
return;
}
Cc: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/arm/aspeed_ast2600.c | 10 ++++------
hw/arm/aspeed_soc.c | 10 ++++------
2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 6da687299f..08b3592e36 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -228,7 +228,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
int i;
AspeedSoCState *s = ASPEED_SOC(dev);
AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
- Error *err = NULL, *local_err = NULL;
+ Error *err = NULL;
qemu_irq irq;
/* IO space */
@@ -394,8 +394,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
return;
}
object_property_set_int(OBJECT(&s->spi[i]), 1, "num-cs", &err);
- sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &local_err);
- error_propagate(&err, local_err);
+ sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &err);
if (err) {
error_propagate(errp, err);
return;
@@ -446,11 +445,10 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
for (i = 0; i < sc->macs_num; i++) {
object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed",
&err);
- sysbus_realize(SYS_BUS_DEVICE(&s->ftgmac100[i]), &local_err);
- error_propagate(&err, local_err);
+ sysbus_realize(SYS_BUS_DEVICE(&s->ftgmac100[i]), &err);
if (err) {
error_propagate(errp, err);
- return;
+ return;
}
sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100[i]), 0,
sc->memmap[ASPEED_ETH1 + i]);
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 810cf9b6cc..ec21de50ce 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -218,7 +218,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
int i;
AspeedSoCState *s = ASPEED_SOC(dev);
AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
- Error *err = NULL, *local_err = NULL;
+ Error *err = NULL;
/* IO space */
create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM],
@@ -340,8 +340,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
/* SPI */
for (i = 0; i < sc->spis_num; i++) {
object_property_set_int(OBJECT(&s->spi[i]), 1, "num-cs", &err);
- sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &local_err);
- error_propagate(&err, local_err);
+ sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &err);
if (err) {
error_propagate(errp, err);
return;
@@ -392,11 +391,10 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
for (i = 0; i < sc->macs_num; i++) {
object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed",
&err);
- sysbus_realize(SYS_BUS_DEVICE(&s->ftgmac100[i]), &local_err);
- error_propagate(&err, local_err);
+ sysbus_realize(SYS_BUS_DEVICE(&s->ftgmac100[i]), &err);
if (err) {
error_propagate(errp, err);
- return;
+ return;
}
sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100[i]), 0,
sc->memmap[ASPEED_ETH1 + i]);
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 14/22] qdev: Drop qbus_set_bus_hotplug_handler() parameter @errp
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (12 preceding siblings ...)
2020-06-22 10:42 ` [PATCH 13/22] aspeed: Clean up roundabout error propagation Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 10:42 ` [PATCH 15/22] qdev: Drop qbus_set_hotplug_handler() " Markus Armbruster
` (7 subsequent siblings)
21 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost
All callers pass &error_abort. Drop the parameter.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/hw/qdev-core.h | 2 +-
hw/core/bus.c | 4 ++--
hw/scsi/scsi-bus.c | 2 +-
hw/usb/bus.c | 2 +-
hw/xen/xen-bus.c | 2 +-
hw/xen/xen-legacy-backend.c | 2 +-
6 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 7dc10be46f..78acdeaed6 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -537,7 +537,7 @@ char *qdev_get_dev_path(DeviceState *dev);
void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error **errp);
-void qbus_set_bus_hotplug_handler(BusState *bus, Error **errp);
+void qbus_set_bus_hotplug_handler(BusState *bus);
static inline bool qbus_is_hotpluggable(BusState *bus)
{
diff --git a/hw/core/bus.c b/hw/core/bus.c
index 6cc28b334e..8d4e810d7f 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -29,9 +29,9 @@ void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error **errp)
QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
}
-void qbus_set_bus_hotplug_handler(BusState *bus, Error **errp)
+void qbus_set_bus_hotplug_handler(BusState *bus)
{
- qbus_set_hotplug_handler(bus, OBJECT(bus), errp);
+ qbus_set_hotplug_handler(bus, OBJECT(bus), &error_abort);
}
int qbus_walk_children(BusState *bus,
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 27843bb04b..b878a08080 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -107,7 +107,7 @@ void scsi_bus_new(SCSIBus *bus, size_t bus_size, DeviceState *host,
qbus_create_inplace(bus, bus_size, TYPE_SCSI_BUS, host, bus_name);
bus->busnr = next_scsi_bus++;
bus->info = info;
- qbus_set_bus_hotplug_handler(BUS(bus), &error_abort);
+ qbus_set_bus_hotplug_handler(BUS(bus));
}
static void scsi_dma_restart_bh(void *opaque)
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index a81aee2051..957559b18d 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -84,7 +84,7 @@ void usb_bus_new(USBBus *bus, size_t bus_size,
USBBusOps *ops, DeviceState *host)
{
qbus_create_inplace(bus, bus_size, TYPE_USB_BUS, host, NULL);
- qbus_set_bus_hotplug_handler(BUS(bus), &error_abort);
+ qbus_set_bus_hotplug_handler(BUS(bus));
bus->ops = ops;
bus->busnr = next_usb_bus++;
QTAILQ_INIT(&bus->free);
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 4b00320f1c..c4e2162ae9 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -1391,5 +1391,5 @@ void xen_bus_init(void)
BusState *bus = qbus_create(TYPE_XEN_BUS, dev, NULL);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
- qbus_set_bus_hotplug_handler(bus, &error_abort);
+ qbus_set_bus_hotplug_handler(bus);
}
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index 2335ee2e65..7d4b13351e 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -705,7 +705,7 @@ int xen_be_init(void)
xen_sysdev = qdev_new(TYPE_XENSYSDEV);
sysbus_realize_and_unref(SYS_BUS_DEVICE(xen_sysdev), &error_fatal);
xen_sysbus = qbus_create(TYPE_XENSYSBUS, xen_sysdev, "xen-sysbus");
- qbus_set_bus_hotplug_handler(xen_sysbus, &error_abort);
+ qbus_set_bus_hotplug_handler(xen_sysbus);
return 0;
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 15/22] qdev: Drop qbus_set_hotplug_handler() parameter @errp
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (13 preceding siblings ...)
2020-06-22 10:42 ` [PATCH 14/22] qdev: Drop qbus_set_bus_hotplug_handler() parameter @errp Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 10:42 ` [PATCH 16/22] hw: Fix error API violation around object_property_set_link() Markus Armbruster
` (6 subsequent siblings)
21 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost
qbus_set_hotplug_handler() is a simple wrapper around
object_property_set_link().
object_property_set_link() fails when the property doesn't exist, is
not settable, or its .check() method fails. These are all programming
errors here, so passing &error_abort to qbus_set_hotplug_handler() is
appropriate.
Most of its callers do. Exceptions:
* pcie_cap_slot_init(), shpc_init(), spapr_phb_realize() pass NULL,
i.e. they ignore errors.
* spapr_machine_init() passes &error_fatal.
* s390_pcihost_realize(), virtio_serial_device_realize(),
s390_pcihost_plug() pass the error to their callers. The latter two
keep going after the error, which looks wrong.
Drop the @errp parameter, and instead pass &error_abort to
object_property_set_link().
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/hw/qdev-core.h | 3 +--
hw/acpi/pcihp.c | 3 +--
hw/acpi/piix4.c | 2 +-
hw/char/virtio-serial-bus.c | 4 ++--
hw/core/bus.c | 6 +++---
hw/pci/pcie.c | 2 +-
hw/pci/shpc.c | 2 +-
hw/ppc/spapr.c | 3 +--
hw/ppc/spapr_pci.c | 4 ++--
hw/s390x/ap-bridge.c | 2 +-
hw/s390x/css-bridge.c | 2 +-
hw/s390x/s390-pci-bus.c | 14 +++-----------
hw/scsi/virtio-scsi.c | 4 ++--
hw/scsi/vmw_pvscsi.c | 2 +-
hw/usb/dev-smartcard-reader.c | 2 +-
15 files changed, 22 insertions(+), 33 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 78acdeaed6..fe78073c70 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -535,8 +535,7 @@ extern bool qdev_hot_removed;
char *qdev_get_dev_path(DeviceState *dev);
-void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error **errp);
-
+void qbus_set_hotplug_handler(BusState *bus, Object *handler);
void qbus_set_bus_hotplug_handler(BusState *bus);
static inline bool qbus_is_hotpluggable(BusState *bus)
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 33ea2b76ae..9e31ab2da4 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -246,8 +246,7 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
PCIBus *sec = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
- qbus_set_hotplug_handler(BUS(sec), OBJECT(hotplug_dev),
- &error_abort);
+ qbus_set_hotplug_handler(BUS(sec), OBJECT(hotplug_dev));
/* We don't have to overwrite any other hotplug handler yet */
assert(QLIST_EMPTY(&sec->child));
}
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 1262abc77a..52bcbd6414 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -505,7 +505,7 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
piix4_acpi_system_hot_add_init(pci_address_space_io(dev),
pci_get_bus(dev), s);
- qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), OBJECT(s), &error_abort);
+ qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), OBJECT(s));
piix4_pm_add_propeties(s);
}
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 262089c0c9..f9a4428bd6 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -1056,7 +1056,7 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
/* Spawn a new virtio-serial bus on which the ports will ride as devices */
qbus_create_inplace(&vser->bus, sizeof(vser->bus), TYPE_VIRTIO_SERIAL_BUS,
dev, vdev->bus_name);
- qbus_set_hotplug_handler(BUS(&vser->bus), OBJECT(vser), errp);
+ qbus_set_hotplug_handler(BUS(&vser->bus), OBJECT(vser));
vser->bus.vser = vser;
QTAILQ_INIT(&vser->ports);
@@ -1147,7 +1147,7 @@ static void virtio_serial_device_unrealize(DeviceState *dev)
g_free(vser->post_load);
}
- qbus_set_hotplug_handler(BUS(&vser->bus), NULL, &error_abort);
+ qbus_set_hotplug_handler(BUS(&vser->bus), NULL);
virtio_cleanup(vdev);
}
diff --git a/hw/core/bus.c b/hw/core/bus.c
index 8d4e810d7f..544dd8a6fa 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -23,15 +23,15 @@
#include "qemu/module.h"
#include "qapi/error.h"
-void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error **errp)
+void qbus_set_hotplug_handler(BusState *bus, Object *handler)
{
object_property_set_link(OBJECT(bus), handler,
- QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
+ QDEV_HOTPLUG_HANDLER_PROPERTY, &error_abort);
}
void qbus_set_bus_hotplug_handler(BusState *bus)
{
- qbus_set_hotplug_handler(bus, OBJECT(bus), &error_abort);
+ qbus_set_hotplug_handler(bus, OBJECT(bus));
}
int qbus_walk_children(BusState *bus,
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 086d0dfceb..5b48bae0f6 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -574,7 +574,7 @@ void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s)
dev->exp.hpev_notified = false;
qbus_set_hotplug_handler(BUS(pci_bridge_get_sec_bus(PCI_BRIDGE(dev))),
- OBJECT(dev), NULL);
+ OBJECT(dev));
}
void pcie_cap_slot_reset(PCIDevice *dev)
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 99d65d5c4c..b00dce629c 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -649,7 +649,7 @@ int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar,
shpc_cap_update_dword(d);
memory_region_add_subregion(bar, offset, &shpc->mmio);
- qbus_set_hotplug_handler(BUS(sec_bus), OBJECT(d), NULL);
+ qbus_set_hotplug_handler(BUS(sec_bus), OBJECT(d));
d->cap_present |= QEMU_PCI_CAP_SHPC;
return 0;
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 9bd2183ead..83b974870c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3033,8 +3033,7 @@ static void spapr_machine_init(MachineState *machine)
register_savevm_live("spapr/htab", VMSTATE_INSTANCE_ID_ANY, 1,
&savevm_htab_handlers, spapr);
- qbus_set_hotplug_handler(sysbus_get_default(), OBJECT(machine),
- &error_fatal);
+ qbus_set_hotplug_handler(sysbus_get_default(), OBJECT(machine));
qemu_register_boot_set(spapr_boot_set, spapr);
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 329002ac04..0f00e2421f 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1719,7 +1719,7 @@ static void spapr_phb_unrealize(DeviceState *dev)
address_space_remove_listeners(&sphb->iommu_as);
address_space_destroy(&sphb->iommu_as);
- qbus_set_hotplug_handler(BUS(phb->bus), NULL, &error_abort);
+ qbus_set_hotplug_handler(BUS(phb->bus), NULL);
pci_unregister_root_bus(phb->bus);
memory_region_del_subregion(get_system_memory(), &sphb->iowindow);
@@ -1868,7 +1868,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
}
phb->bus = bus;
- qbus_set_hotplug_handler(BUS(phb->bus), OBJECT(sphb), NULL);
+ qbus_set_hotplug_handler(BUS(phb->bus), OBJECT(sphb));
/*
* Initialize PHB address space.
diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c
index c4e3188ad6..8bcf8ece9d 100644
--- a/hw/s390x/ap-bridge.c
+++ b/hw/s390x/ap-bridge.c
@@ -58,7 +58,7 @@ void s390_init_ap(void)
bus = qbus_create(TYPE_AP_BUS, dev, TYPE_AP_BUS);
/* Enable hotplugging */
- qbus_set_hotplug_handler(bus, OBJECT(dev), &error_abort);
+ qbus_set_hotplug_handler(bus, OBJECT(dev));
}
static void ap_bridge_class_init(ObjectClass *oc, void *data)
diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
index e37a54d3f2..9d793d671e 100644
--- a/hw/s390x/css-bridge.c
+++ b/hw/s390x/css-bridge.c
@@ -111,7 +111,7 @@ VirtualCssBus *virtual_css_bus_init(void)
cbus = VIRTUAL_CSS_BUS(bus);
/* Enable hotplugging */
- qbus_set_hotplug_handler(bus, OBJECT(dev), &error_abort);
+ qbus_set_hotplug_handler(bus, OBJECT(dev));
css_register_io_adapters(CSS_IO_ADAPTER_VIRTIO, true, false,
0, &error_abort);
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index a13978bb37..142e52a8ff 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -751,19 +751,11 @@ static void s390_pcihost_realize(DeviceState *dev, Error **errp)
pci_setup_iommu(b, s390_pci_dma_iommu, s);
bus = BUS(b);
- qbus_set_hotplug_handler(bus, OBJECT(dev), &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
+ qbus_set_hotplug_handler(bus, OBJECT(dev));
phb->bus = b;
s->bus = S390_PCI_BUS(qbus_create(TYPE_S390_PCI_BUS, dev, NULL));
- qbus_set_hotplug_handler(BUS(s->bus), OBJECT(dev), &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
+ qbus_set_hotplug_handler(BUS(s->bus), OBJECT(dev));
s->iommu_table = g_hash_table_new_full(g_int64_hash, g_int64_equal,
NULL, g_free);
@@ -921,7 +913,7 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
pci_bridge_map_irq(pb, dev->id, s390_pci_map_irq);
pci_setup_iommu(&pb->sec_bus, s390_pci_dma_iommu, s);
- qbus_set_hotplug_handler(BUS(&pb->sec_bus), OBJECT(s), errp);
+ qbus_set_hotplug_handler(BUS(&pb->sec_bus), OBJECT(s));
if (dev->hotplugged) {
pci_default_write_config(pdev, PCI_PRIMARY_BUS,
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 9b72094a61..b49775269e 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -934,7 +934,7 @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
scsi_bus_new(&s->bus, sizeof(s->bus), dev,
&virtio_scsi_scsi_info, vdev->bus_name);
/* override default SCSI bus hotplug-handler, with virtio-scsi's one */
- qbus_set_hotplug_handler(BUS(&s->bus), OBJECT(dev), &error_abort);
+ qbus_set_hotplug_handler(BUS(&s->bus), OBJECT(dev));
virtio_scsi_dataplane_setup(s, errp);
}
@@ -958,7 +958,7 @@ static void virtio_scsi_device_unrealize(DeviceState *dev)
{
VirtIOSCSI *s = VIRTIO_SCSI(dev);
- qbus_set_hotplug_handler(BUS(&s->bus), NULL, &error_abort);
+ qbus_set_hotplug_handler(BUS(&s->bus), NULL);
virtio_scsi_common_unrealize(dev);
}
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index ec5bf9ea34..df07ab6bfb 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1147,7 +1147,7 @@ pvscsi_realizefn(PCIDevice *pci_dev, Error **errp)
scsi_bus_new(&s->bus, sizeof(s->bus), DEVICE(pci_dev),
&pvscsi_scsi_info, NULL);
/* override default SCSI bus hotplug-handler, with pvscsi's one */
- qbus_set_hotplug_handler(BUS(&s->bus), OBJECT(s), &error_abort);
+ qbus_set_hotplug_handler(BUS(&s->bus), OBJECT(s));
pvscsi_reset_state(s);
}
diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index ada18c1983..fcfe216594 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -1320,7 +1320,7 @@ static void ccid_realize(USBDevice *dev, Error **errp)
usb_desc_init(dev);
qbus_create_inplace(&s->bus, sizeof(s->bus), TYPE_CCID_BUS, DEVICE(dev),
NULL);
- qbus_set_hotplug_handler(BUS(&s->bus), OBJECT(dev), &error_abort);
+ qbus_set_hotplug_handler(BUS(&s->bus), OBJECT(dev));
s->intr = usb_ep_get(dev, USB_TOKEN_IN, CCID_INT_IN_EP);
s->bulk = usb_ep_get(dev, USB_TOKEN_IN, CCID_BULK_IN_EP);
s->card = NULL;
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 16/22] hw: Fix error API violation around object_property_set_link()
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (14 preceding siblings ...)
2020-06-22 10:42 ` [PATCH 15/22] qdev: Drop qbus_set_hotplug_handler() " Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 12:47 ` Auger Eric
2020-06-22 21:22 ` Alistair Francis
2020-06-22 10:42 ` [PATCH 17/22] hw/arm: Drop useless object_property_set_link() error handling Markus Armbruster
` (5 subsequent siblings)
21 siblings, 2 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Aleksandar Rikalo, Alistair Francis,
Mark Cave-Ayland, Eric Auger, Aleksandar Markovic, qemu-arm,
Gerd Hoffmann, Edgar E. Iglesias, Aurelien Jarno
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.
virtio_gpu_pci_base_realize(), virtio_vga_base_realize(),
sparc32_ledma_device_realize(), sparc32_dma_realize(),
sparc32_dma_realize() xilinx_axidma_realize(), mips_cps_realize(),
macio_realize_ide(), xilinx_enet_realize(), and
virtio_iommu_pci_realize() are wrong that way: they reuse the argument
they pass to object_property_set_link() for another call.
Harmless, because object_property_set_link() can't actually fail for
them: it fails when the property doesn't exist, is not settable, or
its .check() method fails. Fix by passing &error_abort instead.
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Alistair Francis <alistair@alistair23.me>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Cc: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>
Cc: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/display/virtio-gpu-pci.c | 2 +-
hw/display/virtio-vga.c | 2 +-
hw/dma/sparc32_dma.c | 6 +++---
hw/dma/xilinx_axidma.c | 12 ++----------
hw/mips/cps.c | 6 ++++--
hw/misc/macio/macio.c | 3 ++-
hw/net/xilinx_axienet.c | 12 ++----------
hw/virtio/virtio-iommu-pci.c | 2 +-
8 files changed, 16 insertions(+), 29 deletions(-)
diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
index b532fe8b5f..41b88b878d 100644
--- a/hw/display/virtio-gpu-pci.c
+++ b/hw/display/virtio-gpu-pci.c
@@ -44,7 +44,7 @@ static void virtio_gpu_pci_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
for (i = 0; i < g->conf.max_outputs; i++) {
object_property_set_link(OBJECT(g->scanout[i].con),
OBJECT(vpci_dev),
- "device", errp);
+ "device", &error_abort);
}
}
diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index 68a062ece6..67f409e106 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -154,7 +154,7 @@ static void virtio_vga_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
for (i = 0; i < g->conf.max_outputs; i++) {
object_property_set_link(OBJECT(g->scanout[i].con),
OBJECT(vpci_dev),
- "device", errp);
+ "device", &error_abort);
}
}
diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
index f02aca6f40..2d7dbbb92d 100644
--- a/hw/dma/sparc32_dma.c
+++ b/hw/dma/sparc32_dma.c
@@ -346,7 +346,7 @@ static void sparc32_ledma_device_realize(DeviceState *dev, Error **errp)
d = qdev_new(TYPE_LANCE);
object_property_add_child(OBJECT(dev), "lance", OBJECT(d));
qdev_set_nic_properties(d, nd);
- object_property_set_link(OBJECT(d), OBJECT(dev), "dma", errp);
+ object_property_set_link(OBJECT(d), OBJECT(dev), "dma", &error_abort);
sysbus_realize_and_unref(SYS_BUS_DEVICE(d), &error_fatal);
}
@@ -379,7 +379,7 @@ static void sparc32_dma_realize(DeviceState *dev, Error **errp)
}
espdma = qdev_new(TYPE_SPARC32_ESPDMA_DEVICE);
- object_property_set_link(OBJECT(espdma), iommu, "iommu", errp);
+ object_property_set_link(OBJECT(espdma), iommu, "iommu", &error_abort);
object_property_add_child(OBJECT(s), "espdma", OBJECT(espdma));
sysbus_realize_and_unref(SYS_BUS_DEVICE(espdma), &error_fatal);
@@ -394,7 +394,7 @@ static void sparc32_dma_realize(DeviceState *dev, Error **errp)
sysbus_mmio_get_region(sbd, 0));
ledma = qdev_new(TYPE_SPARC32_LEDMA_DEVICE);
- object_property_set_link(OBJECT(ledma), iommu, "iommu", errp);
+ object_property_set_link(OBJECT(ledma), iommu, "iommu", &error_abort);
object_property_add_child(OBJECT(s), "ledma", OBJECT(ledma));
sysbus_realize_and_unref(SYS_BUS_DEVICE(ledma), &error_fatal);
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index 6a9df2c4db..a069637bf2 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -537,7 +537,6 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
XilinxAXIDMAStreamSlave *ds = XILINX_AXI_DMA_DATA_STREAM(&s->rx_data_dev);
XilinxAXIDMAStreamSlave *cs = XILINX_AXI_DMA_CONTROL_STREAM(
&s->rx_control_dev);
- Error *local_err = NULL;
int i;
object_property_add_link(OBJECT(ds), "dma", TYPE_XILINX_AXI_DMA,
@@ -548,11 +547,8 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
(Object **)&cs->dma,
object_property_allow_set_link,
OBJ_PROP_LINK_STRONG);
- object_property_set_link(OBJECT(ds), OBJECT(s), "dma", &local_err);
- object_property_set_link(OBJECT(cs), OBJECT(s), "dma", &local_err);
- if (local_err) {
- goto xilinx_axidma_realize_fail;
- }
+ object_property_set_link(OBJECT(ds), OBJECT(s), "dma", &error_abort);
+ object_property_set_link(OBJECT(cs), OBJECT(s), "dma", &error_abort);
for (i = 0; i < 2; i++) {
struct Stream *st = &s->streams[i];
@@ -567,10 +563,6 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
address_space_init(&s->as,
s->dma_mr ? s->dma_mr : get_system_memory(), "dma");
- return;
-
-xilinx_axidma_realize_fail:
- error_propagate(errp, local_err);
}
static void xilinx_axidma_init(Object *obj)
diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index cdfab19826..5382bc86f7 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -150,8 +150,10 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
object_property_set_int(OBJECT(&s->gcr), s->num_vp, "num-vp", &err);
object_property_set_int(OBJECT(&s->gcr), 0x800, "gcr-rev", &err);
object_property_set_int(OBJECT(&s->gcr), gcr_base, "gcr-base", &err);
- object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->gic.mr), "gic", &err);
- object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->cpc.mr), "cpc", &err);
+ object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->gic.mr), "gic",
+ &error_abort);
+ object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->cpc.mr), "cpc",
+ &error_abort);
sysbus_realize(SYS_BUS_DEVICE(&s->gcr), &err);
if (err != NULL) {
error_propagate(errp, err);
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index 8ba7af073c..3251c79f46 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -136,7 +136,8 @@ static void macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
sysbus_connect_irq(sysbus_dev, 0, irq0);
sysbus_connect_irq(sysbus_dev, 1, irq1);
qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
- object_property_set_link(OBJECT(ide), OBJECT(&s->dbdma), "dbdma", errp);
+ object_property_set_link(OBJECT(ide), OBJECT(&s->dbdma), "dbdma",
+ &error_abort);
macio_ide_register_dma(ide);
qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
index c2f40b8ea9..679a359f9a 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -980,7 +980,6 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
XilinxAXIEnetStreamSlave *ds = XILINX_AXI_ENET_DATA_STREAM(&s->rx_data_dev);
XilinxAXIEnetStreamSlave *cs = XILINX_AXI_ENET_CONTROL_STREAM(
&s->rx_control_dev);
- Error *local_err = NULL;
object_property_add_link(OBJECT(ds), "enet", "xlnx.axi-ethernet",
(Object **) &ds->enet,
@@ -990,11 +989,8 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
(Object **) &cs->enet,
object_property_allow_set_link,
OBJ_PROP_LINK_STRONG);
- object_property_set_link(OBJECT(ds), OBJECT(s), "enet", &local_err);
- object_property_set_link(OBJECT(cs), OBJECT(s), "enet", &local_err);
- if (local_err) {
- goto xilinx_enet_realize_fail;
- }
+ object_property_set_link(OBJECT(ds), OBJECT(s), "enet", &error_abort);
+ object_property_set_link(OBJECT(cs), OBJECT(s), "enet", &error_abort);
qemu_macaddr_default_if_unset(&s->conf.macaddr);
s->nic = qemu_new_nic(&net_xilinx_enet_info, &s->conf,
@@ -1008,10 +1004,6 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
s->rxmem = g_malloc(s->c_rxmem);
s->txmem = g_malloc(s->c_txmem);
- return;
-
-xilinx_enet_realize_fail:
- error_propagate(errp, local_err);
}
static void xilinx_enet_init(Object *obj)
diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c
index 632533abaf..bd61d6e2f8 100644
--- a/hw/virtio/virtio-iommu-pci.c
+++ b/hw/virtio/virtio-iommu-pci.c
@@ -56,7 +56,7 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
}
object_property_set_link(OBJECT(dev),
OBJECT(pci_get_bus(&vpci_dev->pci_dev)),
- "primary-bus", errp);
+ "primary-bus", &error_abort);
qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 17/22] hw/arm: Drop useless object_property_set_link() error handling
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (15 preceding siblings ...)
2020-06-22 10:42 ` [PATCH 16/22] hw: Fix error API violation around object_property_set_link() Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 12:37 ` Cédric Le Goater
2020-06-22 10:42 ` Markus Armbruster
` (4 subsequent siblings)
21 siblings, 1 reply; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, Cédric Le Goater,
Joel Stanley
object_property_set_link() fails when the property doesn't exist, is
not settable, or its .check() method fails. These are all programming
errors here, so passing it &error_abort is appropriate.
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: "Cédric Le Goater" <clg@kaod.org>
Cc: Andrew Jeffery <andrew@aj.id.au>
Cc: Joel Stanley <joel@jms.id.au>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/arm/armsse.c | 53 ++++++++++-------------------------------
hw/arm/armv7m.c | 7 ++----
hw/arm/aspeed_ast2600.c | 20 ++++------------
hw/arm/aspeed_soc.c | 14 ++++-------
hw/arm/nrf51_soc.c | 6 +----
5 files changed, 24 insertions(+), 76 deletions(-)
diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
index e8f8f60abc..c73cc6badf 100644
--- a/hw/arm/armsse.c
+++ b/hw/arm/armsse.c
@@ -564,16 +564,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
&s->container, -1);
}
object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
- "memory", &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
- object_property_set_link(cpuobj, OBJECT(s), "idau", &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
+ "memory", &error_abort);
+ object_property_set_link(cpuobj, OBJECT(s), "idau", &error_abort);
sysbus_realize(SYS_BUS_DEVICE(cpuobj), &err);
if (err) {
error_propagate(errp, err);
@@ -700,11 +692,7 @@ static void armsse_realize(DeviceState *dev, Error **errp)
return;
}
object_property_set_link(OBJECT(&s->mpc[i]), OBJECT(&s->sram[i]),
- "downstream", &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
+ "downstream", &error_abort);
sysbus_realize(SYS_BUS_DEVICE(&s->mpc[i]), &err);
if (err) {
error_propagate(errp, err);
@@ -755,11 +743,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(&s->timer0), 0,
armsse_get_common_irq_in(s, 3));
mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->timer0), 0);
- object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[0]", &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
+ object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[0]",
+ &error_abort);
qdev_prop_set_uint32(DEVICE(&s->timer1), "pclk-frq", s->mainclk_frq);
sysbus_realize(SYS_BUS_DEVICE(&s->timer1), &err);
@@ -770,12 +755,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(&s->timer1), 0,
armsse_get_common_irq_in(s, 4));
mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->timer1), 0);
- object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[1]", &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
-
+ object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[1]",
+ &error_abort);
qdev_prop_set_uint32(DEVICE(&s->dualtimer), "pclk-frq", s->mainclk_frq);
sysbus_realize(SYS_BUS_DEVICE(&s->dualtimer), &err);
@@ -786,11 +767,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(&s->dualtimer), 0,
armsse_get_common_irq_in(s, 5));
mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->dualtimer), 0);
- object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[2]", &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
+ object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[2]",
+ &error_abort);
if (info->has_mhus) {
/*
@@ -815,12 +793,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
port = g_strdup_printf("port[%d]", i + 3);
mr = sysbus_mmio_get_region(mhu_sbd, 0);
object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr),
- port, &err);
+ port, &error_abort);
g_free(port);
- if (err) {
- error_propagate(errp, err);
- return;
- }
/*
* Each MHU has an irq line for each CPU:
@@ -967,11 +941,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(&s->s32ktimer), 0,
armsse_get_common_irq_in(s, 2));
mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->s32ktimer), 0);
- object_property_set_link(OBJECT(&s->apb_ppc1), OBJECT(mr), "port[0]", &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
+ object_property_set_link(OBJECT(&s->apb_ppc1), OBJECT(mr), "port[0]",
+ &error_abort);
sysbus_realize(SYS_BUS_DEVICE(&s->apb_ppc1), &err);
if (err) {
diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index ce83586e03..3308211e9c 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -170,11 +170,8 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
object_property_set_link(OBJECT(s->cpu), OBJECT(&s->container), "memory",
&error_abort);
if (object_property_find(OBJECT(s->cpu), "idau", NULL)) {
- object_property_set_link(OBJECT(s->cpu), s->idau, "idau", &err);
- if (err != NULL) {
- error_propagate(errp, err);
- return;
- }
+ object_property_set_link(OBJECT(s->cpu), s->idau, "idau",
+ &error_abort);
}
if (object_property_find(OBJECT(s->cpu), "init-svtor", NULL)) {
object_property_set_uint(OBJECT(s->cpu), s->init_svtor,
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 08b3592e36..4efac02e2b 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -341,11 +341,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
}
/* I2C */
- object_property_set_link(OBJECT(&s->i2c), OBJECT(s->dram_mr), "dram", &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
+ object_property_set_link(OBJECT(&s->i2c), OBJECT(s->dram_mr), "dram",
+ &error_abort);
sysbus_realize(SYS_BUS_DEVICE(&s->i2c), &err);
if (err) {
error_propagate(errp, err);
@@ -363,11 +360,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
}
/* FMC, The number of CS is set at the board level */
- object_property_set_link(OBJECT(&s->fmc), OBJECT(s->dram_mr), "dram", &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
+ object_property_set_link(OBJECT(&s->fmc), OBJECT(s->dram_mr), "dram",
+ &error_abort);
object_property_set_int(OBJECT(&s->fmc), sc->memmap[ASPEED_SDRAM],
"sdram-base", &err);
if (err) {
@@ -388,11 +382,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
/* SPI */
for (i = 0; i < sc->spis_num; i++) {
object_property_set_link(OBJECT(&s->spi[i]), OBJECT(s->dram_mr),
- "dram", &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
+ "dram", &error_abort);
object_property_set_int(OBJECT(&s->spi[i]), 1, "num-cs", &err);
sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &err);
if (err) {
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index ec21de50ce..03b91bade6 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -300,11 +300,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
}
/* I2C */
- object_property_set_link(OBJECT(&s->i2c), OBJECT(s->dram_mr), "dram", &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
+ object_property_set_link(OBJECT(&s->i2c), OBJECT(s->dram_mr), "dram",
+ &error_abort);
sysbus_realize(SYS_BUS_DEVICE(&s->i2c), &err);
if (err) {
error_propagate(errp, err);
@@ -315,11 +312,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
aspeed_soc_get_irq(s, ASPEED_I2C));
/* FMC, The number of CS is set at the board level */
- object_property_set_link(OBJECT(&s->fmc), OBJECT(s->dram_mr), "dram", &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
+ object_property_set_link(OBJECT(&s->fmc), OBJECT(s->dram_mr), "dram",
+ &error_abort);
object_property_set_int(OBJECT(&s->fmc), sc->memmap[ASPEED_SDRAM],
"sdram-base", &err);
if (err) {
diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 5a8961ddbb..20dd8b5897 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -66,11 +66,7 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
}
object_property_set_link(OBJECT(&s->cpu), OBJECT(&s->container), "memory",
- &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
+ &error_abort);
sysbus_realize(SYS_BUS_DEVICE(&s->cpu), &err);
if (err) {
error_propagate(errp, err);
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 18/22] riscv/sifive_u: Fix sifive_u_soc_realize() error API violations
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 10:42 ` [PATCH 02/22] pci: Delete useless error_propagate() Markus Armbruster
` (20 subsequent siblings)
21 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, Sagar Karandikar, Bastian Koppelmann,
Alistair Francis, Palmer Dabbelt, Bin Meng
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.
sifive_u_soc_realize() is wrong that way: it passes &err to
sysbus_realize() three times before checking it. Harmless, because
the first two can't actually fail (I think).
Fix by checking for failure right away.
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: qemu-riscv@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/riscv/sifive_u.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index ea197ab64f..3857b92d9a 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -587,11 +587,15 @@ static void sifive_u_soc_realize(DeviceState *dev, Error **errp)
memmap[SIFIVE_U_CLINT].size, ms->smp.cpus,
SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE, false);
- sysbus_realize(SYS_BUS_DEVICE(&s->prci), &err);
+ if (!sysbus_realize(SYS_BUS_DEVICE(&s->prci), errp)) {
+ return;
+ }
sysbus_mmio_map(SYS_BUS_DEVICE(&s->prci), 0, memmap[SIFIVE_U_PRCI].base);
qdev_prop_set_uint32(DEVICE(&s->otp), "serial", s->serial);
- sysbus_realize(SYS_BUS_DEVICE(&s->otp), &err);
+ if (!sysbus_realize(SYS_BUS_DEVICE(&s->otp), errp)) {
+ return;
+ }
sysbus_mmio_map(SYS_BUS_DEVICE(&s->otp), 0, memmap[SIFIVE_U_OTP].base);
for (i = 0; i < SIFIVE_U_PLIC_NUM_SOURCES; i++) {
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 18/22] riscv/sifive_u: Fix sifive_u_soc_realize() error API violations
@ 2020-06-22 10:42 ` Markus Armbruster
0 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel
Cc: Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
Bastian Koppelmann, Bin Meng, qemu-riscv
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.
sifive_u_soc_realize() is wrong that way: it passes &err to
sysbus_realize() three times before checking it. Harmless, because
the first two can't actually fail (I think).
Fix by checking for failure right away.
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: qemu-riscv@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/riscv/sifive_u.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index ea197ab64f..3857b92d9a 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -587,11 +587,15 @@ static void sifive_u_soc_realize(DeviceState *dev, Error **errp)
memmap[SIFIVE_U_CLINT].size, ms->smp.cpus,
SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE, false);
- sysbus_realize(SYS_BUS_DEVICE(&s->prci), &err);
+ if (!sysbus_realize(SYS_BUS_DEVICE(&s->prci), errp)) {
+ return;
+ }
sysbus_mmio_map(SYS_BUS_DEVICE(&s->prci), 0, memmap[SIFIVE_U_PRCI].base);
qdev_prop_set_uint32(DEVICE(&s->otp), "serial", s->serial);
- sysbus_realize(SYS_BUS_DEVICE(&s->otp), &err);
+ if (!sysbus_realize(SYS_BUS_DEVICE(&s->otp), errp)) {
+ return;
+ }
sysbus_mmio_map(SYS_BUS_DEVICE(&s->otp), 0, memmap[SIFIVE_U_OTP].base);
for (i = 0; i < SIFIVE_U_PLIC_NUM_SOURCES; i++) {
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 19/22] riscv_hart: Fix riscv_harts_realize() error API violations
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 10:42 ` [PATCH 02/22] pci: Delete useless error_propagate() Markus Armbruster
` (20 subsequent siblings)
21 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, Sagar Karandikar, Bastian Koppelmann,
Alistair Francis, Palmer Dabbelt, Bin Meng
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.
riscv_harts_realize() is wrong that way: it passes @errp to
riscv_hart_realize() in a loop. I can't tell offhand whether this can
fail.
Fix by checking for failure in each iteration.
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: qemu-riscv@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/riscv/riscv_hart.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
index e26c382259..f59fe52f0f 100644
--- a/hw/riscv/riscv_hart.c
+++ b/hw/riscv/riscv_hart.c
@@ -40,19 +40,13 @@ static void riscv_harts_cpu_reset(void *opaque)
cpu_reset(CPU(cpu));
}
-static void riscv_hart_realize(RISCVHartArrayState *s, int idx,
+static bool riscv_hart_realize(RISCVHartArrayState *s, int idx,
char *cpu_type, Error **errp)
{
- Error *err = NULL;
-
object_initialize_child(OBJECT(s), "harts[*]", &s->harts[idx], cpu_type);
s->harts[idx].env.mhartid = s->hartid_base + idx;
qemu_register_reset(riscv_harts_cpu_reset, &s->harts[idx]);
- qdev_realize(DEVICE(&s->harts[idx]), NULL, &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
+ return qdev_realize(DEVICE(&s->harts[idx]), NULL, errp);
}
static void riscv_harts_realize(DeviceState *dev, Error **errp)
@@ -63,7 +57,9 @@ static void riscv_harts_realize(DeviceState *dev, Error **errp)
s->harts = g_new0(RISCVCPU, s->num_harts);
for (n = 0; n < s->num_harts; n++) {
- riscv_hart_realize(s, n, s->cpu_type, errp);
+ if (!riscv_hart_realize(s, n, s->cpu_type, errp)) {
+ return;
+ }
}
}
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 19/22] riscv_hart: Fix riscv_harts_realize() error API violations
@ 2020-06-22 10:42 ` Markus Armbruster
0 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel
Cc: Palmer Dabbelt, Alistair Francis, Sagar Karandikar,
Bastian Koppelmann, Bin Meng, qemu-riscv
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.
riscv_harts_realize() is wrong that way: it passes @errp to
riscv_hart_realize() in a loop. I can't tell offhand whether this can
fail.
Fix by checking for failure in each iteration.
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: qemu-riscv@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/riscv/riscv_hart.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
index e26c382259..f59fe52f0f 100644
--- a/hw/riscv/riscv_hart.c
+++ b/hw/riscv/riscv_hart.c
@@ -40,19 +40,13 @@ static void riscv_harts_cpu_reset(void *opaque)
cpu_reset(CPU(cpu));
}
-static void riscv_hart_realize(RISCVHartArrayState *s, int idx,
+static bool riscv_hart_realize(RISCVHartArrayState *s, int idx,
char *cpu_type, Error **errp)
{
- Error *err = NULL;
-
object_initialize_child(OBJECT(s), "harts[*]", &s->harts[idx], cpu_type);
s->harts[idx].env.mhartid = s->hartid_base + idx;
qemu_register_reset(riscv_harts_cpu_reset, &s->harts[idx]);
- qdev_realize(DEVICE(&s->harts[idx]), NULL, &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
+ return qdev_realize(DEVICE(&s->harts[idx]), NULL, errp);
}
static void riscv_harts_realize(DeviceState *dev, Error **errp)
@@ -63,7 +57,9 @@ static void riscv_harts_realize(DeviceState *dev, Error **errp)
s->harts = g_new0(RISCVCPU, s->num_harts);
for (n = 0; n < s->num_harts; n++) {
- riscv_hart_realize(s, n, s->cpu_type, errp);
+ if (!riscv_hart_realize(s, n, s->cpu_type, errp)) {
+ return;
+ }
}
}
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 20/22] mips/cps: Fix mips_cps_realize() error API violations
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (18 preceding siblings ...)
2020-06-22 10:42 ` Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 10:42 ` [PATCH 21/22] x86: Fix x86_cpu_new() " Markus Armbruster
2020-06-22 10:42 ` [PATCH 22/22] amd_iommu: Fix amdvi_realize() error API violation Markus Armbruster
21 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Aleksandar Markovic, Aleksandar Rikalo, Aurelien Jarno
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.
mips_cps_realize() is wrong that way: it passes &err to multiple
object_property_set_FOO() without checking for failure, and then to
sysbus_realize(). Harmless, because the object_property_set_FOO()
can't actually fail here.
Fix by passing &error_abort instead.
Cc: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/mips/cps.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 5382bc86f7..0d7f3cf673 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -100,10 +100,12 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
/* Inter-Thread Communication Unit */
if (itu_present) {
object_initialize_child(OBJECT(dev), "itu", &s->itu, TYPE_MIPS_ITU);
- object_property_set_int(OBJECT(&s->itu), 16, "num-fifo", &err);
- object_property_set_int(OBJECT(&s->itu), 16, "num-semaphores", &err);
+ object_property_set_int(OBJECT(&s->itu), 16, "num-fifo",
+ &error_abort);
+ object_property_set_int(OBJECT(&s->itu), 16, "num-semaphores",
+ &error_abort);
object_property_set_bool(OBJECT(&s->itu), saar_present, "saar-present",
- &err);
+ &error_abort);
if (saar_present) {
s->itu.saar = &env->CP0_SAAR;
}
@@ -119,8 +121,10 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
/* Cluster Power Controller */
object_initialize_child(OBJECT(dev), "cpc", &s->cpc, TYPE_MIPS_CPC);
- object_property_set_int(OBJECT(&s->cpc), s->num_vp, "num-vp", &err);
- object_property_set_int(OBJECT(&s->cpc), 1, "vp-start-running", &err);
+ object_property_set_int(OBJECT(&s->cpc), s->num_vp, "num-vp",
+ &error_abort);
+ object_property_set_int(OBJECT(&s->cpc), 1, "vp-start-running",
+ &error_abort);
sysbus_realize(SYS_BUS_DEVICE(&s->cpc), &err);
if (err != NULL) {
error_propagate(errp, err);
@@ -132,8 +136,10 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
/* Global Interrupt Controller */
object_initialize_child(OBJECT(dev), "gic", &s->gic, TYPE_MIPS_GIC);
- object_property_set_int(OBJECT(&s->gic), s->num_vp, "num-vp", &err);
- object_property_set_int(OBJECT(&s->gic), 128, "num-irq", &err);
+ object_property_set_int(OBJECT(&s->gic), s->num_vp, "num-vp",
+ &error_abort);
+ object_property_set_int(OBJECT(&s->gic), 128, "num-irq",
+ &error_abort);
sysbus_realize(SYS_BUS_DEVICE(&s->gic), &err);
if (err != NULL) {
error_propagate(errp, err);
@@ -147,9 +153,12 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
gcr_base = env->CP0_CMGCRBase << 4;
object_initialize_child(OBJECT(dev), "gcr", &s->gcr, TYPE_MIPS_GCR);
- object_property_set_int(OBJECT(&s->gcr), s->num_vp, "num-vp", &err);
- object_property_set_int(OBJECT(&s->gcr), 0x800, "gcr-rev", &err);
- object_property_set_int(OBJECT(&s->gcr), gcr_base, "gcr-base", &err);
+ object_property_set_int(OBJECT(&s->gcr), s->num_vp, "num-vp",
+ &error_abort);
+ object_property_set_int(OBJECT(&s->gcr), 0x800, "gcr-rev",
+ &error_abort);
+ object_property_set_int(OBJECT(&s->gcr), gcr_base, "gcr-base",
+ &error_abort);
object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->gic.mr), "gic",
&error_abort);
object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->cpc.mr), "cpc",
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 21/22] x86: Fix x86_cpu_new() error API violations
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (19 preceding siblings ...)
2020-06-22 10:42 ` [PATCH 20/22] mips/cps: Fix mips_cps_realize() " Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
2020-06-22 10:42 ` [PATCH 22/22] amd_iommu: Fix amdvi_realize() error API violation Markus Armbruster
21 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Eduardo Habkost, Richard Henderson
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.
x86_cpu_new() is wrong that way: it passes &local_err to
object_property_set_uint() without checking it, and then to
qdev_realize(). Harmless, because the former can't actually fail
here.
Fix by checking for failure right away. While there, replace
qdev_realize(); object_unref() by qdev_realize_and_unref().
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/i386/x86.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 34229b45c7..3a7029e6db 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -118,16 +118,10 @@ uint32_t x86_cpu_apic_id_from_index(X86MachineState *x86ms,
void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp)
{
- Object *cpu = NULL;
- Error *local_err = NULL;
+ Object *cpu = object_new(MACHINE(x86ms)->cpu_type);
- cpu = object_new(MACHINE(x86ms)->cpu_type);
-
- object_property_set_uint(cpu, apic_id, "apic-id", &local_err);
- qdev_realize(DEVICE(cpu), NULL, &local_err);
-
- object_unref(cpu);
- error_propagate(errp, local_err);
+ object_property_set_uint(cpu, apic_id, "apic-id", &error_abort);
+ qdev_realize_and_unref(DEVICE(cpu), NULL, errp);
}
void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 22/22] amd_iommu: Fix amdvi_realize() error API violation
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
` (20 preceding siblings ...)
2020-06-22 10:42 ` [PATCH 21/22] x86: Fix x86_cpu_new() " Markus Armbruster
@ 2020-06-22 10:42 ` Markus Armbruster
21 siblings, 0 replies; 41+ messages in thread
From: Markus Armbruster @ 2020-06-22 10:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Eduardo Habkost, Richard Henderson
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.
amdvi_realize() is wrong that way: it passes @errp to qdev_realize(),
object_property_get_int(), and msi_init() without checking it. I
can't tell offhand whether qdev_realize() can fail here. Fix by
checking it for failure. object_property_get_int() can't. Fix by
passing &error_abort instead.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/i386/amd_iommu.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index b26d30da57..087f601666 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1549,7 +1549,9 @@ static void amdvi_realize(DeviceState *dev, Error **errp)
/* This device should take care of IOMMU PCI properties */
x86_iommu->type = TYPE_AMD;
- qdev_realize(DEVICE(&s->pci), &bus->qbus, errp);
+ if (!qdev_realize(DEVICE(&s->pci), &bus->qbus, errp)) {
+ return;
+ }
ret = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
AMDVI_CAPAB_SIZE, errp);
if (ret < 0) {
@@ -1578,7 +1580,7 @@ static void amdvi_realize(DeviceState *dev, Error **errp)
sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->mmio);
sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, AMDVI_BASE_ADDR);
pci_setup_iommu(bus, amdvi_host_dma_iommu, s);
- s->devid = object_property_get_int(OBJECT(&s->pci), "addr", errp);
+ s->devid = object_property_get_int(OBJECT(&s->pci), "addr", &error_abort);
msi_init(&s->pci.dev, 0, 1, true, false, errp);
amdvi_init(s);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 04/22] tests: Use &error_abort where appropriate
2020-06-22 10:42 ` [PATCH 04/22] tests: Use &error_abort where appropriate Markus Armbruster
@ 2020-06-22 11:26 ` Thomas Huth
0 siblings, 0 replies; 41+ messages in thread
From: Thomas Huth @ 2020-06-22 11:26 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
On 22/06/2020 12.42, Markus Armbruster wrote:
> Receiving the error in a local variable only to assert there is none
> is less clear than passing &error_abort. Clean up.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> tests/check-qobject.c | 5 +-
> tests/test-logging.c | 12 +---
> tests/test-qemu-opts.c | 22 ++----
> tests/test-replication.c | 109 +++++++++--------------------
> tests/test-string-input-visitor.c | 33 +++------
> tests/test-string-output-visitor.c | 16 ++---
> 6 files changed, 57 insertions(+), 140 deletions(-)
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 13/22] aspeed: Clean up roundabout error propagation
2020-06-22 10:42 ` [PATCH 13/22] aspeed: Clean up roundabout error propagation Markus Armbruster
@ 2020-06-22 12:37 ` Cédric Le Goater
0 siblings, 0 replies; 41+ messages in thread
From: Cédric Le Goater @ 2020-06-22 12:37 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
On 6/22/20 12:42 PM, Markus Armbruster wrote:
> Replace
>
> sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &local_err);
> error_propagate(&err, local_err);
> if (err) {
> error_propagate(errp, err);
> return;
> }
>
> by
>
> sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &err);
> if (err) {
> error_propagate(errp, err);
> return;
> }
>
> Cc: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/arm/aspeed_ast2600.c | 10 ++++------
> hw/arm/aspeed_soc.c | 10 ++++------
> 2 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 6da687299f..08b3592e36 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -228,7 +228,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
> int i;
> AspeedSoCState *s = ASPEED_SOC(dev);
> AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> - Error *err = NULL, *local_err = NULL;
> + Error *err = NULL;
> qemu_irq irq;
>
> /* IO space */
> @@ -394,8 +394,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
> return;
> }
> object_property_set_int(OBJECT(&s->spi[i]), 1, "num-cs", &err);
> - sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &local_err);
> - error_propagate(&err, local_err);
> + sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &err);
> if (err) {
> error_propagate(errp, err);
> return;
> @@ -446,11 +445,10 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
> for (i = 0; i < sc->macs_num; i++) {
> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed",
> &err);
> - sysbus_realize(SYS_BUS_DEVICE(&s->ftgmac100[i]), &local_err);
> - error_propagate(&err, local_err);
> + sysbus_realize(SYS_BUS_DEVICE(&s->ftgmac100[i]), &err);
> if (err) {
> error_propagate(errp, err);
> - return;
> + return;
> }
> sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100[i]), 0,
> sc->memmap[ASPEED_ETH1 + i]);
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 810cf9b6cc..ec21de50ce 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -218,7 +218,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> int i;
> AspeedSoCState *s = ASPEED_SOC(dev);
> AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> - Error *err = NULL, *local_err = NULL;
> + Error *err = NULL;
>
> /* IO space */
> create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_IOMEM],
> @@ -340,8 +340,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> /* SPI */
> for (i = 0; i < sc->spis_num; i++) {
> object_property_set_int(OBJECT(&s->spi[i]), 1, "num-cs", &err);
> - sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &local_err);
> - error_propagate(&err, local_err);
> + sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &err);
> if (err) {
> error_propagate(errp, err);
> return;
> @@ -392,11 +391,10 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> for (i = 0; i < sc->macs_num; i++) {
> object_property_set_bool(OBJECT(&s->ftgmac100[i]), true, "aspeed",
> &err);
> - sysbus_realize(SYS_BUS_DEVICE(&s->ftgmac100[i]), &local_err);
> - error_propagate(&err, local_err);
> + sysbus_realize(SYS_BUS_DEVICE(&s->ftgmac100[i]), &err);
> if (err) {
> error_propagate(errp, err);
> - return;
> + return;
> }
> sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100[i]), 0,
> sc->memmap[ASPEED_ETH1 + i]);
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 17/22] hw/arm: Drop useless object_property_set_link() error handling
2020-06-22 10:42 ` [PATCH 17/22] hw/arm: Drop useless object_property_set_link() error handling Markus Armbruster
@ 2020-06-22 12:37 ` Cédric Le Goater
0 siblings, 0 replies; 41+ messages in thread
From: Cédric Le Goater @ 2020-06-22 12:37 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, Joel Stanley
On 6/22/20 12:42 PM, Markus Armbruster wrote:
> object_property_set_link() fails when the property doesn't exist, is
> not settable, or its .check() method fails. These are all programming
> errors here, so passing it &error_abort is appropriate.
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: "Cédric Le Goater" <clg@kaod.org>
> Cc: Andrew Jeffery <andrew@aj.id.au>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
For the Aspeed parts,
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/arm/armsse.c | 53 ++++++++++-------------------------------
> hw/arm/armv7m.c | 7 ++----
> hw/arm/aspeed_ast2600.c | 20 ++++------------
> hw/arm/aspeed_soc.c | 14 ++++-------
> hw/arm/nrf51_soc.c | 6 +----
> 5 files changed, 24 insertions(+), 76 deletions(-)
>
> diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
> index e8f8f60abc..c73cc6badf 100644
> --- a/hw/arm/armsse.c
> +++ b/hw/arm/armsse.c
> @@ -564,16 +564,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
> &s->container, -1);
> }
> object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
> - "memory", &err);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> - }
> - object_property_set_link(cpuobj, OBJECT(s), "idau", &err);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> - }
> + "memory", &error_abort);
> + object_property_set_link(cpuobj, OBJECT(s), "idau", &error_abort);
> sysbus_realize(SYS_BUS_DEVICE(cpuobj), &err);
> if (err) {
> error_propagate(errp, err);
> @@ -700,11 +692,7 @@ static void armsse_realize(DeviceState *dev, Error **errp)
> return;
> }
> object_property_set_link(OBJECT(&s->mpc[i]), OBJECT(&s->sram[i]),
> - "downstream", &err);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> - }
> + "downstream", &error_abort);
> sysbus_realize(SYS_BUS_DEVICE(&s->mpc[i]), &err);
> if (err) {
> error_propagate(errp, err);
> @@ -755,11 +743,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
> sysbus_connect_irq(SYS_BUS_DEVICE(&s->timer0), 0,
> armsse_get_common_irq_in(s, 3));
> mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->timer0), 0);
> - object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[0]", &err);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> - }
> + object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[0]",
> + &error_abort);
>
> qdev_prop_set_uint32(DEVICE(&s->timer1), "pclk-frq", s->mainclk_frq);
> sysbus_realize(SYS_BUS_DEVICE(&s->timer1), &err);
> @@ -770,12 +755,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
> sysbus_connect_irq(SYS_BUS_DEVICE(&s->timer1), 0,
> armsse_get_common_irq_in(s, 4));
> mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->timer1), 0);
> - object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[1]", &err);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> - }
> -
> + object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[1]",
> + &error_abort);
>
> qdev_prop_set_uint32(DEVICE(&s->dualtimer), "pclk-frq", s->mainclk_frq);
> sysbus_realize(SYS_BUS_DEVICE(&s->dualtimer), &err);
> @@ -786,11 +767,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
> sysbus_connect_irq(SYS_BUS_DEVICE(&s->dualtimer), 0,
> armsse_get_common_irq_in(s, 5));
> mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->dualtimer), 0);
> - object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[2]", &err);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> - }
> + object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr), "port[2]",
> + &error_abort);
>
> if (info->has_mhus) {
> /*
> @@ -815,12 +793,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
> port = g_strdup_printf("port[%d]", i + 3);
> mr = sysbus_mmio_get_region(mhu_sbd, 0);
> object_property_set_link(OBJECT(&s->apb_ppc0), OBJECT(mr),
> - port, &err);
> + port, &error_abort);
> g_free(port);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> - }
>
> /*
> * Each MHU has an irq line for each CPU:
> @@ -967,11 +941,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
> sysbus_connect_irq(SYS_BUS_DEVICE(&s->s32ktimer), 0,
> armsse_get_common_irq_in(s, 2));
> mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->s32ktimer), 0);
> - object_property_set_link(OBJECT(&s->apb_ppc1), OBJECT(mr), "port[0]", &err);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> - }
> + object_property_set_link(OBJECT(&s->apb_ppc1), OBJECT(mr), "port[0]",
> + &error_abort);
>
> sysbus_realize(SYS_BUS_DEVICE(&s->apb_ppc1), &err);
> if (err) {
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index ce83586e03..3308211e9c 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -170,11 +170,8 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
> object_property_set_link(OBJECT(s->cpu), OBJECT(&s->container), "memory",
> &error_abort);
> if (object_property_find(OBJECT(s->cpu), "idau", NULL)) {
> - object_property_set_link(OBJECT(s->cpu), s->idau, "idau", &err);
> - if (err != NULL) {
> - error_propagate(errp, err);
> - return;
> - }
> + object_property_set_link(OBJECT(s->cpu), s->idau, "idau",
> + &error_abort);
> }
> if (object_property_find(OBJECT(s->cpu), "init-svtor", NULL)) {
> object_property_set_uint(OBJECT(s->cpu), s->init_svtor,
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 08b3592e36..4efac02e2b 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -341,11 +341,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
> }
>
> /* I2C */
> - object_property_set_link(OBJECT(&s->i2c), OBJECT(s->dram_mr), "dram", &err);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> - }
> + object_property_set_link(OBJECT(&s->i2c), OBJECT(s->dram_mr), "dram",
> + &error_abort);
> sysbus_realize(SYS_BUS_DEVICE(&s->i2c), &err);
> if (err) {
> error_propagate(errp, err);
> @@ -363,11 +360,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
> }
>
> /* FMC, The number of CS is set at the board level */
> - object_property_set_link(OBJECT(&s->fmc), OBJECT(s->dram_mr), "dram", &err);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> - }
> + object_property_set_link(OBJECT(&s->fmc), OBJECT(s->dram_mr), "dram",
> + &error_abort);
> object_property_set_int(OBJECT(&s->fmc), sc->memmap[ASPEED_SDRAM],
> "sdram-base", &err);
> if (err) {
> @@ -388,11 +382,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
> /* SPI */
> for (i = 0; i < sc->spis_num; i++) {
> object_property_set_link(OBJECT(&s->spi[i]), OBJECT(s->dram_mr),
> - "dram", &err);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> - }
> + "dram", &error_abort);
> object_property_set_int(OBJECT(&s->spi[i]), 1, "num-cs", &err);
> sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), &err);
> if (err) {
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index ec21de50ce..03b91bade6 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -300,11 +300,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> }
>
> /* I2C */
> - object_property_set_link(OBJECT(&s->i2c), OBJECT(s->dram_mr), "dram", &err);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> - }
> + object_property_set_link(OBJECT(&s->i2c), OBJECT(s->dram_mr), "dram",
> + &error_abort);
> sysbus_realize(SYS_BUS_DEVICE(&s->i2c), &err);
> if (err) {
> error_propagate(errp, err);
> @@ -315,11 +312,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> aspeed_soc_get_irq(s, ASPEED_I2C));
>
> /* FMC, The number of CS is set at the board level */
> - object_property_set_link(OBJECT(&s->fmc), OBJECT(s->dram_mr), "dram", &err);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> - }
> + object_property_set_link(OBJECT(&s->fmc), OBJECT(s->dram_mr), "dram",
> + &error_abort);
> object_property_set_int(OBJECT(&s->fmc), sc->memmap[ASPEED_SDRAM],
> "sdram-base", &err);
> if (err) {
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index 5a8961ddbb..20dd8b5897 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -66,11 +66,7 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
> }
>
> object_property_set_link(OBJECT(&s->cpu), OBJECT(&s->container), "memory",
> - &err);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> - }
> + &error_abort);
> sysbus_realize(SYS_BUS_DEVICE(&s->cpu), &err);
> if (err) {
> error_propagate(errp, err);
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 16/22] hw: Fix error API violation around object_property_set_link()
2020-06-22 10:42 ` [PATCH 16/22] hw: Fix error API violation around object_property_set_link() Markus Armbruster
@ 2020-06-22 12:47 ` Auger Eric
2020-06-22 21:22 ` Alistair Francis
1 sibling, 0 replies; 41+ messages in thread
From: Auger Eric @ 2020-06-22 12:47 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Peter Maydell, Aleksandar Rikalo, Alistair Francis,
Mark Cave-Ayland, Aleksandar Markovic, qemu-arm, Gerd Hoffmann,
Edgar E. Iglesias, Aurelien Jarno
Hi Markus,
On 6/22/20 12:42 PM, 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.
>
> virtio_gpu_pci_base_realize(), virtio_vga_base_realize(),
> sparc32_ledma_device_realize(), sparc32_dma_realize(),
> sparc32_dma_realize() xilinx_axidma_realize(), mips_cps_realize(),
> macio_realize_ide(), xilinx_enet_realize(), and
> virtio_iommu_pci_realize() are wrong that way: they reuse the argument
> they pass to object_property_set_link() for another call.
>
> Harmless, because object_property_set_link() can't actually fail for
> them: it fails when the property doesn't exist, is not settable, or
> its .check() method fails. Fix by passing &error_abort instead.
s/.check/.set()? in object_property_set()
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Alistair Francis <alistair@alistair23.me>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Cc: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
> ---
> hw/display/virtio-gpu-pci.c | 2 +-
> hw/display/virtio-vga.c | 2 +-
> hw/dma/sparc32_dma.c | 6 +++---
> hw/dma/xilinx_axidma.c | 12 ++----------
> hw/mips/cps.c | 6 ++++--
> hw/misc/macio/macio.c | 3 ++-
> hw/net/xilinx_axienet.c | 12 ++----------
> hw/virtio/virtio-iommu-pci.c | 2 +-
> 8 files changed, 16 insertions(+), 29 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
> index b532fe8b5f..41b88b878d 100644
> --- a/hw/display/virtio-gpu-pci.c
> +++ b/hw/display/virtio-gpu-pci.c
> @@ -44,7 +44,7 @@ static void virtio_gpu_pci_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> for (i = 0; i < g->conf.max_outputs; i++) {
> object_property_set_link(OBJECT(g->scanout[i].con),
> OBJECT(vpci_dev),
> - "device", errp);
> + "device", &error_abort);
> }
> }
>
> diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
> index 68a062ece6..67f409e106 100644
> --- a/hw/display/virtio-vga.c
> +++ b/hw/display/virtio-vga.c
> @@ -154,7 +154,7 @@ static void virtio_vga_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> for (i = 0; i < g->conf.max_outputs; i++) {
> object_property_set_link(OBJECT(g->scanout[i].con),
> OBJECT(vpci_dev),
> - "device", errp);
> + "device", &error_abort);
> }
> }
>
> diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
> index f02aca6f40..2d7dbbb92d 100644
> --- a/hw/dma/sparc32_dma.c
> +++ b/hw/dma/sparc32_dma.c
> @@ -346,7 +346,7 @@ static void sparc32_ledma_device_realize(DeviceState *dev, Error **errp)
> d = qdev_new(TYPE_LANCE);
> object_property_add_child(OBJECT(dev), "lance", OBJECT(d));
> qdev_set_nic_properties(d, nd);
> - object_property_set_link(OBJECT(d), OBJECT(dev), "dma", errp);
> + object_property_set_link(OBJECT(d), OBJECT(dev), "dma", &error_abort);
> sysbus_realize_and_unref(SYS_BUS_DEVICE(d), &error_fatal);
> }
>
> @@ -379,7 +379,7 @@ static void sparc32_dma_realize(DeviceState *dev, Error **errp)
> }
>
> espdma = qdev_new(TYPE_SPARC32_ESPDMA_DEVICE);
> - object_property_set_link(OBJECT(espdma), iommu, "iommu", errp);
> + object_property_set_link(OBJECT(espdma), iommu, "iommu", &error_abort);
> object_property_add_child(OBJECT(s), "espdma", OBJECT(espdma));
> sysbus_realize_and_unref(SYS_BUS_DEVICE(espdma), &error_fatal);
>
> @@ -394,7 +394,7 @@ static void sparc32_dma_realize(DeviceState *dev, Error **errp)
> sysbus_mmio_get_region(sbd, 0));
>
> ledma = qdev_new(TYPE_SPARC32_LEDMA_DEVICE);
> - object_property_set_link(OBJECT(ledma), iommu, "iommu", errp);
> + object_property_set_link(OBJECT(ledma), iommu, "iommu", &error_abort);
> object_property_add_child(OBJECT(s), "ledma", OBJECT(ledma));
> sysbus_realize_and_unref(SYS_BUS_DEVICE(ledma), &error_fatal);
>
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index 6a9df2c4db..a069637bf2 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -537,7 +537,6 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
> XilinxAXIDMAStreamSlave *ds = XILINX_AXI_DMA_DATA_STREAM(&s->rx_data_dev);
> XilinxAXIDMAStreamSlave *cs = XILINX_AXI_DMA_CONTROL_STREAM(
> &s->rx_control_dev);
> - Error *local_err = NULL;
> int i;
>
> object_property_add_link(OBJECT(ds), "dma", TYPE_XILINX_AXI_DMA,
> @@ -548,11 +547,8 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
> (Object **)&cs->dma,
> object_property_allow_set_link,
> OBJ_PROP_LINK_STRONG);
> - object_property_set_link(OBJECT(ds), OBJECT(s), "dma", &local_err);
> - object_property_set_link(OBJECT(cs), OBJECT(s), "dma", &local_err);
> - if (local_err) {
> - goto xilinx_axidma_realize_fail;
> - }
> + object_property_set_link(OBJECT(ds), OBJECT(s), "dma", &error_abort);
> + object_property_set_link(OBJECT(cs), OBJECT(s), "dma", &error_abort);
>
> for (i = 0; i < 2; i++) {
> struct Stream *st = &s->streams[i];
> @@ -567,10 +563,6 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
>
> address_space_init(&s->as,
> s->dma_mr ? s->dma_mr : get_system_memory(), "dma");
> - return;
> -
> -xilinx_axidma_realize_fail:
> - error_propagate(errp, local_err);
> }
>
> static void xilinx_axidma_init(Object *obj)
> diff --git a/hw/mips/cps.c b/hw/mips/cps.c
> index cdfab19826..5382bc86f7 100644
> --- a/hw/mips/cps.c
> +++ b/hw/mips/cps.c
> @@ -150,8 +150,10 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
> object_property_set_int(OBJECT(&s->gcr), s->num_vp, "num-vp", &err);
> object_property_set_int(OBJECT(&s->gcr), 0x800, "gcr-rev", &err);
> object_property_set_int(OBJECT(&s->gcr), gcr_base, "gcr-base", &err);
> - object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->gic.mr), "gic", &err);
> - object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->cpc.mr), "cpc", &err);
> + object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->gic.mr), "gic",
> + &error_abort);
> + object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->cpc.mr), "cpc",
> + &error_abort);
> sysbus_realize(SYS_BUS_DEVICE(&s->gcr), &err);
> if (err != NULL) {
> error_propagate(errp, err);
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index 8ba7af073c..3251c79f46 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -136,7 +136,8 @@ static void macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
> sysbus_connect_irq(sysbus_dev, 0, irq0);
> sysbus_connect_irq(sysbus_dev, 1, irq1);
> qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
> - object_property_set_link(OBJECT(ide), OBJECT(&s->dbdma), "dbdma", errp);
> + object_property_set_link(OBJECT(ide), OBJECT(&s->dbdma), "dbdma",
> + &error_abort);
> macio_ide_register_dma(ide);
>
> qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
> diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
> index c2f40b8ea9..679a359f9a 100644
> --- a/hw/net/xilinx_axienet.c
> +++ b/hw/net/xilinx_axienet.c
> @@ -980,7 +980,6 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
> XilinxAXIEnetStreamSlave *ds = XILINX_AXI_ENET_DATA_STREAM(&s->rx_data_dev);
> XilinxAXIEnetStreamSlave *cs = XILINX_AXI_ENET_CONTROL_STREAM(
> &s->rx_control_dev);
> - Error *local_err = NULL;
>
> object_property_add_link(OBJECT(ds), "enet", "xlnx.axi-ethernet",
> (Object **) &ds->enet,
> @@ -990,11 +989,8 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
> (Object **) &cs->enet,
> object_property_allow_set_link,
> OBJ_PROP_LINK_STRONG);
> - object_property_set_link(OBJECT(ds), OBJECT(s), "enet", &local_err);
> - object_property_set_link(OBJECT(cs), OBJECT(s), "enet", &local_err);
> - if (local_err) {
> - goto xilinx_enet_realize_fail;
> - }
> + object_property_set_link(OBJECT(ds), OBJECT(s), "enet", &error_abort);
> + object_property_set_link(OBJECT(cs), OBJECT(s), "enet", &error_abort);
>
> qemu_macaddr_default_if_unset(&s->conf.macaddr);
> s->nic = qemu_new_nic(&net_xilinx_enet_info, &s->conf,
> @@ -1008,10 +1004,6 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
>
> s->rxmem = g_malloc(s->c_rxmem);
> s->txmem = g_malloc(s->c_txmem);
> - return;
> -
> -xilinx_enet_realize_fail:
> - error_propagate(errp, local_err);
> }
>
> static void xilinx_enet_init(Object *obj)
> diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c
> index 632533abaf..bd61d6e2f8 100644
> --- a/hw/virtio/virtio-iommu-pci.c
> +++ b/hw/virtio/virtio-iommu-pci.c
> @@ -56,7 +56,7 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> }
> object_property_set_link(OBJECT(dev),
> OBJECT(pci_get_bus(&vpci_dev->pci_dev)),
> - "primary-bus", errp);
> + "primary-bus", &error_abort);
> qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
> }
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 03/22] Clean up some calls to ignore Error objects the right way
2020-06-22 10:42 ` [PATCH 03/22] Clean up some calls to ignore Error objects the right way Markus Armbruster
@ 2020-06-22 14:43 ` Greg Kurz
0 siblings, 0 replies; 41+ messages in thread
From: Greg Kurz @ 2020-06-22 14:43 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Daniel P . Berrange, Jerome Forissier, qemu-devel
On Mon, 22 Jun 2020 12:42:31 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> Receiving the error in a local variable only to free it is less clear
> (and also less efficient) than passing NULL. Clean up.
>
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Cc: Jerome Forissier <jerome@forissier.org>
> CC: Greg Kurz <groug@kaod.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
and
Acked-by: Greg Kurz <groug@kaod.org> # for 9pfs
> chardev/char-socket.c | 6 ++----
> hw/9pfs/9p.c | 6 ++----
> hw/arm/virt.c | 4 +---
> hw/ppc/spapr_drc.c | 4 +---
> ui/vnc.c | 3 +--
> 5 files changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index afebeec5c3..b0cae97960 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -815,22 +815,20 @@ static void tcp_chr_tls_init(Chardev *chr)
> {
> SocketChardev *s = SOCKET_CHARDEV(chr);
> QIOChannelTLS *tioc;
> - Error *err = NULL;
> gchar *name;
>
> if (s->is_listen) {
> tioc = qio_channel_tls_new_server(
> s->ioc, s->tls_creds,
> s->tls_authz,
> - &err);
> + NULL);
> } else {
> tioc = qio_channel_tls_new_client(
> s->ioc, s->tls_creds,
> s->addr->u.inet.host,
> - &err);
> + NULL);
> }
> if (tioc == NULL) {
> - error_free(err);
> tcp_chr_disconnect(chr);
> return;
> }
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 45a788f6e6..9755fba9a9 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1399,7 +1399,6 @@ static void coroutine_fn v9fs_attach(void *opaque)
> size_t offset = 7;
> V9fsQID qid;
> ssize_t err;
> - Error *local_err = NULL;
>
> v9fs_string_init(&uname);
> v9fs_string_init(&aname);
> @@ -1437,9 +1436,8 @@ static void coroutine_fn v9fs_attach(void *opaque)
> error_setg(&s->migration_blocker,
> "Migration is disabled when VirtFS export path '%s' is mounted in the guest using mount_tag '%s'",
> s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag);
> - err = migrate_add_blocker(s->migration_blocker, &local_err);
> - if (local_err) {
> - error_free(local_err);
> + err = migrate_add_blocker(s->migration_blocker, NULL);
> + if (err < 0) {
> error_free(s->migration_blocker);
> s->migration_blocker = NULL;
> clunk_fid(s, fid);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index caceb1e4a0..29b9d5b2e6 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -217,11 +217,9 @@ static bool cpu_type_valid(const char *cpu)
>
> static void create_kaslr_seed(VirtMachineState *vms, const char *node)
> {
> - Error *err = NULL;
> uint64_t seed;
>
> - if (qemu_guest_getrandom(&seed, sizeof(seed), &err)) {
> - error_free(err);
> + if (qemu_guest_getrandom(&seed, sizeof(seed), NULL)) {
> return;
> }
> qemu_fdt_setprop_u64(vms->fdt, node, "kaslr-seed", seed);
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 2689104295..951bcdf2c0 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -1163,16 +1163,14 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>
> if (!drc->fdt) {
> - Error *local_err = NULL;
> void *fdt;
> int fdt_size;
>
> fdt = create_device_tree(&fdt_size);
>
> if (drck->dt_populate(drc, spapr, fdt, &drc->fdt_start_offset,
> - &local_err)) {
> + NULL)) {
> g_free(fdt);
> - error_free(local_err);
> rc = SPAPR_DR_CC_RESPONSE_ERROR;
> goto out;
> }
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 12a12714e1..0702a76cce 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -458,9 +458,8 @@ static VncServerInfo2List *qmp_query_server_entry(QIOChannelSocket *ioc,
> Error *err = NULL;
> SocketAddress *addr;
>
> - addr = qio_channel_socket_get_local_address(ioc, &err);
> + addr = qio_channel_socket_get_local_address(ioc, NULL);
> if (!addr) {
> - error_free(err);
> return prev;
> }
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 07/22] spapr: Plug minor memory leak in spapr_machine_init()
2020-06-22 10:42 ` [PATCH 07/22] spapr: Plug minor memory leak in spapr_machine_init() Markus Armbruster
@ 2020-06-22 14:54 ` Greg Kurz
0 siblings, 0 replies; 41+ messages in thread
From: Greg Kurz @ 2020-06-22 14:54 UTC (permalink / raw)
To: Markus Armbruster; +Cc: David Gibson, qemu-ppc, qemu-devel
On Mon, 22 Jun 2020 12:42:35 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> spapr_machine_init() leaks an Error object when
> kvmppc_check_papr_resize_hpt() fails and spapr->resize_hpt is
> SPAPR_RESIZE_HPT_DISABLED, i.e. when the host doesn't support hash
> page table resizing, and the user didn't ask for it. As harmless as
> memory leaks can possibly be. Plug it.
>
> Fixes: 30f4b05bd090564181554d0890605eb2c143e4ea
> Cc: David Gibson <dgibson@redhat.com>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> hw/ppc/spapr.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bd9345cdac..9bd2183ead 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2731,6 +2731,7 @@ static void spapr_machine_init(MachineState *machine)
> error_report_err(resize_hpt_err);
> exit(1);
> }
> + error_free(resize_hpt_err);
>
> spapr->rma_size = spapr_rma_size(spapr, &error_fatal);
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 19/22] riscv_hart: Fix riscv_harts_realize() error API violations
2020-06-22 10:42 ` Markus Armbruster
@ 2020-06-22 21:19 ` Alistair Francis
-1 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2020-06-22 21:19 UTC (permalink / raw)
To: Markus Armbruster
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
qemu-devel@nongnu.org Developers, Alistair Francis,
Palmer Dabbelt, Bin Meng
On Mon, Jun 22, 2020 at 3:51 AM Markus Armbruster <armbru@redhat.com> 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.
>
> riscv_harts_realize() is wrong that way: it passes @errp to
> riscv_hart_realize() in a loop. I can't tell offhand whether this can
> fail.
>
> Fix by checking for failure in each iteration.
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> hw/riscv/riscv_hart.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
> index e26c382259..f59fe52f0f 100644
> --- a/hw/riscv/riscv_hart.c
> +++ b/hw/riscv/riscv_hart.c
> @@ -40,19 +40,13 @@ static void riscv_harts_cpu_reset(void *opaque)
> cpu_reset(CPU(cpu));
> }
>
> -static void riscv_hart_realize(RISCVHartArrayState *s, int idx,
> +static bool riscv_hart_realize(RISCVHartArrayState *s, int idx,
> char *cpu_type, Error **errp)
> {
> - Error *err = NULL;
> -
> object_initialize_child(OBJECT(s), "harts[*]", &s->harts[idx], cpu_type);
> s->harts[idx].env.mhartid = s->hartid_base + idx;
> qemu_register_reset(riscv_harts_cpu_reset, &s->harts[idx]);
> - qdev_realize(DEVICE(&s->harts[idx]), NULL, &err);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> - }
> + return qdev_realize(DEVICE(&s->harts[idx]), NULL, errp);
> }
>
> static void riscv_harts_realize(DeviceState *dev, Error **errp)
> @@ -63,7 +57,9 @@ static void riscv_harts_realize(DeviceState *dev, Error **errp)
> s->harts = g_new0(RISCVCPU, s->num_harts);
>
> for (n = 0; n < s->num_harts; n++) {
> - riscv_hart_realize(s, n, s->cpu_type, errp);
> + if (!riscv_hart_realize(s, n, s->cpu_type, errp)) {
> + return;
> + }
> }
> }
>
> --
> 2.26.2
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 19/22] riscv_hart: Fix riscv_harts_realize() error API violations
@ 2020-06-22 21:19 ` Alistair Francis
0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2020-06-22 21:19 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
Sagar Karandikar, Bastian Koppelmann, Alistair Francis,
Palmer Dabbelt, Bin Meng
On Mon, Jun 22, 2020 at 3:51 AM Markus Armbruster <armbru@redhat.com> 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.
>
> riscv_harts_realize() is wrong that way: it passes @errp to
> riscv_hart_realize() in a loop. I can't tell offhand whether this can
> fail.
>
> Fix by checking for failure in each iteration.
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> hw/riscv/riscv_hart.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
> index e26c382259..f59fe52f0f 100644
> --- a/hw/riscv/riscv_hart.c
> +++ b/hw/riscv/riscv_hart.c
> @@ -40,19 +40,13 @@ static void riscv_harts_cpu_reset(void *opaque)
> cpu_reset(CPU(cpu));
> }
>
> -static void riscv_hart_realize(RISCVHartArrayState *s, int idx,
> +static bool riscv_hart_realize(RISCVHartArrayState *s, int idx,
> char *cpu_type, Error **errp)
> {
> - Error *err = NULL;
> -
> object_initialize_child(OBJECT(s), "harts[*]", &s->harts[idx], cpu_type);
> s->harts[idx].env.mhartid = s->hartid_base + idx;
> qemu_register_reset(riscv_harts_cpu_reset, &s->harts[idx]);
> - qdev_realize(DEVICE(&s->harts[idx]), NULL, &err);
> - if (err) {
> - error_propagate(errp, err);
> - return;
> - }
> + return qdev_realize(DEVICE(&s->harts[idx]), NULL, errp);
> }
>
> static void riscv_harts_realize(DeviceState *dev, Error **errp)
> @@ -63,7 +57,9 @@ static void riscv_harts_realize(DeviceState *dev, Error **errp)
> s->harts = g_new0(RISCVCPU, s->num_harts);
>
> for (n = 0; n < s->num_harts; n++) {
> - riscv_hart_realize(s, n, s->cpu_type, errp);
> + if (!riscv_hart_realize(s, n, s->cpu_type, errp)) {
> + return;
> + }
> }
> }
>
> --
> 2.26.2
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 16/22] hw: Fix error API violation around object_property_set_link()
2020-06-22 10:42 ` [PATCH 16/22] hw: Fix error API violation around object_property_set_link() Markus Armbruster
2020-06-22 12:47 ` Auger Eric
@ 2020-06-22 21:22 ` Alistair Francis
1 sibling, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2020-06-22 21:22 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Maydell, Aleksandar Rikalo, Alistair Francis,
Mark Cave-Ayland, qemu-devel@nongnu.org Developers, Eric Auger,
Aleksandar Markovic, qemu-arm, Gerd Hoffmann, Edgar E. Iglesias,
Aurelien Jarno
On Mon, Jun 22, 2020 at 3:53 AM Markus Armbruster <armbru@redhat.com> 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.
>
> virtio_gpu_pci_base_realize(), virtio_vga_base_realize(),
> sparc32_ledma_device_realize(), sparc32_dma_realize(),
> sparc32_dma_realize() xilinx_axidma_realize(), mips_cps_realize(),
> macio_realize_ide(), xilinx_enet_realize(), and
> virtio_iommu_pci_realize() are wrong that way: they reuse the argument
> they pass to object_property_set_link() for another call.
>
> Harmless, because object_property_set_link() can't actually fail for
> them: it fails when the property doesn't exist, is not settable, or
> its .check() method fails. Fix by passing &error_abort instead.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Alistair Francis <alistair@alistair23.me>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Cc: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> hw/display/virtio-gpu-pci.c | 2 +-
> hw/display/virtio-vga.c | 2 +-
> hw/dma/sparc32_dma.c | 6 +++---
> hw/dma/xilinx_axidma.c | 12 ++----------
> hw/mips/cps.c | 6 ++++--
> hw/misc/macio/macio.c | 3 ++-
> hw/net/xilinx_axienet.c | 12 ++----------
> hw/virtio/virtio-iommu-pci.c | 2 +-
> 8 files changed, 16 insertions(+), 29 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
> index b532fe8b5f..41b88b878d 100644
> --- a/hw/display/virtio-gpu-pci.c
> +++ b/hw/display/virtio-gpu-pci.c
> @@ -44,7 +44,7 @@ static void virtio_gpu_pci_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> for (i = 0; i < g->conf.max_outputs; i++) {
> object_property_set_link(OBJECT(g->scanout[i].con),
> OBJECT(vpci_dev),
> - "device", errp);
> + "device", &error_abort);
> }
> }
>
> diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
> index 68a062ece6..67f409e106 100644
> --- a/hw/display/virtio-vga.c
> +++ b/hw/display/virtio-vga.c
> @@ -154,7 +154,7 @@ static void virtio_vga_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> for (i = 0; i < g->conf.max_outputs; i++) {
> object_property_set_link(OBJECT(g->scanout[i].con),
> OBJECT(vpci_dev),
> - "device", errp);
> + "device", &error_abort);
> }
> }
>
> diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
> index f02aca6f40..2d7dbbb92d 100644
> --- a/hw/dma/sparc32_dma.c
> +++ b/hw/dma/sparc32_dma.c
> @@ -346,7 +346,7 @@ static void sparc32_ledma_device_realize(DeviceState *dev, Error **errp)
> d = qdev_new(TYPE_LANCE);
> object_property_add_child(OBJECT(dev), "lance", OBJECT(d));
> qdev_set_nic_properties(d, nd);
> - object_property_set_link(OBJECT(d), OBJECT(dev), "dma", errp);
> + object_property_set_link(OBJECT(d), OBJECT(dev), "dma", &error_abort);
> sysbus_realize_and_unref(SYS_BUS_DEVICE(d), &error_fatal);
> }
>
> @@ -379,7 +379,7 @@ static void sparc32_dma_realize(DeviceState *dev, Error **errp)
> }
>
> espdma = qdev_new(TYPE_SPARC32_ESPDMA_DEVICE);
> - object_property_set_link(OBJECT(espdma), iommu, "iommu", errp);
> + object_property_set_link(OBJECT(espdma), iommu, "iommu", &error_abort);
> object_property_add_child(OBJECT(s), "espdma", OBJECT(espdma));
> sysbus_realize_and_unref(SYS_BUS_DEVICE(espdma), &error_fatal);
>
> @@ -394,7 +394,7 @@ static void sparc32_dma_realize(DeviceState *dev, Error **errp)
> sysbus_mmio_get_region(sbd, 0));
>
> ledma = qdev_new(TYPE_SPARC32_LEDMA_DEVICE);
> - object_property_set_link(OBJECT(ledma), iommu, "iommu", errp);
> + object_property_set_link(OBJECT(ledma), iommu, "iommu", &error_abort);
> object_property_add_child(OBJECT(s), "ledma", OBJECT(ledma));
> sysbus_realize_and_unref(SYS_BUS_DEVICE(ledma), &error_fatal);
>
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index 6a9df2c4db..a069637bf2 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -537,7 +537,6 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
> XilinxAXIDMAStreamSlave *ds = XILINX_AXI_DMA_DATA_STREAM(&s->rx_data_dev);
> XilinxAXIDMAStreamSlave *cs = XILINX_AXI_DMA_CONTROL_STREAM(
> &s->rx_control_dev);
> - Error *local_err = NULL;
> int i;
>
> object_property_add_link(OBJECT(ds), "dma", TYPE_XILINX_AXI_DMA,
> @@ -548,11 +547,8 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
> (Object **)&cs->dma,
> object_property_allow_set_link,
> OBJ_PROP_LINK_STRONG);
> - object_property_set_link(OBJECT(ds), OBJECT(s), "dma", &local_err);
> - object_property_set_link(OBJECT(cs), OBJECT(s), "dma", &local_err);
> - if (local_err) {
> - goto xilinx_axidma_realize_fail;
> - }
> + object_property_set_link(OBJECT(ds), OBJECT(s), "dma", &error_abort);
> + object_property_set_link(OBJECT(cs), OBJECT(s), "dma", &error_abort);
>
> for (i = 0; i < 2; i++) {
> struct Stream *st = &s->streams[i];
> @@ -567,10 +563,6 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
>
> address_space_init(&s->as,
> s->dma_mr ? s->dma_mr : get_system_memory(), "dma");
> - return;
> -
> -xilinx_axidma_realize_fail:
> - error_propagate(errp, local_err);
> }
>
> static void xilinx_axidma_init(Object *obj)
> diff --git a/hw/mips/cps.c b/hw/mips/cps.c
> index cdfab19826..5382bc86f7 100644
> --- a/hw/mips/cps.c
> +++ b/hw/mips/cps.c
> @@ -150,8 +150,10 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
> object_property_set_int(OBJECT(&s->gcr), s->num_vp, "num-vp", &err);
> object_property_set_int(OBJECT(&s->gcr), 0x800, "gcr-rev", &err);
> object_property_set_int(OBJECT(&s->gcr), gcr_base, "gcr-base", &err);
> - object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->gic.mr), "gic", &err);
> - object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->cpc.mr), "cpc", &err);
> + object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->gic.mr), "gic",
> + &error_abort);
> + object_property_set_link(OBJECT(&s->gcr), OBJECT(&s->cpc.mr), "cpc",
> + &error_abort);
> sysbus_realize(SYS_BUS_DEVICE(&s->gcr), &err);
> if (err != NULL) {
> error_propagate(errp, err);
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index 8ba7af073c..3251c79f46 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -136,7 +136,8 @@ static void macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
> sysbus_connect_irq(sysbus_dev, 0, irq0);
> sysbus_connect_irq(sysbus_dev, 1, irq1);
> qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
> - object_property_set_link(OBJECT(ide), OBJECT(&s->dbdma), "dbdma", errp);
> + object_property_set_link(OBJECT(ide), OBJECT(&s->dbdma), "dbdma",
> + &error_abort);
> macio_ide_register_dma(ide);
>
> qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
> diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
> index c2f40b8ea9..679a359f9a 100644
> --- a/hw/net/xilinx_axienet.c
> +++ b/hw/net/xilinx_axienet.c
> @@ -980,7 +980,6 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
> XilinxAXIEnetStreamSlave *ds = XILINX_AXI_ENET_DATA_STREAM(&s->rx_data_dev);
> XilinxAXIEnetStreamSlave *cs = XILINX_AXI_ENET_CONTROL_STREAM(
> &s->rx_control_dev);
> - Error *local_err = NULL;
>
> object_property_add_link(OBJECT(ds), "enet", "xlnx.axi-ethernet",
> (Object **) &ds->enet,
> @@ -990,11 +989,8 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
> (Object **) &cs->enet,
> object_property_allow_set_link,
> OBJ_PROP_LINK_STRONG);
> - object_property_set_link(OBJECT(ds), OBJECT(s), "enet", &local_err);
> - object_property_set_link(OBJECT(cs), OBJECT(s), "enet", &local_err);
> - if (local_err) {
> - goto xilinx_enet_realize_fail;
> - }
> + object_property_set_link(OBJECT(ds), OBJECT(s), "enet", &error_abort);
> + object_property_set_link(OBJECT(cs), OBJECT(s), "enet", &error_abort);
>
> qemu_macaddr_default_if_unset(&s->conf.macaddr);
> s->nic = qemu_new_nic(&net_xilinx_enet_info, &s->conf,
> @@ -1008,10 +1004,6 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
>
> s->rxmem = g_malloc(s->c_rxmem);
> s->txmem = g_malloc(s->c_txmem);
> - return;
> -
> -xilinx_enet_realize_fail:
> - error_propagate(errp, local_err);
> }
>
> static void xilinx_enet_init(Object *obj)
> diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c
> index 632533abaf..bd61d6e2f8 100644
> --- a/hw/virtio/virtio-iommu-pci.c
> +++ b/hw/virtio/virtio-iommu-pci.c
> @@ -56,7 +56,7 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> }
> object_property_set_link(OBJECT(dev),
> OBJECT(pci_get_bus(&vpci_dev->pci_dev)),
> - "primary-bus", errp);
> + "primary-bus", &error_abort);
> qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
> }
>
> --
> 2.26.2
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 18/22] riscv/sifive_u: Fix sifive_u_soc_realize() error API violations
2020-06-22 10:42 ` Markus Armbruster
@ 2020-06-22 21:23 ` Alistair Francis
-1 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2020-06-22 21:23 UTC (permalink / raw)
To: Markus Armbruster
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
qemu-devel@nongnu.org Developers, Alistair Francis,
Palmer Dabbelt, Bin Meng
On Mon, Jun 22, 2020 at 3:55 AM Markus Armbruster <armbru@redhat.com> 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.
>
> sifive_u_soc_realize() is wrong that way: it passes &err to
> sysbus_realize() three times before checking it. Harmless, because
> the first two can't actually fail (I think).
>
> Fix by checking for failure right away.
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> hw/riscv/sifive_u.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index ea197ab64f..3857b92d9a 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -587,11 +587,15 @@ static void sifive_u_soc_realize(DeviceState *dev, Error **errp)
> memmap[SIFIVE_U_CLINT].size, ms->smp.cpus,
> SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE, false);
>
> - sysbus_realize(SYS_BUS_DEVICE(&s->prci), &err);
> + if (!sysbus_realize(SYS_BUS_DEVICE(&s->prci), errp)) {
> + return;
> + }
> sysbus_mmio_map(SYS_BUS_DEVICE(&s->prci), 0, memmap[SIFIVE_U_PRCI].base);
>
> qdev_prop_set_uint32(DEVICE(&s->otp), "serial", s->serial);
> - sysbus_realize(SYS_BUS_DEVICE(&s->otp), &err);
> + if (!sysbus_realize(SYS_BUS_DEVICE(&s->otp), errp)) {
> + return;
> + }
> sysbus_mmio_map(SYS_BUS_DEVICE(&s->otp), 0, memmap[SIFIVE_U_OTP].base);
>
> for (i = 0; i < SIFIVE_U_PLIC_NUM_SOURCES; i++) {
> --
> 2.26.2
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 18/22] riscv/sifive_u: Fix sifive_u_soc_realize() error API violations
@ 2020-06-22 21:23 ` Alistair Francis
0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2020-06-22 21:23 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
Sagar Karandikar, Bastian Koppelmann, Alistair Francis,
Palmer Dabbelt, Bin Meng
On Mon, Jun 22, 2020 at 3:55 AM Markus Armbruster <armbru@redhat.com> 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.
>
> sifive_u_soc_realize() is wrong that way: it passes &err to
> sysbus_realize() three times before checking it. Harmless, because
> the first two can't actually fail (I think).
>
> Fix by checking for failure right away.
>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> hw/riscv/sifive_u.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index ea197ab64f..3857b92d9a 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -587,11 +587,15 @@ static void sifive_u_soc_realize(DeviceState *dev, Error **errp)
> memmap[SIFIVE_U_CLINT].size, ms->smp.cpus,
> SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE, false);
>
> - sysbus_realize(SYS_BUS_DEVICE(&s->prci), &err);
> + if (!sysbus_realize(SYS_BUS_DEVICE(&s->prci), errp)) {
> + return;
> + }
> sysbus_mmio_map(SYS_BUS_DEVICE(&s->prci), 0, memmap[SIFIVE_U_PRCI].base);
>
> qdev_prop_set_uint32(DEVICE(&s->otp), "serial", s->serial);
> - sysbus_realize(SYS_BUS_DEVICE(&s->otp), &err);
> + if (!sysbus_realize(SYS_BUS_DEVICE(&s->otp), errp)) {
> + return;
> + }
> sysbus_mmio_map(SYS_BUS_DEVICE(&s->otp), 0, memmap[SIFIVE_U_OTP].base);
>
> for (i = 0; i < SIFIVE_U_PLIC_NUM_SOURCES; i++) {
> --
> 2.26.2
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 01/22] net/virtio: Fix failover_replug_primary() return value regression
2020-06-22 10:42 ` [PATCH 01/22] net/virtio: Fix failover_replug_primary() return value regression Markus Armbruster
@ 2020-06-22 21:25 ` Jens Freimann
2020-06-24 15:12 ` Michael S. Tsirkin
0 siblings, 1 reply; 41+ messages in thread
From: Jens Freimann @ 2020-06-22 21:25 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael S . Tsirkin, qemu-devel, qemu-stable
On Mon, Jun 22, 2020 at 12:42:29PM +0200, Markus Armbruster wrote:
>Commit 150ab54aa6 "net/virtio: fix re-plugging of primary device"
>fixed failover_replug_primary() to return false on failure. Commit
>5a0948d36c "net/virtio: Fix failover error handling crash bugs" broke
>it again for hotplug_handler_plug() failure. Unbreak it.
>
>Commit 5a0948d36c4cbc1c5534afac6fee99de55245d12
>
>Fixes: 5a0948d36c4cbc1c5534afac6fee99de55245d12
>Cc: Jens Freimann <jfreimann@redhat.com>
>Cc: Michael S. Tsirkin <mst@redhat.com>
>Cc: qemu-stable@nongnu.org
>Signed-off-by: Markus Armbruster <armbru@redhat.com>
>---
> hw/net/virtio-net.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Jens Freimann <jfreimann@redhat.com>
Thanks!
regards,
Jens
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 02/22] pci: Delete useless error_propagate()
2020-06-22 10:42 ` [PATCH 02/22] pci: Delete useless error_propagate() Markus Armbruster
@ 2020-06-22 21:26 ` Jens Freimann
2020-06-24 15:13 ` Michael S. Tsirkin
0 siblings, 1 reply; 41+ messages in thread
From: Jens Freimann @ 2020-06-22 21:26 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael S . Tsirkin
On Mon, Jun 22, 2020 at 12:42:30PM +0200, Markus Armbruster wrote:
>Cc: Jens Freimann <jfreimann@redhat.com>
>Cc: Michael S. Tsirkin <mst@redhat.com>
>Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>Signed-off-by: Markus Armbruster <armbru@redhat.com>
>---
> hw/pci/pci.c | 3 ---
> 1 file changed, 3 deletions(-)
>
Reviewed-by: Jens Freimann <jfreimann@redhat.com>
Thanks!
regards,
Jens
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH 08/22] qga: Plug unlikely memory leak in guest-set-memory-blocks
2020-06-22 10:42 ` [PATCH 08/22] qga: Plug unlikely memory leak in guest-set-memory-blocks Markus Armbruster
@ 2020-06-23 8:35 ` Zhanghailiang
0 siblings, 0 replies; 41+ messages in thread
From: Zhanghailiang @ 2020-06-23 8:35 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Michael Roth
Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> -----Original Message-----
> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Monday, June 22, 2020 6:43 PM
> To: qemu-devel@nongnu.org
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>; Zhanghailiang
> <zhang.zhanghailiang@huawei.com>
> Subject: [PATCH 08/22] qga: Plug unlikely memory leak in
> guest-set-memory-blocks
>
> transfer_memory_block() leaks an Error object when reading file
> /sys/devices/system/memory/memory<INDEX>/state fails with errno other
> than ENOENT, and @sys2memblk is false, i.e. when the state file exists but
> cannot be read (seems quite unlikely), and this is guest-set-memory-blocks,
> not guest-get-memory-blocks.
>
> Plug the leak.
>
> Fixes: bd240fca42d5f072fb758a71720d9de9990ac553
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> Cc: Hailiang Zhang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> qga/commands-posix.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c index
> ae1348dc8f..cdbeb59dcc 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2421,6 +2421,7 @@ static void
> transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
> if (sys2memblk) {
> error_propagate(errp, local_err);
> } else {
> + error_free(local_err);
> result->response =
>
> GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_FAILED;
> }
> --
> 2.26.2
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 01/22] net/virtio: Fix failover_replug_primary() return value regression
2020-06-22 21:25 ` Jens Freimann
@ 2020-06-24 15:12 ` Michael S. Tsirkin
0 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2020-06-24 15:12 UTC (permalink / raw)
To: Jens Freimann; +Cc: qemu-stable, Markus Armbruster, qemu-devel
On Mon, Jun 22, 2020 at 11:25:59PM +0200, Jens Freimann wrote:
> On Mon, Jun 22, 2020 at 12:42:29PM +0200, Markus Armbruster wrote:
> > Commit 150ab54aa6 "net/virtio: fix re-plugging of primary device"
> > fixed failover_replug_primary() to return false on failure. Commit
> > 5a0948d36c "net/virtio: Fix failover error handling crash bugs" broke
> > it again for hotplug_handler_plug() failure. Unbreak it.
> >
> > Commit 5a0948d36c4cbc1c5534afac6fee99de55245d12
> >
> > Fixes: 5a0948d36c4cbc1c5534afac6fee99de55245d12
> > Cc: Jens Freimann <jfreimann@redhat.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> > hw/net/virtio-net.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
>
> Reviewed-by: Jens Freimann <jfreimann@redhat.com>
>
> Thanks!
>
> regards,
> Jens
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Who's merging this? Jason? Or is all this going through a single tree?
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 02/22] pci: Delete useless error_propagate()
2020-06-22 21:26 ` Jens Freimann
@ 2020-06-24 15:13 ` Michael S. Tsirkin
0 siblings, 0 replies; 41+ messages in thread
From: Michael S. Tsirkin @ 2020-06-24 15:13 UTC (permalink / raw)
To: Jens Freimann; +Cc: Markus Armbruster, qemu-devel
On Mon, Jun 22, 2020 at 11:26:22PM +0200, Jens Freimann wrote:
> On Mon, Jun 22, 2020 at 12:42:30PM +0200, Markus Armbruster wrote:
> > Cc: Jens Freimann <jfreimann@redhat.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> > hw/pci/pci.c | 3 ---
> > 1 file changed, 3 deletions(-)
> >
>
> Reviewed-by: Jens Freimann <jfreimann@redhat.com>
>
> Thanks!
>
> regards,
> Jens
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Are you merging this? Or want me to?
^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2020-06-24 15:14 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-22 10:42 [PATCH 00/22] Error handling fixes & cleanups Markus Armbruster
2020-06-22 10:42 ` [PATCH 01/22] net/virtio: Fix failover_replug_primary() return value regression Markus Armbruster
2020-06-22 21:25 ` Jens Freimann
2020-06-24 15:12 ` Michael S. Tsirkin
2020-06-22 10:42 ` [PATCH 02/22] pci: Delete useless error_propagate() Markus Armbruster
2020-06-22 21:26 ` Jens Freimann
2020-06-24 15:13 ` Michael S. Tsirkin
2020-06-22 10:42 ` [PATCH 03/22] Clean up some calls to ignore Error objects the right way Markus Armbruster
2020-06-22 14:43 ` Greg Kurz
2020-06-22 10:42 ` [PATCH 04/22] tests: Use &error_abort where appropriate Markus Armbruster
2020-06-22 11:26 ` Thomas Huth
2020-06-22 10:42 ` [PATCH 05/22] tests: Use error_free_or_abort() " Markus Armbruster
2020-06-22 10:42 ` [PATCH 06/22] usb/dev-mtp: Fix Error double free after inotify failure Markus Armbruster
2020-06-22 10:42 ` [PATCH 07/22] spapr: Plug minor memory leak in spapr_machine_init() Markus Armbruster
2020-06-22 14:54 ` Greg Kurz
2020-06-22 10:42 ` [PATCH 08/22] qga: Plug unlikely memory leak in guest-set-memory-blocks Markus Armbruster
2020-06-23 8:35 ` Zhanghailiang
2020-06-22 10:42 ` [PATCH 09/22] sd/milkymist-memcard: Plug minor memory leak in realize Markus Armbruster
2020-06-22 10:42 ` [PATCH 10/22] test-util-filemonitor: Plug unlikely memory leak Markus Armbruster
2020-06-22 10:42 ` [PATCH 11/22] vnc: Plug minor memory leak in vnc_display_open() Markus Armbruster
2020-06-22 10:42 ` [PATCH 12/22] tests/qom-proplist: Delete a superfluous error_free() Markus Armbruster
2020-06-22 10:42 ` [PATCH 13/22] aspeed: Clean up roundabout error propagation Markus Armbruster
2020-06-22 12:37 ` Cédric Le Goater
2020-06-22 10:42 ` [PATCH 14/22] qdev: Drop qbus_set_bus_hotplug_handler() parameter @errp Markus Armbruster
2020-06-22 10:42 ` [PATCH 15/22] qdev: Drop qbus_set_hotplug_handler() " Markus Armbruster
2020-06-22 10:42 ` [PATCH 16/22] hw: Fix error API violation around object_property_set_link() Markus Armbruster
2020-06-22 12:47 ` Auger Eric
2020-06-22 21:22 ` Alistair Francis
2020-06-22 10:42 ` [PATCH 17/22] hw/arm: Drop useless object_property_set_link() error handling Markus Armbruster
2020-06-22 12:37 ` Cédric Le Goater
2020-06-22 10:42 ` [PATCH 18/22] riscv/sifive_u: Fix sifive_u_soc_realize() error API violations Markus Armbruster
2020-06-22 10:42 ` Markus Armbruster
2020-06-22 21:23 ` Alistair Francis
2020-06-22 21:23 ` Alistair Francis
2020-06-22 10:42 ` [PATCH 19/22] riscv_hart: Fix riscv_harts_realize() " Markus Armbruster
2020-06-22 10:42 ` Markus Armbruster
2020-06-22 21:19 ` Alistair Francis
2020-06-22 21:19 ` Alistair Francis
2020-06-22 10:42 ` [PATCH 20/22] mips/cps: Fix mips_cps_realize() " Markus Armbruster
2020-06-22 10:42 ` [PATCH 21/22] x86: Fix x86_cpu_new() " Markus Armbruster
2020-06-22 10:42 ` [PATCH 22/22] amd_iommu: Fix amdvi_realize() error API violation Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.