1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH 0/6] spapr/xics: fix migration of older machine types
  4. View patch

[Qemu-devel] [PATCH 2/6] spapr: fix error path of required kernel-irqchip

Greg Kurz posted 6 patches 8 years, 9 months ago
There is a newer version of this series
[Qemu-devel] [PATCH 2/6] spapr: fix error path of required kernel-irqchip
Posted by Greg Kurz 8 years, 9 months ago 
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);
         }
Re: [Qemu-devel] [PATCH 2/6] spapr: fix error path of required kernel-irqchip
Posted by Cédric Le Goater 8 years, 9 months ago 
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);
>          }
>
Re: [Qemu-devel] [PATCH 2/6] spapr: fix error path of required kernel-irqchip
Posted by David Gibson 8 years, 8 months ago 
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.

-- 
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
Re: [Qemu-devel] [PATCH 2/6] spapr: fix error path of required kernel-irqchip
Posted by Greg Kurz 8 years, 8 months ago 
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 ?
Re: [Qemu-devel] [PATCH 2/6] spapr: fix error path of required kernel-irqchip
Posted by David Gibson 8 years, 8 months ago 
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..

-- 
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
Re: [Qemu-devel] [PATCH 2/6] spapr: fix error path of required kernel-irqchip
Posted by Greg Kurz 8 years, 8 months ago 
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.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.