[PATCH 0/3] tests/unit/test-qmp-event: Unconfuse Coverity & simplify

[PATCH 0/3] tests/unit/test-qmp-event: Unconfuse Coverity & simplify

[Markus Armbruster]

Markus Armbruster (3):
  tests/unit/test-qmp-event: Drop superfluous mutex
  tests/unit/test-qmp-event: Simplify event emission check
  tests/unit/test-qmp-event: Replace fixture by global variables

 tests/unit/test-qmp-event.c | 108 +++++++++++-------------------------
 1 file changed, 31 insertions(+), 77 deletions(-)

-- 
2.41.0

[PATCH 1/3] tests/unit/test-qmp-event: Drop superfluous mutex

[Markus Armbruster]

Mutex @test_event_lock is held from fixture setup to teardown,
protecting global variable @test_event_data.  But tests always run one
after the other, so this is superfluous.  It also confuses Coverity.
Drop the mutex.

Fixes: CID 1527425
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/unit/test-qmp-event.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/tests/unit/test-qmp-event.c b/tests/unit/test-qmp-event.c
index 3626d2372f..c2c44687d5 100644
--- a/tests/unit/test-qmp-event.c
+++ b/tests/unit/test-qmp-event.c
@@ -30,7 +30,6 @@ typedef struct TestEventData {
 } TestEventData;
 
 TestEventData *test_event_data;
-static GMutex test_event_lock;
 
 void test_qapi_event_emit(test_QAPIEvent event, QDict *d)
 {
@@ -59,9 +58,6 @@ void test_qapi_event_emit(test_QAPIEvent event, QDict *d)
 static void event_prepare(TestEventData *data,
                           const void *unused)
 {
-    /* Global variable test_event_data was used to pass the expectation, so
-       test cases can't be executed at same time. */
-    g_mutex_lock(&test_event_lock);
     test_event_data = data;
 }
 
@@ -69,7 +65,6 @@ static void event_teardown(TestEventData *data,
                            const void *unused)
 {
     test_event_data = NULL;
-    g_mutex_unlock(&test_event_lock);
 }
 
 static void event_test_add(const char *testpath,
-- 
2.41.0

Re: [PATCH 1/3] tests/unit/test-qmp-event: Drop superfluous mutex

[Thomas Huth]

On 22/11/2023 08.24, Markus Armbruster wrote:
> Mutex @test_event_lock is held from fixture setup to teardown,
> protecting global variable @test_event_data.  But tests always run one
> after the other, so this is superfluous.  It also confuses Coverity.
> Drop the mutex.
> 
> Fixes: CID 1527425
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/unit/test-qmp-event.c | 5 -----
>   1 file changed, 5 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 2/3] tests/unit/test-qmp-event: Simplify event emission check

[Markus Armbruster]

The generated qapi_event_send_FOO() call an event emitter function.
It's test_qapi_event_emit() in this test.  It compares the actual
event to the expected event, and sets a flag to record it was called.
The test functions set expected data and clear the flag before calling
qapi_event_send_FOO(), and check the flag afterwards.

Make test_qapi_event_emit() consume expected data, and the test
functions check it was consumed.  Delete the flag.  This is simpler.
It also catches extraneous calls of test_qapi_event_emit().  Catching
that is not worthwhile, but since the cost is negative...

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/unit/test-qmp-event.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/tests/unit/test-qmp-event.c b/tests/unit/test-qmp-event.c
index c2c44687d5..5c9837e849 100644
--- a/tests/unit/test-qmp-event.c
+++ b/tests/unit/test-qmp-event.c
@@ -26,7 +26,6 @@
 
 typedef struct TestEventData {
     QDict *expect;
-    bool emitted;
 } TestEventData;
 
 TestEventData *test_event_data;
@@ -36,6 +35,8 @@ void test_qapi_event_emit(test_QAPIEvent event, QDict *d)
     QDict *t;
     int64_t s, ms;
 
+    g_assert(test_event_data->expect);
+
     /* Verify that we have timestamp, then remove it to compare other fields */
     t = qdict_get_qdict(d, "timestamp");
     g_assert(t);
@@ -52,7 +53,8 @@ void test_qapi_event_emit(test_QAPIEvent event, QDict *d)
     qdict_del(d, "timestamp");
 
     g_assert(qobject_is_equal(QOBJECT(d), QOBJECT(test_event_data->expect)));
-    test_event_data->emitted = true;
+    qobject_unref(test_event_data->expect);
+    test_event_data->expect = NULL;
 }
 
 static void event_prepare(TestEventData *data,
@@ -83,8 +85,7 @@ static void test_event_a(TestEventData *data,
 {
     data->expect = qdict_from_jsonf_nofail("{ 'event': 'EVENT_A' }");
     qapi_event_send_event_a();
-    g_assert(data->emitted);
-    qobject_unref(data->expect);
+    g_assert(!data->expect);
 }
 
 static void test_event_b(TestEventData *data,
@@ -92,8 +93,7 @@ static void test_event_b(TestEventData *data,
 {
     data->expect = qdict_from_jsonf_nofail("{ 'event': 'EVENT_B' }");
     qapi_event_send_event_b();
-    g_assert(data->emitted);
-    qobject_unref(data->expect);
+    g_assert(!data->expect);
 }
 
 static void test_event_c(TestEventData *data,
@@ -105,8 +105,7 @@ static void test_event_c(TestEventData *data,
         "{ 'event': 'EVENT_C', 'data': {"
         " 'a': 1, 'b': { 'integer': 2, 'string': 'test1' }, 'c': 'test2' } }");
     qapi_event_send_event_c(true, 1, &b, "test2");
-    g_assert(data->emitted);
-    qobject_unref(data->expect);
+    g_assert(!data->expect);
 }
 
 /* Complex type */
@@ -131,8 +130,7 @@ static void test_event_d(TestEventData *data,
         "  'string': 'test2', 'enum2': 'value2' },"
         " 'b': 'test3', 'enum3': 'value3' } }");
     qapi_event_send_event_d(&a, "test3", NULL, true, ENUM_ONE_VALUE3);
-    g_assert(data->emitted);
-    qobject_unref(data->expect);
+    g_assert(!data->expect);
 }
 
 static void test_event_deprecated(TestEventData *data, const void *unused)
@@ -142,15 +140,11 @@ static void test_event_deprecated(TestEventData *data, const void *unused)
     memset(&compat_policy, 0, sizeof(compat_policy));
 
     qapi_event_send_test_event_features1();
-    g_assert(data->emitted);
+    g_assert(!data->expect);
 
     compat_policy.has_deprecated_output = true;
     compat_policy.deprecated_output = COMPAT_POLICY_OUTPUT_HIDE;
-    data->emitted = false;
     qapi_event_send_test_event_features1();
-    g_assert(!data->emitted);
-
-    qobject_unref(data->expect);
 }
 
 static void test_event_deprecated_data(TestEventData *data, const void *unused)
@@ -160,17 +154,13 @@ static void test_event_deprecated_data(TestEventData *data, const void *unused)
     data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES0',"
                                            " 'data': { 'foo': 42 } }");
     qapi_event_send_test_event_features0(42);
-    g_assert(data->emitted);
+    g_assert(!data->expect);
 
-    qobject_unref(data->expect);
 
     compat_policy.has_deprecated_output = true;
     compat_policy.deprecated_output = COMPAT_POLICY_OUTPUT_HIDE;
     data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES0' }");
     qapi_event_send_test_event_features0(42);
-    g_assert(data->emitted);
-
-    qobject_unref(data->expect);
 }
 
 int main(int argc, char **argv)
-- 
2.41.0

Re: [PATCH 2/3] tests/unit/test-qmp-event: Simplify event emission check

[Thomas Huth]

On 22/11/2023 08.24, Markus Armbruster wrote:
> The generated qapi_event_send_FOO() call an event emitter function.
> It's test_qapi_event_emit() in this test.  It compares the actual
> event to the expected event, and sets a flag to record it was called.
> The test functions set expected data and clear the flag before calling
> qapi_event_send_FOO(), and check the flag afterwards.
> 
> Make test_qapi_event_emit() consume expected data, and the test
> functions check it was consumed.  Delete the flag.  This is simpler.
> It also catches extraneous calls of test_qapi_event_emit().  Catching
> that is not worthwhile, but since the cost is negative...
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/unit/test-qmp-event.c | 30 ++++++++++--------------------
>   1 file changed, 10 insertions(+), 20 deletions(-)

Good idea!
Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 3/3] tests/unit/test-qmp-event: Replace fixture by global variables

[Markus Armbruster]

The fixture buys us exactly nothing, as we need a global variable
anyway, for test_qapi_event_emit().  Drop it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/unit/test-qmp-event.c | 91 ++++++++++++-------------------------
 1 file changed, 30 insertions(+), 61 deletions(-)

diff --git a/tests/unit/test-qmp-event.c b/tests/unit/test-qmp-event.c
index 5c9837e849..08e95a382b 100644
--- a/tests/unit/test-qmp-event.c
+++ b/tests/unit/test-qmp-event.c
@@ -24,18 +24,14 @@
 #include "test-qapi-events.h"
 #include "test-qapi-emit-events.h"
 
-typedef struct TestEventData {
-    QDict *expect;
-} TestEventData;
-
-TestEventData *test_event_data;
+static QDict *expected_event;
 
 void test_qapi_event_emit(test_QAPIEvent event, QDict *d)
 {
     QDict *t;
     int64_t s, ms;
 
-    g_assert(test_event_data->expect);
+    g_assert(expected_event);
 
     /* Verify that we have timestamp, then remove it to compare other fields */
     t = qdict_get_qdict(d, "timestamp");
@@ -52,65 +48,38 @@ void test_qapi_event_emit(test_QAPIEvent event, QDict *d)
 
     qdict_del(d, "timestamp");
 
-    g_assert(qobject_is_equal(QOBJECT(d), QOBJECT(test_event_data->expect)));
-    qobject_unref(test_event_data->expect);
-    test_event_data->expect = NULL;
+    g_assert(qobject_is_equal(QOBJECT(d), QOBJECT(expected_event)));
+    qobject_unref(expected_event);
+    expected_event = NULL;
 }
 
-static void event_prepare(TestEventData *data,
-                          const void *unused)
+static void test_event_a(void)
 {
-    test_event_data = data;
-}
-
-static void event_teardown(TestEventData *data,
-                           const void *unused)
-{
-    test_event_data = NULL;
-}
-
-static void event_test_add(const char *testpath,
-                           void (*test_func)(TestEventData *data,
-                                             const void *user_data))
-{
-    g_test_add(testpath, TestEventData, NULL, event_prepare, test_func,
-               event_teardown);
-}
-
-
-/* Test cases */
-
-static void test_event_a(TestEventData *data,
-                         const void *unused)
-{
-    data->expect = qdict_from_jsonf_nofail("{ 'event': 'EVENT_A' }");
+    expected_event = qdict_from_jsonf_nofail("{ 'event': 'EVENT_A' }");
     qapi_event_send_event_a();
-    g_assert(!data->expect);
+    g_assert(!expected_event);
 }
 
-static void test_event_b(TestEventData *data,
-                         const void *unused)
+static void test_event_b(void)
 {
-    data->expect = qdict_from_jsonf_nofail("{ 'event': 'EVENT_B' }");
+    expected_event = qdict_from_jsonf_nofail("{ 'event': 'EVENT_B' }");
     qapi_event_send_event_b();
-    g_assert(!data->expect);
+    g_assert(!expected_event);
 }
 
-static void test_event_c(TestEventData *data,
-                         const void *unused)
+static void test_event_c(void)
 {
     UserDefOne b = { .integer = 2, .string = (char *)"test1" };
 
-    data->expect = qdict_from_jsonf_nofail(
+    expected_event = qdict_from_jsonf_nofail(
         "{ 'event': 'EVENT_C', 'data': {"
         " 'a': 1, 'b': { 'integer': 2, 'string': 'test1' }, 'c': 'test2' } }");
     qapi_event_send_event_c(true, 1, &b, "test2");
-    g_assert(!data->expect);
+    g_assert(!expected_event);
 }
 
 /* Complex type */
-static void test_event_d(TestEventData *data,
-                         const void *unused)
+static void test_event_d(void)
 {
     UserDefOne struct1 = {
         .integer = 2, .string = (char *)"test1",
@@ -123,43 +92,43 @@ static void test_event_d(TestEventData *data,
         .enum2 = ENUM_ONE_VALUE2,
     };
 
-    data->expect = qdict_from_jsonf_nofail(
+    expected_event = qdict_from_jsonf_nofail(
         "{ 'event': 'EVENT_D', 'data': {"
         " 'a': {"
         "  'struct1': { 'integer': 2, 'string': 'test1', 'enum1': 'value1' },"
         "  'string': 'test2', 'enum2': 'value2' },"
         " 'b': 'test3', 'enum3': 'value3' } }");
     qapi_event_send_event_d(&a, "test3", NULL, true, ENUM_ONE_VALUE3);
-    g_assert(!data->expect);
+    g_assert(!expected_event);
 }
 
-static void test_event_deprecated(TestEventData *data, const void *unused)
+static void test_event_deprecated(void)
 {
-    data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES1' }");
+    expected_event = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES1' }");
 
     memset(&compat_policy, 0, sizeof(compat_policy));
 
     qapi_event_send_test_event_features1();
-    g_assert(!data->expect);
+    g_assert(!expected_event);
 
     compat_policy.has_deprecated_output = true;
     compat_policy.deprecated_output = COMPAT_POLICY_OUTPUT_HIDE;
     qapi_event_send_test_event_features1();
 }
 
-static void test_event_deprecated_data(TestEventData *data, const void *unused)
+static void test_event_deprecated_data(void)
 {
     memset(&compat_policy, 0, sizeof(compat_policy));
 
-    data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES0',"
+    expected_event = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES0',"
                                            " 'data': { 'foo': 42 } }");
     qapi_event_send_test_event_features0(42);
-    g_assert(!data->expect);
+    g_assert(!expected_event);
 
 
     compat_policy.has_deprecated_output = true;
     compat_policy.deprecated_output = COMPAT_POLICY_OUTPUT_HIDE;
-    data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES0' }");
+    expected_event = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES0' }");
     qapi_event_send_test_event_features0(42);
 }
 
@@ -167,12 +136,12 @@ int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
 
-    event_test_add("/event/event_a", test_event_a);
-    event_test_add("/event/event_b", test_event_b);
-    event_test_add("/event/event_c", test_event_c);
-    event_test_add("/event/event_d", test_event_d);
-    event_test_add("/event/deprecated", test_event_deprecated);
-    event_test_add("/event/deprecated_data", test_event_deprecated_data);
+    g_test_add_func("/event/event_a", test_event_a);
+    g_test_add_func("/event/event_b", test_event_b);
+    g_test_add_func("/event/event_c", test_event_c);
+    g_test_add_func("/event/event_d", test_event_d);
+    g_test_add_func("/event/deprecated", test_event_deprecated);
+    g_test_add_func("/event/deprecated_data", test_event_deprecated_data);
     g_test_run();
 
     return 0;
-- 
2.41.0

Re: [PATCH 3/3] tests/unit/test-qmp-event: Replace fixture by global variables

[Thomas Huth]

On 22/11/2023 08.24, Markus Armbruster wrote:
> The fixture buys us exactly nothing, as we need a global variable
> anyway, for test_qapi_event_emit().  Drop it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/unit/test-qmp-event.c | 91 ++++++++++++-------------------------
>   1 file changed, 30 insertions(+), 61 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>