| Message ID | 1477323383-12380-3-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Add a test that proves (at least when run under valgrind) that > we are correctly handling allocated memory even when a visit > is aborted in the middle for whatever other reason. > > See commit f24582d "qapi: fix double free in > qmp_output_visitor_cleanup()" for a fix that was lacking > testsuite exposure prior to this patch. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tests/test-qmp-output-visitor.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c > index f7213d2..2368171 100644 > --- a/tests/test-qmp-output-visitor.c > +++ b/tests/test-qmp-output-visitor.c > @@ -254,6 +254,16 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data, > } > > > +static void test_visitor_out_partial_visit(TestOutputVisitorData *data, > + const void *unused) > +{ > + /* Check that aborting mid-visit doesn't leak or double-free */ > + visit_start_struct(data->ov, NULL, NULL, 0, &error_abort); > + visit_start_struct(data->ov, "nested", NULL, 0, &error_abort); > + visitor_reset(data); Add a second test that additionally visits a member? Or is visiting the "nested" member enough? > +} > + > + > static void test_visitor_out_list(TestOutputVisitorData *data, > const void *unused) > { > @@ -817,6 +827,8 @@ int main(int argc, char **argv) > &out_visitor_data, test_visitor_out_struct_nested); > output_visitor_test_add("/visitor/output/struct-errors", > &out_visitor_data, test_visitor_out_struct_errors); > + output_visitor_test_add("/visitor/output/partial-visit", > + &out_visitor_data, test_visitor_out_partial_visit); > output_visitor_test_add("/visitor/output/list", > &out_visitor_data, test_visitor_out_list); > output_visitor_test_add("/visitor/output/any", Would you be willing to do the same for lists and alternates? Speaking of alternates, visitor.h's virtual walk example covers struct and list, but not alternate.
On 10/25/2016 06:07 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Add a test that proves (at least when run under valgrind) that >> we are correctly handling allocated memory even when a visit >> is aborted in the middle for whatever other reason. >> >> See commit f24582d "qapi: fix double free in >> qmp_output_visitor_cleanup()" for a fix that was lacking >> testsuite exposure prior to this patch. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> +static void test_visitor_out_partial_visit(TestOutputVisitorData *data, >> + const void *unused) >> +{ >> + /* Check that aborting mid-visit doesn't leak or double-free */ >> + visit_start_struct(data->ov, NULL, NULL, 0, &error_abort); >> + visit_start_struct(data->ov, "nested", NULL, 0, &error_abort); >> + visitor_reset(data); > > Add a second test that additionally visits a member? Or is visiting the > "nested" member enough? Visiting "nested" was enough to trigger the particular valgrind trace I wanted, but more coverage never hurts. I'll send v2... > Would you be willing to do the same for lists and alternates? ...and it will also cover these. > > Speaking of alternates, visitor.h's virtual walk example covers struct > and list, but not alternate. Sounds like my v2 will be a longer series :)
On 10/25/2016 08:41 AM, Eric Blake wrote: >>> +static void test_visitor_out_partial_visit(TestOutputVisitorData *data, >>> + const void *unused) >>> +{ >>> + /* Check that aborting mid-visit doesn't leak or double-free */ >>> + visit_start_struct(data->ov, NULL, NULL, 0, &error_abort); >>> + visit_start_struct(data->ov, "nested", NULL, 0, &error_abort); >>> + visitor_reset(data); >> >> Add a second test that additionally visits a member? Or is visiting the >> "nested" member enough? > > Visiting "nested" was enough to trigger the particular valgrind trace I > wanted, but more coverage never hurts. I'll send v2... > >> Would you be willing to do the same for lists and alternates? > > ...and it will also cover these. Aborting in the middle of an alternate is trickier, since the current code for alternates does not allow a non-NULL *obj to visit_start_alternate() (that is, no virtual alternate walk is needed; to output an alternate, just call directly to the branch of interest, without a need to wrap things in visit_start/end_alternate; and on input, an alternate is currently useful only into a QAPI C struct). > >> >> Speaking of alternates, visitor.h's virtual walk example covers struct >> and list, but not alternate. > > Sounds like my v2 will be a longer series :) I'm still trying to see whether tweaking the docs to cover alternates makes much sense; we'll see what the patch looks like, but may decide it was not worth it.
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index f7213d2..2368171 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -254,6 +254,16 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data, } +static void test_visitor_out_partial_visit(TestOutputVisitorData *data, + const void *unused) +{ + /* Check that aborting mid-visit doesn't leak or double-free */ + visit_start_struct(data->ov, NULL, NULL, 0, &error_abort); + visit_start_struct(data->ov, "nested", NULL, 0, &error_abort); + visitor_reset(data); +} + + static void test_visitor_out_list(TestOutputVisitorData *data, const void *unused) { @@ -817,6 +827,8 @@ int main(int argc, char **argv) &out_visitor_data, test_visitor_out_struct_nested); output_visitor_test_add("/visitor/output/struct-errors", &out_visitor_data, test_visitor_out_struct_errors); + output_visitor_test_add("/visitor/output/partial-visit", + &out_visitor_data, test_visitor_out_partial_visit); output_visitor_test_add("/visitor/output/list", &out_visitor_data, test_visitor_out_list); output_visitor_test_add("/visitor/output/any",
Add a test that proves (at least when run under valgrind) that we are correctly handling allocated memory even when a visit is aborted in the middle for whatever other reason. See commit f24582d "qapi: fix double free in qmp_output_visitor_cleanup()" for a fix that was lacking testsuite exposure prior to this patch. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/test-qmp-output-visitor.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)