Re: [PATCH 07/11] mips/malta: Fix create_cps() error handling

[Markus Armbruster]

Philippe Mathieu-Daudé <address@hidden> writes:

> +Peter for crediting his advice.
>
> On 4/29/20 7:59 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <address@hidden> writes:
>>
>>> On 4/24/20 9:20 PM, Markus Armbruster wrote:
>>>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>>>> pointer to a variable containing NULL.  Passing an argument of the
>>>> latter kind twice without clearing it in between is wrong: if the
>>>> first call sets an error, it no longer points to NULL for the second
>>>>
>>>> create_cps() is wrong that way.  The last calls treats an error as
>>>> fatal.  Do that for the prior ones, too.
>>>>
>>>> Fixes: bff384a4fbd5d0e86939092e74e766ef0f5f592c
>>>> Cc: Aleksandar Markovic <address@hidden>
>>>> Cc: "Philippe Mathieu-Daudé" <address@hidden>
>>>> Cc: Aurelien Jarno <address@hidden>
>>>> Signed-off-by: Markus Armbruster <address@hidden>
>>>> ---
>>>>   hw/mips/mips_malta.c | 15 ++++++---------
>>>>   1 file changed, 6 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
>>>> index e4c4de1b4e..17bf41616b 100644
>>>> --- a/hw/mips/mips_malta.c
>>>> +++ b/hw/mips/mips_malta.c
>>>> @@ -1185,17 +1185,14 @@ static void create_cpu_without_cps(MachineState 
>>>> *ms,
>>>>   static void create_cps(MachineState *ms, MaltaState *s,
>>>>                          qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>>>>   {
>>>> -    Error *err = NULL;
>>>> -
>>>>       sysbus_init_child_obj(OBJECT(s), "cps", OBJECT(&s->cps), 
>>>> sizeof(s->cps),
>>>>                             TYPE_MIPS_CPS);
>>>> -    object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type", 
>>>> &err);
>>>> -    object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp", 
>>>> &err);
>>>> -    object_property_set_bool(OBJECT(&s->cps), true, "realized", &err);
>>>> -    if (err != NULL) {
>>>> -        error_report("%s", error_get_pretty(err));
>>>
>>> In https://www.mail-archive.com/address@hidden/msg695645.html I
>>> also remove "qemu/error-report.h" which is not needed once you remove
>>> this call.
>>
>> Missed it, sorry.  I only reviewed the Coccinelle scripts [PATCH 1+3/7].
>
> My bad for not Cc'ing you on the whole series, which is Error related,
> and use the default get_maintainer.pl selection.
>
>> I'd replace my patch by yours to give you proper credit, but your commit
>> message mentions "the coccinelle script", presumably the one from PATCH
>> 1/7, and that patch isn't quite ready in my opinion.
>
> I'm not worried about credit, but duplicating effort or wasting time.
>
> Peter once warned the problem with Coccinelle scripts is finding the
> correct balance between time spent to improve QEMU codebase, and time
> spent learning Coccinelle and improving a script that is often used
> only once in a lifetime.
> If the script is not provided, we ask for the script. If the script is
> embedded in various patch descriptions, we ask to add it independently
> for reuse or as example. Then the script must be almost
> perfect. Meanwhile all the following patches referencing it, while
> reviewed, are stuck.

True.  I try not to ask for undue perfection, but perfection eludes me
in that, too :)

For PATCH 1/7, I only asked you to explain the script's limitations in
the script and the commit message.  Her's a bit of inspiration from the
kernel's scripts/coccinelle/misc/doubleinit.cocci (picked almost at
random):

    /// Find duplicate field initializations.  This has a high rate of false
    /// positives due to #ifdefs, which Coccinelle is not aware of in a 
structure
    /// initialization.
    ///
    // Confidence: Low

I like the Confidence: tag.  It should come with an explanation, as it
does here.

For PATCH 3/7, I had a question.

> Anyway back to your patch:
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> Tested-by: Philippe Mathieu-Daudé <address@hidden>

Thanks!