[Qemu-devel] [PATCH 0/4] qapi/error: converts error_setg(&error_fatal) to error_report() + exit()

[Qemu-devel] [PATCH 0/4] qapi/error: converts error_setg(&error_fatal) to error_report() + exit()

[Philippe Mathieu-Daudé]

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.

Regards,

Phil.

Philippe Mathieu-Daudé (4):
  hw/block/fdc: Replace error_setg(&error_abort) by error_report() + abort()
  hw/ppc/spapr_drc: Replace error_setg(&error_abort) by error_report() +
    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 | 45 ++++++++++++++++++++++++++-------------------
 hw/block/fdc.c      |  7 ++++---
 hw/ppc/spapr_drc.c  |  3 ++-
 4 files changed, 45 insertions(+), 33 deletions(-)

-- 
2.17.0

[Qemu-arm] [PATCH 3/4] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit()

[Philippe Mathieu-Daudé]

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)
Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/sysbus-fdt.c | 45 ++++++++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index e4c492ea44..3af87fc184 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,10 @@ static void copy_properties_from_host(HostProperty *props, int nb_props,
             } else {
                 error_free(err);
             }
+            if (!props[i].optional) {
+                /* mandatory property not found: bail out */
+                exit(1);
+            }
         }
     }
 }
@@ -137,9 +141,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 +152,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 +304,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.0

Re: [Qemu-arm] [Qemu-devel] [PATCH 3/4] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit()

[Markus Armbruster]

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)
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Same code smell as in PATCH 4/4.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 0/4] qapi/error: converts error_setg(&error_fatal) to error_report() + exit()

[Auger Eric]

Hi,

On 05/29/2018 07:48 PM, Philippe Mathieu-Daudé wrote:
> 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.

Thanks for the fix.

For the series:
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> 
> Regards,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (4):
>   hw/block/fdc: Replace error_setg(&error_abort) by error_report() + abort()
>   hw/ppc/spapr_drc: Replace error_setg(&error_abort) by error_report() +
>     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 | 45 ++++++++++++++++++++++++++-------------------
>  hw/block/fdc.c      |  7 ++++---
>  hw/ppc/spapr_drc.c  |  3 ++-
>  4 files changed, 45 insertions(+), 33 deletions(-)
> 

Re: [Qemu-arm] [Qemu-devel] [PATCH 0/4] qapi/error: converts error_setg(&error_fatal) to error_report() + exit()

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Hi,
>
> This series converts error_setg(&error_fatal) to error_report() + exit() as
> suggested by the "qapi/error.h" documentation.

Appreciated!

The series actually converts two anti-patterns.

1. From

    if (COND) {
        error_setg(&error_fatal, ...);
    }

to

    if (COND) {
        error_report(...);
        exit(1);
    }

This is exactly what error.h asks for.

2. From

    if (COND) {
        error_setg(&error_abort, ...);
    }
to

    if (COND) {
        error_report(...);
        abort();
    }

error.h asks for

    assert(!COND);

instead.  See my reply to PATCH 1 for why.

Re: [Qemu-arm] [Qemu-devel] [PATCH 0/4] qapi/error: converts error_setg(&error_fatal) to error_report() + exit()

[Philippe Mathieu-Daudé]

On 06/07/2018 10:48 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> Hi,
>>
>> This series converts error_setg(&error_fatal) to error_report() + exit() as
>> suggested by the "qapi/error.h" documentation.
> 
> Appreciated!
> 
> The series actually converts two anti-patterns.
> 
> 1. From
> 
>     if (COND) {
>         error_setg(&error_fatal, ...);
>     }
> 
> to
> 
>     if (COND) {
>         error_report(...);
>         exit(1);
>     }
> 
> This is exactly what error.h asks for.
> 
> 2. From
> 
>     if (COND) {
>         error_setg(&error_abort, ...);
>     }
> to
> 
>     if (COND) {
>         error_report(...);
>         abort();
>     }
> 
> error.h asks for
> 
>     assert(!COND);
> 
> instead.  See my reply to PATCH 1 for why.

My bad, I misread your "error.h" explanation.

These patterns are now cleaned from the codebase.

Your examples are clearer and might be added in the header comments if
we keep using the bad patterns.

Regards,

Phil.