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

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>> ---
>>  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 <address@hidden>

Thanks for your quick review!

>> +        if (!err) {
>> +            visit_end_struct(v, &err);
>> +        }
>> +        error_propagate(errp, err);
>>          return;
>>      }
>>  
>>