[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-devel] [PATCH 1/4] hw/block/fdc: Replace error_setg(&error_abort) by error_report() + abort()

[Philippe Mathieu-Daudé]

Use error_report() + 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 error message
already got displayed.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/block/fdc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index cd29e27d8f..048467c00b 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -401,9 +401,10 @@ static int pick_geometry(FDrive *drv)
 
     /* 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));
+        error_report("No candidate geometries present in table "
+                     " for floppy drive type '%s'",
+                     FloppyDriveType_str(drv->drive));
+        abort();
     }
 
     parse = &(fd_formats[match]);
-- 
2.17.0

Re: [Qemu-devel] [PATCH 1/4] hw/block/fdc: Replace error_setg(&error_abort) by error_report() + abort()

[John Snow]

On 05/29/2018 01:48 PM, Philippe Mathieu-Daudé wrote:
> Use error_report() + 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().
> 

Didn't realize this was bad form (Why do we have it?)

> Use abort() instead of the suggested assert() because the error message
> already got displayed.
> 

Update the suggestion?

> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: John Snow <jsnow@redhat.com>
Acked-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 1/4] hw/block/fdc: Replace error_setg(&error_abort) by error_report() + abort()

[Philippe Mathieu-Daudé]

On 05/30/2018 05:23 PM, John Snow wrote:
> On 05/29/2018 01:48 PM, Philippe Mathieu-Daudé wrote:
>> Use error_report() + 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().
>>
> 
> Didn't realize this was bad form (Why do we have it?)
> 
>> Use abort() instead of the suggested assert() because the error message
>> already got displayed.
>>
> 
> Update the suggestion?

Fair enough.

>> Suggested-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> Acked-by: John Snow <jsnow@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 1/4] hw/block/fdc: Replace error_setg(&error_abort) by error_report() + abort()

[Markus Armbruster]

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

> On 05/30/2018 05:23 PM, John Snow wrote:
>> On 05/29/2018 01:48 PM, Philippe Mathieu-Daudé wrote:
>>> Use error_report() + 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().
>>>
>> 
>> Didn't realize this was bad form (Why do we have it?)

Because &error_abort has other uses, and I can't see how we can get rid
of just error_setg(&error_abort, ...).  Open to ideas.

>>> Use abort() instead of the suggested assert() because the error message
>>> already got displayed.
>>>
>> 
>> Update the suggestion?
>
> Fair enough.

No.  I intentionally pointed to assert(), because ...

>>> Suggested-by: Eric Blake <eblake@redhat.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> 
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> Acked-by: John Snow <jsnow@redhat.com>
>
> Thanks!
>
>>> ---
>>>  hw/block/fdc.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>> index cd29e27d8f..048467c00b 100644
>>> --- a/hw/block/fdc.c
>>> +++ b/hw/block/fdc.c
>>> @@ -401,9 +401,10 @@ static int pick_geometry(FDrive *drv)
>>>  
>>>      /* 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));
>>> +        error_report("No candidate geometries present in table "
>>> +                     " for floppy drive type '%s'",
>>> +                     FloppyDriveType_str(drv->drive));
>>> +        abort();

... no matter how "nice" you make the message before an abort, it's
still an abort.  It should not happen (or else aborting would be wrong),
and when it does, somebody will have to stare at the code anyway.  I
recommend not to bother with niceties there.

Pig:

    hw/block/fdc.c:403:pick_geometry: assertion failed: (match != -1)
    Aborted (core dumped)

Pig with lipstick on:

    No candidate geometries present in table for floppy drive type 'FOO'
    Aborted (core dumped)

Now, if you're really, really into lipstick: to each his own.  But
please return the courtesy and keep it out of the stuff I maintain :)

>>>      }
>>>  
>>>      parse = &(fd_formats[match]);
>>> -- 
>>> 2.17.0

Re: [Qemu-devel] [PATCH 1/4] hw/block/fdc: Replace error_setg(&error_abort) by error_report() + abort()

[Philippe Mathieu-Daudé]

On 06/07/2018 10:41 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>> On 05/30/2018 05:23 PM, John Snow wrote:
>>> On 05/29/2018 01:48 PM, Philippe Mathieu-Daudé wrote:
>>>> Use error_report() + 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().
>>>>
>>>
>>> Didn't realize this was bad form (Why do we have it?)
> 
> Because &error_abort has other uses, and I can't see how we can get rid
> of just error_setg(&error_abort, ...).  Open to ideas.
> 
>>>> Use abort() instead of the suggested assert() because the error message
>>>> already got displayed.
>>>>
>>>
>>> Update the suggestion?
>>
>> Fair enough.
> 
> No.  I intentionally pointed to assert(), because ...
> 
>>>> Suggested-by: Eric Blake <eblake@redhat.com>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>
>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>> Acked-by: John Snow <jsnow@redhat.com>
>>
>> Thanks!
>>
>>>> ---
>>>>  hw/block/fdc.c | 7 ++++---
>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>>> index cd29e27d8f..048467c00b 100644
>>>> --- a/hw/block/fdc.c
>>>> +++ b/hw/block/fdc.c
>>>> @@ -401,9 +401,10 @@ static int pick_geometry(FDrive *drv)
>>>>  
>>>>      /* 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));
>>>> +        error_report("No candidate geometries present in table "
>>>> +                     " for floppy drive type '%s'",
>>>> +                     FloppyDriveType_str(drv->drive));
>>>> +        abort();
> 
> ... no matter how "nice" you make the message before an abort, it's
> still an abort.  It should not happen (or else aborting would be wrong),
> and when it does, somebody will have to stare at the code anyway.  I
> recommend not to bother with niceties there.

OK I see, thanks for the clear explanation.

> 
> Pig:
> 
>     hw/block/fdc.c:403:pick_geometry: assertion failed: (match != -1)
>     Aborted (core dumped)
> 
> Pig with lipstick on:
> 
>     No candidate geometries present in table for floppy drive type 'FOO'
>     Aborted (core dumped)
> 
> Now, if you're really, really into lipstick: to each his own.  But
> please return the courtesy and keep it out of the stuff I maintain :)
> 
>>>>      }
>>>>  
>>>>      parse = &(fd_formats[match]);
>>>> -- 
>>>> 2.17.0

[Qemu-devel] [PATCH 2/4] hw/ppc/spapr_drc: Replace error_setg(&error_abort) by error_report() + abort()

[Philippe Mathieu-Daudé]

Use error_report() + 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 error message
already got displayed.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ppc/spapr_drc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 8a045d6b93..2edb7d1e9c 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -366,7 +366,8 @@ 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);
+            error_report("device FDT in unexpected state: %d", tag);
+            abort();
         }
         fdt_offset = fdt_offset_next;
     } while (fdt_depth != 0);
-- 
2.17.0

Re: [Qemu-devel] [PATCH 2/4] hw/ppc/spapr_drc: Replace error_setg(&error_abort) by error_report() + abort()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1501 bytes --]

On Tue, May 29, 2018 at 02:48:19PM -0300, Philippe Mathieu-Daudé wrote:
> Use error_report() + 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 error message
> already got displayed.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Applied to ppc-for-2.13, thanks.

> ---
>  hw/ppc/spapr_drc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 8a045d6b93..2edb7d1e9c 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -366,7 +366,8 @@ 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);
> +            error_report("device FDT in unexpected state: %d", tag);
> +            abort();
>          }
>          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 --]

[Qemu-devel] [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-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>

[Qemu-devel] [PATCH 4/4] device_tree: 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.

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

Re: [Qemu-devel] [PATCH 4/4] device_tree: 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.
>
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  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);

Preexisting code smell: __func__ in an error message.  Not this patch's
problem.

>      }
>      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;
>  }

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-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-devel] [Qemu-arm] [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.