mbox series

[0/5] spapr: Error handling fixes and cleanups (round 3)

Message ID 160309727218.2739814.14722724927730985344.stgit@bahia.lan
Headers show
Series spapr: Error handling fixes and cleanups (round 3) | expand

Message

Greg Kurz Oct. 19, 2020, 8:47 a.m. UTC
Hi,

This is a followup to a previous cleanup for the sPAPR code:

https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg04860.html

The last two patches had to be dropped because they were wrongly assuming
that object_property_get_uint() returning zero meant failure. This led to
a discussion in which arose a consensus that most of the time (not to say
always) object property getters should never fail actually, ie. failure
is very likely the result of a programming error and QEMU should abort.

This series aims at demonstrating a revelant case I've found while auditing
object property getters (this is patch 4 that I've isolated from a huge
50-patch series I haven't dared to post yet). The sPAPR memory hotplug code
is tailored to support either regular PC DIMMs or NVDIMMs, which inherit
from PC DIMMs. They expect to get some properties from the DIMM object,
which happens to be set by default at the PC DIMM class level. It thus
doesn't make sense to pass an error object and propagate it when getting
them since this would lure the user into thinking they did something wrong.

Some preliminary cleanup is done on the way, especially dropping an unused
@errp argument of pc_dimm_plug(). This affects several platforms other than
sPAPR but I guess the patch is trivial enough to go through David's tree
if it gets acks from the relevant maintainers.

---

Greg Kurz (5):
      pc-dimm: Drop @errp argument of pc_dimm_plug()
      spapr: Use appropriate getter for PC_DIMM_ADDR_PROP
      spapr: Use appropriate getter for PC_DIMM_SLOT_PROP
      spapr: Pass &error_abort when getting some PC DIMM properties
      spapr: Simplify error handling in spapr_memory_plug()


 hw/arm/virt.c                 |    9 +-------
 hw/i386/pc.c                  |    8 +------
 hw/mem/pc-dimm.c              |    2 +-
 hw/ppc/spapr.c                |   48 +++++++++++++++--------------------------
 hw/ppc/spapr_nvdimm.c         |    5 +++-
 include/hw/mem/pc-dimm.h      |    2 +-
 include/hw/ppc/spapr_nvdimm.h |    2 +-
 7 files changed, 25 insertions(+), 51 deletions(-)

--
Greg

Comments

David Gibson Oct. 22, 2020, 4:11 a.m. UTC | #1
On Mon, Oct 19, 2020 at 10:47:52AM +0200, Greg Kurz wrote:
> Hi,

> 

> This is a followup to a previous cleanup for the sPAPR code:

> 

> https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg04860.html

> 

> The last two patches had to be dropped because they were wrongly assuming

> that object_property_get_uint() returning zero meant failure. This led to

> a discussion in which arose a consensus that most of the time (not to say

> always) object property getters should never fail actually, ie. failure

> is very likely the result of a programming error and QEMU should abort.

> 

> This series aims at demonstrating a revelant case I've found while auditing

> object property getters (this is patch 4 that I've isolated from a huge

> 50-patch series I haven't dared to post yet). The sPAPR memory hotplug code

> is tailored to support either regular PC DIMMs or NVDIMMs, which inherit

> from PC DIMMs. They expect to get some properties from the DIMM object,

> which happens to be set by default at the PC DIMM class level. It thus

> doesn't make sense to pass an error object and propagate it when getting

> them since this would lure the user into thinking they did something wrong.

> 

> Some preliminary cleanup is done on the way, especially dropping an unused

> @errp argument of pc_dimm_plug(). This affects several platforms other than

> sPAPR but I guess the patch is trivial enough to go through David's tree

> if it gets acks from the relevant maintainers.


Since this series mostly affects ppc, I've applied it to ppc-for-5.2.

It would be nice to have an acked-by from Igor or Michael for the
first patch, though.

> 

> ---

> 

> Greg Kurz (5):

>       pc-dimm: Drop @errp argument of pc_dimm_plug()

>       spapr: Use appropriate getter for PC_DIMM_ADDR_PROP

>       spapr: Use appropriate getter for PC_DIMM_SLOT_PROP

>       spapr: Pass &error_abort when getting some PC DIMM properties

>       spapr: Simplify error handling in spapr_memory_plug()

> 

> 

>  hw/arm/virt.c                 |    9 +-------

>  hw/i386/pc.c                  |    8 +------

>  hw/mem/pc-dimm.c              |    2 +-

>  hw/ppc/spapr.c                |   48 +++++++++++++++--------------------------

>  hw/ppc/spapr_nvdimm.c         |    5 +++-

>  include/hw/mem/pc-dimm.h      |    2 +-

>  include/hw/ppc/spapr_nvdimm.h |    2 +-

>  7 files changed, 25 insertions(+), 51 deletions(-)

> 


-- 
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
Greg Kurz Oct. 25, 2020, 10:13 a.m. UTC | #2
On Thu, 22 Oct 2020 15:11:42 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Oct 19, 2020 at 10:47:52AM +0200, Greg Kurz wrote:

> > Hi,

> > 

> > This is a followup to a previous cleanup for the sPAPR code:

> > 

> > https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg04860.html

> > 

> > The last two patches had to be dropped because they were wrongly assuming

> > that object_property_get_uint() returning zero meant failure. This led to

> > a discussion in which arose a consensus that most of the time (not to say

> > always) object property getters should never fail actually, ie. failure

> > is very likely the result of a programming error and QEMU should abort.

> > 

> > This series aims at demonstrating a revelant case I've found while auditing

> > object property getters (this is patch 4 that I've isolated from a huge

> > 50-patch series I haven't dared to post yet). The sPAPR memory hotplug code

> > is tailored to support either regular PC DIMMs or NVDIMMs, which inherit

> > from PC DIMMs. They expect to get some properties from the DIMM object,

> > which happens to be set by default at the PC DIMM class level. It thus

> > doesn't make sense to pass an error object and propagate it when getting

> > them since this would lure the user into thinking they did something wrong.

> > 

> > Some preliminary cleanup is done on the way, especially dropping an unused

> > @errp argument of pc_dimm_plug(). This affects several platforms other than

> > sPAPR but I guess the patch is trivial enough to go through David's tree

> > if it gets acks from the relevant maintainers.

> 

> Since this series mostly affects ppc, I've applied it to ppc-for-5.2.

> 

> It would be nice to have an acked-by from Igor or Michael for the

> first patch, though.

> 


David,

Igor sent a R-b for patches 1 and 4. He also suggested to call
spapr_drc_attach() at pre-plug time. I'll look into this, so maybe
you can drop patch 5 from ppc-for-5.2 (or the entire series at
your convenience).

Cheers,

--
Greg

> > 

> > ---

> > 

> > Greg Kurz (5):

> >       pc-dimm: Drop @errp argument of pc_dimm_plug()

> >       spapr: Use appropriate getter for PC_DIMM_ADDR_PROP

> >       spapr: Use appropriate getter for PC_DIMM_SLOT_PROP

> >       spapr: Pass &error_abort when getting some PC DIMM properties

> >       spapr: Simplify error handling in spapr_memory_plug()

> > 

> > 

> >  hw/arm/virt.c                 |    9 +-------

> >  hw/i386/pc.c                  |    8 +------

> >  hw/mem/pc-dimm.c              |    2 +-

> >  hw/ppc/spapr.c                |   48 +++++++++++++++--------------------------

> >  hw/ppc/spapr_nvdimm.c         |    5 +++-

> >  include/hw/mem/pc-dimm.h      |    2 +-

> >  include/hw/ppc/spapr_nvdimm.h |    2 +-

> >  7 files changed, 25 insertions(+), 51 deletions(-)

> > 

>
Greg Kurz Oct. 25, 2020, 9:33 p.m. UTC | #3
On Sun, 25 Oct 2020 11:13:40 +0100
Greg Kurz <groug@kaod.org> wrote:

> On Thu, 22 Oct 2020 15:11:42 +1100

> David Gibson <david@gibson.dropbear.id.au> wrote:

> 

> > On Mon, Oct 19, 2020 at 10:47:52AM +0200, Greg Kurz wrote:

> > > Hi,

> > > 

> > > This is a followup to a previous cleanup for the sPAPR code:

> > > 

> > > https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg04860.html

> > > 

> > > The last two patches had to be dropped because they were wrongly assuming

> > > that object_property_get_uint() returning zero meant failure. This led to

> > > a discussion in which arose a consensus that most of the time (not to say

> > > always) object property getters should never fail actually, ie. failure

> > > is very likely the result of a programming error and QEMU should abort.

> > > 

> > > This series aims at demonstrating a revelant case I've found while auditing

> > > object property getters (this is patch 4 that I've isolated from a huge

> > > 50-patch series I haven't dared to post yet). The sPAPR memory hotplug code

> > > is tailored to support either regular PC DIMMs or NVDIMMs, which inherit

> > > from PC DIMMs. They expect to get some properties from the DIMM object,

> > > which happens to be set by default at the PC DIMM class level. It thus

> > > doesn't make sense to pass an error object and propagate it when getting

> > > them since this would lure the user into thinking they did something wrong.

> > > 

> > > Some preliminary cleanup is done on the way, especially dropping an unused

> > > @errp argument of pc_dimm_plug(). This affects several platforms other than

> > > sPAPR but I guess the patch is trivial enough to go through David's tree

> > > if it gets acks from the relevant maintainers.

> > 

> > Since this series mostly affects ppc, I've applied it to ppc-for-5.2.

> > 

> > It would be nice to have an acked-by from Igor or Michael for the

> > first patch, though.

> > 

> 

> David,

> 

> Igor sent a R-b for patches 1 and 4. He also suggested to call

> spapr_drc_attach() at pre-plug time. I'll look into this, so maybe

> you can drop patch 5 from ppc-for-5.2 (or the entire series at

> your convenience).

> 


It seems that spapr_drc_attach() cannot be called at pre-plug time
actually because there is no way to call spapr_drc_detach() if
the device fails to realize. I think you there's nothing else to do
for this series than adding Igor's r-b to patches 1 and 4.

> Cheers,

> 

> --

> Greg

> 

> > > 

> > > ---

> > > 

> > > Greg Kurz (5):

> > >       pc-dimm: Drop @errp argument of pc_dimm_plug()

> > >       spapr: Use appropriate getter for PC_DIMM_ADDR_PROP

> > >       spapr: Use appropriate getter for PC_DIMM_SLOT_PROP

> > >       spapr: Pass &error_abort when getting some PC DIMM properties

> > >       spapr: Simplify error handling in spapr_memory_plug()

> > > 

> > > 

> > >  hw/arm/virt.c                 |    9 +-------

> > >  hw/i386/pc.c                  |    8 +------

> > >  hw/mem/pc-dimm.c              |    2 +-

> > >  hw/ppc/spapr.c                |   48 +++++++++++++++--------------------------

> > >  hw/ppc/spapr_nvdimm.c         |    5 +++-

> > >  include/hw/mem/pc-dimm.h      |    2 +-

> > >  include/hw/ppc/spapr_nvdimm.h |    2 +-

> > >  7 files changed, 25 insertions(+), 51 deletions(-)

> > > 

> > 

>
David Gibson Oct. 26, 2020, 5:44 a.m. UTC | #4
On Sun, Oct 25, 2020 at 10:33:06PM +0100, Greg Kurz wrote:
> On Sun, 25 Oct 2020 11:13:40 +0100

> Greg Kurz <groug@kaod.org> wrote:

> 

> > On Thu, 22 Oct 2020 15:11:42 +1100

> > David Gibson <david@gibson.dropbear.id.au> wrote:

> > 

> > > On Mon, Oct 19, 2020 at 10:47:52AM +0200, Greg Kurz wrote:

> > > > Hi,

> > > > 

> > > > This is a followup to a previous cleanup for the sPAPR code:

> > > > 

> > > > https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg04860.html

> > > > 

> > > > The last two patches had to be dropped because they were wrongly assuming

> > > > that object_property_get_uint() returning zero meant failure. This led to

> > > > a discussion in which arose a consensus that most of the time (not to say

> > > > always) object property getters should never fail actually, ie. failure

> > > > is very likely the result of a programming error and QEMU should abort.

> > > > 

> > > > This series aims at demonstrating a revelant case I've found while auditing

> > > > object property getters (this is patch 4 that I've isolated from a huge

> > > > 50-patch series I haven't dared to post yet). The sPAPR memory hotplug code

> > > > is tailored to support either regular PC DIMMs or NVDIMMs, which inherit

> > > > from PC DIMMs. They expect to get some properties from the DIMM object,

> > > > which happens to be set by default at the PC DIMM class level. It thus

> > > > doesn't make sense to pass an error object and propagate it when getting

> > > > them since this would lure the user into thinking they did something wrong.

> > > > 

> > > > Some preliminary cleanup is done on the way, especially dropping an unused

> > > > @errp argument of pc_dimm_plug(). This affects several platforms other than

> > > > sPAPR but I guess the patch is trivial enough to go through David's tree

> > > > if it gets acks from the relevant maintainers.

> > > 

> > > Since this series mostly affects ppc, I've applied it to ppc-for-5.2.

> > > 

> > > It would be nice to have an acked-by from Igor or Michael for the

> > > first patch, though.

> > > 

> > 

> > David,

> > 

> > Igor sent a R-b for patches 1 and 4. He also suggested to call

> > spapr_drc_attach() at pre-plug time. I'll look into this, so maybe

> > you can drop patch 5 from ppc-for-5.2 (or the entire series at

> > your convenience).

> > 

> 

> It seems that spapr_drc_attach() cannot be called at pre-plug time

> actually because there is no way to call spapr_drc_detach() if

> the device fails to realize. I think you there's nothing else to do

> for this series than adding Igor's r-b to patches 1 and 4.


Ok.  In fact I'd already added Igor's r-b, just hadn't pushed out my
latest tree.

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