config: fix order of processing [connection] sections in NMConfig

We support the "NetworkManager.conf" sections '[connection]' and
'[connection.\+]' (with arbitrary suffix).

Fix the order of how we evaluate these section.

Note that the literal '[connection]' section is always evaluated lastly
after any other '[connection.\+]' section.

Within one file, we want to evaluate the sections in top-to-bottom
order. But accross multiple files, we want to order them
later-files-first. That gives a reasonable behavior if the user
looks at one file, and also if he wants to overwrite configuration
via configuration snippets like "conf.d/99-last.conf".

Note that if a later file extends/overwrites a section defined in an
earlier file, the section is still considered with lower priority
This is intentional, because the user ~extends~ a lower priority
section. If he wants to add a higher priority section, he should
choose a new suffix.

Fixes: dc0193ac02

man/NetworkManager.conf.xml.in

@@ -482,7 +482,7 @@ ipv6.ip6-privacy=1
     </para>
 
     <para>
-        The sections are considered in order of appearance, with the
+        The sections within one file are considered in order of appearance, with the
         exception that the <literal>[connection]</literal> section is always
         considered last. In the example above, this order is <literal>[connection-wifi-wlan0]</literal>,
         <literal>[connection-wlan-other]</literal>, and <literal>[connection]</literal>.
@@ -495,6 +495,11 @@ ipv6.ip6-privacy=1
         "[connection-wifi-wlan0]" matches the device, it does not contain that property
         and the search continues.
     </para>
+    <para>
+        When having different sections in multiple files, sections from files that are read
+        later have higher priority. So within one file the priority of the sections is
+        top-to-bottom. Across multiple files later definitions take precedence.
+    </para>
 
     <para>
       <variablelist>

src/nm-config-data.c

@@ -225,58 +225,63 @@ nm_config_data_get_connection_default (const NMConfigData *self,
 	return NULL;
 }
 
-static ConnectionInfo *
+static void
-_get_connection_infos (GKeyFile *keyfile)
+_get_connection_info_init (ConnectionInfo *connection_info, GKeyFile *keyfile, char *group)
 {
-	char **groups;
-	guint i;
-	char *connection_tag = NULL;
-	GSList *connection_groups = NULL;
-	ConnectionInfo *connection_infos = NULL;
-
-	/* get the list of existing [connection.\+] sections that we consider
-	 * for nm_config_data_get_connection_default(). Also, get them
-	 * in the right order. */
-	groups = g_key_file_get_groups (keyfile, NULL);
-	for (i = 0; groups && groups[i]; i++) {
-		if (g_str_has_prefix (groups[i], "connection")) {
-			if (strlen (groups[i]) == STRLEN ("connection"))
-				connection_tag = groups[i];
-			else
-				connection_groups = g_slist_prepend (connection_groups, groups[i]);
-		} else
-			g_free (groups[i]);
-	}
-	g_free (groups);
-	if (connection_tag) {
-		/* We want the group "connection" checked at last, so that
-		 * all other "connection.\+" have preference. Those other
-		 * groups are checked in order of appearance. */
-		connection_groups = g_slist_prepend (connection_groups, connection_tag);
-	}
-	if (connection_groups) {
-		guint len = g_slist_length (connection_groups);
-		GSList *iter;
-
-		connection_infos = g_new0 (ConnectionInfo, len + 1);
-		for (iter = connection_groups; iter; iter = iter->next) {
-			ConnectionInfo *connection_info;
 	char *value;
 
-			nm_assert (len >= 1);
+	/* pass ownership of @group on... */
-			connection_info = &connection_infos[--len];
+	connection_info->group_name = group;
-			connection_info->group_name = iter->data;
 
-			value = g_key_file_get_value (keyfile, iter->data, "match-device", NULL);
+	value = g_key_file_get_value (keyfile, group, "match-device", NULL);
 	if (value) {
 		connection_info->match_device.has = TRUE;
 		connection_info->match_device.spec = nm_match_spec_split (value);
 		g_free (value);
 	}
-			connection_info->stop_match = nm_config_keyfile_get_boolean (keyfile, iter->data, "stop-match", FALSE);
+	connection_info->stop_match = nm_config_keyfile_get_boolean (keyfile, group, "stop-match", FALSE);
 }
-		g_slist_free (connection_groups);
+
+static ConnectionInfo *
+_get_connection_infos (GKeyFile *keyfile)
+{
+	char **groups;
+	gsize i, j, ngroups;
+	char *connection_tag = NULL;
+	ConnectionInfo *connection_infos = NULL;
+
+	/* get the list of existing [connection.\+] sections that we consider
+	 * for nm_config_data_get_connection_default().
+	 *
+	 * We expect the sections in their right order, with lowest priority
+	 * first. Only exception is the (literal) [connection] section, which
+	 * we will always reorder to the end. */
+	groups = g_key_file_get_groups (keyfile, &ngroups);
+	if (!groups)
+		ngroups = 0;
+	else if (ngroups > 0) {
+		for (i = 0, j = 0; i < ngroups; i++) {
+			if (g_str_has_prefix (groups[i], "connection")) {
+				if (groups[i][STRLEN ("connection")] == '\0')
+					connection_tag = groups[i];
+				else
+					groups[j++] = groups[i];
+			} else
+				g_free (groups[i]);
 		}
+		ngroups = j;
+	}
+
+	connection_infos = g_new0 (ConnectionInfo, ngroups + 1 + (connection_tag ? 1 : 0));
+	for (i = 0; i < ngroups; i++) {
+		/* pass ownership of @group on... */
+		_get_connection_info_init (&connection_infos[i], keyfile, groups[ngroups - i - 1]);
+	}
+	if (connection_tag) {
+		/* pass ownership of @connection_tag on... */
+		_get_connection_info_init (&connection_infos[i], keyfile, connection_tag);
+	}
+	g_free (groups);
 
 	return connection_infos;
 }

src/nm-config.c

@@ -408,6 +408,45 @@ nm_config_create_keyfile ()
 	return keyfile;
 }
 
+static int
+_sort_groups_cmp (const char **pa, const char **pb, gpointer dummy)
+{
+	const char *a, *b;
+	gboolean a_is_connection, b_is_connection;
+
+	/* basic NULL checking... */
+	if (pa == pb)
+		return 0;
+	if (!pa)
+		return -1;
+	if (!pb)
+		return 1;
+
+	a = *pa;
+	b = *pb;
+
+	a_is_connection = g_str_has_prefix (a, "connection");
+	b_is_connection = g_str_has_prefix (b, "connection");
+
+	if (a_is_connection != b_is_connection) {
+		/* one is a [connection*] entry, the other not. We sort [connection*] entires
+		 * after.  */
+		if (a_is_connection)
+			return 1;
+		return -1;
+	}
+	if (!a_is_connection) {
+		/* both are non-connection entries. Don't reorder. */
+		return 0;
+	}
+
+	/* both are [connection.\+] entires. Reverse their order.
+	 * One of the sections might be literally [connection]. That section
+	 * is special and it's order will be fixed later. It doesn't actually
+	 * matter here how it compares with [connection.\+] sections. */
+	return pa > pb ? -1 : 1;
+}
+
 static gboolean
 read_config (GKeyFile *keyfile, const char *path, GError **error)
 {
@@ -435,6 +474,25 @@ read_config (GKeyFile *keyfile, const char *path, GError **error)
 
 	/* Override the current settings with the new ones */
 	groups = g_key_file_get_groups (kf, &ngroups);
+	if (!groups)
+		ngroups = 0;
+
+	/* Within one file we reverse the order of the '[connection.\+] sections.
+	 * Here we merge the current file (@kf) into @keyfile. As we merge multiple
+	 * files, earlier sections (with lower priority) will be added first.
+	 * But within one file, we want a top-to-bottom order. This means we
+	 * must reverse the order within each file.
+	 * At the very end, we will revert the order of all sections again and
+	 * get thus the right behavior. This final reversing is done in
+	 * NMConfigData:_get_connection_infos().  */
+	if (ngroups > 1) {
+		g_qsort_with_data (groups,
+		                   ngroups,
+		                   sizeof (char *),
+		                   (GCompareDataFunc) _sort_groups_cmp,
+		                   NULL);
+	}
+
 	for (g = 0; groups[g]; g++) {
 		keys = g_key_file_get_keys (kf, groups[g], &nkeys, NULL);
 		if (!keys)

src/tests/config/NetworkManager.conf

@@ -22,6 +22,17 @@ ipv6.ip6_privacy=0
 dummy.test1=no
 dummy.test2=no
 
+ord.key00=A-0.0.00
+ord.key01=A-0.0.01
+ord.key02=A-0.0.02
+ord.key03=A-0.0.03
+ord.key04=A-0.0.04
+ord.key05=A-0.0.05
+ord.key06=A-0.0.06
+ord.key07=A-0.0.07
+ord.key08=A-0.0.08
+ord.key09=A-0.0.09
+
 [connection.dev51]
 match-device=mac:00:00:00:00:00:51
 stop-match=yes
@@ -37,3 +48,36 @@ match-device=interface-name:wlan1
 # match-wifi is not yet implemented. Just an idea what could be useful.
 match-wifi=ssid:*[Ss]tarbucks*|*University*
 ipv6.ip6_privacy=2
+
+
+# the following sections are tested for their order across
+# multiple files.
+[connection.ord.0.1]
+ord.key03=A-0.1.03
+ord.key04=A-0.1.04
+ord.key05=A-0.1.05
+ord.key06=A-0.1.06
+ord.key07=A-0.1.07
+ord.key08=A-0.1.08
+ord.key09=A-0.1.09
+ord.ovw01=A-0.1.ovw01
+[connection.ord.0.2]
+ord.key02=A-0.2.02
+ord.key03=A-0.2.03
+ord.key04=A-0.2.04
+ord.key05=A-0.2.05
+ord.key06=A-0.2.06
+ord.key07=A-0.2.07
+ord.key08=A-0.2.08
+ord.key09=A-0.2.09
+[connection.ord.0.3]
+ord.key01=A-0.3.01
+ord.key02=A-0.3.02
+ord.key03=A-0.3.03
+ord.key04=A-0.3.04
+ord.key05=A-0.3.05
+ord.key06=A-0.3.06
+ord.key07=A-0.3.07
+ord.key08=A-0.3.08
+ord.key09=A-0.3.09
+ord.ovw01=A-0.3.ovw01

src/tests/config/conf.d/00-overrides.conf

@@ -9,3 +9,25 @@ a=0
 b=0
 c=0
 
+
+# the following sections are tested for their order across
+# multiple files.
+[connection.ord.1.1]
+ord.key06=B-1.1.06
+ord.key07=B-1.1.07
+ord.key08=B-1.1.08
+ord.key09=B-1.1.09
+[connection.ord.1.2]
+ord.key05=B-1.2.05
+ord.key06=B-1.2.06
+ord.key07=B-1.2.07
+ord.key08=B-1.2.08
+ord.key09=B-1.2.09
+[connection.ord.1.3]
+ord.key04=B-1.3.04
+ord.key05=B-1.3.05
+ord.key06=B-1.3.06
+ord.key07=B-1.3.07
+ord.key08=B-1.3.08
+ord.key09=B-1.3.09
+

src/tests/config/conf.d/10-more.conf

@@ -9,3 +9,21 @@ uri=http://example.net
 a=10
 b=10
 
+# the following sections are tested for their order across
+# multiple files.
+[connection.ord.2.1]
+ord.key09=C-2.1.09
+[connection.ord.2.2]
+ord.key08=C-2.2.08
+ord.key09=C-2.2.09
+[connection.ord.2.3]
+ord.key07=C-2.3.07
+ord.key08=C-2.3.08
+ord.key09=C-2.3.09
+
+# you can overwrite individual settings in a file loaded
+# previously. But note that this does not bump the priority
+# of the section, i.e. [connection.ord.0.1] still has a pretty
+# low priority and is shadowed by [connection.ord.2.1].
+[connection.ord.0.1]
+ord.ovw01=C-0.1.ovw01

src/tests/config/test-config.c

@@ -324,6 +324,23 @@ test_config_confdir (void)
 	g_assert_cmpstr (value, ==, "0");
 	g_free (value);
 
+#define ASSERT_GET_CONN_DEFAULT(xconfig, xname, xvalue) \
+	G_STMT_START { \
+		gs_free char *_value = nm_config_data_get_connection_default (nm_config_get_data_orig (xconfig), (xname), NULL); \
+		g_assert_cmpstr (_value, ==, (xvalue)); \
+	} G_STMT_END
+	ASSERT_GET_CONN_DEFAULT (config, "ord.key00", "A-0.0.00");
+	ASSERT_GET_CONN_DEFAULT (config, "ord.key01", "A-0.3.01");
+	ASSERT_GET_CONN_DEFAULT (config, "ord.key02", "A-0.2.02");
+	ASSERT_GET_CONN_DEFAULT (config, "ord.key03", "A-0.1.03");
+	ASSERT_GET_CONN_DEFAULT (config, "ord.key04", "B-1.3.04");
+	ASSERT_GET_CONN_DEFAULT (config, "ord.key05", "B-1.2.05");
+	ASSERT_GET_CONN_DEFAULT (config, "ord.key06", "B-1.1.06");
+	ASSERT_GET_CONN_DEFAULT (config, "ord.key07", "C-2.3.07");
+	ASSERT_GET_CONN_DEFAULT (config, "ord.key08", "C-2.2.08");
+	ASSERT_GET_CONN_DEFAULT (config, "ord.key09", "C-2.1.09");
+	ASSERT_GET_CONN_DEFAULT (config, "ord.ovw01", "C-0.1.ovw01");
+
 	g_object_unref (config);
 }