From b74457ac82a360feb57c226f7f8a31a2d159ba17 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 7 Sep 2015 10:48:48 +0200 Subject: [PATCH 1/3] config/test: add a test for nm_config_reload() with different signals (cherry picked from commit e6c64af8be89374ed318ed1a395ead7cd00a6753) (cherry picked from commit ce7687e49900a72dd2cdd9ee7595004e4f3cf4e6) --- src/tests/config/test-config.c | 82 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/src/tests/config/test-config.c b/src/tests/config/test-config.c index 8b8028e..a5975c1 100644 --- a/src/tests/config/test-config.c +++ b/src/tests/config/test-config.c @@ -396,6 +396,86 @@ test_config_confdir_parse_error (void) g_clear_error (&error); } +/*****************************************************************************/ + +static void +_test_signal_config_changed_cb (NMConfig *config, + NMConfigData *config_data, + NMConfigChangeFlags changes, + NMConfigData *old_data, + gpointer user_data) +{ + const NMConfigChangeFlags *expected = user_data; + + g_assert (changes); + g_assert_cmpint (changes, ==, *expected); + g_assert (NM_IS_CONFIG (config)); + g_assert (NM_IS_CONFIG_DATA (config_data)); + + g_assert (config_data == old_data); + g_assert (config_data == nm_config_get_data (config)); +} + +static void +_test_signal_config_changed_cb2 (NMConfig *config, + NMConfigData *config_data, + NMConfigChangeFlags changes, + NMConfigData *old_data, + gpointer user_data) +{ + const NMConfigChangeFlags *expected = user_data; + + g_assert (changes); + g_assert_cmpint (changes, ==, *expected); +} + +static void +test_config_signal (void) +{ + gs_unref_object NMConfig *config = NULL; + NMConfigChangeFlags expected; + gs_unref_object NMConfigData *config_data_orig = NULL; + + config = setup_config (NULL, SRCDIR "/NetworkManager.conf", "/no/such/dir", NULL); + + config_data_orig = g_object_ref (nm_config_get_data_orig (config)); + + g_signal_connect (G_OBJECT (config), + NM_CONFIG_SIGNAL_CONFIG_CHANGED, + G_CALLBACK (_test_signal_config_changed_cb), + &expected); + + expected = NM_CONFIG_CHANGE_SIGUSR1; + g_test_expect_message ("NetworkManager", G_LOG_LEVEL_MESSAGE, "*config: signal SIGUSR1"); + nm_config_reload (config, SIGUSR1); + + expected = NM_CONFIG_CHANGE_SIGUSR2; + g_test_expect_message ("NetworkManager", G_LOG_LEVEL_MESSAGE, "*config: signal SIGUSR2"); + nm_config_reload (config, SIGUSR2); + + expected = NM_CONFIG_CHANGE_SIGHUP; + g_test_expect_message ("NetworkManager", G_LOG_LEVEL_MESSAGE, "*config: signal SIGHUP (no changes from disk)*"); + nm_config_reload (config, SIGHUP); + + + /* test with subscribing two signals... */ + g_signal_connect (G_OBJECT (config), + NM_CONFIG_SIGNAL_CONFIG_CHANGED, + G_CALLBACK (_test_signal_config_changed_cb2), + &expected); + expected = NM_CONFIG_CHANGE_SIGUSR2; + g_test_expect_message ("NetworkManager", G_LOG_LEVEL_MESSAGE, "*config: signal SIGUSR2"); + nm_config_reload (config, SIGUSR2); + g_signal_handlers_disconnect_by_func (config, _test_signal_config_changed_cb2, &expected); + + + g_signal_handlers_disconnect_by_func (config, _test_signal_config_changed_cb, &expected); + + g_assert (config_data_orig == nm_config_get_data (config)); +} + +/*****************************************************************************/ + NMTST_DEFINE (); int @@ -419,6 +499,8 @@ main (int argc, char **argv) g_test_add_func ("/config/confdir", test_config_confdir); g_test_add_func ("/config/confdir-parse-error", test_config_confdir_parse_error); + g_test_add_func ("/config/signal", test_config_signal); + /* This one has to come last, because it leaves its values in * nm-config.c's global variables, and there's no way to reset * those to NULL. -- 2.4.3 From ac6ed03af5be72b3fe0e6796804129e02043e18e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 8 Sep 2015 21:39:40 +0200 Subject: [PATCH 2/3] config: workaround invocation of "config-changed" signal There seems to be a bug in glib/ffi that hits on s390x/ppc64 architecture. It causes @changes in nm-dns-manager.c:config_changed_cb() to be NONE, although it is clearly set (see the related bug rh #1260577 for glib). Workaround this, by making the argument type a plain guint. Note that the ill behavior is caught by test_config_signal() in "src/tests/config/test-config.c". Related: https://bugzilla.redhat.com/show_bug.cgi?id=1062301 (cherry picked from commit e7d66f1df61ebdc7652ba34a4e1baddcc86b9e26) (cherry picked from commit 9aecabe29f39350c5e31a61af4d49062108dfc89) --- src/nm-config.c | 12 +++++++++++- src/tests/config/test-config.c | 5 ++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/nm-config.c b/src/nm-config.c index 86b3d92..011e57f 100644 --- a/src/nm-config.c +++ b/src/nm-config.c @@ -1214,7 +1214,17 @@ nm_config_class_init (NMConfigClass *config_class) G_SIGNAL_RUN_FIRST, G_STRUCT_OFFSET (NMConfigClass, config_changed), NULL, NULL, NULL, - G_TYPE_NONE, 3, NM_TYPE_CONFIG_DATA, NM_TYPE_CONFIG_CHANGE_FLAGS, NM_TYPE_CONFIG_DATA); + G_TYPE_NONE, + 3, + NM_TYPE_CONFIG_DATA, + /* Use plain guint type for changes argument. This avoids + * glib/ffi bug https://bugzilla.redhat.com/show_bug.cgi?id=1260577 */ + /* NM_TYPE_CONFIG_CHANGE_FLAGS, */ + G_TYPE_UINT, + NM_TYPE_CONFIG_DATA); + + G_STATIC_ASSERT_EXPR (sizeof (guint) == sizeof (NMConfigChangeFlags)); + G_STATIC_ASSERT_EXPR (((gint64) ((NMConfigChangeFlags) -1)) > ((gint64) 0)); } static void diff --git a/src/tests/config/test-config.c b/src/tests/config/test-config.c index a5975c1..b973e60 100644 --- a/src/tests/config/test-config.c +++ b/src/tests/config/test-config.c @@ -458,7 +458,10 @@ test_config_signal (void) nm_config_reload (config, SIGHUP); - /* test with subscribing two signals... */ + /* test with subscribing two signals... + * + * This test exposes glib bug https://bugzilla.redhat.com/show_bug.cgi?id=1260577 + * for which we however have a workaround in 'nm-config.c' */ g_signal_connect (G_OBJECT (config), NM_CONFIG_SIGNAL_CONFIG_CHANGED, G_CALLBACK (_test_signal_config_changed_cb2), -- 2.4.3 From 8bb4c36cdc6033a2216fbe3e8fbac23c9b3bc5f6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 9 Sep 2015 12:22:16 +0200 Subject: [PATCH 3/3] platform/test: add test for invoking platform signals There seems to be an issue with glib/ffi that causes failures to pass enum-typed arguments to signals (related bug rh#1260577). Add a test for platform signals which, beside NM_CONFIG_SIGNAL_CONFIG_CHANGED, is the only place where we use enum-typed arguments for signals. Strangely, this test doesn't cause the failure, so it's unclear why the workaround was necessary for "config-changed" signal (commit e7d66f1df61ebdc7652ba34a4e1baddcc86b9e26). (cherry picked from commit 52cd5ee6120b643aa143cbd439f81a0c02c8313e) (cherry picked from commit d7970c58eb10e9d59c855d61017299a1a811ee2a) --- src/platform/tests/test-link.c | 47 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/src/platform/tests/test-link.c b/src/platform/tests/test-link.c index ce371b7..f9645d8 100644 --- a/src/platform/tests/test-link.c +++ b/src/platform/tests/test-link.c @@ -118,6 +118,41 @@ software_add (NMLinkType link_type, const char *name) } static void +test_link_changed_signal_cb (NMPlatform *platform, + NMPObjectType obj_type, + int ifindex, + const NMPlatformIP4Route *route, + NMPlatformSignalChangeType change_type, + NMPlatformReason reason, + gboolean *p_test_link_changed_signal_arg) +{ + /* test invocation of platform signals with multiple listeners + * connected to the signal. Platform signals have enum-typed + * arguments and there seem to be an issue with invoking such + * signals on s390x and ppc64 archs. + * https://bugzilla.redhat.com/show_bug.cgi?id=1260577 + * + * As the test shows, the failure is not reproducible for + * platform signals. + */ + g_assert (NM_IS_PLATFORM (platform)); + g_assert (platform == NM_PLATFORM_GET); + + g_assert (ifindex > 0); + g_assert (route); + + g_assert_cmpint (obj_type, ==, NMP_OBJECT_TYPE_LINK); + + g_assert_cmpint ((gint64) change_type, !=, (gint64) 0); + g_assert_cmpint (change_type, !=, NM_PLATFORM_SIGNAL_NONE); + + g_assert_cmpint ((gint64) reason, !=, (gint64) 0); + g_assert_cmpint (reason, !=, NM_PLATFORM_REASON_NONE); + + *p_test_link_changed_signal_arg = TRUE; +} + +static void test_slave (int master, int type, SignalData *master_changed) { int ifindex; @@ -125,6 +160,8 @@ test_slave (int master, int type, SignalData *master_changed) SignalData *link_changed, *link_removed; char *value; NMLinkType link_type = nm_platform_link_get_type (NM_PLATFORM_GET, master); + gboolean test_link_changed_signal_arg1; + gboolean test_link_changed_signal_arg2; g_assert (NM_IN_SET (link_type, NM_LINK_TYPE_TEAM, NM_LINK_TYPE_BOND, NM_LINK_TYPE_BRIDGE)); @@ -158,11 +195,21 @@ test_slave (int master, int type, SignalData *master_changed) else g_assert (!nm_platform_link_is_up (NM_PLATFORM_GET, ifindex)); + test_link_changed_signal_arg1 = FALSE; + test_link_changed_signal_arg2 = FALSE; + g_signal_connect (NM_PLATFORM_GET, NM_PLATFORM_SIGNAL_LINK_CHANGED, G_CALLBACK (test_link_changed_signal_cb), &test_link_changed_signal_arg1); + g_signal_connect (NM_PLATFORM_GET, NM_PLATFORM_SIGNAL_LINK_CHANGED, G_CALLBACK (test_link_changed_signal_cb), &test_link_changed_signal_arg2); + /* Set master up */ g_assert (nm_platform_link_set_up (NM_PLATFORM_GET, master, NULL)); g_assert (nm_platform_link_is_up (NM_PLATFORM_GET, master)); accept_signals (master_changed, 1, 2); + g_signal_handlers_disconnect_by_func (NM_PLATFORM_GET, G_CALLBACK (test_link_changed_signal_cb), &test_link_changed_signal_arg1); + g_signal_handlers_disconnect_by_func (NM_PLATFORM_GET, G_CALLBACK (test_link_changed_signal_cb), &test_link_changed_signal_arg2); + g_assert (test_link_changed_signal_arg1); + g_assert (test_link_changed_signal_arg2); + /* Master with a disconnected slave is disconnected * * For some reason, bonding and teaming slaves are automatically set up. We -- 2.4.3