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