* [Qemu-devel] [PATCH v2 0/4] qapi/error: converts error_setg(&error_fatal) to error_report() + exit()
@ 2018-06-07 14:46 Philippe Mathieu-Daudé
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 1/4] hw/block/fdc: Replace error_setg(&error_abort) by assert() Philippe Mathieu-Daudé
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-07 14:46 UTC (permalink / raw)
To: Eric Blake, Markus Armbruster
Cc: Philippe Mathieu-Daudé,
qemu-devel, Peter Maydell, qemu-block, qemu-ppc, qemu-arm,
John Snow, Kevin Wolf, Max Reitz, David Gibson,
Peter Crosthwaite, Alexander Graf
Hi,
This series converts error_setg(&error_fatal) to error_report() + exit() as
suggested by the "qapi/error.h" documentation.
This reduce Coverity and Clang static analyzer positive falses.
See http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg07585.html:
On 07/24/2017 04:52 PM, Eric Blake wrote:
That's a shame. Rather, we should patch this file (and others) to avoid
all the inconsistent uses of error_setg(&error_*), to comply with the
error.h documentation.
Since v1:
- patch #1: use assert() directly (Markus explanation)
- patch #2: use abort() without error_report() (Markus 'no lipstick')
- patch #3: replaced exit() by assert() (Markus)
- patch #4: no change, added R-b
Regards,
Phil.
Philippe Mathieu-Daudé (4):
hw/block/fdc: Replace error_setg(&error_abort) by assert()
hw/ppc/spapr_drc: Replace error_setg(&error_abort) by abort()
hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit()
device_tree: Replace error_setg(&error_fatal) by error_report() + exit()
device_tree.c | 23 +++++++++++++----------
hw/arm/sysbus-fdt.c | 42 +++++++++++++++++++++++-------------------
hw/block/fdc.c | 9 +--------
hw/ppc/spapr_drc.c | 2 +-
4 files changed, 38 insertions(+), 38 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v2 1/4] hw/block/fdc: Replace error_setg(&error_abort) by assert()
2018-06-07 14:46 [Qemu-devel] [PATCH v2 0/4] qapi/error: converts error_setg(&error_fatal) to error_report() + exit() Philippe Mathieu-Daudé
@ 2018-06-07 14:46 ` Philippe Mathieu-Daudé
2018-06-07 17:18 ` John Snow
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 2/4] hw/ppc/spapr_drc: Replace error_setg(&error_abort) by abort() Philippe Mathieu-Daudé
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-07 14:46 UTC (permalink / raw)
To: Eric Blake, Markus Armbruster, John Snow, Kevin Wolf, Max Reitz
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-block, Peter Maydell
Use assert() instead of error_setg(&error_abort),
as suggested by the "qapi/error.h" documentation:
Please don't error_setg(&error_fatal, ...), use error_report() and
exit(), because that's more obvious.
Likewise, don't error_setg(&error_abort, ...), use assert().
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/block/fdc.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index cd29e27d8f..7c1c57f57f 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -396,16 +396,9 @@ static int pick_geometry(FDrive *drv)
nb_sectors,
FloppyDriveType_str(parse->drive));
}
+ assert(type_match != -1 && "misconfigured fd_format");
match = type_match;
}
-
- /* No match of any kind found -- fd_format is misconfigured, abort. */
- if (match == -1) {
- error_setg(&error_abort, "No candidate geometries present in table "
- " for floppy drive type '%s'",
- FloppyDriveType_str(drv->drive));
- }
-
parse = &(fd_formats[match]);
out:
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] hw/ppc/spapr_drc: Replace error_setg(&error_abort) by abort()
2018-06-07 14:46 [Qemu-devel] [PATCH v2 0/4] qapi/error: converts error_setg(&error_fatal) to error_report() + exit() Philippe Mathieu-Daudé
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 1/4] hw/block/fdc: Replace error_setg(&error_abort) by assert() Philippe Mathieu-Daudé
@ 2018-06-07 14:46 ` Philippe Mathieu-Daudé
2018-06-08 3:03 ` David Gibson
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 3/4] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit() Philippe Mathieu-Daudé
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 4/4] device_tree: " Philippe Mathieu-Daudé
3 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-07 14:46 UTC (permalink / raw)
To: Eric Blake, Markus Armbruster, David Gibson, Alexander Graf
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-ppc, Peter Maydell
Use abort() instead of error_setg(&error_abort),
as suggested by the "qapi/error.h" documentation:
Please don't error_setg(&error_fatal, ...), use error_report() and
exit(), because that's more obvious.
Likewise, don't error_setg(&error_abort, ...), use assert().
Use abort() instead of the suggested assert() because the assertion is
already verified by the switch case.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/ppc/spapr_drc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 8a045d6b93..b934b9c9ed 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -366,7 +366,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
break;
}
default:
- error_setg(&error_abort, "device FDT in unexpected state: %d", tag);
+ abort(); /* device FDT in unexpected state */
}
fdt_offset = fdt_offset_next;
} while (fdt_depth != 0);
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v2 3/4] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit()
2018-06-07 14:46 [Qemu-devel] [PATCH v2 0/4] qapi/error: converts error_setg(&error_fatal) to error_report() + exit() Philippe Mathieu-Daudé
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 1/4] hw/block/fdc: Replace error_setg(&error_abort) by assert() Philippe Mathieu-Daudé
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 2/4] hw/ppc/spapr_drc: Replace error_setg(&error_abort) by abort() Philippe Mathieu-Daudé
@ 2018-06-07 14:46 ` Philippe Mathieu-Daudé
2018-06-08 6:27 ` Markus Armbruster
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 4/4] device_tree: " Philippe Mathieu-Daudé
3 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-07 14:46 UTC (permalink / raw)
To: Eric Blake, Markus Armbruster, Peter Maydell, Peter Crosthwaite,
Alexander Graf
Cc: Philippe Mathieu-Daudé, qemu-devel, David Gibson, qemu-arm
Use error_report() + exit() instead of error_setg(&error_fatal),
as suggested by the "qapi/error.h" documentation:
Please don't error_setg(&error_fatal, ...), use error_report() and
exit(), because that's more obvious.
This fixes CID 1352173:
"Passing null pointer dt_name to qemu_fdt_node_path, which dereferences it."
And this also fixes:
hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable 'node_path') results in a null pointer dereference
if (node_path[1]) {
^~~~~~~~~~~~
Fixes: Coverity CID 1352173 (Dereference after null check)
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/arm/sysbus-fdt.c | 42 +++++++++++++++++++++++-------------------
1 file changed, 23 insertions(+), 19 deletions(-)
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index e4c492ea44..8e2784fa11 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -91,7 +91,7 @@ static void copy_properties_from_host(HostProperty *props, int nb_props,
r = qemu_fdt_getprop(host_fdt, node_path,
props[i].name,
&prop_len,
- props[i].optional ? &err : &error_fatal);
+ &err);
if (r) {
qemu_fdt_setprop(guest_fdt, nodename,
props[i].name, r, prop_len);
@@ -102,6 +102,7 @@ static void copy_properties_from_host(HostProperty *props, int nb_props,
} else {
error_free(err);
}
+ assert(props[i].optional); /* mandatory property not found */
}
}
}
@@ -137,9 +138,9 @@ static void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
node_offset = fdt_node_offset_by_phandle(host_fdt, host_phandle);
if (node_offset <= 0) {
- error_setg(&error_fatal,
- "not able to locate clock handle %d in host device tree",
- host_phandle);
+ error_report("not able to locate clock handle %d in host device tree",
+ host_phandle);
+ exit(1);
}
node_path = g_malloc(path_len);
while ((ret = fdt_get_path(host_fdt, node_offset, node_path, path_len))
@@ -148,16 +149,16 @@ static void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
node_path = g_realloc(node_path, path_len);
}
if (ret < 0) {
- error_setg(&error_fatal,
- "not able to retrieve node path for clock handle %d",
- host_phandle);
+ error_report("not able to retrieve node path for clock handle %d",
+ host_phandle);
+ exit(1);
}
r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len,
&error_fatal);
if (strcmp(r, "fixed-clock")) {
- error_setg(&error_fatal,
- "clock handle %d is not a fixed clock", host_phandle);
+ error_report("clock handle %d is not a fixed clock", host_phandle);
+ exit(1);
}
nodename = strrchr(node_path, '/');
@@ -300,34 +301,37 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
dt_name = sysfs_to_dt_name(vbasedev->name);
if (!dt_name) {
- error_setg(&error_fatal, "%s incorrect sysfs device name %s",
- __func__, vbasedev->name);
+ error_report("%s incorrect sysfs device name %s",
+ __func__, vbasedev->name);
+ exit(1);
}
node_path = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat,
&error_fatal);
if (!node_path || !node_path[0]) {
- error_setg(&error_fatal, "%s unable to retrieve node path for %s/%s",
- __func__, dt_name, vdev->compat);
+ error_report("%s unable to retrieve node path for %s/%s",
+ __func__, dt_name, vdev->compat);
+ exit(1);
}
if (node_path[1]) {
- error_setg(&error_fatal, "%s more than one node matching %s/%s!",
- __func__, dt_name, vdev->compat);
+ error_report("%s more than one node matching %s/%s!",
+ __func__, dt_name, vdev->compat);
+ exit(1);
}
g_free(dt_name);
if (vbasedev->num_regions != 5) {
- error_setg(&error_fatal, "%s Does the host dt node combine XGBE/PHY?",
- __func__);
+ error_report("%s Does the host dt node combine XGBE/PHY?", __func__);
+ exit(1);
}
/* generate nodes for DMA_CLK and PTP_CLK */
r = qemu_fdt_getprop(host_fdt, node_path[0], "clocks",
&prop_len, &error_fatal);
if (prop_len != 8) {
- error_setg(&error_fatal, "%s clocks property should contain 2 handles",
- __func__);
+ error_report("%s clocks property should contain 2 handles", __func__);
+ exit(1);
}
host_clock_phandles = (uint32_t *)r;
guest_clock_phandles[0] = qemu_fdt_alloc_phandle(guest_fdt);
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v2 4/4] device_tree: Replace error_setg(&error_fatal) by error_report() + exit()
2018-06-07 14:46 [Qemu-devel] [PATCH v2 0/4] qapi/error: converts error_setg(&error_fatal) to error_report() + exit() Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 3/4] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit() Philippe Mathieu-Daudé
@ 2018-06-07 14:46 ` Philippe Mathieu-Daudé
2018-06-08 3:04 ` David Gibson
3 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-07 14:46 UTC (permalink / raw)
To: Eric Blake, Markus Armbruster, Peter Maydell, Peter Crosthwaite,
Alexander Graf
Cc: Philippe Mathieu-Daudé, qemu-devel, David Gibson
Use error_report() + exit() instead of error_setg(&error_fatal),
as suggested by the "qapi/error.h" documentation:
Please don't error_setg(&error_fatal, ...), use error_report() and
exit(), because that's more obvious.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
device_tree.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/device_tree.c b/device_tree.c
index 52c3358a55..3553819257 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -140,15 +140,16 @@ static void read_fstree(void *fdt, const char *dirname)
const char *parent_node;
if (strstr(dirname, root_dir) != dirname) {
- error_setg(&error_fatal, "%s: %s must be searched within %s",
- __func__, dirname, root_dir);
+ error_report("%s: %s must be searched within %s",
+ __func__, dirname, root_dir);
+ exit(1);
}
parent_node = &dirname[strlen(SYSFS_DT_BASEDIR)];
d = opendir(dirname);
if (!d) {
- error_setg(&error_fatal, "%s cannot open %s", __func__, dirname);
- return;
+ error_report("%s cannot open %s", __func__, dirname);
+ exit(1);
}
while ((de = readdir(d)) != NULL) {
@@ -162,7 +163,8 @@ static void read_fstree(void *fdt, const char *dirname)
tmpnam = g_strdup_printf("%s/%s", dirname, de->d_name);
if (lstat(tmpnam, &st) < 0) {
- error_setg(&error_fatal, "%s cannot lstat %s", __func__, tmpnam);
+ error_report("%s cannot lstat %s", __func__, tmpnam);
+ exit(1);
}
if (S_ISREG(st.st_mode)) {
@@ -170,8 +172,9 @@ static void read_fstree(void *fdt, const char *dirname)
gsize len;
if (!g_file_get_contents(tmpnam, &val, &len, NULL)) {
- error_setg(&error_fatal, "%s not able to extract info from %s",
- __func__, tmpnam);
+ error_report("%s not able to extract info from %s",
+ __func__, tmpnam);
+ exit(1);
}
if (strlen(parent_node) > 0) {
@@ -206,9 +209,9 @@ void *load_device_tree_from_sysfs(void)
host_fdt = create_device_tree(&host_fdt_size);
read_fstree(host_fdt, SYSFS_DT_BASEDIR);
if (fdt_check_header(host_fdt)) {
- error_setg(&error_fatal,
- "%s host device tree extracted into memory is invalid",
- __func__);
+ error_report("%s host device tree extracted into memory is invalid",
+ __func__);
+ exit(1);
}
return host_fdt;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hw/block/fdc: Replace error_setg(&error_abort) by assert()
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 1/4] hw/block/fdc: Replace error_setg(&error_abort) by assert() Philippe Mathieu-Daudé
@ 2018-06-07 17:18 ` John Snow
2018-06-08 6:05 ` Markus Armbruster
0 siblings, 1 reply; 15+ messages in thread
From: John Snow @ 2018-06-07 17:18 UTC (permalink / raw)
To: Philippe Mathieu-Daudé,
Eric Blake, Markus Armbruster, Kevin Wolf, Max Reitz
Cc: qemu-devel, qemu-block, Peter Maydell
On 06/07/2018 10:46 AM, Philippe Mathieu-Daudé wrote:
> Use assert() instead of error_setg(&error_abort),
> as suggested by the "qapi/error.h" documentation:
>
> Please don't error_setg(&error_fatal, ...), use error_report() and
> exit(), because that's more obvious.
> Likewise, don't error_setg(&error_abort, ...), use assert().
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/block/fdc.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index cd29e27d8f..7c1c57f57f 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -396,16 +396,9 @@ static int pick_geometry(FDrive *drv)
> nb_sectors,
> FloppyDriveType_str(parse->drive));
> }
> + assert(type_match != -1 && "misconfigured fd_format");
> match = type_match;
> }
> -
> - /* No match of any kind found -- fd_format is misconfigured, abort. */
> - if (match == -1) {
> - error_setg(&error_abort, "No candidate geometries present in table "
> - " for floppy drive type '%s'",
> - FloppyDriveType_str(drv->drive));
> - }
> -
> parse = &(fd_formats[match]);
>
> out:
>
Truthfully sad to see the lipstick go, because this is my pig.
Oh well, it doesn't matter. Not really, anyway.
ACK
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] hw/ppc/spapr_drc: Replace error_setg(&error_abort) by abort()
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 2/4] hw/ppc/spapr_drc: Replace error_setg(&error_abort) by abort() Philippe Mathieu-Daudé
@ 2018-06-08 3:03 ` David Gibson
2018-06-08 3:54 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 15+ messages in thread
From: David Gibson @ 2018-06-08 3:03 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Eric Blake, Markus Armbruster, Alexander Graf, qemu-devel,
qemu-ppc, Peter Maydell
[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]
On Thu, Jun 07, 2018 at 11:46:43AM -0300, Philippe Mathieu-Daudé wrote:
> Use abort() instead of error_setg(&error_abort),
> as suggested by the "qapi/error.h" documentation:
>
> Please don't error_setg(&error_fatal, ...), use error_report() and
> exit(), because that's more obvious.
> Likewise, don't error_setg(&error_abort, ...), use assert().
>
> Use abort() instead of the suggested assert() because the assertion is
> already verified by the switch case.
I think g_assert_not_reached() would be the right thing here.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/ppc/spapr_drc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 8a045d6b93..b934b9c9ed 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -366,7 +366,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
> break;
> }
> default:
> - error_setg(&error_abort, "device FDT in unexpected state: %d", tag);
> + abort(); /* device FDT in unexpected state */
> }
> fdt_offset = fdt_offset_next;
> } while (fdt_depth != 0);
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] device_tree: Replace error_setg(&error_fatal) by error_report() + exit()
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 4/4] device_tree: " Philippe Mathieu-Daudé
@ 2018-06-08 3:04 ` David Gibson
0 siblings, 0 replies; 15+ messages in thread
From: David Gibson @ 2018-06-08 3:04 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Eric Blake, Markus Armbruster, Peter Maydell, Peter Crosthwaite,
Alexander Graf, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3288 bytes --]
On Thu, Jun 07, 2018 at 11:46:45AM -0300, Philippe Mathieu-Daudé wrote:
> Use error_report() + exit() instead of error_setg(&error_fatal),
> as suggested by the "qapi/error.h" documentation:
>
> Please don't error_setg(&error_fatal, ...), use error_report() and
> exit(), because that's more obvious.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> device_tree.c | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/device_tree.c b/device_tree.c
> index 52c3358a55..3553819257 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -140,15 +140,16 @@ static void read_fstree(void *fdt, const char *dirname)
> const char *parent_node;
>
> if (strstr(dirname, root_dir) != dirname) {
> - error_setg(&error_fatal, "%s: %s must be searched within %s",
> - __func__, dirname, root_dir);
> + error_report("%s: %s must be searched within %s",
> + __func__, dirname, root_dir);
> + exit(1);
> }
> parent_node = &dirname[strlen(SYSFS_DT_BASEDIR)];
>
> d = opendir(dirname);
> if (!d) {
> - error_setg(&error_fatal, "%s cannot open %s", __func__, dirname);
> - return;
> + error_report("%s cannot open %s", __func__, dirname);
> + exit(1);
> }
>
> while ((de = readdir(d)) != NULL) {
> @@ -162,7 +163,8 @@ static void read_fstree(void *fdt, const char *dirname)
> tmpnam = g_strdup_printf("%s/%s", dirname, de->d_name);
>
> if (lstat(tmpnam, &st) < 0) {
> - error_setg(&error_fatal, "%s cannot lstat %s", __func__, tmpnam);
> + error_report("%s cannot lstat %s", __func__, tmpnam);
> + exit(1);
> }
>
> if (S_ISREG(st.st_mode)) {
> @@ -170,8 +172,9 @@ static void read_fstree(void *fdt, const char *dirname)
> gsize len;
>
> if (!g_file_get_contents(tmpnam, &val, &len, NULL)) {
> - error_setg(&error_fatal, "%s not able to extract info from %s",
> - __func__, tmpnam);
> + error_report("%s not able to extract info from %s",
> + __func__, tmpnam);
> + exit(1);
> }
>
> if (strlen(parent_node) > 0) {
> @@ -206,9 +209,9 @@ void *load_device_tree_from_sysfs(void)
> host_fdt = create_device_tree(&host_fdt_size);
> read_fstree(host_fdt, SYSFS_DT_BASEDIR);
> if (fdt_check_header(host_fdt)) {
> - error_setg(&error_fatal,
> - "%s host device tree extracted into memory is invalid",
> - __func__);
> + error_report("%s host device tree extracted into memory is invalid",
> + __func__);
> + exit(1);
> }
> return host_fdt;
> }
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] hw/ppc/spapr_drc: Replace error_setg(&error_abort) by abort()
2018-06-08 3:03 ` David Gibson
@ 2018-06-08 3:54 ` Philippe Mathieu-Daudé
2018-06-08 4:23 ` David Gibson
0 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-08 3:54 UTC (permalink / raw)
To: David Gibson, Markus Armbruster, Eric Blake
Cc: Peter Maydell, qemu-devel, Alexander Graf, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]
Hi David,
On 06/08/2018 12:03 AM, David Gibson wrote:
> On Thu, Jun 07, 2018 at 11:46:43AM -0300, Philippe Mathieu-Daudé wrote:
>> Use abort() instead of error_setg(&error_abort),
>> as suggested by the "qapi/error.h" documentation:
>>
>> Please don't error_setg(&error_fatal, ...), use error_report() and
>> exit(), because that's more obvious.
>> Likewise, don't error_setg(&error_abort, ...), use assert().
>>
>> Use abort() instead of the suggested assert() because the assertion is
>> already verified by the switch case.
>
> I think g_assert_not_reached() would be the right thing here.
I try to follow Eric advice (who recalled Markus).
As I understand:
http://lists.gnu.org/archive/html/qemu-devel/2017-08/msg03605.html
"glib-Testing [...] should not be used outside of tests/."
I can respin if you prefer.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> hw/ppc/spapr_drc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>> index 8a045d6b93..b934b9c9ed 100644
>> --- a/hw/ppc/spapr_drc.c
>> +++ b/hw/ppc/spapr_drc.c
>> @@ -366,7 +366,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
>> break;
>> }
>> default:
>> - error_setg(&error_abort, "device FDT in unexpected state: %d", tag);
>> + abort(); /* device FDT in unexpected state */
>> }
>> fdt_offset = fdt_offset_next;
>> } while (fdt_depth != 0);
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] hw/ppc/spapr_drc: Replace error_setg(&error_abort) by abort()
2018-06-08 3:54 ` Philippe Mathieu-Daudé
@ 2018-06-08 4:23 ` David Gibson
2018-06-08 6:22 ` Markus Armbruster
0 siblings, 1 reply; 15+ messages in thread
From: David Gibson @ 2018-06-08 4:23 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Markus Armbruster, Eric Blake, Peter Maydell, qemu-devel,
Alexander Graf, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 2026 bytes --]
On Fri, Jun 08, 2018 at 12:54:36AM -0300, Philippe Mathieu-Daudé wrote:
> Hi David,
>
> On 06/08/2018 12:03 AM, David Gibson wrote:
> > On Thu, Jun 07, 2018 at 11:46:43AM -0300, Philippe Mathieu-Daudé wrote:
> >> Use abort() instead of error_setg(&error_abort),
> >> as suggested by the "qapi/error.h" documentation:
> >>
> >> Please don't error_setg(&error_fatal, ...), use error_report() and
> >> exit(), because that's more obvious.
> >> Likewise, don't error_setg(&error_abort, ...), use assert().
> >>
> >> Use abort() instead of the suggested assert() because the assertion is
> >> already verified by the switch case.
> >
> > I think g_assert_not_reached() would be the right thing here.
>
> I try to follow Eric advice (who recalled Markus).
> As I understand:
> http://lists.gnu.org/archive/html/qemu-devel/2017-08/msg03605.html
>
> "glib-Testing [...] should not be used outside of tests/."
Oh, ok, go with that then.
Acked-by: David Gibson <david@gibson.dropbear.id.au>
> I can respin if you prefer.
>
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
> >> hw/ppc/spapr_drc.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> >> index 8a045d6b93..b934b9c9ed 100644
> >> --- a/hw/ppc/spapr_drc.c
> >> +++ b/hw/ppc/spapr_drc.c
> >> @@ -366,7 +366,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
> >> break;
> >> }
> >> default:
> >> - error_setg(&error_abort, "device FDT in unexpected state: %d", tag);
> >> + abort(); /* device FDT in unexpected state */
> >> }
> >> fdt_offset = fdt_offset_next;
> >> } while (fdt_depth != 0);
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hw/block/fdc: Replace error_setg(&error_abort) by assert()
2018-06-07 17:18 ` John Snow
@ 2018-06-08 6:05 ` Markus Armbruster
2018-06-08 15:03 ` John Snow
0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2018-06-08 6:05 UTC (permalink / raw)
To: John Snow
Cc: Philippe Mathieu-Daudé,
Eric Blake, Kevin Wolf, Max Reitz, Peter Maydell, qemu-devel,
qemu-block
John Snow <jsnow@redhat.com> writes:
> On 06/07/2018 10:46 AM, Philippe Mathieu-Daudé wrote:
>> Use assert() instead of error_setg(&error_abort),
>> as suggested by the "qapi/error.h" documentation:
>>
>> Please don't error_setg(&error_fatal, ...), use error_report() and
>> exit(), because that's more obvious.
>> Likewise, don't error_setg(&error_abort, ...), use assert().
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> hw/block/fdc.c | 9 +--------
>> 1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index cd29e27d8f..7c1c57f57f 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -396,16 +396,9 @@ static int pick_geometry(FDrive *drv)
>> nb_sectors,
>> FloppyDriveType_str(parse->drive));
>> }
>> + assert(type_match != -1 && "misconfigured fd_format");
>> match = type_match;
>> }
>> -
>> - /* No match of any kind found -- fd_format is misconfigured, abort. */
>> - if (match == -1) {
>> - error_setg(&error_abort, "No candidate geometries present in table "
>> - " for floppy drive type '%s'",
>> - FloppyDriveType_str(drv->drive));
>> - }
>> -
>> parse = &(fd_formats[match]);
>>
>> out:
>>
>
> Truthfully sad to see the lipstick go, because this is my pig.
> Oh well, it doesn't matter. Not really, anyway.
>
> ACK
There's still a bit of lipstick in assert()'s argument.
v1 has the error_report() lipstick. Would you like me to merge that one
instead? Your pig, your choice :)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] hw/ppc/spapr_drc: Replace error_setg(&error_abort) by abort()
2018-06-08 4:23 ` David Gibson
@ 2018-06-08 6:22 ` Markus Armbruster
0 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2018-06-08 6:22 UTC (permalink / raw)
To: David Gibson
Cc: Philippe Mathieu-Daudé,
Peter Maydell, qemu-devel, Alexander Graf, qemu-ppc
David Gibson <david@gibson.dropbear.id.au> writes:
> On Fri, Jun 08, 2018 at 12:54:36AM -0300, Philippe Mathieu-Daudé wrote:
>> Hi David,
>>
>> On 06/08/2018 12:03 AM, David Gibson wrote:
>> > On Thu, Jun 07, 2018 at 11:46:43AM -0300, Philippe Mathieu-Daudé wrote:
>> >> Use abort() instead of error_setg(&error_abort),
>> >> as suggested by the "qapi/error.h" documentation:
>> >>
>> >> Please don't error_setg(&error_fatal, ...), use error_report() and
>> >> exit(), because that's more obvious.
>> >> Likewise, don't error_setg(&error_abort, ...), use assert().
>> >>
>> >> Use abort() instead of the suggested assert() because the assertion is
>> >> already verified by the switch case.
>> >
>> > I think g_assert_not_reached() would be the right thing here.
>>
>> I try to follow Eric advice (who recalled Markus).
>> As I understand:
>> http://lists.gnu.org/archive/html/qemu-devel/2017-08/msg03605.html
>>
>> "glib-Testing [...] should not be used outside of tests/."
>
> Oh, ok, go with that then.
Most g_assert_FOO() are indeed tests-only, but g_assert_not_reached() is
actually acceptable elsewhere, see commit 6e9389563e5, and its review
thread
https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg05499.html
Message-Id: <20170427165526.19836-1-dgilbert@redhat.com>
That said, I fail to see the value of this kind of lipstick, too.
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
>
>> I can respin if you prefer.
I can replace by g_assert_not_reached() on commit if you prefer.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit()
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 3/4] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit() Philippe Mathieu-Daudé
@ 2018-06-08 6:27 ` Markus Armbruster
2018-06-21 11:14 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2018-06-08 6:27 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Eric Blake, Peter Maydell, Peter Crosthwaite, Alexander Graf,
David Gibson, qemu-arm, qemu-devel
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> Use error_report() + exit() instead of error_setg(&error_fatal),
> as suggested by the "qapi/error.h" documentation:
>
> Please don't error_setg(&error_fatal, ...), use error_report() and
> exit(), because that's more obvious.
>
> This fixes CID 1352173:
> "Passing null pointer dt_name to qemu_fdt_node_path, which dereferences it."
>
> And this also fixes:
>
> hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable 'node_path') results in a null pointer dereference
> if (node_path[1]) {
> ^~~~~~~~~~~~
>
> Fixes: Coverity CID 1352173 (Dereference after null check)
You lost
Suggested-by: Eric Blake <eblake@redhat.com>
Intentional?
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/arm/sysbus-fdt.c | 42 +++++++++++++++++++++++-------------------
> 1 file changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index e4c492ea44..8e2784fa11 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -91,7 +91,7 @@ static void copy_properties_from_host(HostProperty *props, int nb_props,
> r = qemu_fdt_getprop(host_fdt, node_path,
> props[i].name,
> &prop_len,
> - props[i].optional ? &err : &error_fatal);
> + &err);
> if (r) {
> qemu_fdt_setprop(guest_fdt, nodename,
> props[i].name, r, prop_len);
> @@ -102,6 +102,7 @@ static void copy_properties_from_host(HostProperty *props, int nb_props,
> } else {
> error_free(err);
> }
> + assert(props[i].optional); /* mandatory property not found */
> }
> }
> }
This is not the conversion the commit message promised: it replaces
exit(1) by abort(). Why?
[Remainder of the patch is unchanged]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] hw/block/fdc: Replace error_setg(&error_abort) by assert()
2018-06-08 6:05 ` Markus Armbruster
@ 2018-06-08 15:03 ` John Snow
0 siblings, 0 replies; 15+ messages in thread
From: John Snow @ 2018-06-08 15:03 UTC (permalink / raw)
To: Markus Armbruster
Cc: Philippe Mathieu-Daudé,
Eric Blake, Kevin Wolf, Max Reitz, Peter Maydell, qemu-devel,
qemu-block
On 06/08/2018 02:05 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 06/07/2018 10:46 AM, Philippe Mathieu-Daudé wrote:
>>> Use assert() instead of error_setg(&error_abort),
>>> as suggested by the "qapi/error.h" documentation:
>>>
>>> Please don't error_setg(&error_fatal, ...), use error_report() and
>>> exit(), because that's more obvious.
>>> Likewise, don't error_setg(&error_abort, ...), use assert().
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>> hw/block/fdc.c | 9 +--------
>>> 1 file changed, 1 insertion(+), 8 deletions(-)
>>>
>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>> index cd29e27d8f..7c1c57f57f 100644
>>> --- a/hw/block/fdc.c
>>> +++ b/hw/block/fdc.c
>>> @@ -396,16 +396,9 @@ static int pick_geometry(FDrive *drv)
>>> nb_sectors,
>>> FloppyDriveType_str(parse->drive));
>>> }
>>> + assert(type_match != -1 && "misconfigured fd_format");
>>> match = type_match;
>>> }
>>> -
>>> - /* No match of any kind found -- fd_format is misconfigured, abort. */
>>> - if (match == -1) {
>>> - error_setg(&error_abort, "No candidate geometries present in table "
>>> - " for floppy drive type '%s'",
>>> - FloppyDriveType_str(drv->drive));
>>> - }
>>> -
>>> parse = &(fd_formats[match]);
>>>
>>> out:
>>>
>>
>> Truthfully sad to see the lipstick go, because this is my pig.
>> Oh well, it doesn't matter. Not really, anyway.
>>
>> ACK
>
> There's still a bit of lipstick in assert()'s argument.
>
> v1 has the error_report() lipstick. Would you like me to merge that one
> instead? Your pig, your choice :)
>
In the end I trust your judgement. I like the more verbose error message
because it tells you precisely what's going on. I like the assert
because it tells you WHERE in the code you've messed up.
I am often very keen on making things more usable and less tersely
worded. I think you favor some of the same things, but with years more
experience than I have.
So I defer to you; after all -- they're just pigs :)
--js
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/4] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit()
2018-06-08 6:27 ` Markus Armbruster
@ 2018-06-21 11:14 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-21 11:14 UTC (permalink / raw)
To: Markus Armbruster
Cc: Eric Blake, Peter Maydell, Peter Crosthwaite, Alexander Graf,
David Gibson, qemu-arm, qemu-devel
Hi Markus,
On 06/08/2018 03:27 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>
>> Use error_report() + exit() instead of error_setg(&error_fatal),
>> as suggested by the "qapi/error.h" documentation:
>>
>> Please don't error_setg(&error_fatal, ...), use error_report() and
>> exit(), because that's more obvious.
>>
>> This fixes CID 1352173:
>> "Passing null pointer dt_name to qemu_fdt_node_path, which dereferences it."
>>
>> And this also fixes:
>>
>> hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable 'node_path') results in a null pointer dereference
>> if (node_path[1]) {
>> ^~~~~~~~~~~~
>>
>> Fixes: Coverity CID 1352173 (Dereference after null check)
>
> You lost
>
> Suggested-by: Eric Blake <eblake@redhat.com>
>
> Intentional?
Not at all :/
>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> hw/arm/sysbus-fdt.c | 42 +++++++++++++++++++++++-------------------
>> 1 file changed, 23 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>> index e4c492ea44..8e2784fa11 100644
>> --- a/hw/arm/sysbus-fdt.c
>> +++ b/hw/arm/sysbus-fdt.c
>> @@ -91,7 +91,7 @@ static void copy_properties_from_host(HostProperty *props, int nb_props,
>> r = qemu_fdt_getprop(host_fdt, node_path,
>> props[i].name,
>> &prop_len,
>> - props[i].optional ? &err : &error_fatal);
>> + &err);
>> if (r) {
>> qemu_fdt_setprop(guest_fdt, nodename,
>> props[i].name, r, prop_len);
>> @@ -102,6 +102,7 @@ static void copy_properties_from_host(HostProperty *props, int nb_props,
>> } else {
>> error_free(err);
>> }
>> + assert(props[i].optional); /* mandatory property not found */
>> }
>> }
>> }
>
> This is not the conversion the commit message promised: it replaces
> exit(1) by abort(). Why?
I can't understand it either, I suppose I cherry-picked from an
incorrect branch, because I also added your R-b locally.
I'll resend.
Thanks,
Phil.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-06-21 11:14 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-07 14:46 [Qemu-devel] [PATCH v2 0/4] qapi/error: converts error_setg(&error_fatal) to error_report() + exit() Philippe Mathieu-Daudé
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 1/4] hw/block/fdc: Replace error_setg(&error_abort) by assert() Philippe Mathieu-Daudé
2018-06-07 17:18 ` John Snow
2018-06-08 6:05 ` Markus Armbruster
2018-06-08 15:03 ` John Snow
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 2/4] hw/ppc/spapr_drc: Replace error_setg(&error_abort) by abort() Philippe Mathieu-Daudé
2018-06-08 3:03 ` David Gibson
2018-06-08 3:54 ` Philippe Mathieu-Daudé
2018-06-08 4:23 ` David Gibson
2018-06-08 6:22 ` Markus Armbruster
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 3/4] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit() Philippe Mathieu-Daudé
2018-06-08 6:27 ` Markus Armbruster
2018-06-21 11:14 ` Philippe Mathieu-Daudé
2018-06-07 14:46 ` [Qemu-devel] [PATCH v2 4/4] device_tree: " Philippe Mathieu-Daudé
2018-06-08 3:04 ` David Gibson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.