Eric Blake <ebl...@redhat.com> 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 <arm...@redhat.com> >> --- > >> >> 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.