Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {}

From: David Gibson <david@gibson.dropbear.id.au>
To: Eric Blake <eblake@redhat.com>
Cc: peter.maydell@linaro.org, agraf@suse.de, qemu-ppc@nongnu.org,
	Markus Armbruster <armbru@redhat.com>,
	qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH for-2.5 2/3] spapr_drc: Change value of property "fdt" from null back to {}
Date: Fri, 4 Dec 2015 16:50:37 +1100	[thread overview]
Message-ID: <20151204055037.GG9559@voom.redhat.com> (raw)
In-Reply-To: <5660FD77.90500@redhat.com>

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

On Thu, Dec 03, 2015 at 07:41:59PM -0700, Eric Blake wrote:
> On 12/03/2015 04:54 PM, David Gibson wrote:
> > On Thu, Dec 03, 2015 at 05:37:39PM +0100, Markus Armbruster wrote:
> >> prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't
> >> visit anything.  object_property_get_qobject() happily
> >> object_property_get_qobject().  Amazingly, the latter survives the
> >> misuse.  Turns out we've papered over it long before prop_get_fdt()
> >> existed, in commit 1d10b44.
> >>
> >> However, commit 6c2f9a1 changed how we paper over it, and as a side
> >> effect changed qom-get's value from {} to null.  Change it right back
> >> by fixing the visitor misuse.
> >>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  hw/ppc/spapr_drc.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> 
> > I'm not entirely convinced by this.  IIUC, this makes the output in
> > the case of NULL (i.e. missing) fdt identical to the output in the
> > case of an empty, valid fdt - in dtc syntax, this:
> > 	/ {
> > 	};
> > 
> > Those are different cases from the point of view of the code which
> > actually uses the fdt, and for purposes of debugging it, I suspect we
> > want to expose that difference.
> 
> Expressing null may be the right thing, but it should be a conscious
> decision, and not a side-effect of an unrelated patch.  This patch is
> just about avoiding a regression for 2.5, because outputting {} for both
> a missing fdt and an empty one was the behavior we had back in 2.4 (that
> is, we've already returned {} in at least one release, so it won't hurt
> to do it for one more).  For 2.6 we can revisit things to actually
> express what is wanted.
> 
> > 
> > I don't know what the QOMishly correct way of doing that is, though.
> > Can we somehow make the "fdt" property disappear entirely if fdt is
> > NULL?
> 
> In qapi terms, if a variable is marked optional and has_FOO is false,
> then the variable disappears completely.  But I'm not sure if that maps
> over to qom.  Maybe you do it by setting errp if drc->fdt is NULL, so
> that prop_get_fdt() only succeeds when there is something for it to
> return.  Or maybe returning qnull() is right after all, but in that
> case, explicitly calling 'QObject *n = qnull(); visit_type_any(v, &n,
> NULL, &err) seems like a nicer way than relying on side effects of how
> the qmp output visitor behaves when nothing was visited.

Ok, thank makes sense.

Acked-by: David Gibson <david@gibson.dropbear.id.au>

-- 
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: 819 bytes --]