libnm/keyfile: sort keyfile entries and nm_connection_for_each_setting_value()

Fix the order for keyfile writer. It is nicer to have a fixed, sensible
order with [connection] first.

Do this by sorting the order in nm_connection_for_each_setting_value()
and nm_setting_enumerate_values().

https://mail.gnome.org/archives/networkmanager-list/2015-March/msg00050.html

4 files changed, 221 insertions, 71 deletions

diff --git a/include/nm-test-utils.h b/include/nm-test-utils.h
index 187a0f1931..ddea03745d 100644
--- a/include/nm-test-utils.h
+++ b/include/nm-test-utils.h
@@ -32,6 +32,7 @@
 #include <errno.h>
 
 #include "nm-utils.h"
+#include "nm-utils-internal.h"
 #include "nm-glib-compat.h"
 #include "gsystem-local-alloc.h"
 
@@ -91,13 +92,13 @@ nmtst_initialized (void)
 	return !!__nmtst_internal.rand0;
 }
 
-#define __NMTST_LOG(cmd, fmt, ...) \
+#define __NMTST_LOG(cmd, ...) \
 	G_STMT_START { \
 		g_assert (nmtst_initialized ()); \
 		if (!__nmtst_internal.assert_logging || __nmtst_internal.no_expect_message) { \
-			cmd (fmt, __VA_ARGS__); \
+			cmd (__VA_ARGS__); \
 		} else { \
-			printf (fmt "\n", __VA_ARGS__); \
+			printf (_NM_UTILS_MACRO_FIRST (__VA_ARGS__) "\n" _NM_UTILS_MACRO_REST (__VA_ARGS__)); \
 		} \
 	} G_STMT_END
 
@@ -870,19 +871,40 @@ nmtst_assert_connection_equals (NMConnection *a, gboolean normalize_a, NMConnect
 		b = b2 = nmtst_connection_duplicate_and_normalize (b);
 
 	compare = nm_connection_diff (a, b, NM_SETTING_COMPARE_FLAG_EXACT, &out_settings);
-	if (!compare && out_settings) {
+	if (!compare || out_settings) {
 		const char *name, *pname;
 		GHashTable *setting;
 		GHashTableIter iter, iter2;
 
-		g_hash_table_iter_init (&iter, out_settings);
-		while (g_hash_table_iter_next (&iter, (gpointer *) &name, (gpointer *) &setting)) {
-			__NMTST_LOG (g_message, ">>> differences in setting '%s':", name);
+		__NMTST_LOG (g_message, ">>> ASSERTION nmtst_assert_connection_equals() fails");
+		if (out_settings) {
+			g_hash_table_iter_init (&iter, out_settings);
+			while (g_hash_table_iter_next (&iter, (gpointer *) &name, (gpointer *) &setting)) {
+				__NMTST_LOG (g_message, ">>> differences in setting '%s':", name);
 
-			g_hash_table_iter_init (&iter2, out_settings);
-			while (g_hash_table_iter_next (&iter2, (gpointer *) &pname, NULL))
-				__NMTST_LOG (g_message, ">>> differences in setting '%s.%s':", name, pname);
+				g_hash_table_iter_init (&iter2, setting);
+				while (g_hash_table_iter_next (&iter2, (gpointer *) &pname, NULL))
+					__NMTST_LOG (g_message, ">>> differences in setting '%s.%s'", name, pname);
+			}
 		}
+
+#ifdef __NM_KEYFILE_INTERNAL_H__
+		{
+			gs_unref_keyfile GKeyFile *kf_a = NULL, *kf_b = NULL;
+			gs_free char *str_a = NULL, *str_b = NULL;
+
+			kf_a = nm_keyfile_write (a, NULL, NULL, NULL);
+			kf_b = nm_keyfile_write (b, NULL, NULL, NULL);
+
+			if (kf_a)
+				str_a = g_key_file_to_data (kf_a, NULL, NULL);
+			if (kf_b)
+				str_b = g_key_file_to_data (kf_b, NULL, NULL);
+
+			__NMTST_LOG (g_message, ">>> Connection A as kf (*WARNING: keyfile representation might not show the difference*):\n%s", str_a);
+			__NMTST_LOG (g_message, ">>> Connection B as kf (*WARNING: keyfile representation might not show the difference*):\n%s", str_b);
+		}
+#endif
 	}
 	g_assert (compare);
 	g_assert (!out_settings);
diff --git a/libnm-core/nm-connection.c b/libnm-core/nm-connection.c
index 79ff31d89d..acdc8b89c7 100644
--- a/libnm-core/nm-connection.c
+++ b/libnm-core/nm-connection.c
@@ -30,6 +30,7 @@
 #include "nm-utils.h"
 #include "nm-setting-private.h"
 #include "nm-core-internal.h"
+#include "gsystem-local-alloc.h"
 
 /**
  * SECTION:nm-connection
@@ -1306,6 +1307,19 @@ nm_connection_is_type (NMConnection *connection, const char *type)
 	return (g_strcmp0 (type2, type) == 0);
 }
 
+static int
+_for_each_sort (NMSetting **p_a, NMSetting **p_b, void *unused)
+{
+	NMSetting *a = *p_a;
+	NMSetting *b = *p_b;
+	int c;
+
+	c = _nm_setting_compare_priority (a, b);
+	if (c != 0)
+		return c;
+	return strcmp (nm_setting_get_name (a), nm_setting_get_name (b));
+}
+
 /**
  * nm_connection_for_each_setting_value:
  * @connection: the #NMConnection
@@ -1320,15 +1334,39 @@ nm_connection_for_each_setting_value (NMConnection *connection,
                                       NMSettingValueIterFn func,
                                       gpointer user_data)
 {
+	NMConnectionPrivate *priv;
+	gs_free NMSetting **arr_free = NULL;
+	NMSetting *arr_temp[20], **arr;
 	GHashTableIter iter;
 	gpointer value;
+	guint i, size;
 
 	g_return_if_fail (NM_IS_CONNECTION (connection));
 	g_return_if_fail (func != NULL);
 
-	g_hash_table_iter_init (&iter, NM_CONNECTION_GET_PRIVATE (connection)->settings);
-	while (g_hash_table_iter_next (&iter, NULL, &value))
-		nm_setting_enumerate_values (NM_SETTING (value), func, user_data);
+	priv = NM_CONNECTION_GET_PRIVATE (connection);
+
+	size = g_hash_table_size (priv->settings);
+	if (!size)
+		return;
+
+	if (size > G_N_ELEMENTS (arr_temp))
+		arr = arr_free = g_new (NMSetting *, size);
+	else
+		arr = arr_temp;
+
+	g_hash_table_iter_init (&iter, priv->settings);
+	for (i = 0; g_hash_table_iter_next (&iter, NULL, &value); i++)
+		arr[i] = NM_SETTING (value);
+	g_assert (i == size);
+
+	/* sort the settings. This has an effect on the order in which keyfile
+	 * prints them. */
+	if (size > 1)
+		g_qsort_with_data (arr, size, sizeof (NMSetting *), (GCompareDataFunc) _for_each_sort, NULL);
+
+	for (i = 0; i < size; i++)
+		nm_setting_enumerate_values (arr[i], func, user_data);
 }
 
 /**
diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c
index 2730742654..a292bfca6e 100644
--- a/libnm-core/nm-setting.c
+++ b/libnm-core/nm-setting.c
@@ -1314,6 +1314,33 @@ nm_setting_diff (NMSetting *a,
 	return !(*results);
 }
 
+#define CMP_AND_RETURN(n_a, n_b, name) \
+	G_STMT_START { \
+		gboolean _is = (strcmp (n_a, ""name) == 0); \
+		\
+		if (_is || (strcmp (n_b, ""name) == 0)) \
+			return _is ? -1 : 1; \
+	} G_STMT_END;
+
+static int
+_enumerate_values_sort (GParamSpec **p_a, GParamSpec **p_b, GType *p_type)
+{
+	const char *n_a = (*p_a)->name;
+	const char *n_b = (*p_b)->name;
+	int c = strcmp (n_a, n_b);
+
+	if (c) {
+		if (*p_type == NM_TYPE_SETTING_CONNECTION) {
+			/* for [connection], report first id, uuid, type in that order. */
+			CMP_AND_RETURN (n_a, n_b, NM_SETTING_CONNECTION_ID);
+			CMP_AND_RETURN (n_a, n_b, NM_SETTING_CONNECTION_UUID);
+			CMP_AND_RETURN (n_a, n_b, NM_SETTING_CONNECTION_TYPE);
+		}
+	}
+	return c;
+}
+#undef CMP_AND_RETURN
+
 /**
  * nm_setting_enumerate_values:
  * @setting: the #NMSetting
@@ -1331,11 +1358,19 @@ nm_setting_enumerate_values (NMSetting *setting,
 	GParamSpec **property_specs;
 	guint n_property_specs;
 	int i;
+	GType type;
 
 	g_return_if_fail (NM_IS_SETTING (setting));
 	g_return_if_fail (func != NULL);
 
 	property_specs = g_object_class_list_properties (G_OBJECT_GET_CLASS (setting), &n_property_specs);
+
+	/* sort the properties. This has an effect on the order in which keyfile
+	 * prints them. */
+	type = G_OBJECT_TYPE (setting);
+	g_qsort_with_data (property_specs, n_property_specs, sizeof (gpointer),
+	                   (GCompareDataFunc) _enumerate_values_sort, &type);
+
 	for (i = 0; i < n_property_specs; i++) {
 		GParamSpec *prop_spec = property_specs[i];
 		GValue value = G_VALUE_INIT;
diff --git a/libnm-core/tests/test-keyfile.c b/libnm-core/tests/test-keyfile.c
index 43958cb6bb..0f0548ad85 100644
--- a/libnm-core/tests/test-keyfile.c
+++ b/libnm-core/tests/test-keyfile.c
@@ -114,6 +114,46 @@ _keyfile_equals (GKeyFile *kf_a, GKeyFile *kf_b)
 	return _keyfile_a_contains_all_in_b (kf_a, kf_b) && _keyfile_a_contains_all_in_b (kf_b, kf_a);
 }
 
+static GKeyFile *
+_nm_keyfile_write (NMConnection *connection,
+                   NMKeyfileWriteHandler handler,
+                   void *user_data)
+{
+	GError *error = NULL;
+	GKeyFile *kf;
+
+	g_assert (NM_IS_CONNECTION (connection));
+
+	kf = nm_keyfile_write (connection, handler, user_data, &error);
+	g_assert_no_error (error);
+	g_assert (kf);
+	return kf;
+}
+
+static NMConnection *
+_nm_keyfile_read (GKeyFile *keyfile,
+                  const char *keyfile_name,
+                  const char *base_dir,
+                  NMKeyfileReadHandler read_handler,
+                  void *read_data,
+                  gboolean needs_normalization)
+{
+	GError *error = NULL;
+	NMConnection *con;
+
+	g_assert (keyfile);
+
+	con = nm_keyfile_read (keyfile, keyfile_name, base_dir, read_handler, read_data, &error);
+	g_assert_no_error (error);
+	g_assert (NM_IS_CONNECTION (con));
+	if (needs_normalization)
+		nmtst_assert_connection_verifies_after_normalization (con, 0, 0);
+	else
+		nmtst_assert_connection_verifies_without_normalization (con);
+	return con;
+}
+
+
 static void
 _keyfile_convert (NMConnection **con,
                   GKeyFile **keyfile,
@@ -125,10 +165,10 @@ _keyfile_convert (NMConnection **con,
                   void *write_data,
                   gboolean needs_normalization)
 {
-	NMConnection *c, *c2;
-	GKeyFile *k, *k2;
-	GError *error = NULL;
-	NMSetting8021x *s1, *s2;
+	NMConnection *c0;
+	GKeyFile *k0;
+	gs_unref_object NMConnection *c0_k1_c2 = NULL, *k0_c1 = NULL, *k0_c1_k2_c3 = NULL;
+	gs_unref_keyfile GKeyFile *k0_c1_k2 = NULL, *c0_k1 = NULL, *c0_k1_c2_k3 = NULL;
 
 	/* convert from @con to @keyfile and check that we can make
 	 * full round trips and obtaining the same result. */
@@ -137,64 +177,79 @@ _keyfile_convert (NMConnection **con,
 	g_assert (keyfile);
 	g_assert (*con || *keyfile);
 
-	if (!*keyfile) {
-		k = nm_keyfile_write (*con, write_handler, read_data, &error);
-		g_assert_no_error (error);
-		g_assert (k);
-		*keyfile = k;
-	} else
-		k = *keyfile;
-	if (!*con) {
-		c = nm_keyfile_read (*keyfile, keyfile_name, base_dir, read_handler, read_data, &error);
-		g_assert_no_error (error);
-		g_assert (c);
-		if (needs_normalization)
-			nmtst_assert_connection_verifies_after_normalization (c, 0, 0);
-		else
-			nmtst_assert_connection_verifies_without_normalization (c);
-		*con = c;
-	} else
-		c = *con;
-
-	k2 = nm_keyfile_write (c, write_handler, read_data, &error);
-	g_assert_no_error (error);
-	g_assert (k2);
+	c0 = *con;
+	k0 = *keyfile;
 
-	c2 = nm_keyfile_read (k, keyfile_name, base_dir, read_handler, read_data, &error);
-	g_assert_no_error (error);
-	g_assert (c2);
-	if (needs_normalization)
-		nmtst_assert_connection_verifies_after_normalization (c2, 0, 0);
-	else
-		nmtst_assert_connection_verifies_without_normalization (c2);
-
-	s1 = nm_connection_get_setting_802_1x (*con);
-	s2 = nm_connection_get_setting_802_1x (c2);
-	if (s1 || s2) {
-		g_assert_cmpint (nm_setting_802_1x_get_ca_cert_scheme (s1), ==, nm_setting_802_1x_get_ca_cert_scheme (s2));
-		switch (nm_setting_802_1x_get_ca_cert_scheme (s1)) {
-		case NM_SETTING_802_1X_CK_SCHEME_PATH:
-			nmtst_assert_resolve_relative_path_equals (nm_setting_802_1x_get_ca_cert_path (s1), nm_setting_802_1x_get_ca_cert_path (s2));
-			break;
-		case NM_SETTING_802_1X_CK_SCHEME_BLOB: {
-			GBytes *b1, *b2;
-
-			b1 = nm_setting_802_1x_get_ca_cert_blob (s1);
-			b2 = nm_setting_802_1x_get_ca_cert_blob (s2);
-			g_assert_cmpint (g_bytes_get_size (b1), ==, g_bytes_get_size (b2));
-			g_assert (memcmp (g_bytes_get_data (b1, NULL), g_bytes_get_data (b2, NULL), g_bytes_get_size (b1)) == 0);
-			break;
-		}
-		default:
-			break;
-		}
+	if (c0) {
+		c0_k1 = _nm_keyfile_write (c0, write_handler, write_data);
+		c0_k1_c2 = _nm_keyfile_read (c0_k1, keyfile_name, base_dir, read_handler, read_data, FALSE);
+		c0_k1_c2_k3 = _nm_keyfile_write (c0_k1_c2, write_handler, write_data);
+
+		_keyfile_equals (c0_k1, c0_k1_c2_k3);
 	}
+	if (k0) {
+		NMSetting8021x *s1, *s2;
+
+		k0_c1 = _nm_keyfile_read (k0, keyfile_name, base_dir, read_handler, read_data, needs_normalization);
+		k0_c1_k2 = _nm_keyfile_write (k0_c1, write_handler, write_data);
+		k0_c1_k2_c3 = _nm_keyfile_read (k0_c1_k2, keyfile_name, base_dir, read_handler, read_data, FALSE);
+
+		/* It is a expeced behavior, that if @k0 contains a relative path ca-cert, @k0_c1 will
+		 * contain that path as relative. But @k0_c1_k2 and @k0_c1_k2_c3 will have absolute paths.
+		 * In this case, hack up @k0_c1_k2_c3 to contain the same relative path. */
+		s1 = nm_connection_get_setting_802_1x (k0_c1);
+		s2 = nm_connection_get_setting_802_1x (k0_c1_k2_c3);
+		if (s1 || s2) {
+			g_assert_cmpint (nm_setting_802_1x_get_ca_cert_scheme (s1), ==, nm_setting_802_1x_get_ca_cert_scheme (s2));
+			switch (nm_setting_802_1x_get_ca_cert_scheme (s1)) {
+			case NM_SETTING_802_1X_CK_SCHEME_PATH:
+				{
+					const char *p1 = nm_setting_802_1x_get_ca_cert_path (s1);
+					const char *p2 = nm_setting_802_1x_get_ca_cert_path (s2);
+
+					nmtst_assert_resolve_relative_path_equals (p1, p2);
+					if (strcmp (p1, p2) != 0) {
+						gs_free char *puri = NULL;
+						gs_unref_bytes GBytes *pfile = NULL;
+
+						g_assert (p1[0] != '/' && p2[0] == '/');
+
+						/* one of the paths is a relative path and the other is absolute. This is an
+						 * expected difference.
+						 * Make the paths of s2 identical to s1... */
+						puri = g_strconcat (NM_SETTING_802_1X_CERT_SCHEME_PREFIX_PATH, p1, NULL);
+						pfile = g_bytes_new (puri, strlen (puri) + 1);
+						g_object_set (s2, NM_SETTING_802_1X_CA_CERT, pfile, NULL);
+					}
+				}
+				break;
+			case NM_SETTING_802_1X_CK_SCHEME_BLOB: {
+				GBytes *b1, *b2;
+
+				b1 = nm_setting_802_1x_get_ca_cert_blob (s1);
+				b2 = nm_setting_802_1x_get_ca_cert_blob (s2);
+				g_assert_cmpint (g_bytes_get_size (b1), ==, g_bytes_get_size (b2));
+				g_assert (memcmp (g_bytes_get_data (b1, NULL), g_bytes_get_data (b2, NULL), g_bytes_get_size (b1)) == 0);
+				break;
+			}
+			default:
+				break;
+			}
+		}
 
-	nmtst_assert_connection_equals (c2, FALSE, *con, FALSE);
-	_keyfile_equals (k2, *keyfile);
+		nmtst_assert_connection_equals (k0_c1, FALSE, k0_c1_k2_c3, FALSE);
+	}
 
-	g_object_unref (c2);
-	g_key_file_unref (k2);
+	if (!k0)
+		*keyfile = g_key_file_ref (c0_k1);
+	else if (!c0)
+		*con = g_object_ref (k0_c1);
+	else {
+		/* finally, if both a keyfile and a connection are given, assert that they are equal
+		 * after a round of conversion. */
+		_keyfile_equals (c0_k1, k0_c1_k2);
+		nmtst_assert_connection_equals (k0_c1, FALSE, c0_k1_c2, FALSE);
+	}
 }
 
 /******************************************************************************/