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

[Philippe Mathieu-Daudé]

+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.

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

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