From 13e9967a3ae343c831652f617fa149e4fcb440d2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 28 Feb 2017 12:59:56 +0100 Subject: ifcfg-rh: add internal API to re-read connection after write Our reader/writer has flaws. We easily write out something that is re-read differently. That is a problem and should be fixed. Add API to re-read the connection after writing. Extend the tests to check that the re-read value is identical to what we wrote. In some cases, this does not hold. That is usually a bug which needs fixing. Note that for certificate blobs and paths we may intentionally mutate the connection during writing, so there are valid cases where a connection is re-read differently. --- .../plugins/ifcfg-rh/nms-ifcfg-rh-connection.c | 4 + .../plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c | 2 +- .../plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 45 ++++++- .../plugins/ifcfg-rh/nms-ifcfg-rh-writer.h | 4 + .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 150 +++++++++++++++++---- 5 files changed, 174 insertions(+), 31 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-connection.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-connection.c index ba8ea323e..b54f9549a 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-connection.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-connection.c @@ -345,11 +345,15 @@ commit_changes (NMSettingsConnection *connection, success = writer_update_connection (NM_CONNECTION (connection), IFCFG_DIR, filename, + NULL, + NULL, &error); } else { success = writer_new_connection (NM_CONNECTION (connection), IFCFG_DIR, &ifcfg_path, + NULL, + NULL, &error); if (success) { nm_settings_connection_set_filename (connection, ifcfg_path); diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c index 414f593b6..3c5ae91ff 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-plugin.c @@ -687,7 +687,7 @@ add_connection (NMSettingsPlugin *config, return NULL; if (save_to_disk) { - if (!writer_new_connection (connection, IFCFG_DIR, &path, error)) + if (!writer_new_connection (connection, IFCFG_DIR, &path, NULL, NULL, error)) return NULL; } return NM_SETTINGS_CONNECTION (update_connection (self, connection, path, NULL, FALSE, NULL, error)); diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index ce38bf867..4e025c245 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -2714,6 +2714,8 @@ write_connection (NMConnection *connection, const char *ifcfg_dir, const char *filename, char **out_filename, + NMConnection **out_reread, + gboolean *out_reread_same, GError **error) { NMSettingConnection *s_con; @@ -2725,6 +2727,7 @@ write_connection (NMConnection *connection, nm_assert (NM_IS_CONNECTION (connection)); nm_assert (nm_connection_verify (connection, NULL)); + nm_assert (!out_reread || !*out_reread); if (!writer_can_write_connection (connection, error)) return FALSE; @@ -2850,6 +2853,40 @@ write_connection (NMConnection *connection, if (!svWriteFile (ifcfg, 0644, error)) return FALSE; + if (out_reread || out_reread_same) { + gs_unref_object NMConnection *reread = NULL; + gs_free_error GError *local = NULL; + gs_free char *unhandled = NULL; + gboolean reread_same = FALSE; + + reread = connection_from_file (ifcfg_name, &unhandled, &local, NULL); + + if (unhandled) { + _LOGW ("failure to re-read the new connection from file \"%s\": %s", + ifcfg_name, "connection is unhandled"); + g_clear_object (&reread); + } else if (!reread) { + _LOGW ("failure to re-read the new connection from file \"%s\": %s", + ifcfg_name, local ? local->message : ""); + } else { + if (out_reread_same) { + if (nm_connection_compare (reread, connection, NM_SETTING_COMPARE_FLAG_EXACT)) + reread_same = TRUE; + + nm_assert (reread_same == nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT)); + nm_assert (reread_same == ({ + gs_unref_hashtable GHashTable *_settings = NULL; + + ( nm_connection_diff (reread, connection, NM_SETTING_COMPARE_FLAG_EXACT, &_settings) + && !_settings); + })); + } + } + + NM_SET_OUT (out_reread, g_steal_pointer (&reread)); + NM_SET_OUT (out_reread_same, reread_same); + } + /* Only return the filename if this was a newly written ifcfg */ if (out_filename && !filename) *out_filename = g_steal_pointer (&ifcfg_name); @@ -2886,15 +2923,19 @@ gboolean writer_new_connection (NMConnection *connection, const char *ifcfg_dir, char **out_filename, + NMConnection **out_reread, + gboolean *out_reread_same, GError **error) { - return write_connection (connection, ifcfg_dir, NULL, out_filename, error); + return write_connection (connection, ifcfg_dir, NULL, out_filename, out_reread, out_reread_same, error); } gboolean writer_update_connection (NMConnection *connection, const char *ifcfg_dir, const char *filename, + NMConnection **out_reread, + gboolean *out_reread_same, GError **error) { if (utils_has_complex_routes (filename)) { @@ -2903,6 +2944,6 @@ writer_update_connection (NMConnection *connection, return FALSE; } - return write_connection (connection, ifcfg_dir, filename, NULL, error); + return write_connection (connection, ifcfg_dir, filename, NULL, out_reread, out_reread_same, error); } diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.h b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.h index 2662f79b0..9cd9513ec 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.h +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.h @@ -29,11 +29,15 @@ gboolean writer_can_write_connection (NMConnection *connection, gboolean writer_new_connection (NMConnection *connection, const char *ifcfg_dir, char **out_filename, + NMConnection **out_reread, + gboolean *out_reread_same, GError **error); gboolean writer_update_connection (NMConnection *connection, const char *ifcfg_dir, const char *filename, + NMConnection **out_reread, + gboolean *out_reread_same, GError **error); #endif /* _WRITER_H_ */ diff --git a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c index fd4bdfdf7..6b0395ba8 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -88,22 +88,72 @@ || ( _val_string && nm_streq0 (_val, _val_string))); \ } G_STMT_END -#define _writer_update_connection(connection, ifcfg_dir, filename) \ +static void +_assert_reread_same (NMConnection *connection, NMConnection *reread) +{ + nmtst_assert_connection_verifies_without_normalization (reread); + nmtst_assert_connection_equals (connection, TRUE, reread, FALSE); +} + +static void +_assert_reread_same_FIXME (NMConnection *connection, NMConnection *reread) +{ + gs_unref_object NMConnection *connection_normalized = NULL; + gs_unref_hashtable GHashTable *settings = NULL; + + /* FIXME: these assertion failures should not happen as we expect + * that re-reading a connection after write yields the same result. + * + * Needs investation and fixing. */ + nmtst_assert_connection_verifies_without_normalization (reread); + + connection_normalized = nmtst_connection_duplicate_and_normalize (connection); + + g_assert (!nm_connection_compare (connection_normalized, reread, NM_SETTING_COMPARE_FLAG_EXACT)); + g_assert (!nm_connection_diff (connection_normalized, reread, NM_SETTING_COMPARE_FLAG_EXACT, &settings)); +} + +#define _writer_update_connection_reread(connection, ifcfg_dir, filename, out_reread, out_reread_same) \ G_STMT_START { \ NMConnection *_connection = (connection); \ + NMConnection **_out_reread = (out_reread); \ + gboolean *_out_reread_same = (out_reread_same); \ const char *_ifcfg_dir = (ifcfg_dir); \ const char *_filename = (filename); \ GError *_error = NULL; \ gboolean _success; \ \ - g_assert (NM_IS_CONNECTION (connection)); \ g_assert (_ifcfg_dir && _ifcfg_dir[0]); \ g_assert (_filename && _filename[0]); \ \ - _success = writer_update_connection (_connection, _ifcfg_dir, _filename, &_error); \ + _success = writer_update_connection (_connection, _ifcfg_dir, _filename, _out_reread, _out_reread_same, &_error); \ nmtst_assert_success (_success, _error); \ } G_STMT_END +#define _writer_update_connection(connection, ifcfg_dir, filename) \ + G_STMT_START { \ + gs_unref_object NMConnection *_reread = NULL; \ + NMConnection *_c = (connection); \ + gboolean _reread_same = FALSE; \ + \ + _writer_update_connection_reread (_c, ifcfg_dir, filename, &_reread, &_reread_same); \ + _assert_reread_same (_c, _reread); \ + g_assert (_reread_same); \ + } G_STMT_END + +#define _writer_update_connection_FIXME(connection, ifcfg_dir, filename) \ + G_STMT_START { \ + gs_unref_object NMConnection *_reread = NULL; \ + NMConnection *_c = (connection); \ + gboolean _reread_same = FALSE; \ + \ + /* FIXME: this should not happen. Fix to use _writer_update_connection() */ \ + \ + _writer_update_connection_reread (_c, ifcfg_dir, filename, &_reread, &_reread_same); \ + _assert_reread_same_FIXME (_c, _reread); \ + g_assert (!_reread_same); \ + } G_STMT_END + static NMConnection * _connection_from_file (const char *filename, const char *network_file, @@ -147,14 +197,18 @@ _connection_from_file_fail (const char *filename, } static void -_writer_new_connection (NMConnection *connection, - const char *ifcfg_dir, - char **out_filename) +_writer_new_connection_reread (NMConnection *connection, + const char *ifcfg_dir, + char **out_filename, + NMConnection **out_reread, + gboolean *out_reread_same) { gboolean success; GError *error = NULL; char *filename = NULL; gs_unref_object NMConnection *con_verified = NULL; + gs_unref_object NMConnection *reread_copy = NULL; + NMConnection **reread = out_reread ?: ((nmtst_get_rand_int () % 2) ? &reread_copy : NULL); g_assert (NM_IS_CONNECTION (connection)); g_assert (ifcfg_dir); @@ -164,21 +218,55 @@ _writer_new_connection (NMConnection *connection, success = writer_new_connection (con_verified, ifcfg_dir, &filename, + reread, + out_reread_same, &error); nmtst_assert_success (success, error); g_assert (filename && filename[0]); + if (reread) + nmtst_assert_connection_verifies_without_normalization (*reread); + if (out_filename) *out_filename = filename; else g_free (filename); } +static void +_writer_new_connection (NMConnection *connection, + const char *ifcfg_dir, + char **out_filename) +{ + gs_unref_object NMConnection *reread = NULL; + gboolean reread_same = FALSE; + + _writer_new_connection_reread (connection, ifcfg_dir, out_filename, &reread, &reread_same); + _assert_reread_same (connection, reread); + g_assert (reread_same); +} + +static void +_writer_new_connection_FIXME (NMConnection *connection, + const char *ifcfg_dir, + char **out_filename) +{ + gs_unref_object NMConnection *reread = NULL; + gboolean reread_same = FALSE; + + /* FIXME: this should not happen. Fix it to use _writer_new_connection() instead. */ + + _writer_new_connection_reread (connection, ifcfg_dir, out_filename, &reread, &reread_same); + _assert_reread_same_FIXME (connection, reread); + g_assert (!reread_same); +} + static void _writer_new_connection_fail (NMConnection *connection, const char *ifcfg_dir, GError **error) { + gs_unref_object NMConnection *reread = NULL; gboolean success; GError *local = NULL; char *filename = NULL; @@ -189,9 +277,12 @@ _writer_new_connection_fail (NMConnection *connection, success = writer_new_connection (connection, ifcfg_dir, &filename, + &reread, + NULL, &local); nmtst_assert_no_success (success, local); g_assert (!filename); + g_assert (!reread); g_propagate_error (error, local); } @@ -1567,12 +1658,15 @@ test_read_write_802_1X_subj_matches (void) g_assert_cmpstr (nm_setting_802_1x_get_phase2_altsubject_match (s_8021x, 0), ==, "x.yourdomain.tld"); g_assert_cmpstr (nm_setting_802_1x_get_phase2_altsubject_match (s_8021x, 1), ==, "y.yourdomain.tld"); + g_test_expect_message ("NetworkManager", G_LOG_LEVEL_MESSAGE, + "*missing IEEE_8021X_CA_CERT for EAP method 'peap'; this is insecure!"); _writer_new_connection (connection, TEST_SCRATCH_DIR "/network-scripts/", &testfile); + g_test_assert_expected_messages (); g_test_expect_message ("NetworkManager", G_LOG_LEVEL_MESSAGE, - "*missing IEEE_8021X_CA_CERT*peap*"); + "*missing IEEE_8021X_CA_CERT for EAP method 'peap'; this is insecure!"); reread = _connection_from_file (testfile, NULL, TYPE_ETHERNET, NULL); g_test_assert_expected_messages (); @@ -1832,9 +1926,9 @@ test_clear_master (void) g_assert_cmpstr (nm_setting_connection_get_slave_type (s_con), ==, NULL); /* 4. update the connection on disk */ - _writer_update_connection (connection, - TEST_SCRATCH_DIR "/network-scripts/", - testfile); + _writer_update_connection_FIXME (connection, + TEST_SCRATCH_DIR "/network-scripts/", + testfile); keyfile = utils_get_keys_path (testfile); g_assert (!g_file_test (keyfile, G_FILE_TEST_EXISTS)); @@ -1917,9 +2011,9 @@ test_write_dns_options (void) nmtst_assert_connection_verifies (connection); - _writer_new_connection (connection, - TEST_SCRATCH_DIR "/network-scripts/", - &testfile); + _writer_new_connection_FIXME (connection, + TEST_SCRATCH_DIR "/network-scripts/", + &testfile); reread = _connection_from_file (testfile, NULL, TYPE_ETHERNET, NULL); @@ -3781,9 +3875,9 @@ test_write_wired_static (void) nmtst_assert_connection_verifies (connection); - _writer_new_connection (connection, - TEST_SCRATCH_DIR "/network-scripts/", - &testfile); + _writer_new_connection_FIXME (connection, + TEST_SCRATCH_DIR "/network-scripts/", + &testfile); route6file = utils_get_route6_path (testfile); reread = _connection_from_file (testfile, NULL, TYPE_ETHERNET, NULL); @@ -4451,9 +4545,9 @@ test_write_wired_8021x_tls (gconstpointer test_data) nmtst_assert_connection_verifies (connection); - _writer_new_connection (connection, - TEST_SCRATCH_DIR "/network-scripts/", - &testfile); + _writer_new_connection_FIXME (connection, + TEST_SCRATCH_DIR "/network-scripts/", + &testfile); reread = _connection_from_file (testfile, NULL, TYPE_WIRELESS, NULL); @@ -4590,9 +4684,9 @@ test_write_wired_aliases (void) svCloseFile (ifcfg); g_assert (g_file_test (TEST_SCRATCH_ALIAS_BASE ":5", G_FILE_TEST_EXISTS)); - _writer_new_connection (connection, - TEST_SCRATCH_DIR "/network-scripts/", - &testfile); + _writer_new_connection_FIXME (connection, + TEST_SCRATCH_DIR "/network-scripts/", + &testfile); /* Re-check the alias files */ g_assert (g_file_test (TEST_SCRATCH_ALIAS_BASE ":2", G_FILE_TEST_EXISTS)); @@ -5444,9 +5538,9 @@ test_write_wifi_leap_secret_flags (gconstpointer data) nmtst_assert_connection_verifies (connection); - _writer_new_connection (connection, - TEST_SCRATCH_DIR "/network-scripts/", - &testfile); + _writer_new_connection_FIXME (connection, + TEST_SCRATCH_DIR "/network-scripts/", + &testfile); reread = _connection_from_file (testfile, NULL, TYPE_WIRELESS, NULL); @@ -6592,9 +6686,9 @@ test_write_wifi_wep_agent_keys (void) nmtst_assert_connection_verifies (connection); - _writer_new_connection (connection, - TEST_SCRATCH_DIR "/network-scripts/", - &testfile); + _writer_new_connection_FIXME (connection, + TEST_SCRATCH_DIR "/network-scripts/", + &testfile); reread = _connection_from_file (testfile, NULL, TYPE_WIRELESS, NULL); -- cgit v1.2.3