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

From: David Gibson <david@gibson.dropbear.id.au>
To: Greg Kurz <groug@kaod.org>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	"Daniel P. Berrange" <berrange@redhat.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Markus Armbruster <armbru@redhat.com>,
	qemu-devel@nongnu.org, qemu-ppc@nongnu.org,
	Igor Mammedov <imammedo@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH 0/5] spapr: Error handling fixes and cleanups (round 3)
Date: Mon, 26 Oct 2020 16:44:08 +1100	[thread overview]
Message-ID: <20201026054408.GA4671@yekko.fritz.box> (raw)
In-Reply-To: <20201025223306.0ca3ee0a@bahia.lan>

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

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

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