Re: [PATCH v4 4/4] target/loongarch: Set dest error with error_abort in virt_cpu_irq_init

[Markus Armbruster <armbru@redhat.com>]

Bibo Mao <maobibo@loongson.cn> writes:

> In function virt_cpu_irq_init(), there is notification with ipi and extioi
> interrupt controller for cpu creation. Local variable with error type is
> used, however there is no check with its return value.

Good catch.

When the first call fails, we pass non-null @err to the second call,
which is wrong.  If that one also fails, it'll likely trip
error_setv()'s assertion.

> Here set dest error object with error_abort, rather than local variable, so
> application will abort to run if there is error.

Why is failure impossible there?

If failure is impossible, the code before the patch is harmlessly wrong.

If failure is possible, the code before the patch has a crash bug, and
the patch makes it crash harder, i.e. when either call fails instead of
when both fail.

> Fixes: 50ebc3fc47fe (hw/intc/loongarch_ipi: Notify ipi object when cpu is plugged)
> Fixes: 087a23a87c57 (hw/intc/loongarch_extioi: Use cpu plug notification)
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>  hw/loongarch/virt.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index a9fab39dd8..f10a4704ab 100644
> --- a/hw/loongarch/virt.c
> +++ b/hw/loongarch/virt.c
> @@ -327,7 +327,6 @@ static void virt_cpu_irq_init(LoongArchVirtMachineState *lvms)
>      MachineClass *mc = MACHINE_GET_CLASS(ms);
>      const CPUArchIdList *possible_cpus;
>      CPUState *cs;
> -    Error *err = NULL;
>  
>      /* cpu nodes */
>      possible_cpus = mc->possible_cpu_arch_ids(ms);
> @@ -337,8 +336,10 @@ static void virt_cpu_irq_init(LoongArchVirtMachineState *lvms)
>              continue;
>          }
>  
> -        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), DEVICE(cs), &err);
> -        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), DEVICE(cs), &err);
> +        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), DEVICE(cs),
> +                             &error_abort);
> +        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), DEVICE(cs),
> +                             &error_abort);
>      }
>  }