Re: [Qemu-devel] [PATCH v2 3/3] device_tree: qemu_fdt_setprop: Fixup error reporting
On 11.11.2013, at 09:16, peter.crosthwa...@xilinx.com wrote:
From: Peter Crosthwaite peter.crosthwa...@xilinx.com
There are a mix of usages of the qemu_fdt_* API calls, some which
wish to assert and abort QEMU on failure and some of which wish to do
their own error handling. The latter in more correct, but the former
is in the majority and more pragmatic. However the asserting clients
are usually doing large numbers fdt ops and only want to assert if any
one of them breaks. So the cleanest compromising solution is:
1. Require an Error ** to be passes to all fdt ops.
2. And perform no action if its already non-null (error condition).
3. If no Error ** is given report errors to stderr
Step one allows clients to do their own error handling if they wish
too, which is the most flexible:
Error *err = NULL;
fdt_foo( ... , err);
if (err) {
/* handle error my special way */
}
Step two allows you to do a large number of fdt ops then check them
all only the once at the end:
Error *err = NULL;
fdt_foo( ... , err);
fdt_bar( ... , err);
fdt_baz( ... , err);
if (err) {
/* First encountered error will be in err */
}
Step 3, handles the common case where the error is not an issue, but
just needs to make noise at the user (via stderr). This seems common for
bootloader code that sets chosen and initrd props etc (rather than machine
description).
All error reporting is stylistically udpated to use Error ** instead of
integer return codes and no more exit(1) on failures.
Signed-off-by: Peter Crosthwaite peter.crosthwa...@xilinx.com
Thanks a lot. I've applied the first two patches to ppc-next with some
adjustments to also cover hw/arm/virt.c. But this patch is too much work to
rebase to current master :).
Please repost a version that also covers virt.c. I'd also like to see an Ack
from Peter at least.
Alex
Re: [Qemu-devel] [PATCH v2 3/3] device_tree: qemu_fdt_setprop: Fixup error reporting
On Wed, Dec 18, 2013 at 11:48 PM, Alexander Graf ag...@suse.de wrote:
On 11.11.2013, at 09:16, peter.crosthwa...@xilinx.com wrote:
From: Peter Crosthwaite peter.crosthwa...@xilinx.com
There are a mix of usages of the qemu_fdt_* API calls, some which
wish to assert and abort QEMU on failure and some of which wish to do
their own error handling. The latter in more correct, but the former
is in the majority and more pragmatic. However the asserting clients
are usually doing large numbers fdt ops and only want to assert if any
one of them breaks. So the cleanest compromising solution is:
1. Require an Error ** to be passes to all fdt ops.
2. And perform no action if its already non-null (error condition).
3. If no Error ** is given report errors to stderr
Step one allows clients to do their own error handling if they wish
too, which is the most flexible:
Error *err = NULL;
fdt_foo( ... , err);
if (err) {
/* handle error my special way */
}
Step two allows you to do a large number of fdt ops then check them
all only the once at the end:
Error *err = NULL;
fdt_foo( ... , err);
fdt_bar( ... , err);
fdt_baz( ... , err);
if (err) {
/* First encountered error will be in err */
}
Step 3, handles the common case where the error is not an issue, but
just needs to make noise at the user (via stderr). This seems common for
bootloader code that sets chosen and initrd props etc (rather than machine
description).
All error reporting is stylistically udpated to use Error ** instead of
integer return codes and no more exit(1) on failures.
Signed-off-by: Peter Crosthwaite peter.crosthwa...@xilinx.com
Thanks a lot. I've applied the first two patches to ppc-next with some
adjustments to also cover hw/arm/virt.c.
Thanks,
That make my respin of this one much easier.
But this patch is too much work to rebase to current master :).
This patch is going to see a major change pattern once the
error_abort series goes through, as the assert_no_error scheme is now
gone. This would have conflicted with the qmp queue that holds those
patches (I probably should have put a note on list to retract this
patch).
Please repost a version that also covers virt.c. I'd also like to see an Ack
from Peter at least.
I'll wait for both yours and Luiz' queues to flush and spin it.
Regards,
Peter
Alex
[Qemu-devel] [PATCH v2 3/3] device_tree: qemu_fdt_setprop: Fixup error reporting
From: Peter Crosthwaite peter.crosthwa...@xilinx.com
There are a mix of usages of the qemu_fdt_* API calls, some which
wish to assert and abort QEMU on failure and some of which wish to do
their own error handling. The latter in more correct, but the former
is in the majority and more pragmatic. However the asserting clients
are usually doing large numbers fdt ops and only want to assert if any
one of them breaks. So the cleanest compromising solution is:
1. Require an Error ** to be passes to all fdt ops.
2. And perform no action if its already non-null (error condition).
3. If no Error ** is given report errors to stderr
Step one allows clients to do their own error handling if they wish
too, which is the most flexible:
Error *err = NULL;
fdt_foo( ... , err);
if (err) {
/* handle error my special way */
}
Step two allows you to do a large number of fdt ops then check them
all only the once at the end:
Error *err = NULL;
fdt_foo( ... , err);
fdt_bar( ... , err);
fdt_baz( ... , err);
if (err) {
/* First encountered error will be in err */
}
Step 3, handles the common case where the error is not an issue, but
just needs to make noise at the user (via stderr). This seems common for
bootloader code that sets chosen and initrd props etc (rather than machine
description).
All error reporting is stylistically udpated to use Error ** instead of
integer return codes and no more exit(1) on failures.
Signed-off-by: Peter Crosthwaite peter.crosthwa...@xilinx.com
---
device_tree.c| 222 ---
hw/arm/boot.c| 49 --
hw/arm/vexpress.c| 23 ++---
hw/microblaze/boot.c | 13 +--
hw/ppc/e500.c| 214 -
hw/ppc/e500.h| 3 +-
hw/ppc/e500plat.c| 9 +-
hw/ppc/mpc8544ds.c | 9 +-
hw/ppc/ppc440_bamboo.c | 30 ++
hw/ppc/spapr_rtas.c | 40 ++--
hw/ppc/virtex_ml507.c| 5 +-
include/sysemu/device_tree.h | 193 ++---
12 files changed, 483 insertions(+), 327 deletions(-)
diff --git a/device_tree.c b/device_tree.c
index 1e6bbad..525cdaa 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -4,8 +4,10 @@
* interface.
*
* Copyright 2008 IBM Corporation.
+ * Copyright 2013 Xilinx Inc.
* Authors: Jerone Young jyou...@us.ibm.com
* Hollis Blanchard holl...@us.ibm.com
+ * Peter Crosthwaite peter.crosthwa...@xilinx.com
*
* This work is licensed under the GNU GPL license version 2 or later.
*
@@ -113,122 +115,176 @@ fail:
return NULL;
}
-static int findnode_nofail(void *fdt, const char *node_path)
+#define error_set_or_print(errp, ...) \
+do {\
+if (errp) { \
+error_setg(errp, __VA_ARGS__); \
+} else {\
+fprintf(stderr, __VA_ARGS__); \
+} \
+} while (0)
+
+static int findnode(void *fdt, const char *node_path, Error **errp)
{
int offset;
offset = fdt_path_offset(fdt, node_path);
if (offset 0) {
-fprintf(stderr, %s Couldn't find node %s: %s\n, __func__, node_path,
-fdt_strerror(offset));
-exit(1);
+error_set_or_print(errp, %s Couldn't find node %s: %s\n, __func__,
+ node_path, fdt_strerror(offset));
}
return offset;
}
-int qemu_fdt_setprop(void *fdt, const char *node_path,
- const char *property, const void *val, int size)
+void qemu_fdt_setprop(void *fdt, const char *node_path, const char *property,
+ const void *val, int size, Error **errp)
{
int r;
-r = fdt_setprop(fdt, findnode_nofail(fdt, node_path), property, val, size);
-if (r 0) {
-fprintf(stderr, %s: Couldn't set %s/%s: %s\n, __func__, node_path,
-property, fdt_strerror(r));
-exit(1);
+if (errp *errp) {
+return;
}
-return r;
+r = findnode(fdt, node_path, errp);
+if (r 0) {
+return;
+}
+r = fdt_setprop(fdt, r, property, val, size);
+if (r 0) {
+error_set_or_print(errp, %s: Couldn't set %s/%s: %s\n, __func__,
+ node_path, property, fdt_strerror(r));
+}
}
-int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
- const char *property, uint32_t val)
+void qemu_fdt_setprop_cell(void *fdt, const char *node_path,
+ const char *property, uint32_t val, Error **errp)
{
int r;
-r = fdt_setprop_cell(fdt, findnode_nofail(fdt, node_path), property, val);
-if (r 0) {
-
