| Message ID | 149484836649.20089.8174753739169885859.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
On 05/15/2017 01:39 PM, Greg Kurz wrote: > QEMU should exit if the user explicitely asked for kernel-irqchip support > and "xics-kvm" initialization fails. > > The changelog of commit 34f2af3d3edf ("spapr: Clean up misuse of qdev_init() > in xics-kvm creation") reads: > > While there, improve the error message when we can't satisfy an > explicit user request for "xics-kvm", and exit(1) instead of abort(). > Simplify the abort when we can't create "xics". > > This patch adds the missing call to exit(). > > Signed-off-by: Greg Kurz <groug@kaod.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/spapr.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index abfb99b71b7d..f477d7b8a210 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -133,6 +133,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp) > if (machine_kernel_irqchip_required(machine) && !spapr->ics) { > error_reportf_err(err, > "kernel_irqchip requested but unavailable: "); > + exit(EXIT_FAILURE); > } else { > error_free(err); > } >
On Mon, May 15, 2017 at 01:39:26PM +0200, Greg Kurz wrote: > QEMU should exit if the user explicitely asked for kernel-irqchip support > and "xics-kvm" initialization fails. > > The changelog of commit 34f2af3d3edf ("spapr: Clean up misuse of qdev_init() > in xics-kvm creation") reads: > > While there, improve the error message when we can't satisfy an > explicit user request for "xics-kvm", and exit(1) instead of abort(). > Simplify the abort when we can't create "xics". > > This patch adds the missing call to exit(). > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/ppc/spapr.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index abfb99b71b7d..f477d7b8a210 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -133,6 +133,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp) > if (machine_kernel_irqchip_required(machine) && !spapr->ics) { > error_reportf_err(err, > "kernel_irqchip requested but unavailable: "); > + exit(EXIT_FAILURE); > } else { > error_free(err); > } > This doesn't look right. We have an errp pointer in the caller. So on failure we should error_propagate(), rather than deciding for ourselves that exiting is the right course of action.
On Tue, 16 May 2017 14:35:55 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Mon, May 15, 2017 at 01:39:26PM +0200, Greg Kurz wrote: > > QEMU should exit if the user explicitely asked for kernel-irqchip support > > and "xics-kvm" initialization fails. > > > > The changelog of commit 34f2af3d3edf ("spapr: Clean up misuse of qdev_init() > > in xics-kvm creation") reads: > > > > While there, improve the error message when we can't satisfy an > > explicit user request for "xics-kvm", and exit(1) instead of abort(). > > Simplify the abort when we can't create "xics". > > > > This patch adds the missing call to exit(). > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/ppc/spapr.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index abfb99b71b7d..f477d7b8a210 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -133,6 +133,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp) > > if (machine_kernel_irqchip_required(machine) && !spapr->ics) { > > error_reportf_err(err, > > "kernel_irqchip requested but unavailable: "); > > + exit(EXIT_FAILURE); > > } else { > > error_free(err); > > } > > > > > This doesn't look right. We have an errp pointer in the caller. So > on failure we should error_propagate(), rather than deciding for > ourselves that exiting is the right course of action. > I generally agree with that but can the caller cope with the fact that the user passed -machine accel=kvm,kernel_irqchip=on and this cannot be satisfied ?
On Tue, May 16, 2017 at 07:56:05AM +0200, Greg Kurz wrote: > On Tue, 16 May 2017 14:35:55 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Mon, May 15, 2017 at 01:39:26PM +0200, Greg Kurz wrote: > > > QEMU should exit if the user explicitely asked for kernel-irqchip support > > > and "xics-kvm" initialization fails. > > > > > > The changelog of commit 34f2af3d3edf ("spapr: Clean up misuse of qdev_init() > > > in xics-kvm creation") reads: > > > > > > While there, improve the error message when we can't satisfy an > > > explicit user request for "xics-kvm", and exit(1) instead of abort(). > > > Simplify the abort when we can't create "xics". > > > > > > This patch adds the missing call to exit(). > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > --- > > > hw/ppc/spapr.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index abfb99b71b7d..f477d7b8a210 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -133,6 +133,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp) > > > if (machine_kernel_irqchip_required(machine) && !spapr->ics) { > > > error_reportf_err(err, > > > "kernel_irqchip requested but unavailable: "); > > > + exit(EXIT_FAILURE); > > > } else { > > > error_free(err); > > > } > > > > > > > > > This doesn't look right. We have an errp pointer in the caller. So > > on failure we should error_propagate(), rather than deciding for > > ourselves that exiting is the right course of action. > > > > I generally agree with that but can the caller cope with the fact that > the user passed -machine accel=kvm,kernel_irqchip=on and this cannot > be satisfied ? AFAICT the only caller passes &error_fatal, so, yes..
On Tue, 16 May 2017 16:08:46 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Tue, May 16, 2017 at 07:56:05AM +0200, Greg Kurz wrote: > > On Tue, 16 May 2017 14:35:55 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Mon, May 15, 2017 at 01:39:26PM +0200, Greg Kurz wrote: > > > > QEMU should exit if the user explicitely asked for kernel-irqchip support > > > > and "xics-kvm" initialization fails. > > > > > > > > The changelog of commit 34f2af3d3edf ("spapr: Clean up misuse of qdev_init() > > > > in xics-kvm creation") reads: > > > > > > > > While there, improve the error message when we can't satisfy an > > > > explicit user request for "xics-kvm", and exit(1) instead of abort(). > > > > Simplify the abort when we can't create "xics". > > > > > > > > This patch adds the missing call to exit(). > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > --- > > > > hw/ppc/spapr.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > > index abfb99b71b7d..f477d7b8a210 100644 > > > > --- a/hw/ppc/spapr.c > > > > +++ b/hw/ppc/spapr.c > > > > @@ -133,6 +133,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp) > > > > if (machine_kernel_irqchip_required(machine) && !spapr->ics) { > > > > error_reportf_err(err, > > > > "kernel_irqchip requested but unavailable: "); > > > > + exit(EXIT_FAILURE); > > > > } else { > > > > error_free(err); > > > > } > > > > > > > > > > > > > This doesn't look right. We have an errp pointer in the caller. So > > > on failure we should error_propagate(), rather than deciding for > > > ourselves that exiting is the right course of action. > > > > > > > I generally agree with that but can the caller cope with the fact that > > the user passed -machine accel=kvm,kernel_irqchip=on and this cannot > > be satisfied ? > > AFAICT the only caller passes &error_fatal, so, yes.. > Ok, then I'll rewrite xics_system_init() so that it propagates the error message instead of printing it to the monitor.
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index abfb99b71b7d..f477d7b8a210 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -133,6 +133,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp) if (machine_kernel_irqchip_required(machine) && !spapr->ics) { error_reportf_err(err, "kernel_irqchip requested but unavailable: "); + exit(EXIT_FAILURE); } else { error_free(err); }
QEMU should exit if the user explicitely asked for kernel-irqchip support and "xics-kvm" initialization fails. The changelog of commit 34f2af3d3edf ("spapr: Clean up misuse of qdev_init() in xics-kvm creation") reads: While there, improve the error message when we can't satisfy an explicit user request for "xics-kvm", and exit(1) instead of abort(). Simplify the abort when we can't create "xics". This patch adds the missing call to exit(). Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/spapr.c | 1 + 1 file changed, 1 insertion(+)