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 <aleksandar.qemu.devel@gmail.com>
Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 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));
-        exit(1);
-    }
+    object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type",
+                            &error_fatal);
+    object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp",
+                            &error_fatal);
+    object_property_set_bool(OBJECT(&s->cps), true, "realized",
+                             &error_fatal);
 
     sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
 
-- 
2.21.1

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 <aleksandar.qemu.devel@gmail.com>
> Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  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/qemu-devel@nongnu.org/msg695645.html I
also remove "qemu/error-report.h" which is not needed once you remove
this call.

> -        exit(1);
> -    }
> +    object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type",
> +                            &error_fatal);
> +    object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp",
> +                            &error_fatal);
> +    object_property_set_bool(OBJECT(&s->cps), true, "realized",
> +                             &error_fatal);
>  
>      sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
>  
> 

Philippe Mathieu-Daudé <f4bug@amsat.org> 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 <aleksandar.qemu.devel@gmail.com>
>> Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  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/qemu-devel@nongnu.org/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].

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.

>> -        exit(1);
>> -    }
>> +    object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type",
>> +                            &error_fatal);
>> +    object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp",
>> +                            &error_fatal);
>> +    object_property_set_bool(OBJECT(&s->cps), true, "realized",
>> +                             &error_fatal);
>>  
>>      sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
>>  
>> 

+Peter for crediting his advice.

On 4/29/20 7:59 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> 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 <aleksandar.qemu.devel@gmail.com>
>>> Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>   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/qemu-devel@nongnu.org/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.

Anyway back to your patch:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
>>> -        exit(1);
>>> -    }
>>> +    object_property_set_str(OBJECT(&s->cps), ms->cpu_type, "cpu-type",
>>> +                            &error_fatal);
>>> +    object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp",
>>> +                            &error_fatal);
>>> +    object_property_set_bool(OBJECT(&s->cps), true, "realized",
>>> +                             &error_fatal);
>>>   
>>>       sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
>>>   
>>>
> 

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

> +Peter for crediting his advice.
>
> On 4/29/20 7:59 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <f4bug@amsat.org> 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 <aleksandar.qemu.devel@gmail.com>
>>>> Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>>>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>   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/qemu-devel@nongnu.org/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é <f4bug@amsat.org>
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks!