From 6850766679b236e9a38f17a2826f8832d75f6fb1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 4 Jan 2023 13:59:53 +0100 Subject: [PATCH 1/7] glib-aux/tests: expose current test name that is running under nmtst_add_test_func_full*() --- src/libnm-glib-aux/nm-test-utils.h | 48 +++++++++++++++++++++++------- 1 file changed, 38 insertions(+), 10 deletions(-) diff --git a/src/libnm-glib-aux/nm-test-utils.h b/src/libnm-glib-aux/nm-test-utils.h index 3e7a122125..e8fd464862 100644 --- a/src/libnm-glib-aux/nm-test-utils.h +++ b/src/libnm-glib-aux/nm-test-utils.h @@ -225,16 +225,17 @@ /*****************************************************************************/ struct __nmtst_internal { - GRand *rand0; - guint32 rand_seed; - GRand *rand; - gboolean is_debug; - gboolean assert_logging; - gboolean no_expect_message; - gboolean test_quick; - gboolean test_tap_log; - char *sudo_cmd; - char **orig_argv; + GRand *rand0; + guint32 rand_seed; + GRand *rand; + gboolean is_debug; + gboolean assert_logging; + gboolean no_expect_message; + gboolean test_quick; + gboolean test_tap_log; + char *sudo_cmd; + char **orig_argv; + const char *testpath; }; extern struct __nmtst_internal __nmtst_internal; @@ -810,6 +811,22 @@ _nmtst_test_data_unpack(const NmtstTestData *test_data, gsize n_args, ...) #define nmtst_test_data_unpack(test_data, ...) \ _nmtst_test_data_unpack(test_data, NM_NARG(__VA_ARGS__), ##__VA_ARGS__) +static inline const char * +nmtst_test_get_path(void) +{ + g_assert(nmtst_initialized()); + g_assert(__nmtst_internal.testpath); + + /* Similar to g_test_get_path() (which only exists since glib 2.68). + * + * This is the test name while running the test added with + * nmtst_add_test_func*(). + * + * You are only allowed to call this from inside such a test. */ + + return __nmtst_internal.testpath; +} + static inline void _nmtst_test_data_free(gpointer data) { @@ -826,6 +843,13 @@ _nmtst_test_run(gconstpointer data) { const NmtstTestData *test_data = data; + g_assert(test_data); + g_assert(nmtst_initialized()); + g_assert(test_data->testpath); + g_assert(!__nmtst_internal.testpath); + + __nmtst_internal.testpath = test_data->testpath; + if (test_data->_func_setup) test_data->_func_setup(test_data); @@ -833,6 +857,10 @@ _nmtst_test_run(gconstpointer data) if (test_data->_func_teardown) test_data->_func_teardown(test_data); + + g_assert(__nmtst_internal.testpath); + g_assert(__nmtst_internal.testpath == test_data->testpath); + __nmtst_internal.testpath = NULL; } static inline void From eda7d08a025a07d7e62497611fefe7aa2f1457a6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Dec 2022 16:39:42 +0100 Subject: [PATCH 2/7] glib-aux/tests: use C99 flexible array members for pointers in NmtstTestData It's just nicer code. The previous code was correct, in particular, the alignment of the data was most likely correct. Still, this is nicer. --- src/libnm-glib-aux/nm-test-utils.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libnm-glib-aux/nm-test-utils.h b/src/libnm-glib-aux/nm-test-utils.h index e8fd464862..4d140bd52e 100644 --- a/src/libnm-glib-aux/nm-test-utils.h +++ b/src/libnm-glib-aux/nm-test-utils.h @@ -783,10 +783,10 @@ struct _NmtstTestData { char *_testpath; }; gsize n_args; - gpointer *args; NmtstTestHandler _func_setup; GTestDataFunc _func_test; NmtstTestHandler _func_teardown; + gpointer args[]; }; static inline void @@ -878,14 +878,13 @@ _nmtst_add_test_func_full(const char *testpath, g_assert(testpath && testpath[0]); g_assert(func_test); - data = g_malloc0(sizeof(NmtstTestData) + (sizeof(gpointer) * (n_args + 1))); + data = g_malloc0(G_STRUCT_OFFSET(NmtstTestData, args) + (sizeof(gpointer) * (n_args + 1u))); data->_testpath = g_strdup(testpath); data->_func_test = func_test; data->_func_setup = func_setup; data->_func_teardown = func_teardown; data->n_args = n_args; - data->args = (gpointer) &data[1]; va_start(ap, n_args); for (i = 0; i < n_args; i++) data->args[i] = va_arg(ap, gpointer); From e4104a9f1253acdad47bdd95c5c9c4813e09936e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 4 Jan 2023 08:15:26 +0100 Subject: [PATCH 3/7] glib-aux/tests: use struct initialization in _nmtst_add_test_func_full() It's nicer, and doesn't require g_malloc0() to ensure all fields are initialized. --- src/libnm-glib-aux/nm-test-utils.h | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/libnm-glib-aux/nm-test-utils.h b/src/libnm-glib-aux/nm-test-utils.h index 4d140bd52e..041f3d34ac 100644 --- a/src/libnm-glib-aux/nm-test-utils.h +++ b/src/libnm-glib-aux/nm-test-utils.h @@ -878,13 +878,16 @@ _nmtst_add_test_func_full(const char *testpath, g_assert(testpath && testpath[0]); g_assert(func_test); - data = g_malloc0(G_STRUCT_OFFSET(NmtstTestData, args) + (sizeof(gpointer) * (n_args + 1u))); + data = g_malloc(G_STRUCT_OFFSET(NmtstTestData, args) + (sizeof(gpointer) * (n_args + 1u))); + + *data = (NmtstTestData){ + ._testpath = g_strdup(testpath), + ._func_test = func_test, + ._func_setup = func_setup, + ._func_teardown = func_teardown, + .n_args = n_args, + }; - data->_testpath = g_strdup(testpath); - data->_func_test = func_test; - data->_func_setup = func_setup; - data->_func_teardown = func_teardown; - data->n_args = n_args; va_start(ap, n_args); for (i = 0; i < n_args; i++) data->args[i] = va_arg(ap, gpointer); From d0dff07687b2c32cb0816ea6481e3b712ba8e452 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 4 Jan 2023 08:27:49 +0100 Subject: [PATCH 4/7] glib-aux/tests: embed testpath in NmtstTestData struct Only allocate one chunk of memory to contain all data of NmtstTestData. This isn't about performance (which doesn't matter for test code). It's about packing all in one struct and being able to free all at once with a simple g_free(). We no longer need _nmtst_test_data_free() with this. Note that NmtstTestData is never mutated, it just holds some data. As such, the single place where such a structure gets initialized, can become a bit more complicated, in exchange for having a trivial free operation (and anyway there no functions that modify the data or that would care about the data layout). --- src/libnm-glib-aux/nm-test-utils.h | 32 +++++++++++++----------------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/src/libnm-glib-aux/nm-test-utils.h b/src/libnm-glib-aux/nm-test-utils.h index 041f3d34ac..9694b25c37 100644 --- a/src/libnm-glib-aux/nm-test-utils.h +++ b/src/libnm-glib-aux/nm-test-utils.h @@ -778,10 +778,7 @@ typedef struct _NmtstTestData NmtstTestData; typedef void (*NmtstTestHandler)(const NmtstTestData *test_data); struct _NmtstTestData { - union { - const char *testpath; - char *_testpath; - }; + const char *testpath; gsize n_args; NmtstTestHandler _func_setup; GTestDataFunc _func_test; @@ -827,17 +824,6 @@ nmtst_test_get_path(void) return __nmtst_internal.testpath; } -static inline void -_nmtst_test_data_free(gpointer data) -{ - NmtstTestData *test_data = data; - - g_assert(test_data); - - g_free(test_data->_testpath); - g_free(test_data); -} - static inline void _nmtst_test_run(gconstpointer data) { @@ -874,14 +860,18 @@ _nmtst_add_test_func_full(const char *testpath, gsize i; NmtstTestData *data; va_list ap; + gsize testpath_len; g_assert(testpath && testpath[0]); g_assert(func_test); - data = g_malloc(G_STRUCT_OFFSET(NmtstTestData, args) + (sizeof(gpointer) * (n_args + 1u))); + testpath_len = strlen(testpath) + 1u; + + data = g_malloc(G_STRUCT_OFFSET(NmtstTestData, args) + + (sizeof(gpointer) * (n_args + 1u) + testpath_len)); *data = (NmtstTestData){ - ._testpath = g_strdup(testpath), + .testpath = (gpointer) &data->args[n_args + 1u], ._func_test = func_test, ._func_setup = func_setup, ._func_teardown = func_teardown, @@ -894,8 +884,13 @@ _nmtst_add_test_func_full(const char *testpath, data->args[i] = NULL; va_end(ap); - g_test_add_data_func_full(testpath, data, _nmtst_test_run, _nmtst_test_data_free); + g_assert(data->testpath == (gpointer) &data->args[i + 1]); + + memcpy((char *) data->testpath, testpath, testpath_len); + + g_test_add_data_func_full(testpath, data, _nmtst_test_run, g_free); } + #define nmtst_add_test_func_full(testpath, func_test, func_setup, func_teardown, ...) \ _nmtst_add_test_func_full(testpath, \ func_test, \ @@ -903,6 +898,7 @@ _nmtst_add_test_func_full(const char *testpath, func_teardown, \ NM_NARG(__VA_ARGS__), \ ##__VA_ARGS__) + #define nmtst_add_test_func(testpath, func_test, ...) \ nmtst_add_test_func_full(testpath, func_test, NULL, NULL, ##__VA_ARGS__) From 43860e2b7402eef076c4ecd4153244f34576c3cf Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Dec 2022 17:49:10 +0100 Subject: [PATCH 5/7] glib-aux/tests: add mechanims to track and free test data This allows to free resources (a pointer) at the end of the test. The purpose is to avoid valgrind warnings about leaks. While a leak in the test is not a severe issue by itself, it does interfere with checking for actual leaks. Thus every leak must be avoided. --- src/libnm-glib-aux/nm-test-utils.h | 69 ++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/src/libnm-glib-aux/nm-test-utils.h b/src/libnm-glib-aux/nm-test-utils.h index 9694b25c37..9299b39fa4 100644 --- a/src/libnm-glib-aux/nm-test-utils.h +++ b/src/libnm-glib-aux/nm-test-utils.h @@ -224,6 +224,11 @@ /*****************************************************************************/ +struct __nmtst_testdata_track { + gpointer data; + GDestroyNotify destroy_notify; +}; + struct __nmtst_internal { GRand *rand0; guint32 rand_seed; @@ -236,6 +241,7 @@ struct __nmtst_internal { char *sudo_cmd; char **orig_argv; const char *testpath; + GArray *testdata_track_array; }; extern struct __nmtst_internal __nmtst_internal; @@ -256,6 +262,62 @@ nmtst_initialized(void) return !!__nmtst_internal.rand0; } +/*****************************************************************************/ + +static inline void +_nmtst_testdata_track_clear_func(gpointer ptr) +{ + struct __nmtst_testdata_track *d = ptr; + + if (d->destroy_notify) + d->destroy_notify(d->data); + memset(d, 0, sizeof(*d)); +} + +static inline void +_nmtst_testdata_track_add(gpointer data, GDestroyNotify destroy_notify) +{ + struct __nmtst_testdata_track d = { + .data = data, + .destroy_notify = destroy_notify, + }; + + g_assert(data); + g_assert(destroy_notify); + g_assert(__nmtst_internal.testdata_track_array); + + g_array_append_val(__nmtst_internal.testdata_track_array, d); +} + +static inline void +_nmtst_testdata_track_steal(gpointer data) +{ + struct __nmtst_testdata_track *d; + guint i; + + g_assert(data); + g_assert(__nmtst_internal.testdata_track_array); + + for (i = 0; i < __nmtst_internal.testdata_track_array->len; i++) { + d = &g_array_index(__nmtst_internal.testdata_track_array, struct __nmtst_testdata_track, i); + + if (d->data != data) + continue; + + d->destroy_notify = NULL; + g_array_remove_index_fast(__nmtst_internal.testdata_track_array, i); + return; + } + g_assert_not_reached(); +} + +static inline void +_nmtst_testdata_track_steal_and_free(gpointer data) +{ + _nmtst_testdata_track_steal(data); + g_free(data); +} + #define __NMTST_LOG(cmd, ...) \ G_STMT_START \ { \ @@ -324,6 +386,9 @@ nmtst_free(void) if (!nmtst_initialized()) return; + g_array_set_size(__nmtst_internal.testdata_track_array, 0); + g_array_unref(__nmtst_internal.testdata_track_array); + g_rand_free(__nmtst_internal.rand0); if (__nmtst_internal.rand) g_rand_free(__nmtst_internal.rand); @@ -580,6 +645,10 @@ __nmtst_init(int *argc, __nmtst_internal.sudo_cmd = sudo_cmd; __nmtst_internal.no_expect_message = no_expect_message; + __nmtst_internal.testdata_track_array = + g_array_new(FALSE, FALSE, sizeof(struct __nmtst_testdata_track)); + g_array_set_clear_func(__nmtst_internal.testdata_track_array, _nmtst_testdata_track_clear_func); + if (!log_level && log_domains) { /* if the log level is not specified (but the domain is), we assume * the caller wants to set it depending on is_debug */ From f1874e67901fef0054b73afc9de3d7935bae99ce Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Dec 2022 17:50:09 +0100 Subject: [PATCH 6/7] glib-aux/tests: avoid valgrind leak with nmtst_add_test_func() When only running a subset of the tests (with "-p"), then valgrind indicates a leak. Avoid that. $ ./tools/run-nm-test.sh -m src/core/platform/tests/test-route-linux -v # no leak $ ./tools/run-nm-test.sh -m src/core/platform/tests/test-route-linux -v -p /route/ip4 # many leaks: ==1662102== 107 (96 direct, 11 indirect) bytes in 1 blocks are definitely lost in loss record 388 of 448 ==1662102== at 0x4848464: calloc (vg_replace_malloc.c:1340) ==1662102== by 0x4F615F0: g_malloc0 (gmem.c:163) ==1662102== by 0x1621A6: _nmtst_add_test_func_full (nm-test-utils.h:918) ==1662102== by 0x1623EB: _nmtstp_setup_tests (test-route.c:2179) ==1662102== by 0x16E53D: main (test-common.c:2693) ==1662102== { Memcheck:Leak match-leak-kinds: definite fun:calloc fun:g_malloc0 fun:_nmtst_add_test_func_full fun:_nmtstp_setup_tests fun:main } --- src/libnm-glib-aux/nm-test-utils.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libnm-glib-aux/nm-test-utils.h b/src/libnm-glib-aux/nm-test-utils.h index 9299b39fa4..5196c9a076 100644 --- a/src/libnm-glib-aux/nm-test-utils.h +++ b/src/libnm-glib-aux/nm-test-utils.h @@ -957,7 +957,11 @@ _nmtst_add_test_func_full(const char *testpath, memcpy((char *) data->testpath, testpath, testpath_len); - g_test_add_data_func_full(testpath, data, _nmtst_test_run, g_free); + _nmtst_testdata_track_add(data, g_free); + g_test_add_data_func_full(testpath, + data, + _nmtst_test_run, + _nmtst_testdata_track_steal_and_free); } #define nmtst_add_test_func_full(testpath, func_test, func_setup, func_teardown, ...) \ From 5d81b472dca61a13a5c7bb555536500e80d8478c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Dec 2022 14:21:08 +0100 Subject: [PATCH 7/7] glib-aux: use struct initialization in nm_dedup_multi_index_new() --- src/libnm-glib-aux/nm-dedup-multi.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/libnm-glib-aux/nm-dedup-multi.c b/src/libnm-glib-aux/nm-dedup-multi.c index 7d451d0d35..625c4ef31e 100644 --- a/src/libnm-glib-aux/nm-dedup-multi.c +++ b/src/libnm-glib-aux/nm-dedup-multi.c @@ -1004,12 +1004,14 @@ nm_dedup_multi_index_new(void) { NMDedupMultiIndex *self; - self = g_slice_new0(NMDedupMultiIndex); - self->ref_count = 1; - self->idx_entries = - g_hash_table_new((GHashFunc) _dict_idx_entries_hash, (GEqualFunc) _dict_idx_entries_equal); - self->idx_objs = - g_hash_table_new((GHashFunc) _dict_idx_objs_hash, (GEqualFunc) _dict_idx_objs_equal); + self = g_slice_new(NMDedupMultiIndex); + *self = (NMDedupMultiIndex){ + .ref_count = 1, + .idx_entries = g_hash_table_new((GHashFunc) _dict_idx_entries_hash, + (GEqualFunc) _dict_idx_entries_equal), + .idx_objs = + g_hash_table_new((GHashFunc) _dict_idx_objs_hash, (GEqualFunc) _dict_idx_objs_equal), + }; return self; } @@ -1018,6 +1020,7 @@ nm_dedup_multi_index_ref(NMDedupMultiIndex *self) { g_return_val_if_fail(self, NULL); g_return_val_if_fail(self->ref_count > 0, NULL); + nm_assert(self->ref_count < G_MAXINT32); self->ref_count++; return self;