Re: [PATCH-for-5.1 v2 53/54] hw/mips/mips_malta: Add missing error-propagation code

[Philippe Mathieu-Daudé]

On 4/7/20 8:36 PM, Richard Henderson wrote:
> On 4/6/20 10:47 AM, Philippe Mathieu-Daudé wrote:
>> Running the coccinelle script produced:
>>
>>   $ spatch \
>>     --macro-file scripts/cocci-macro-file.h --include-headers \
>>     --sp-file scripts/coccinelle/find-missing-error_propagate.cocci \
>>     --keep-comments --smpl-spacing --dir .
>>   HANDLING: ./hw/mips/mips_malta.c
>>   [[manual check required: error_propagate() might be missing in 
>> object_property_set_int() ./hw/mips/mips_malta.c:1193:4]]
>>   [[manual check required: error_propagate() might be missing in 
>> object_property_set_str() ./hw/mips/mips_malta.c:1192:4]]
>>
>> Add the missing error_propagate() after review by adding
>> a Error* parameter to create_cps().
>>
>> Reviewed-by: Peter Maydell <address@hidden>
>> Reviewed-by: Aleksandar Markovic <address@hidden>
>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>> ---
>>  hw/mips/mips_malta.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
>> index e4c4de1b4e..8d43cfd41b 100644
>> --- a/hw/mips/mips_malta.c
>> +++ b/hw/mips/mips_malta.c
>> @@ -1183,18 +1183,27 @@ static void create_cpu_without_cps(MachineState *ms,
>>  }
>>  
>>  static void create_cps(MachineState *ms, MaltaState *s,
>> -                       qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>> +                       qemu_irq *cbus_irq, qemu_irq *i8259_irq,
>> +                       Error **errp)
>>  {
>>      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);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>>      object_property_set_int(OBJECT(&s->cps), ms->smp.cpus, "num-vp", &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>>      object_property_set_bool(OBJECT(&s->cps), true, "realized", &err);
>> -    if (err != NULL) {
>> -        error_report("%s", error_get_pretty(err));
>> -        exit(1);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>>      }
>>  
>>      sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->cps), 0, 0, 1);
>> @@ -1207,7 +1216,7 @@ static void mips_create_cpu(MachineState *ms, 
>> MaltaState *s,
>>                              qemu_irq *cbus_irq, qemu_irq *i8259_irq)
>>  {
>>      if ((ms->smp.cpus > 1) && cpu_supports_cps_smp(ms->cpu_type)) {
>> -        create_cps(ms, s, cbus_irq, i8259_irq);
>> +        create_cps(ms, s, cbus_irq, i8259_irq, &error_fatal);
> 
> Are you going to replace this with something else later?

No, indeed.

> Otherwise it doesn't seem useful to add the argument here, as opposed to using
> error_fatal in create_cpus()

Good suggestion, thanks!