Re: [Qemu-devel] [PATCH v2 23/26] qapi: Make input visitors detect unvisited list tails

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/26/2017 03:43 PM, Markus Armbruster wrote:
>> Fix the design flaw demonstrated in the previous commit: new method
>> check_list() lets input visitors report that unvisited input remains
>> for a list, exactly like check_struct() lets them report that
>> unvisited input remains for a struct or union.
>> 
>> Implement the method for the qobject input visitor (straightforward),
>> and the string input visitor (less so, due to the magic list syntax
>> there).  The opts visitor's list magic is even more impenetrable, and
>> all I can do there today is a stub with a FIXME comment.  No worse
>> than before.
>
> Yeah, I know what you mean (having worked on all three visitors in prior
> patches).  The opts visitor is just painful.
>
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>
>> 
>> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
>> index 2de6377..fe7b988 100644
>> --- a/hw/ppc/spapr_drc.c
>> +++ b/hw/ppc/spapr_drc.c
>> @@ -326,6 +326,11 @@ static void prop_get_fdt(Object *obj, Visitor *v, const 
>> char *name,
>>                      return;
>>                  }
>>              }
>> +            visit_check_list(v, &err);
>> +            if (err) {
>> +                error_propagate(errp, err);
>> +                return;
>> +            }
>>              visit_end_list(v, NULL);
>
> This doesn't look right.  You want the visit_end_list() call to occur
> unconditionally, even when visit_check_list() fails, so as to free any
> resources allocated by the visitor.

Will fix.

>> +++ b/tests/test-string-input-visitor.c
>> @@ -160,17 +160,9 @@ static void 
>> test_visitor_in_intList(TestInputVisitorData *data,
>>      /* Would be simpler if the visitor genuinely supported virtual walks */
>>      visit_start_list(v, NULL, (GenericList **)&res, sizeof(*res),
>>                       &error_abort);
>> -    tail = res;
>> -    visit_type_int64(v, NULL, &tail->value, &error_abort);
>> -    g_assert_cmpint(tail->value, ==, 0);
>> -    tail = (int64List *)visit_next_list(v, (GenericList *)tail, 
>> sizeof(*res));
>> -    g_assert(tail);
>> -    visit_type_int64(v, NULL, &tail->value, &error_abort);
>> -    g_assert_cmpint(tail->value, ==, 2);
>> -    tail = (int64List *)visit_next_list(v, (GenericList *)tail, 
>> sizeof(*res));
>> -    g_assert(tail);
>> +    visit_check_list(v, &err);
>
> You are still calling visit_check_list() after a partial visit, but why
> change from a 2/3 visit to a 0/3 visit?

Suspect it's just an editing accident.  I'll add it back.