* [Qemu-devel] [PATCH] spapr: fix xics_system_init() error path
@ 2018-06-15 16:58 Greg Kurz
2018-06-17 23:43 ` David Gibson
0 siblings, 1 reply; 2+ messages in thread
From: Greg Kurz @ 2018-06-15 16:58 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-ppc, David Gibson, Cédric Le Goater
Commit 3d85885a1b1f3 tried to fix error handling, but it actually
went into the wrong direction by dropping the local Error *.
In the default KVM case, the rationale is to try the in-kernel XICS first,
and if not possible, to fallback to userland XICS. Passing errp everywhere
makes this fallback impossible if errp is &error_fatal (which happens to
be the case). And anyway, if the caller would pass a regular &local_err,
things would be worse: we could possibly pass an already set *errp to
error_setg() and crash, or return an error even in case of success.
So we definitely need a local Error * and only propagate it when we're
done with the fallback logic. This is what this patch does.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/spapr.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f59999daacfc..db0fb385d4e0 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -186,27 +186,33 @@ static int xics_max_server_number(sPAPRMachineState *spapr)
static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
{
sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
+ Error *local_err = NULL;
if (kvm_enabled()) {
if (machine_kernel_irqchip_allowed(machine) &&
- !xics_kvm_init(spapr, errp)) {
+ !xics_kvm_init(spapr, &local_err)) {
spapr->icp_type = TYPE_KVM_ICP;
- spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, errp);
+ spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
+ &local_err);
}
if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
- error_prepend(errp, "kernel_irqchip requested but unavailable: ");
- return;
+ error_prepend(&local_err,
+ "kernel_irqchip requested but unavailable: ");
+ goto error;
}
+ error_free(local_err);
+ local_err = NULL;
}
if (!spapr->ics) {
xics_spapr_init(spapr);
spapr->icp_type = TYPE_ICP;
- spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
- if (!spapr->ics) {
- return;
- }
+ spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
+ &local_err);
}
+
+error:
+ error_propagate(errp, local_err);
}
static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [Qemu-devel] [PATCH] spapr: fix xics_system_init() error path
2018-06-15 16:58 [Qemu-devel] [PATCH] spapr: fix xics_system_init() error path Greg Kurz
@ 2018-06-17 23:43 ` David Gibson
0 siblings, 0 replies; 2+ messages in thread
From: David Gibson @ 2018-06-17 23:43 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-devel, qemu-ppc, Cédric Le Goater
[-- Attachment #1: Type: text/plain, Size: 3031 bytes --]
On Fri, Jun 15, 2018 at 06:58:00PM +0200, Greg Kurz wrote:
> Commit 3d85885a1b1f3 tried to fix error handling, but it actually
> went into the wrong direction by dropping the local Error *.
>
> In the default KVM case, the rationale is to try the in-kernel XICS first,
> and if not possible, to fallback to userland XICS. Passing errp everywhere
> makes this fallback impossible if errp is &error_fatal (which happens to
> be the case). And anyway, if the caller would pass a regular &local_err,
> things would be worse: we could possibly pass an already set *errp to
> error_setg() and crash, or return an error even in case of success.
>
> So we definitely need a local Error * and only propagate it when we're
> done with the fallback logic. This is what this patch does.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied to ppc-for-3.0, thanks.
> ---
> hw/ppc/spapr.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f59999daacfc..db0fb385d4e0 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -186,27 +186,33 @@ static int xics_max_server_number(sPAPRMachineState *spapr)
> static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> {
> sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> + Error *local_err = NULL;
>
> if (kvm_enabled()) {
> if (machine_kernel_irqchip_allowed(machine) &&
> - !xics_kvm_init(spapr, errp)) {
> + !xics_kvm_init(spapr, &local_err)) {
> spapr->icp_type = TYPE_KVM_ICP;
> - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, errp);
> + spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
> + &local_err);
> }
> if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> - error_prepend(errp, "kernel_irqchip requested but unavailable: ");
> - return;
> + error_prepend(&local_err,
> + "kernel_irqchip requested but unavailable: ");
> + goto error;
> }
> + error_free(local_err);
> + local_err = NULL;
> }
>
> if (!spapr->ics) {
> xics_spapr_init(spapr);
> spapr->icp_type = TYPE_ICP;
> - spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
> - if (!spapr->ics) {
> - return;
> - }
> + spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
> + &local_err);
> }
> +
> +error:
> + error_propagate(errp, local_err);
> }
>
> static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-06-17 23:53 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-15 16:58 [Qemu-devel] [PATCH] spapr: fix xics_system_init() error path Greg Kurz
2018-06-17 23:43 ` David Gibson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.