Eric Blake <ebl...@redhat.com> writes:
> 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 <arm...@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.
Yes.
>> 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.
If I understand QOM correctly, we should be able to add the property
dynamically, so that it exists exactly when fdt is non-null.
> 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.
Works, but is it really an erroneous state or operation? If not, a
special value seems more appropriate than an error.
> 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.
I think we should have visit_none(), and attempting to retrieve a visits
value when you haven't visited anything should be an error.
