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

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 3369 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]