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

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

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

[Qemu-devel] [PATCH v2 1/4] hw/block/fdc: Replace error_setg(&error_abort) by assert()

[Philippe Mathieu-Daudé]

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

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

[John Snow]

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

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

[Markus Armbruster]

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 :)

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

[John Snow]

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

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

[Philippe Mathieu-Daudé]

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

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

[David Gibson]

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

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

[Philippe Mathieu-Daudé]

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

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

[David Gibson]

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

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

[Markus Armbruster]

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.

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

Re: [Qemu-devel] [PATCH v2 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)

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]

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

[Philippe Mathieu-Daudé]

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.

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

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

Re: [Qemu-devel] [PATCH v2 4/4] device_tree: Replace error_setg(&error_fatal) by error_report() + exit()

[David Gibson]

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