Eric Blake <ebl...@redhat.com> writes:
> On 12/03/2015 09:37 AM, 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
>
> Something got lost or otherwise corrupted in that sentence. Were you
> trying to say one function happily calls another? If so, which of the
> two "object_property_get_qobject()" strings should be changed, to what?
No idea what happened. Correction: insert "calls" after "happily":
prop_get_fdt() misuses the visitor API: when fdt is null, it doesn't
visit anything. object_property_get_qobject() happily calls
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.
>> 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(+)
>>
>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>> index 4e7a1d3..dad157f 100644
>> --- a/hw/ppc/spapr_drc.c
>> +++ b/hw/ppc/spapr_drc.c
>> @@ -259,6 +259,11 @@ static void prop_get_fdt(Object *obj, Visitor *v, void
>> *opaque,
>> void *fdt;
>>
>> if (!drc->fdt) {
>> + visit_start_struct(v, NULL, NULL, name, 0, &err);
>
> visit_start_struct(v, NULL, "fdt", name, 0, &err)
>
> would give a nicer error message, but only if there were an error
> message to give. As already mentioned on 1/3, we are only using this
> for output visitors which don't raise errors (in fact, &error_abort
> would do, rather than worrying about error propagation).
>
> But I'm fine with the patch as you proposed it, once the commit message
> is fixed.
Some callers pass a third argument, some don't. We'll have to audit all
visitor users anyway. We can decide whether and what to do about this
issue then.
> Reviewed-by: Eric Blake <ebl...@redhat.com>
Thanks for your quick review!
>> + if (!err) {
>> + visit_end_struct(v, &err);
>> + }
>> + error_propagate(errp, err);
>> return;
>> }
>>
>>
