Re: [Qemu-devel] [PATCH for 2.10 21/35] arm/sysbus-fdt: fix null pointer dereference

[Peter Maydell]

On 24 July 2017 at 22:20, Philippe Mathieu-Daudé <address@hidden> wrote:
> On 07/24/2017 06:09 PM, Peter Maydell wrote:
>>
>> On 24 July 2017 at 19:27, Philippe Mathieu-Daudé <address@hidden> 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