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.

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 : "<unknown>");
+		} 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,10 +218,15 @@ _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
@@ -175,10 +234,39 @@ _writer_new_connection (NMConnection *connection,
 }
 
 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);