| Message ID | 1400598479-27118-1-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
Quoting Marcel Apfelbaum (2014-05-20 10:07:59) > Checks the output visitor behaviour for NULL values. > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > --- > Notes: > - I didn't add Michael's Sob because I tweaked the test a little. Tweaked it so it didn't crash 100% of the time even after your fix? :) Another approach, since the expected behavior now is for qmp_output_get_qobject() to return NULL in this case, is to just assert(arg == NULL) and drop the qdict/QDECREF completely. > - To be added on top of "qapi: output visitor crashes qemu if it encounters a NULL value", > otherwise the test will fail. > > tests/test-qmp-output-visitor.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c > index 9c15458..de1bf83 100644 > --- a/tests/test-qmp-output-visitor.c > +++ b/tests/test-qmp-output-visitor.c > @@ -507,6 +507,19 @@ static void test_visitor_out_union_anon(TestOutputVisitorData *data, > qapi_free_UserDefAnonUnion(tmp); > } > > +static void test_visitor_out_empty(TestOutputVisitorData *data, > + const void *unused) > +{ > + QObject *arg; > + QDict *qdict; > + > + arg = qmp_output_get_qobject(data->qov); > + if (arg) { > + qdict = qobject_to_qdict(arg); > + QDECREF(qdict); > + } > +} > + > static void init_native_list(UserDefNativeListUnion *cvalue) > { > int i; > @@ -859,6 +872,8 @@ int main(int argc, char **argv) > &out_visitor_data, test_visitor_out_union_flat); > output_visitor_test_add("/visitor/output/union-anon", > &out_visitor_data, test_visitor_out_union_anon); > + output_visitor_test_add("/visitor/output/empty", > + &out_visitor_data, test_visitor_out_empty); > output_visitor_test_add("/visitor/output/native_list/int", > &out_visitor_data, test_visitor_out_native_list_int); > output_visitor_test_add("/visitor/output/native_list/int8", > -- > 1.8.3.1
On Tue, May 20, 2014 at 07:19:49PM -0500, Michael Roth wrote: > Quoting Marcel Apfelbaum (2014-05-20 10:07:59) > > Checks the output visitor behaviour for NULL values. > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > --- > > Notes: > > - I didn't add Michael's Sob because I tweaked the test a little. > > Tweaked it so it didn't crash 100% of the time even after your fix? :) > > Another approach, since the expected behavior now is for > qmp_output_get_qobject() to return NULL in this case, is to just > assert(arg == NULL) and drop the qdict/QDECREF completely. Agree. We expecte arg is NULL, but this patch will still success when arg isn't NULL. Can we add the empty test after g_test_init()? We can make sure the out_visitor_data is really empty. Amos. > > - To be added on top of "qapi: output visitor crashes qemu if it encounters a NULL value", > > otherwise the test will fail. > > > > tests/test-qmp-output-visitor.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c > > index 9c15458..de1bf83 100644 > > --- a/tests/test-qmp-output-visitor.c > > +++ b/tests/test-qmp-output-visitor.c > > @@ -507,6 +507,19 @@ static void test_visitor_out_union_anon(TestOutputVisitorData *data, > > qapi_free_UserDefAnonUnion(tmp); > > } > > > > +static void test_visitor_out_empty(TestOutputVisitorData *data, > > + const void *unused) > > +{ > > + QObject *arg; > > + QDict *qdict; > > + > > + arg = qmp_output_get_qobject(data->qov); > > + if (arg) { > > + qdict = qobject_to_qdict(arg); > > + QDECREF(qdict); > > + } > > +} > > + > > static void init_native_list(UserDefNativeListUnion *cvalue) > > { > > int i; > > @@ -859,6 +872,8 @@ int main(int argc, char **argv) > > &out_visitor_data, test_visitor_out_union_flat); > > output_visitor_test_add("/visitor/output/union-anon", > > &out_visitor_data, test_visitor_out_union_anon); > > + output_visitor_test_add("/visitor/output/empty", > > + &out_visitor_data, test_visitor_out_empty); > > output_visitor_test_add("/visitor/output/native_list/int", > > &out_visitor_data, test_visitor_out_native_list_int); > > output_visitor_test_add("/visitor/output/native_list/int8", > > -- > > 1.8.3.1 >
On Tue, 2014-05-27 at 09:53 +0800, Amos Kong wrote: > On Tue, May 20, 2014 at 07:19:49PM -0500, Michael Roth wrote: > > Quoting Marcel Apfelbaum (2014-05-20 10:07:59) > > > Checks the output visitor behaviour for NULL values. > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > --- > > > Notes: > > > - I didn't add Michael's Sob because I tweaked the test a little. > > > > Tweaked it so it didn't crash 100% of the time even after your fix? :) > > > > Another approach, since the expected behavior now is for > > qmp_output_get_qobject() to return NULL in this case, is to just > > assert(arg == NULL) and drop the qdict/QDECREF completely. > > Agree. > > We expecte arg is NULL, but this patch will still success when > arg isn't NULL. Hi Amos, Thank you for your review. I already tweaked it and added it to my latest series. However, my previous concept was to check that it doesn't crash, and not to care if the result is NULL, or a "Null Object" pattern. But since now it will always return NULL, there is no reason not to assert it. > > Can we add the empty test after g_test_init()? We can make sure > the out_visitor_data is really empty. I checked it and it is empty :), If it is really important for you, I'll re-spin only this patch. Thanks again, Marcel > > Amos. > > > > - To be added on top of "qapi: output visitor crashes qemu if it encounters a NULL value", > > > otherwise the test will fail. > > > > > > tests/test-qmp-output-visitor.c | 15 +++++++++++++++ > > > 1 file changed, 15 insertions(+) > > > > > > diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c > > > index 9c15458..de1bf83 100644 > > > --- a/tests/test-qmp-output-visitor.c > > > +++ b/tests/test-qmp-output-visitor.c > > > @@ -507,6 +507,19 @@ static void test_visitor_out_union_anon(TestOutputVisitorData *data, > > > qapi_free_UserDefAnonUnion(tmp); > > > } > > > > > > +static void test_visitor_out_empty(TestOutputVisitorData *data, > > > + const void *unused) > > > +{ > > > + QObject *arg; > > > + QDict *qdict; > > > + > > > + arg = qmp_output_get_qobject(data->qov); > > > + if (arg) { > > > + qdict = qobject_to_qdict(arg); > > > + QDECREF(qdict); > > > + } > > > +} > > > + > > > static void init_native_list(UserDefNativeListUnion *cvalue) > > > { > > > int i; > > > @@ -859,6 +872,8 @@ int main(int argc, char **argv) > > > &out_visitor_data, test_visitor_out_union_flat); > > > output_visitor_test_add("/visitor/output/union-anon", > > > &out_visitor_data, test_visitor_out_union_anon); > > > + output_visitor_test_add("/visitor/output/empty", > > > + &out_visitor_data, test_visitor_out_empty); > > > output_visitor_test_add("/visitor/output/native_list/int", > > > &out_visitor_data, test_visitor_out_native_list_int); > > > output_visitor_test_add("/visitor/output/native_list/int8", > > > -- > > > 1.8.3.1 > > >
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index 9c15458..de1bf83 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -507,6 +507,19 @@ static void test_visitor_out_union_anon(TestOutputVisitorData *data, qapi_free_UserDefAnonUnion(tmp); } +static void test_visitor_out_empty(TestOutputVisitorData *data, + const void *unused) +{ + QObject *arg; + QDict *qdict; + + arg = qmp_output_get_qobject(data->qov); + if (arg) { + qdict = qobject_to_qdict(arg); + QDECREF(qdict); + } +} + static void init_native_list(UserDefNativeListUnion *cvalue) { int i; @@ -859,6 +872,8 @@ int main(int argc, char **argv) &out_visitor_data, test_visitor_out_union_flat); output_visitor_test_add("/visitor/output/union-anon", &out_visitor_data, test_visitor_out_union_anon); + output_visitor_test_add("/visitor/output/empty", + &out_visitor_data, test_visitor_out_empty); output_visitor_test_add("/visitor/output/native_list/int", &out_visitor_data, test_visitor_out_native_list_int); output_visitor_test_add("/visitor/output/native_list/int8",
Checks the output visitor behaviour for NULL values. Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> --- Notes: - I didn't add Michael's Sob because I tweaked the test a little. - To be added on top of "qapi: output visitor crashes qemu if it encounters a NULL value", otherwise the test will fail. tests/test-qmp-output-visitor.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)