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

[David Gibson]

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

signature.asc
Description: PGP signature