settings/keyfile: check whether profile can be re-read before writing to disk and fail

First of all, keyfile writer (and reader) are supposed to be able to store
every profile to disk and re-read a valid profile back. Note that the profile
might be modified in the process, for example, blob certificates are written
to a file. So, the result might no be exactly the same, but it must still be
valid (and should only diverge in expected ways from the original, like mangled
certificates).

Previously, we would re-read the profile after writing to disk. If that failed,
we would only fail an assertion but otherwise proceeed. It is a bug
after all. However, it's bad to check only after writing to file,
because it results in a unreadable profile on disk, and in the first
moment it appears that noting went wrong. Instead, we should fail early.

Note that nms_keyfile_reader_from_keyfile() must entirely operate on the in-memory
representation of the keyfile. It must not actually access any files on disk. Hence,
moving this check before writing the profile must work. Otherwise, that would be
a separate bug. Actually, keyfile reader and writer violate this. I
added FIXME comments for that. But it doesn't interfere with this
patch.

2 files changed, 45 insertions, 34 deletions

diff --git a/libnm-core/nm-keyfile.c b/libnm-core/nm-keyfile.c
index 9b032c4cd8..dbffe65e18 100644
--- a/libnm-core/nm-keyfile.c
+++ b/libnm-core/nm-keyfile.c
@@ -1369,6 +1369,9 @@ nm_keyfile_detect_unqualified_path_scheme (const char *base_dir,
 	 */
 
 	path = get_cert_path (base_dir, (const guint8 *) data, data_len);
+
+	/* FIXME(keyfile-parse-in-memory): it is wrong that keyfile reader makes decisions based on
+	 * the file systems content. The serialization/parsing should be entirely in-memory. */
 	if (   !memchr (data, '/', data_len)
 	    && !has_cert_ext (path)) {
 		if (!consider_exists)
@@ -1449,6 +1452,10 @@ cert_parser (KeyfileReaderInfo *info, NMSetting *setting, const char *key)
 			path2 = path2_free;
 		}
 
+		/* FIXME(keyfile-parse-in-memory): keyfile reader must not access the file system and
+		 * (in a first step) only operate in memory-only. If the presence of files should be checked,
+		 * then by invoking a callback (and possibly keyfile settings plugin would
+		 * collect the file names to be checked and check them later). */
 		if (!g_file_test (path2, G_FILE_TEST_EXISTS)) {
 			handle_warn (info, key, NM_KEYFILE_WARN_SEVERITY_INFO_MISSING_FILE,
 			             _("certificate or key file '%s' does not exist"),
diff --git a/src/settings/plugins/keyfile/nms-keyfile-writer.c b/src/settings/plugins/keyfile/nms-keyfile-writer.c
index 464832d103..515376f8df 100644
--- a/src/settings/plugins/keyfile/nms-keyfile-writer.c
+++ b/src/settings/plugins/keyfile/nms-keyfile-writer.c
@@ -126,6 +126,10 @@ cert_writer (NMConnection *connection,
 		new_path = g_strdup_printf ("%s/%s-%s.%s", info->keyfile_dir, nm_connection_get_uuid (connection),
 		                            cert_data->vtable->file_suffix, ext);
 
+		/* FIXME(keyfile-parse-in-memory): writer must not access/write to the file system before
+		 * being sure that the entire profile can be written and all circumstances are good to
+		 * proceed. That means, while writing we must only collect the blogs in-memory, and write
+		 * them all in the end together (or not at all). */
 		success = nm_utils_file_set_contents (new_path,
 		                                      (const char *) blob_data,
 		                                      blob_len,
@@ -197,10 +201,12 @@ _internal_write_connection (NMConnection *connection,
 	gs_free char *path = NULL;
 	const char *id;
 	WriteInfo info = { 0 };
-	GError *local_err = NULL;
+	gs_free_error GError *local_err = NULL;
 	int errsv;
 	gboolean rename;
 	int i_path;
+	gs_unref_object NMConnection *reread = NULL;
+	gboolean reread_same = FALSE;
 
 	g_return_val_if_fail (!out_path || !*out_path, FALSE);
 	g_return_val_if_fail (keyfile_dir && keyfile_dir[0] == '/', FALSE);
@@ -313,6 +319,35 @@ _internal_write_connection (NMConnection *connection,
 		return FALSE;
 	}
 
+	if (   out_reread
+	    || out_reread_same) {
+		gs_free_error GError *reread_error = NULL;
+
+		reread = nms_keyfile_reader_from_keyfile (kf_file, path, NULL, profile_dir, FALSE, &reread_error);
+
+		if (   !reread
+		    || !nm_connection_normalize (reread, NULL, NULL, &reread_error)) {
+			nm_log_err (LOGD_SETTINGS, "BUG: the profile cannot be stored in keyfile format without becoming unusable: %s", reread_error->message);
+			g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED,
+			             "keyfile writer produces an invalid connection: %s",
+			             reread_error->message);
+			nm_assert_not_reached ();
+			return FALSE;
+		}
+
+		if (out_reread_same) {
+			reread_same = !!nm_connection_compare (reread, connection, NM_SETTING_COMPARE_FLAG_EXACT);
+
+			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_utils_file_set_contents (path,
 	                            kf_content_buf,
 	                            kf_content_len,
@@ -323,7 +358,6 @@ _internal_write_connection (NMConnection *connection,
 		g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED,
 		             "error writing to file '%s': %s",
 		             path, local_err->message);
-		g_error_free (local_err);
 		return FALSE;
 	}
 
@@ -344,38 +378,8 @@ _internal_write_connection (NMConnection *connection,
 	    && !nm_streq (path, existing_path))
 		unlink (existing_path);
 
-	if (out_reread || out_reread_same) {
-		gs_unref_object NMConnection *reread = NULL;
-		gs_free_error GError *reread_error = NULL;
-		gboolean reread_same = FALSE;
-
-		reread = nms_keyfile_reader_from_keyfile (kf_file, path, NULL, profile_dir, FALSE, NULL);
-
-		nm_assert (NM_IS_CONNECTION (reread));
-
-		if (   reread
-		    && !nm_connection_normalize (reread, NULL, NULL, &reread_error)) {
-			nm_log_err (LOGD_SETTINGS, "BUG: failure to normalize profile that we just wrote to disk: %s", reread_error->message);
-			nm_assert_not_reached ();
-			g_clear_object (&reread);
-		}
-
-		if (reread && out_reread_same) {
-			reread_same = !!nm_connection_compare (reread, connection, NM_SETTING_COMPARE_FLAG_EXACT);
-
-			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);
-	}
-
+	NM_SET_OUT (out_reread, g_steal_pointer (&reread));
+	NM_SET_OUT (out_reread_same, reread_same);
 	NM_SET_OUT (out_path, g_steal_pointer (&path));
 
 	return TRUE;