Use error_report() + exit() instead of error_setg(&error_fatal).

hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable 'node_path') results in a null pointer dereference
    if (node_path[1]) {
        ^~~~~~~~~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/sysbus-fdt.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index d68e3dcdbd..ad0cc49b19 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -315,15 +315,14 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
     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",
+        error_report("%s unable to retrieve node path for %s/%s",
                    __func__, dt_name, vdev->compat);
-    }
-
-    if (node_path[1]) {
-        error_setg(&error_fatal, "%s more than one node matching %s/%s!",
+        exit(1);
+    } else if (node_path[1]) {
+        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) {
-- 
2.13.3

On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Use error_report() + exit() instead of error_setg(&error_fatal).
>
> hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable 'node_path') results in a null pointer dereference
>     if (node_path[1]) {
>         ^~~~~~~~~~~~

I don't understand what this warning is trying to say.
We can't get to this point with a NULL node_path,
because of the previous conditional, which is using
error_setg(&error_fatal).

> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/sysbus-fdt.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index d68e3dcdbd..ad0cc49b19 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -315,15 +315,14 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
>      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",
> +        error_report("%s unable to retrieve node path for %s/%s",
>                     __func__, dt_name, vdev->compat);
> -    }
> -
> -    if (node_path[1]) {
> -        error_setg(&error_fatal, "%s more than one node matching %s/%s!",
> +        exit(1);
> +    } else if (node_path[1]) {
> +        error_report("%s more than one node matching %s/%s!",
>                     __func__, dt_name, vdev->compat);
> +        exit(1);
>      }
> -
>      g_free(dt_name);

This doesn't seem like an improvement. Now the
error handling in the function is an inconsistent
mix of error_report()+exit() and error_setg(&error_fatal).

thanks
-- PMM

On 07/24/2017 06:09 PM, Peter Maydell wrote:
> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
...
>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>> index d68e3dcdbd..ad0cc49b19 100644
>> --- a/hw/arm/sysbus-fdt.c
>> +++ b/hw/arm/sysbus-fdt.c
>> @@ -315,15 +315,14 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
>>       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",
>> +        error_report("%s unable to retrieve node path for %s/%s",
>>                      __func__, dt_name, vdev->compat);
>> -    }
>> -
>> -    if (node_path[1]) {
>> -        error_setg(&error_fatal, "%s more than one node matching %s/%s!",
>> +        exit(1);
>> +    } else if (node_path[1]) {
>> +        error_report("%s more than one node matching %s/%s!",
>>                      __func__, dt_name, vdev->compat);
>> +        exit(1);
>>       }
>> -
>>       g_free(dt_name);
> 
> This doesn't seem like an improvement. Now the
> error handling in the function is an inconsistent
> mix of error_report()+exit() and error_setg(&error_fatal).

I got this from "qapi/error.h":

156 * Please don't error_setg(&error_fatal, ...), use error_report() and 

157 * exit(), because that's more obvious. 

158 * Likewise, don't error_setg(&error_abort, ...), use assert(). 


Is this comment outdated?

On 24 July 2017 at 22:20, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 07/24/2017 06:09 PM, Peter Maydell wrote:
>>
>> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> ...
>
>>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>>> index d68e3dcdbd..ad0cc49b19 100644
>>> --- a/hw/arm/sysbus-fdt.c
>>> +++ b/hw/arm/sysbus-fdt.c
>>> @@ -315,15 +315,14 @@ static int add_amd_xgbe_fdt_node(SysBusDevice
>>> *sbdev, void *opaque)
>>>       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",
>>> +        error_report("%s unable to retrieve node path for %s/%s",
>>>                      __func__, dt_name, vdev->compat);
>>> -    }
>>> -
>>> -    if (node_path[1]) {
>>> -        error_setg(&error_fatal, "%s more than one node matching
>>> %s/%s!",
>>> +        exit(1);
>>> +    } else if (node_path[1]) {
>>> +        error_report("%s more than one node matching %s/%s!",
>>>                      __func__, dt_name, vdev->compat);
>>> +        exit(1);
>>>       }
>>> -
>>>       g_free(dt_name);
>>
>>
>> This doesn't seem like an improvement. Now the
>> error handling in the function is an inconsistent
>> mix of error_report()+exit() and error_setg(&error_fatal).
>
>
> I got this from "qapi/error.h":
>
> 156 * Please don't error_setg(&error_fatal, ...), use error_report() and
> 157 * exit(), because that's more obvious.
> 158 * Likewise, don't error_setg(&error_abort, ...), use assert().
>
> Is this comment outdated?

Probably not, I expect this code predates it. However
my point about inconsistency still stands.

thanks
-- PMM

On 07/24/2017 06:09 PM, Peter Maydell wrote:
> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Use error_report() + exit() instead of error_setg(&error_fatal).
>>
>> hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable 'node_path') results in a null pointer dereference
>>      if (node_path[1]) {
>>          ^~~~~~~~~~~~
> 
> I don't understand what this warning is trying to say.
> We can't get to this point with a NULL node_path,
> because of the previous conditional, which is using
> error_setg(&error_fatal).

Ok I see, Clang is unaware than error_setg(&error_fatal) is a noreturn.

Patch dropped.

Thanks,

Phil.

On 07/24/2017 04:48 PM, Philippe Mathieu-Daudé wrote:
> On 07/24/2017 06:09 PM, Peter Maydell wrote:
>> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> Use error_report() + exit() instead of error_setg(&error_fatal).
>>>
>>> hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable
>>> 'node_path') results in a null pointer dereference
>>>      if (node_path[1]) {
>>>          ^~~~~~~~~~~~
>>
>> I don't understand what this warning is trying to say.
>> We can't get to this point with a NULL node_path,
>> because of the previous conditional, which is using
>> error_setg(&error_fatal).
> 
> Ok I see, Clang is unaware than error_setg(&error_fatal) is a noreturn.

Indeed, and that's because error_setg(&error_fatal) is not in preferred
form.

> 
> Patch dropped.

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 07/24/2017 04:52 PM, Eric Blake wrote:
> On 07/24/2017 04:48 PM, Philippe Mathieu-Daudé wrote:
>> On 07/24/2017 06:09 PM, Peter Maydell wrote:
>>> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> Use error_report() + exit() instead of error_setg(&error_fatal).
>>>>
>>>> hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable
>>>> 'node_path') results in a null pointer dereference
>>>>      if (node_path[1]) {
>>>>          ^~~~~~~~~~~~
>>>
>>> I don't understand what this warning is trying to say.
>>> We can't get to this point with a NULL node_path,
>>> because of the previous conditional, which is using
>>> error_setg(&error_fatal).
>>
>> Ok I see, Clang is unaware than error_setg(&error_fatal) is a noreturn.
> 
> Indeed, and that's because error_setg(&error_fatal) is not in preferred
> form.
> 
>>
>> Patch dropped.
> 
> 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.

In other words, switching to the preferred spelling in the following files:
device_tree.c
hw/arm/sysbus-fdt.c
hw/block/fdc.c
hw/ppc/spapr_drc.c

is desirable, and has the added benefit of also silencing a Coverity
false positive.  But it should be done in terms of switching to the
preferred spelling, as it touches more instances than just the one that
shuts up Coverity.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Hi Eric,

On 07/24/2017 06:57 PM, Eric Blake wrote:
> On 07/24/2017 04:52 PM, Eric Blake wrote:
>> On 07/24/2017 04:48 PM, Philippe Mathieu-Daudé wrote:
>>> On 07/24/2017 06:09 PM, Peter Maydell wrote:
>>>> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>> Use error_report() + exit() instead of error_setg(&error_fatal).
>>>>>
>>>>> hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable
>>>>> 'node_path') results in a null pointer dereference
>>>>>      if (node_path[1]) {
>>>>>          ^~~~~~~~~~~~
>>>>
>>>> I don't understand what this warning is trying to say.
>>>> We can't get to this point with a NULL node_path,
>>>> because of the previous conditional, which is using
>>>> error_setg(&error_fatal).
>>>
>>> Ok I see, Clang is unaware than error_setg(&error_fatal) is a noreturn.
>>
>> Indeed, and that's because error_setg(&error_fatal) is not in preferred
>> form.
>>
>>>
>>> Patch dropped.
>>
>> 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.

I started to port/clean this up.
To avoid future inconsistencies, do you think we should/can enforce this
check in checkpatch (which is stricter than human review)?
Is the "Qemu error function tests" section a good place to put this check?

> 
> In other words, switching to the preferred spelling in the following files:
> device_tree.c
> hw/arm/sysbus-fdt.c
> hw/block/fdc.c
> hw/ppc/spapr_drc.c
> 
> is desirable, and has the added benefit of also silencing a Coverity
> false positive.  But it should be done in terms of switching to the
> preferred spelling, as it touches more instances than just the one that
> shuts up Coverity.
> 

[adding Markus, as the error maintainer]

On 05/29/2018 09:33 AM, Philippe Mathieu-Daudé wrote:

>>>> Ok I see, Clang is unaware than error_setg(&error_fatal) is a noreturn.
>>>
>>> Indeed, and that's because error_setg(&error_fatal) is not in preferred
>>> form.
>>>
>>>>
>>>> Patch dropped.
>>>
>>> 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.
> 
> I started to port/clean this up.
> To avoid future inconsistencies, do you think we should/can enforce this
> check in checkpatch (which is stricter than human review)?

Sure, automating good code style is worthwhile.

> Is the "Qemu error function tests" section a good place to put this check?

Probably works.  I'm not familiar enough with checkpatch as a whole to 
have any different suggestions off the top of my head (basically, any 
time I've touched that file, it's been grepping for something that is 
similar to what I want to be tweaking, rather than reading the whole thing).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org