From 5235b1740fd4c1ac412b56b805342900cde989f4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 31 May 2017 10:45:47 +0200 Subject: [PATCH 1/5] shared/tests: document how to disable slow tests at compile-time --- shared/nm-utils/nm-test-utils.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h index d2d96620cc..5be8b8047b 100644 --- a/shared/nm-utils/nm-test-utils.h +++ b/shared/nm-utils/nm-test-utils.h @@ -82,7 +82,8 @@ * Whether long-running tests are enabled is determined as follows (highest priority first): * - specifying the value in NMTST_DEBUG has highest priority * - respect g_test_quick(), if the command line contains '-mslow', '-mquick', '-mthorough'. - * - use compile time default + * - use compile time default (CFLAGS=-DNMTST_TEST_QUICK=TRUE) + * - enable slow tests by default * * "p=PATH"|"s=PATH": passes the path to g_test_init() as "-p" and "-s", respectively. * Unfortunately, these options conflict with "--tap" which our makefile passes to the From 75f3c026ebe86628d2ea275d83df504186b38106 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 31 May 2017 10:03:50 +0200 Subject: [PATCH 2/5] shared/tests: expose end-time from NMTST_WAIT() macro --- shared/nm-utils/nm-test-utils.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h index 5be8b8047b..66118fe9cf 100644 --- a/shared/nm-utils/nm-test-utils.h +++ b/shared/nm-utils/nm-test-utils.h @@ -138,12 +138,13 @@ #define NMTST_WAIT(max_wait_ms, wait) \ ({ \ gboolean _not_expired = TRUE; \ - gint64 _nmtst_end, _nmtst_max_wait_us = (max_wait_ms) * 1000L; \ + const gint64 nmtst_wait_start_us = g_get_monotonic_time (); \ + const gint64 nmtst_wait_duration_us = (max_wait_ms) * 1000L; \ + const gint64 nmtst_wait_end_us = nmtst_wait_start_us + nmtst_wait_duration_us; \ \ - _nmtst_end = g_get_monotonic_time () + _nmtst_max_wait_us; \ while (TRUE) { \ { wait }; \ - if (g_get_monotonic_time () > _nmtst_end) { \ + if (g_get_monotonic_time () > nmtst_wait_end_us) { \ _not_expired = FALSE; \ break; \ } \ From af9c474844fb109c49adba2653a2af9bf6423f29 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 31 May 2017 10:03:32 +0200 Subject: [PATCH 3/5] platform/tests: change nmtstp_wait_for_signal() to wait with zero timeout The previous behavior, of treating timeout_ms as *no timeout*, makes no sense. At least not for unit tests. If you have a really long timeout, then set it. "0" should really mean to schedule a zero timeout. --- src/platform/tests/test-common.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/platform/tests/test-common.c b/src/platform/tests/test-common.c index 2abf6b2cea..85dbefdac4 100644 --- a/src/platform/tests/test-common.c +++ b/src/platform/tests/test-common.c @@ -387,8 +387,7 @@ nmtstp_wait_for_signal (NMPlatform *platform, guint timeout_ms) id_ip4_route = g_signal_connect (platform, NM_PLATFORM_SIGNAL_IP4_ROUTE_CHANGED, G_CALLBACK (_wait_for_signal_cb), &data); id_ip6_route = g_signal_connect (platform, NM_PLATFORM_SIGNAL_IP6_ROUTE_CHANGED, G_CALLBACK (_wait_for_signal_cb), &data); - if (timeout_ms != 0) - data.id = g_timeout_add (timeout_ms, _wait_for_signal_timeout, &data); + data.id = g_timeout_add (timeout_ms, _wait_for_signal_timeout, &data); g_main_loop_run (data.loop); @@ -417,7 +416,7 @@ nmtstp_wait_for_signal_until (NMPlatform *platform, gint64 until_ms) if (until_ms < now) return 0; - signal_counts = nmtstp_wait_for_signal (platform, MAX (1, until_ms - now)); + signal_counts = nmtstp_wait_for_signal (platform, MAX (0, until_ms - now)); if (signal_counts) return signal_counts; } @@ -448,7 +447,7 @@ nmtstp_wait_for_link_until (NMPlatform *platform, const char *ifname, NMLinkType if (until_ms < now) return NULL; - nmtstp_wait_for_signal (platform, MAX (1, until_ms - now)); + nmtstp_wait_for_signal (platform, MAX (0, until_ms - now)); } } From 07751b444bbf92b77687ba5a2cfeaeb3c449bbd2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 31 May 2017 10:09:10 +0200 Subject: [PATCH 4/5] platform/tests: make timeout_ms argument for nmtstp_wait_for_signal() signed Having an unsigned "guint timeout_ms" argument is very inconvenient, because nmtstp_wait_for_signal (NM_PLATFORM_GET (), end_time - now_time); might easily be negative. In such a case, the correct behavior is to wait not at all. --- src/platform/tests/test-common.c | 21 ++++++++++++++++----- src/platform/tests/test-common.h | 4 ++-- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/platform/tests/test-common.c b/src/platform/tests/test-common.c index 85dbefdac4..eec92af1fd 100644 --- a/src/platform/tests/test-common.c +++ b/src/platform/tests/test-common.c @@ -372,7 +372,7 @@ _wait_for_signal_timeout (gpointer user_data) } guint -nmtstp_wait_for_signal (NMPlatform *platform, guint timeout_ms) +nmtstp_wait_for_signal (NMPlatform *platform, gint64 timeout_ms) { WaitForSignalData data = { 0 }; gulong id_link, id_ip4_address, id_ip6_address, id_ip4_route, id_ip6_route; @@ -387,7 +387,18 @@ nmtstp_wait_for_signal (NMPlatform *platform, guint timeout_ms) id_ip4_route = g_signal_connect (platform, NM_PLATFORM_SIGNAL_IP4_ROUTE_CHANGED, G_CALLBACK (_wait_for_signal_cb), &data); id_ip6_route = g_signal_connect (platform, NM_PLATFORM_SIGNAL_IP6_ROUTE_CHANGED, G_CALLBACK (_wait_for_signal_cb), &data); - data.id = g_timeout_add (timeout_ms, _wait_for_signal_timeout, &data); + /* if timeout_ms is negative, it means the wait-time already expired. + * Maybe, we should do nothing and return right away, without even + * processing events from platform. However, that inconsistency (of not + * processing events from mainloop) is inconvenient. + * + * It's better that on the return of nmtstp_wait_for_signal(), we always + * have no events pending. So, a negative timeout is treated the same as + * a zero timeout: we check whether there are any events pending in platform, + * and quite the mainloop immediately afterwards. But we always check. */ + + data.id = g_timeout_add (CLAMP (timeout_ms, 0, G_MAXUINT32), + _wait_for_signal_timeout, &data); g_main_loop_run (data.loop); @@ -416,14 +427,14 @@ nmtstp_wait_for_signal_until (NMPlatform *platform, gint64 until_ms) if (until_ms < now) return 0; - signal_counts = nmtstp_wait_for_signal (platform, MAX (0, until_ms - now)); + signal_counts = nmtstp_wait_for_signal (platform, until_ms - now); if (signal_counts) return signal_counts; } } const NMPlatformLink * -nmtstp_wait_for_link (NMPlatform *platform, const char *ifname, NMLinkType expected_link_type, guint timeout_ms) +nmtstp_wait_for_link (NMPlatform *platform, const char *ifname, NMLinkType expected_link_type, gint64 timeout_ms) { return nmtstp_wait_for_link_until (platform, ifname, expected_link_type, nm_utils_get_monotonic_timestamp_ms () + timeout_ms); } @@ -447,7 +458,7 @@ nmtstp_wait_for_link_until (NMPlatform *platform, const char *ifname, NMLinkType if (until_ms < now) return NULL; - nmtstp_wait_for_signal (platform, MAX (0, until_ms - now)); + nmtstp_wait_for_signal (platform, until_ms - now); } } diff --git a/src/platform/tests/test-common.h b/src/platform/tests/test-common.h index 373a1fe0da..b4ed0682ea 100644 --- a/src/platform/tests/test-common.h +++ b/src/platform/tests/test-common.h @@ -93,9 +93,9 @@ int nmtstp_run_command (const char *format, ...) _nm_printf (1, 2); /*****************************************************************************/ -guint nmtstp_wait_for_signal (NMPlatform *platform, guint timeout_ms); +guint nmtstp_wait_for_signal (NMPlatform *platform, gint64 timeout_ms); guint nmtstp_wait_for_signal_until (NMPlatform *platform, gint64 until_ms); -const NMPlatformLink *nmtstp_wait_for_link (NMPlatform *platform, const char *ifname, NMLinkType expected_link_type, guint timeout_ms); +const NMPlatformLink *nmtstp_wait_for_link (NMPlatform *platform, const char *ifname, NMLinkType expected_link_type, gint64 timeout_ms); const NMPlatformLink *nmtstp_wait_for_link_until (NMPlatform *platform, const char *ifname, NMLinkType expected_link_type, gint64 until_ms); #define nmtstp_assert_wait_for_signal(platform, timeout_ms) \ From d6aae6af7202e7d28d6a52fad2376ee3ce6e1583 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 31 May 2017 09:41:39 +0200 Subject: [PATCH 5/5] platform/tests: reorder wait-loop in test_ip6_route_options() - no need to call nm_platform_process_events() after nmtstp_wait_for_signal(). The latter processes all events that are pending. - with addr_n number of addresses, we still only want to wait a maximum time, not for each addresss individually. Basically, the for-loop must be inside NMTST_WAIT(), not the other way around. --- src/platform/tests/test-route.c | 71 +++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 30 deletions(-) diff --git a/src/platform/tests/test-route.c b/src/platform/tests/test-route.c index fc395c84ef..2c709d730a 100644 --- a/src/platform/tests/test-route.c +++ b/src/platform/tests/test-route.c @@ -30,6 +30,40 @@ #define DEVICE_IFINDEX NMTSTP_ENV1_IFINDEX #define EX NMTSTP_ENV1_EX +static void +_wait_for_ipv6_addr_non_tentative (NMPlatform *platform, + gint64 timeout_ms, + int ifindex, + guint addr_n, + const struct in6_addr *addrs) +{ + guint i; + + /* Wait that the addresses become non-tentative. Dummy interfaces are NOARP + * and thus don't do DAD, but the kernel sets the address as tentative for a + * small amount of time, which prevents the immediate addition of the route + * with RTA_PREFSRC */ + + NMTST_WAIT_ASSERT (400, { + gboolean should_wait = FALSE; + const NMPlatformIP6Address *plt_addr; + + for (i = 0; i < addr_n; i++) { + plt_addr = nm_platform_ip6_address_get (NM_PLATFORM_GET, ifindex, addrs[i]); + if ( !plt_addr + || NM_FLAGS_HAS (plt_addr->n_ifa_flags, IFA_F_TENTATIVE)) { + should_wait = TRUE; + break; + } + } + if (!should_wait) + return; + nmtstp_assert_wait_for_signal (NM_PLATFORM_GET, + (nmtst_wait_end_us - g_get_monotonic_time ()) / 1000); + }); +} + + static void ip4_route_callback (NMPlatform *platform, int obj_type_i, int ifindex, const NMPlatformIP4Route *received, int change_type_i, SignalData *data) { @@ -260,19 +294,7 @@ test_ip6_route (void) NM_PLATFORM_LIFETIME_PERMANENT, NM_PLATFORM_LIFETIME_PERMANENT, 0)); accept_signals (route_added, 0, 1); - /* Wait that the address becomes non-tentative. Dummy interfaces are NOARP - * and thus don't do DAD, but the kernel sets the address as tentative for a - * small amount of time, which prevents the immediate addition of the route - * with RTA_PREFSRC */ - NMTST_WAIT_ASSERT (200, { - const NMPlatformIP6Address *plt_addr; - - nmtstp_wait_for_signal (NM_PLATFORM_GET, 50); - nm_platform_process_events (NM_PLATFORM_GET); - plt_addr = nm_platform_ip6_address_get (NM_PLATFORM_GET, ifindex, pref_src); - if (plt_addr && !NM_FLAGS_HAS (plt_addr->n_ifa_flags, IFA_F_TENTATIVE)) - break; - }); + _wait_for_ipv6_addr_non_tentative (NM_PLATFORM_GET, 200, ifindex, 1, &pref_src); /* Add route to gateway */ nmtstp_ip6_route_add (NM_PLATFORM_GET, ifindex, NM_IP_CONFIG_SOURCE_USER, gateway, 128, in6addr_any, in6addr_any, metric, mss); @@ -370,7 +392,6 @@ test_ip4_zero_gateway (void) nmtstp_run_command_check ("ip route flush dev %s", DEVICE_NAME); nmtstp_wait_for_signal (NM_PLATFORM_GET, 50); - nm_platform_process_events (NM_PLATFORM_GET); } static void @@ -438,6 +459,7 @@ test_ip6_route_options (gconstpointer test_data) NMPlatformIP6Route rts_add[RTS_MAX] = { }; NMPlatformIP6Route rts_cmp[RTS_MAX] = { }; NMPlatformIP6Address addr[1] = { }; + struct in6_addr addr_in6[G_N_ELEMENTS (addr)] = { }; guint rts_n = 0; guint addr_n = 0; guint i; @@ -484,7 +506,10 @@ test_ip6_route_options (gconstpointer test_data) } for (i = 0; i < addr_n; i++) { - g_assert (nm_platform_ip6_address_add (NM_PLATFORM_GET, IFINDEX, + g_assert (addr[i].ifindex == IFINDEX); + addr_in6[i] = addr[i].address; + g_assert (nm_platform_ip6_address_add (NM_PLATFORM_GET, + IFINDEX, addr[i].address, addr[i].plen, addr[i].peer_address, @@ -493,21 +518,7 @@ test_ip6_route_options (gconstpointer test_data) addr[i].n_ifa_flags)); } - /* Wait that the addresses become non-tentative. Dummy interfaces are NOARP - * and thus don't do DAD, but the kernel sets the address as tentative for a - * small amount of time, which prevents the immediate addition of the route - * with RTA_PREFSRC */ - for (i = 0; i < addr_n; i++) { - NMTST_WAIT_ASSERT (200, { - const NMPlatformIP6Address *plt_addr; - - nmtstp_wait_for_signal (NM_PLATFORM_GET, 50); - nm_platform_process_events (NM_PLATFORM_GET); - plt_addr = nm_platform_ip6_address_get (NM_PLATFORM_GET, addr[i].ifindex, addr[i].address); - if (plt_addr && !NM_FLAGS_HAS (plt_addr->n_ifa_flags, IFA_F_TENTATIVE)) - break; - }); - } + _wait_for_ipv6_addr_non_tentative (NM_PLATFORM_GET, 400, IFINDEX, addr_n, addr_in6); for (i = 0; i < rts_n; i++) g_assert (nm_platform_ip6_route_add (NM_PLATFORM_GET, &rts_add[i]));