diff mbox

[v10,07/30] qapi: Simplify error cleanup in test-qmp-*

Message ID 1446791754-23823-8-git-send-email-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake Nov. 6, 2015, 6:35 a.m. UTC 
By moving err into data, we can let test teardown take care
of cleaning up any collected error; it also gives us fewer
lines of code between repeated tests where init runs teardown
on our behalf.

Rather than duplicate code between .c files, I added a new
test-qmp-common.h.  I debated about putting
error_free_or_abort() in error.h, but it seems like something
that is only useful for tests.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v10: split in two pieces
v9: move earlier in series (was 14/17), reword commit message
v8: no change
v7: pick up whitespace changes dropped from earlier commit
v6: new patch
---
 tests/test-qmp-commands.c      |  8 ++++----
 tests/test-qmp-common.h        | 22 ++++++++++++++++++++++
 tests/test-qmp-event.c         |  1 +
 tests/test-qmp-input-strict.c  | 22 ++++++++--------------
 tests/test-qmp-input-visitor.c | 27 ++++++++-------------------
 5 files changed, 43 insertions(+), 37 deletions(-)
 create mode 100644 tests/test-qmp-common.h

Comments

Markus Armbruster Nov. 6, 2015, 3:40 p.m. UTC | #1 
Eric Blake <eblake@redhat.com> writes:

> By moving err into data, we can let test teardown take care
> of cleaning up any collected error; it also gives us fewer
> lines of code between repeated tests where init runs teardown
> on our behalf.

I think this paragraph is no longer valid: you aren't moving err
anywhere in this version.

> Rather than duplicate code between .c files, I added a new
> test-qmp-common.h.  I debated about putting
> error_free_or_abort() in error.h, but it seems like something
> that is only useful for tests.

Maybe, maybe not.  I'd accept it into error.h.

> Signed-off-by: Eric Blake <eblake@redhat.com>

Patch looks okay.
Eric Blake Nov. 6, 2015, 3:59 p.m. UTC | #2 
On 11/06/2015 08:40 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> By moving err into data, we can let test teardown take care
>> of cleaning up any collected error; it also gives us fewer
>> lines of code between repeated tests where init runs teardown
>> on our behalf.
> 
> I think this paragraph is no longer valid: you aren't moving err
> anywhere in this version.

D'oh. I scrubbed the code, but not the commit message.  At least it was
a faithful split of the v9 version of the commit, before I reworked the
mechanism :)

How about:

We have several tests that perform multiple sub-actions that are
expected to fail.  Asserting that an error occurred, then clearing it up
to prepare for the next action, turned into enough boilerplate that it
was sometimes forgotten, risking memory leak or invalidating the efforts
of the second action (passing a non-NULL err into a function is
generally a bad idea).  Encapsulate the boilerplate into a single helper
function error_free_or_abort(), and consistently use it.

> 
>> Rather than duplicate code between .c files, I added a new
>> test-qmp-common.h.  I debated about putting
>> error_free_or_abort() in error.h, but it seems like something
>> that is only useful for tests.
> 
> Maybe, maybe not.  I'd accept it into error.h.

Do you want me to post a fixup patch that relocates it into error.h?

> 
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Patch looks okay.
>
Markus Armbruster Nov. 6, 2015, 4:23 p.m. UTC | #3 
Eric Blake <eblake@redhat.com> writes:

> On 11/06/2015 08:40 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> By moving err into data, we can let test teardown take care
>>> of cleaning up any collected error; it also gives us fewer
>>> lines of code between repeated tests where init runs teardown
>>> on our behalf.
>> 
>> I think this paragraph is no longer valid: you aren't moving err
>> anywhere in this version.
>
> D'oh. I scrubbed the code, but not the commit message.  At least it was
> a faithful split of the v9 version of the commit, before I reworked the
> mechanism :)
>
> How about:
>
> We have several tests that perform multiple sub-actions that are
> expected to fail.  Asserting that an error occurred, then clearing it up
> to prepare for the next action, turned into enough boilerplate that it
> was sometimes forgotten,

If you got suitable commit SHAs handy, lets insert some here.

>                          risking memory leak or invalidating the efforts
> of the second action (passing a non-NULL err into a function is
> generally a bad idea).  Encapsulate the boilerplate into a single helper
> function error_free_or_abort(), and consistently use it.

Works for me.

>>> Rather than duplicate code between .c files, I added a new
>>> test-qmp-common.h.  I debated about putting
>>> error_free_or_abort() in error.h, but it seems like something
>>> that is only useful for tests.
>> 
>> Maybe, maybe not.  I'd accept it into error.h.
>
> Do you want me to post a fixup patch that relocates it into error.h?

I guess the result would be slightly simpler, so go ahead.

>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> 
>> Patch looks okay.
Eric Blake Nov. 6, 2015, 4:32 p.m. UTC | #4 
On 11/06/2015 09:23 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 11/06/2015 08:40 AM, Markus Armbruster wrote:
>>> Eric Blake <eblake@redhat.com> writes:
>>>
>>>> By moving err into data, we can let test teardown take care
>>>> of cleaning up any collected error; it also gives us fewer
>>>> lines of code between repeated tests where init runs teardown
>>>> on our behalf.
>>>
>>> I think this paragraph is no longer valid: you aren't moving err
>>> anywhere in this version.
>>
>> D'oh. I scrubbed the code, but not the commit message.  At least it was
>> a faithful split of the v9 version of the commit, before I reworked the
>> mechanism :)
>>
>> How about:
>>
>> We have several tests that perform multiple sub-actions that are
>> expected to fail.  Asserting that an error occurred, then clearing it up
>> to prepare for the next action, turned into enough boilerplate that it
>> was sometimes forgotten,
> 
> If you got suitable commit SHAs handy, lets insert some here.

A number of them were introduced in d88f5fd (look at
test-qmp-input-visitor.c:test_validate_fail_struct_nested(), for
example), and only barely cleaned up in 5/30 of this series.  I'm not
finding other commits off-hand, though.

> 
>>                          risking memory leak or invalidating the efforts
>> of the second action (passing a non-NULL err into a function is
>> generally a bad idea).  Encapsulate the boilerplate into a single helper
>> function error_free_or_abort(), and consistently use it.
> 
> Works for me.
> 
>>>> Rather than duplicate code between .c files, I added a new
>>>> test-qmp-common.h.  I debated about putting
>>>> error_free_or_abort() in error.h, but it seems like something
>>>> that is only useful for tests.
>>>
>>> Maybe, maybe not.  I'd accept it into error.h.
>>
>> Do you want me to post a fixup patch that relocates it into error.h?
> 
> I guess the result would be slightly simpler, so go ahead.

Coming up soon.
diff mbox

Patch

diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index f23d8ea..9f65fc2 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -1,12 +1,13 @@ 
 #include <glib.h>
 #include "qemu-common.h"
+#include "test-qmp-common.h"
 #include "qapi/qmp/types.h"
 #include "test-qmp-commands.h"
 #include "qapi/qmp/dispatch.h"
 #include "qemu/module.h"
 #include "qapi/qmp-input-visitor.h"
-#include "tests/test-qapi-types.h"
-#include "tests/test-qapi-visit.h"
+#include "test-qapi-types.h"
+#include "test-qapi-visit.h"

 void qmp_user_def_cmd(Error **errp)
 {
@@ -225,8 +226,7 @@  static void test_dealloc_partial(void)
     assert(ud2->dict1 == NULL);

     /* confirm & release construction error */
-    assert(err != NULL);
-    error_free(err);
+    error_free_or_abort(&err);

     /* tear down partial object */
     qapi_free_UserDefTwo(ud2);
diff --git a/tests/test-qmp-common.h b/tests/test-qmp-common.h
new file mode 100644
index 0000000..043f49c
--- /dev/null
+++ b/tests/test-qmp-common.h
@@ -0,0 +1,22 @@ 
+/*
+ * Code common to qmp/qapi unit-tests.
+ *
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef TEST_QMP_COMMON_H__
+#define TEST_QMP_COMMON_H__
+
+/* Expect an error, abort() if there is none. */
+static inline void error_free_or_abort(Error **errp)
+{
+    g_assert(*errp);
+    error_free(*errp);
+    *errp = NULL;
+}
+
+#endif
diff --git a/tests/test-qmp-event.c b/tests/test-qmp-event.c
index 035c65c..c0ac3ea 100644
--- a/tests/test-qmp-event.c
+++ b/tests/test-qmp-event.c
@@ -22,6 +22,7 @@ 
 #include "qapi/qmp/qint.h"
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp-event.h"
+#include "test-qmp-common.h"

 typedef struct TestEventData {
     QDict *expect;
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index 6226397..7e15b09 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -15,6 +15,7 @@ 
 #include <stdarg.h>

 #include "qemu-common.h"
+#include "test-qmp-common.h"
 #include "qapi/qmp-input-visitor.h"
 #include "test-qapi-types.h"
 #include "test-qapi-visit.h"
@@ -180,8 +181,7 @@  static void test_validate_fail_struct(TestInputVisitorData *data,
     v = validate_test_init(data, "{ 'integer': -42, 'boolean': true, 'string': 'foo', 'extra': 42 }");

     visit_type_TestStruct(v, &p, NULL, &err);
-    g_assert(err);
-    error_free(err);
+    error_free_or_abort(&err);
     if (p) {
         g_free(p->string);
     }
@@ -198,8 +198,7 @@  static void test_validate_fail_struct_nested(TestInputVisitorData *data,
     v = validate_test_init(data, "{ 'string0': 'string0', 'dict1': { 'string1': 'string1', 'dict2': { 'userdef1': { 'integer': 42, 'string': 'string', 'extra': [42, 23, {'foo':'bar'}] }, 'string2': 'string2'}}}");

     visit_type_UserDefTwo(v, &udp, NULL, &err);
-    g_assert(err);
-    error_free(err);
+    error_free_or_abort(&err);
     qapi_free_UserDefTwo(udp);
 }

@@ -213,8 +212,7 @@  static void test_validate_fail_list(TestInputVisitorData *data,
     v = validate_test_init(data, "[ { 'string': 'string0', 'integer': 42 }, { 'string': 'string1', 'integer': 43 }, { 'string': 'string2', 'integer': 44, 'extra': 'ggg' } ]");

     visit_type_UserDefOneList(v, &head, NULL, &err);
-    g_assert(err);
-    error_free(err);
+    error_free_or_abort(&err);
     qapi_free_UserDefOneList(head);
 }

@@ -229,8 +227,7 @@  static void test_validate_fail_union_native_list(TestInputVisitorData *data,
                            "{ 'type': 'integer', 'data' : [ 'string' ] }");

     visit_type_UserDefNativeListUnion(v, &tmp, NULL, &err);
-    g_assert(err);
-    error_free(err);
+    error_free_or_abort(&err);
     qapi_free_UserDefNativeListUnion(tmp);
 }

@@ -244,8 +241,7 @@  static void test_validate_fail_union_flat(TestInputVisitorData *data,
     v = validate_test_init(data, "{ 'string': 'c', 'integer': 41, 'boolean': true }");

     visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
-    g_assert(err);
-    error_free(err);
+    error_free_or_abort(&err);
     qapi_free_UserDefFlatUnion(tmp);
 }

@@ -260,8 +256,7 @@  static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data,
     v = validate_test_init(data, "{ 'integer': 42, 'string': 'c', 'string1': 'd', 'string2': 'e' }");

     visit_type_UserDefFlatUnion2(v, &tmp, NULL, &err);
-    g_assert(err);
-    error_free(err);
+    error_free_or_abort(&err);
     qapi_free_UserDefFlatUnion2(tmp);
 }

@@ -275,8 +270,7 @@  static void test_validate_fail_alternate(TestInputVisitorData *data,
     v = validate_test_init(data, "3.14");

     visit_type_UserDefAlternate(v, &tmp, NULL, &err);
-    g_assert(err);
-    error_free(err);
+    error_free_or_abort(&err);
     qapi_free_UserDefAlternate(tmp);
 }

diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 46566bc..2b59b6f 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -14,6 +14,7 @@ 
 #include <stdarg.h>

 #include "qemu-common.h"
+#include "test-qmp-common.h"
 #include "qapi/qmp-input-visitor.h"
 #include "test-qapi-types.h"
 #include "test-qapi-visit.h"
@@ -111,8 +112,7 @@  static void test_visitor_in_int_overflow(TestInputVisitorData *data,
     v = visitor_input_test_init(data, "%f", DBL_MAX);

     visit_type_int(v, &res, NULL, &err);
-    g_assert(err);
-    error_free(err);
+    error_free_or_abort(&err);
 }

 static void test_visitor_in_bool(TestInputVisitorData *data,
@@ -319,9 +319,7 @@  static void test_visitor_in_alternate(TestInputVisitorData *data,

     v = visitor_input_test_init(data, "false");
     visit_type_UserDefAlternate(v, &tmp, NULL, &err);
-    g_assert(err);
-    error_free(err);
-    err = NULL;
+    error_free_or_abort(&err);
     qapi_free_UserDefAlternate(tmp);
 }

@@ -341,9 +339,7 @@  static void test_visitor_in_alternate_number(TestInputVisitorData *data,

     v = visitor_input_test_init(data, "42");
     visit_type_AltStrBool(v, &asb, NULL, &err);
-    g_assert(err);
-    error_free(err);
-    err = NULL;
+    error_free_or_abort(&err);
     qapi_free_AltStrBool(asb);

     /* FIXME: Order of alternate should not affect semantics; asn should
@@ -352,9 +348,7 @@  static void test_visitor_in_alternate_number(TestInputVisitorData *data,
     visit_type_AltStrNum(v, &asn, NULL, &err);
     /* FIXME g_assert_cmpint(asn->type, == ALT_STR_NUM_KIND_N); */
     /* FIXME g_assert_cmpfloat(asn->u.n, ==, 42); */
-    g_assert(err);
-    error_free(err);
-    err = NULL;
+    error_free_or_abort(&err);
     qapi_free_AltStrNum(asn);

     v = visitor_input_test_init(data, "42");
@@ -385,9 +379,7 @@  static void test_visitor_in_alternate_number(TestInputVisitorData *data,

     v = visitor_input_test_init(data, "42.5");
     visit_type_AltStrBool(v, &asb, NULL, &err);
-    g_assert(err);
-    error_free(err);
-    err = NULL;
+    error_free_or_abort(&err);
     qapi_free_AltStrBool(asb);

     v = visitor_input_test_init(data, "42.5");
@@ -404,9 +396,7 @@  static void test_visitor_in_alternate_number(TestInputVisitorData *data,

     v = visitor_input_test_init(data, "42.5");
     visit_type_AltStrInt(v, &asi, NULL, &err);
-    g_assert(err);
-    error_free(err);
-    err = NULL;
+    error_free_or_abort(&err);
     qapi_free_AltStrInt(asi);

     v = visitor_input_test_init(data, "42.5");
@@ -713,12 +703,11 @@  static void test_visitor_in_errors(TestInputVisitorData *data,
     v = visitor_input_test_init(data, "{ 'integer': false, 'boolean': 'foo', 'string': -42 }");

     visit_type_TestStruct(v, &p, NULL, &err);
-    g_assert(err);
+    error_free_or_abort(&err);
     /* FIXME - a failed parse should not leave a partially-allocated p
      * for us to clean up; this could cause callers to leak memory. */
     g_assert(p->string == NULL);

-    error_free(err);
     g_free(p->string);
     g_free(p);
 }