Re: [Qemu-devel] [PATCH v2 2/4] spapr: fix error reporting in xics_system_init()

[David Gibson]

On Mon, May 22, 2017 at 09:41:48AM +0200, Markus Armbruster wrote:
> Greg Kurz  writes:
> 
> > On Sat, 20 May 2017 16:45:09 +1000
> > David Gibson  wrote:
> >
> >> On Fri, May 19, 2017 at 12:32:12PM +0200, Greg Kurz wrote:
> >> > If the user explicitely asked for kernel-irqchip support and "xics-kvm"
> >> > initialization fails, we shouldn't fallback to emulated "xics" as we
> >> > do now. It is also awkward to print an error message when we have an
> >> > errp pointer argument.
> >> > 
> >> > Let's use the errp argument to report the error and let the caller 
> >> > decide.
> >> > This simplifies the code as we don't need a local Error * here.
> >> > 
> >> > Signed-off-by: Greg Kurz   
> >> 
> >> Concept looks good, but..
> >> 
> >> > ---
> >> > v2: - total rewrite
> >> > ---
> >> >  hw/ppc/spapr.c |   13 ++---
> >> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >> > 
> >> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> > index 91f7434861a8..75e298b4c6be 100644
> >> > --- a/hw/ppc/spapr.c
> >> > +++ b/hw/ppc/spapr.c
> >> > @@ -128,18 +128,14 @@ static void xics_system_init(MachineState 
> >> > *machine, int nr_irqs, Error **errp)
> >> >  sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> >> >  
> >> >  if (kvm_enabled()) {
> >> > -Error *err = NULL;
> >> > -
> >> >  if (machine_kernel_irqchip_allowed(machine) &&
> >> >  !xics_kvm_init(spapr, errp)) {
> >> >  spapr->icp_type = TYPE_KVM_ICP;
> >> > -spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, 
> >> > );
> >> > +spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, 
> >> > errp);  
> >> 
> >> I believe there are reasons you're not supposed to just pass an errp
> >> through to a subordinate function.  Instead you're supposed to have a
> >> local Error * and use error_propagate().
> >> 
> >
> > You only need to have a local Error * if it is used to check the return 
> > status
> > of the function (ie, you cannot check *errp because errp could be NULL) as
> > described in error.h.
> 
> Correct.  Quote:
> 
>  * Receive an error and pass it on to the caller:
>  * Error *err = NULL;
>  * foo(arg, );
>  * if (err) {
>  * handle the error...
>  * error_propagate(errp, err);
>  * }
>  * where Error **errp is a parameter, by convention the last one.
>  *
>  * Do *not* "optimize" this to
>  * foo(arg, errp);
>  * if (*errp) { // WRONG!
>  * handle the error...
>  * }
>  * because errp may be NULL!
>  *
>  * But when all you do with the error is pass it on, please use
>  * foo(arg, errp);
>  * for readability.

So, I already merged based on Greg's comment, but it's nice to have
confirmation; thanks Markus.


> >   This isn't the case here but...
> >
> >> >  }
> >> >  if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> >> > -error_reportf_err(err,
> >> > -  "kernel_irqchip requested but 
> >> > unavailable: ");
> >> > -} else {
> >> > -error_free(err);
> >> > +error_prepend(errp, "kernel_irqchip requested but 
> >> > unavailable: ");
> >> > +return;
> >> >  }
> >> >  }
> >> >  
> >> > @@ -147,6 +143,9 @@ static void xics_system_init(MachineState *machine, 
> >> > int nr_irqs, Error **errp)
> >> >  xics_spapr_init(spapr);
> >> >  spapr->icp_type = TYPE_ICP;
> >> >  spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, 
> >> > errp);
> >> > +if (!spapr->ics) {  
> >> 
> >> It would also be more standard to check the returned error, rather
> >> than the other result.
> >> 
> >
> > ... if you prefer to use a local Error *, I'll gladly do that. :)
> 
> Opinions and practice vary on this one.
> 
> I prefer checking the return value because it lets me avoid the
> error_propagate() boiler-plate more often.

Noted for future reference.

> Having both an Error parameter and an error return value begs the
> question whether the two agree.

[Irrelevant aside: this is not what "begging the question" means.  Or
 at least, it's not what it used to mean; it's probably a lost cause
 at this point, even with those who don't get a free pass for being
 non-native speakers.  https://en.wikipedia.org/wiki/Begging_the_question]
 
> You can assert they do, but it's distracting.  We generally don't.
> 
> When there's no success value to transmit, you avoid the problem by
> making the function return void.  We used to favor that, but it has
> turned out not to be a success, because it leads to cumbersome code.
> For what it's worth, GLib wants you to transmit success / failure in the
> return value, too:
> 
> https://developer.gnome.org/glib/unstable/glib-Error-Reporting.html#gerror-rules

Also noted, thanks.

-- 
David Gibson| I'll have my music baroque, and my code

Re: [Qemu-devel] [PATCH v2 2/4] spapr: fix error reporting in xics_system_init()

[Markus Armbruster]

Greg Kurz  writes:

> On Sat, 20 May 2017 16:45:09 +1000
> David Gibson  wrote:
>
>> On Fri, May 19, 2017 at 12:32:12PM +0200, Greg Kurz wrote:
>> > If the user explicitely asked for kernel-irqchip support and "xics-kvm"
>> > initialization fails, we shouldn't fallback to emulated "xics" as we
>> > do now. It is also awkward to print an error message when we have an
>> > errp pointer argument.
>> > 
>> > Let's use the errp argument to report the error and let the caller decide.
>> > This simplifies the code as we don't need a local Error * here.
>> > 
>> > Signed-off-by: Greg Kurz   
>> 
>> Concept looks good, but..
>> 
>> > ---
>> > v2: - total rewrite
>> > ---
>> >  hw/ppc/spapr.c |   13 ++---
>> >  1 file changed, 6 insertions(+), 7 deletions(-)
>> > 
>> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> > index 91f7434861a8..75e298b4c6be 100644
>> > --- a/hw/ppc/spapr.c
>> > +++ b/hw/ppc/spapr.c
>> > @@ -128,18 +128,14 @@ static void xics_system_init(MachineState *machine, 
>> > int nr_irqs, Error **errp)
>> >  sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
>> >  
>> >  if (kvm_enabled()) {
>> > -Error *err = NULL;
>> > -
>> >  if (machine_kernel_irqchip_allowed(machine) &&
>> >  !xics_kvm_init(spapr, errp)) {
>> >  spapr->icp_type = TYPE_KVM_ICP;
>> > -spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, 
>> > );
>> > +spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, 
>> > errp);  
>> 
>> I believe there are reasons you're not supposed to just pass an errp
>> through to a subordinate function.  Instead you're supposed to have a
>> local Error * and use error_propagate().
>> 
>
> You only need to have a local Error * if it is used to check the return status
> of the function (ie, you cannot check *errp because errp could be NULL) as
> described in error.h.

Correct.  Quote:

 * Receive an error and pass it on to the caller:
 * Error *err = NULL;
 * foo(arg, );
 * if (err) {
 * handle the error...
 * error_propagate(errp, err);
 * }
 * where Error **errp is a parameter, by convention the last one.
 *
 * Do *not* "optimize" this to
 * foo(arg, errp);
 * if (*errp) { // WRONG!
 * handle the error...
 * }
 * because errp may be NULL!
 *
 * But when all you do with the error is pass it on, please use
 * foo(arg, errp);
 * for readability.

>   This isn't the case here but...
>
>> >  }
>> >  if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
>> > -error_reportf_err(err,
>> > -  "kernel_irqchip requested but unavailable: 
>> > ");
>> > -} else {
>> > -error_free(err);
>> > +error_prepend(errp, "kernel_irqchip requested but 
>> > unavailable: ");
>> > +return;
>> >  }
>> >  }
>> >  
>> > @@ -147,6 +143,9 @@ static void xics_system_init(MachineState *machine, 
>> > int nr_irqs, Error **errp)
>> >  xics_spapr_init(spapr);
>> >  spapr->icp_type = TYPE_ICP;
>> >  spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, 
>> > errp);
>> > +if (!spapr->ics) {  
>> 
>> It would also be more standard to check the returned error, rather
>> than the other result.
>> 
>
> ... if you prefer to use a local Error *, I'll gladly do that. :)

Opinions and practice vary on this one.

I prefer checking the return value because it lets me avoid the
error_propagate() boiler-plate more often.

Having both an Error parameter and an error return value begs the
question whether the two agree.

You can assert they do, but it's distracting.  We generally don't.

When there's no success value to transmit, you avoid the problem by
making the function return void.  We used to favor that, but it has
turned out not to be a success, because it leads to cumbersome code.
For what it's worth, GLib wants you to transmit success / failure in the
return value, too:

https://developer.gnome.org/glib/unstable/glib-Error-Reporting.html#gerror-rules

>> > +return;
>> > +}
>> >  }
>> >  }
>> >  
>> >   
>> 

Re: [Qemu-devel] [PATCH v2 2/4] spapr: fix error reporting in xics_system_init()

[David Gibson]

On Sun, May 21, 2017 at 07:03:33PM +0200, Greg Kurz wrote:
> On Sat, 20 May 2017 16:45:09 +1000
> David Gibson  wrote:
> 
> > On Fri, May 19, 2017 at 12:32:12PM +0200, Greg Kurz wrote:
> > > If the user explicitely asked for kernel-irqchip support and "xics-kvm"
> > > initialization fails, we shouldn't fallback to emulated "xics" as we
> > > do now. It is also awkward to print an error message when we have an
> > > errp pointer argument.
> > > 
> > > Let's use the errp argument to report the error and let the caller decide.
> > > This simplifies the code as we don't need a local Error * here.
> > > 
> > > Signed-off-by: Greg Kurz   
> > 
> > Concept looks good, but..
> > 
> > > ---
> > > v2: - total rewrite
> > > ---
> > >  hw/ppc/spapr.c |   13 ++---
> > >  1 file changed, 6 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 91f7434861a8..75e298b4c6be 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -128,18 +128,14 @@ static void xics_system_init(MachineState *machine, 
> > > int nr_irqs, Error **errp)
> > >  sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > >  
> > >  if (kvm_enabled()) {
> > > -Error *err = NULL;
> > > -
> > >  if (machine_kernel_irqchip_allowed(machine) &&
> > >  !xics_kvm_init(spapr, errp)) {
> > >  spapr->icp_type = TYPE_KVM_ICP;
> > > -spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, 
> > > );
> > > +spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, 
> > > errp);  
> > 
> > I believe there are reasons you're not supposed to just pass an errp
> > through to a subordinate function.  Instead you're supposed to have a
> > local Error * and use error_propagate().
> > 
> 
> You only need to have a local Error * if it is used to check the return status
> of the function (ie, you cannot check *errp because errp could be NULL) as
> described in error.h. This isn't the case here but...

Fair point; patch applied to ppc-for-2.10.

> 
> > >  }
> > >  if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> > > -error_reportf_err(err,
> > > -  "kernel_irqchip requested but unavailable: 
> > > ");
> > > -} else {
> > > -error_free(err);
> > > +error_prepend(errp, "kernel_irqchip requested but 
> > > unavailable: ");
> > > +return;
> > >  }
> > >  }
> > >  
> > > @@ -147,6 +143,9 @@ static void xics_system_init(MachineState *machine, 
> > > int nr_irqs, Error **errp)
> > >  xics_spapr_init(spapr);
> > >  spapr->icp_type = TYPE_ICP;
> > >  spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, 
> > > errp);
> > > +if (!spapr->ics) {  
> > 
> > It would also be more standard to check the returned error, rather
> > than the other result.
> > 
> 
> ... if you prefer to use a local Error *, I'll gladly do that. :)
> 
> > > +return;
> > > +}
> > >  }
> > >  }
> > >  
> > >   
> > 
> 



-- 
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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 2/4] spapr: fix error reporting in xics_system_init()

[Greg Kurz]

On Sat, 20 May 2017 16:45:09 +1000
David Gibson  wrote:

> On Fri, May 19, 2017 at 12:32:12PM +0200, Greg Kurz wrote:
> > If the user explicitely asked for kernel-irqchip support and "xics-kvm"
> > initialization fails, we shouldn't fallback to emulated "xics" as we
> > do now. It is also awkward to print an error message when we have an
> > errp pointer argument.
> > 
> > Let's use the errp argument to report the error and let the caller decide.
> > This simplifies the code as we don't need a local Error * here.
> > 
> > Signed-off-by: Greg Kurz   
> 
> Concept looks good, but..
> 
> > ---
> > v2: - total rewrite
> > ---
> >  hw/ppc/spapr.c |   13 ++---
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 91f7434861a8..75e298b4c6be 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -128,18 +128,14 @@ static void xics_system_init(MachineState *machine, 
> > int nr_irqs, Error **errp)
> >  sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> >  
> >  if (kvm_enabled()) {
> > -Error *err = NULL;
> > -
> >  if (machine_kernel_irqchip_allowed(machine) &&
> >  !xics_kvm_init(spapr, errp)) {
> >  spapr->icp_type = TYPE_KVM_ICP;
> > -spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, 
> > );
> > +spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, 
> > errp);  
> 
> I believe there are reasons you're not supposed to just pass an errp
> through to a subordinate function.  Instead you're supposed to have a
> local Error * and use error_propagate().
> 

You only need to have a local Error * if it is used to check the return status
of the function (ie, you cannot check *errp because errp could be NULL) as
described in error.h. This isn't the case here but...

> >  }
> >  if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> > -error_reportf_err(err,
> > -  "kernel_irqchip requested but unavailable: 
> > ");
> > -} else {
> > -error_free(err);
> > +error_prepend(errp, "kernel_irqchip requested but unavailable: 
> > ");
> > +return;
> >  }
> >  }
> >  
> > @@ -147,6 +143,9 @@ static void xics_system_init(MachineState *machine, int 
> > nr_irqs, Error **errp)
> >  xics_spapr_init(spapr);
> >  spapr->icp_type = TYPE_ICP;
> >  spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, 
> > errp);
> > +if (!spapr->ics) {  
> 
> It would also be more standard to check the returned error, rather
> than the other result.
> 

... if you prefer to use a local Error *, I'll gladly do that. :)

> > +return;
> > +}
> >  }
> >  }
> >  
> >   
> 



pgprnKK2ei3Zq.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 2/4] spapr: fix error reporting in xics_system_init()

[David Gibson]

On Fri, May 19, 2017 at 12:32:12PM +0200, Greg Kurz wrote:
> If the user explicitely asked for kernel-irqchip support and "xics-kvm"
> initialization fails, we shouldn't fallback to emulated "xics" as we
> do now. It is also awkward to print an error message when we have an
> errp pointer argument.
> 
> Let's use the errp argument to report the error and let the caller decide.
> This simplifies the code as we don't need a local Error * here.
> 
> Signed-off-by: Greg Kurz 

Concept looks good, but..

> ---
> v2: - total rewrite
> ---
>  hw/ppc/spapr.c |   13 ++---
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 91f7434861a8..75e298b4c6be 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -128,18 +128,14 @@ static void xics_system_init(MachineState *machine, int 
> nr_irqs, Error **errp)
>  sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
>  
>  if (kvm_enabled()) {
> -Error *err = NULL;
> -
>  if (machine_kernel_irqchip_allowed(machine) &&
>  !xics_kvm_init(spapr, errp)) {
>  spapr->icp_type = TYPE_KVM_ICP;
> -spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, 
> );
> +spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, 
> errp);

I believe there are reasons you're not supposed to just pass an errp
through to a subordinate function.  Instead you're supposed to have a
local Error * and use error_propagate().

>  }
>  if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> -error_reportf_err(err,
> -  "kernel_irqchip requested but unavailable: ");
> -} else {
> -error_free(err);
> +error_prepend(errp, "kernel_irqchip requested but unavailable: 
> ");
> +return;
>  }
>  }
>  
> @@ -147,6 +143,9 @@ static void xics_system_init(MachineState *machine, int 
> nr_irqs, Error **errp)
>  xics_spapr_init(spapr);
>  spapr->icp_type = TYPE_ICP;
>  spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
> +if (!spapr->ics) {

It would also be more standard to check the returned error, rather
than the other result.

> +return;
> +}
>  }
>  }
>  
> 

-- 
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


signature.asc
Description: PGP signature