| Message ID | 32b6baf4-cd23-c277-4dfb-79922c710efe@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > On 10/21/2016 04:32 PM, Eric Blake wrote: >> In fact, applying this patch regresses to the very state that f24582d >> tried to prevent. However, I'm unable to see a difference in valgrind >> on tests/test-qmp-output-visitor either with or without this patch, >> which sadly means our testsuite is not actually testing this scenario. >> >>>> Should this go into -stable? >> >> NACK. >> >> As mentioned in the v1 thread, the leak that Pino was seeing is fixed by >> http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg04023.html >> I don't think we don't want this patch. > > And below is my hack to test-qmp-output-visitor, which shows (at least > under valgrind) that we don't want the patch. I should probably turn > that into a formal patch submission; but should I piggyback the existing > test or add a new one? With the patch omitted, valgrind is happy; with > the patch applied, I see: > > /visitor/output/struct-errors: ==8458== Invalid read of size 8 > ==8458== at 0x175DC2: qobject_decref (qobject.h:81) > ==8458== by 0x1767A5: qentry_destroy (qdict.c:413) > ==8458== by 0x17690B: qdict_destroy_obj (qdict.c:451) > ==8458== by 0x1784AB: qobject_destroy (qobject.c:29) > ==8458== by 0x171927: qobject_decref (qobject.h:83) > ==8458== by 0x1721B4: qmp_output_free (qmp-output-visitor.c:223) > ==8458== by 0x16ED8E: visit_free (qapi-visit-core.c:34) > ==8458== by 0x130F39: visitor_output_teardown > (test-qmp-output-visitor.c:44) > ==8458== by 0x130FE1: visitor_reset (test-qmp-output-visitor.c:59) > ==8458== by 0x1326EF: test_visitor_out_struct_errors > (test-qmp-output-visitor.c:283) [...] > > > diff --git i/tests/test-qmp-output-visitor.c > w/tests/test-qmp-output-visitor.c > index 5e926d3..f1b5591 100644 > --- i/tests/test-qmp-output-visitor.c > +++ w/tests/test-qmp-output-visitor.c > @@ -265,18 +265,22 @@ static void > test_visitor_out_struct_errors(TestOutputVisitorData *data, > EnumOne bad_values[] = { ENUM_ONE__MAX, -1 }; > UserDefOne u = {0}; > UserDefOne *pu = &u; > - Error *err; > + Error *err = NULL; > int i; > > + /* First check: error within struct is detected */ > for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) { > - err = NULL; > u.has_enum1 = true; > u.enum1 = bad_values[i]; > visit_type_UserDefOne(data->ov, "unused", &pu, &err); > - g_assert(err); > - error_free(err); > + error_free_or_abort(&err); > visitor_reset(data); > } Just cleanup so far. > + > + /* Second check: aborting visit early doesn't leak */ > + visit_start_struct(data->ov, NULL, NULL, 0, &error_abort); > + visit_start_struct(data->ov, "nested", NULL, 0, &error_abort); > + visitor_reset(data); > } Since this isn't about errors, adding it to test_visitor_out_struct_errors() is perhaps questionable. Use your judgement.
diff --git i/tests/test-qmp-output-visitor.c w/tests/test-qmp-output-visitor.c index 5e926d3..f1b5591 100644 --- i/tests/test-qmp-output-visitor.c +++ w/tests/test-qmp-output-visitor.c @@ -265,18 +265,22 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data, EnumOne bad_values[] = { ENUM_ONE__MAX, -1 }; UserDefOne u = {0}; UserDefOne *pu = &u; - Error *err; + Error *err = NULL; int i; + /* First check: error within struct is detected */ for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) { - err = NULL; u.has_enum1 = true; u.enum1 = bad_values[i]; visit_type_UserDefOne(data->ov, "unused", &pu, &err); - g_assert(err); - error_free(err); + error_free_or_abort(&err); visitor_reset(data); } + + /* Second check: aborting visit early doesn't leak */ + visit_start_struct(data->ov, NULL, NULL, 0, &error_abort); + visit_start_struct(data->ov, "nested", NULL, 0, &error_abort); + visitor_reset(data); }