config: only handle 'option+' and 'option-' keys for known settings

It is wrong to blindly merge keys that have an 'option+' or 'option-'. Merging options is only possibly when we understand what the option means and how to merge it. No longer handle every setting but only those that are explicitly known to be string-lists (or device-specs).
2026-01-03 19:50:14 +01:00 · 2015-07-01 13:59:06 +02:00 · 2015-07-01 13:59:06 +02:00 · a1ea678f78
commit a1ea678f78
parent bd57d76af8
6 changed files with 133 additions and 30 deletions
--- a/src/nm-config.c
+++ b/src/nm-config.c
@ -528,6 +528,27 @@ _sort_groups_cmp (const char **pa, const char **pb, gpointer dummy)
 	return pa > pb ? -1 : 1;
 }

+static gboolean
+_setting_is_device_spec (const char *group, const char *key)
+{
+#define _IS(group_v, key_v) (strcmp (group, (""group_v)) == 0 && strcmp (key, (""key_v)) == 0)
+	return    _IS (NM_CONFIG_KEYFILE_GROUP_MAIN, "no-auto-default")
+	       || _IS (NM_CONFIG_KEYFILE_GROUP_MAIN, "ignore-carrier")
+	       || _IS (NM_CONFIG_KEYFILE_GROUP_MAIN, "assume-ipv6ll-only")
+	       || _IS (NM_CONFIG_KEYFILE_GROUP_KEYFILE, "unmanaged-devices")
+	       || (g_str_has_prefix (group, NM_CONFIG_KEYFILE_GROUPPREFIX_CONNECTION) && !strcmp (key, "match-device"));
+}
+
+static gboolean
+_setting_is_string_list (const char *group, const char *key)
+{
+	return    _IS (NM_CONFIG_KEYFILE_GROUP_MAIN, "plugins")
+	       || _IS (NM_CONFIG_KEYFILE_GROUP_MAIN, "debug")
+	       || _IS (NM_CONFIG_KEYFILE_GROUP_LOGGING, "domains")
+	       || g_str_has_prefix (group, NM_CONFIG_KEYFILE_GROUPPREFIX_TEST_APPEND_STRINGLIST);
+#undef _IS
+}
+
 static gboolean
 read_config (GKeyFile *keyfile, const char *path, GError **error)
 {
@ -593,29 +614,72 @@ read_config (GKeyFile *keyfile, const char *path, GError **error)
 			if (   key_len > 1
 			    && (last_char == '+' || last_char == '-')) {
 				gs_free char *base_key = g_strndup (key, key_len - 1);
-				gs_strfreev  char **old_val = g_key_file_get_string_list (keyfile, group, base_key, NULL, NULL);
-				gs_free char **new_val = g_key_file_get_string_list (kf, group, key, NULL, NULL);
-				gs_unref_ptrarray GPtrArray *new = g_ptr_array_new_with_free_func (g_free);
-				char **iter_val;
+				gboolean is_string_list;

-				for (iter_val = old_val; iter_val && *iter_val; iter_val++) {
-					if (   last_char != '-'
-					    || _nm_utils_strv_find_first (new_val, -1, *iter_val) < 0)
-						g_ptr_array_add (new, g_strdup (*iter_val));
-				}
-				for (iter_val = new_val; iter_val && *iter_val; iter_val++) {
-					/* don't add duplicates. That means an "option=a,b"; "option+=a,c" results in "option=a,b,c" */
-					if (   last_char == '+'
-					    && _nm_utils_strv_find_first (old_val, -1, *iter_val) < 0)
-						g_ptr_array_add (new, *iter_val);
-					else
-						g_free (*iter_val);
-				}
+				is_string_list = _setting_is_string_list (group, base_key);

-				if (new->len > 0)
-					nm_config_keyfile_set_string_list (keyfile, group, base_key, (const char *const*) new->pdata, new->len);
-				else
-					g_key_file_remove_key (keyfile, group, base_key, NULL);
+				if (   is_string_list
+				    || _setting_is_device_spec (group, base_key)) {
+					gs_unref_ptrarray GPtrArray *new = g_ptr_array_new_with_free_func (g_free);
+					char **iter_val;
+					gs_strfreev  char **old_val = NULL;
+					gs_free char **new_val = NULL;
+
+					if (is_string_list) {
+						old_val = g_key_file_get_string_list (keyfile, group, base_key, NULL, NULL);
+						new_val = g_key_file_get_string_list (kf, group, key, NULL, NULL);
+					} else {
+						gs_free char *old_sval = nm_config_keyfile_get_value (keyfile, group, base_key, NM_CONFIG_GET_VALUE_TYPE_SPEC);
+						gs_free char *new_sval = nm_config_keyfile_get_value (kf, group, key, NM_CONFIG_GET_VALUE_TYPE_SPEC);
+						gs_free_slist GSList *old_specs = nm_match_spec_split (old_sval);
+						gs_free_slist GSList *new_specs = nm_match_spec_split (new_sval);
+
+						/* the key is a device spec. This is a special kind of string-list, that
+						 * we must split differently. */
+						old_val = _nm_utils_slist_to_strv (old_specs, FALSE);
+						new_val = _nm_utils_slist_to_strv (new_specs, FALSE);
+					}
+
+					/* merge the string lists, by omiting duplicates. */
+
+					for (iter_val = old_val; iter_val && *iter_val; iter_val++) {
+						if (   last_char != '-'
+						    || _nm_utils_strv_find_first (new_val, -1, *iter_val) < 0)
+							g_ptr_array_add (new, g_strdup (*iter_val));
+					}
+					for (iter_val = new_val; iter_val && *iter_val; iter_val++) {
+						/* don't add duplicates. That means an "option=a,b"; "option+=a,c" results in "option=a,b,c" */
+						if (   last_char == '+'
+						    && _nm_utils_strv_find_first (old_val, -1, *iter_val) < 0)
+							g_ptr_array_add (new, *iter_val);
+						else
+							g_free (*iter_val);
+					}
+
+					if (new->len > 0) {
+						if (is_string_list)
+							nm_config_keyfile_set_string_list (keyfile, group, base_key, (const char *const*) new->pdata, new->len);
+						else {
+							gs_free_slist GSList *specs = NULL;
+							gs_free char *specs_joined = NULL;
+
+							g_ptr_array_add (new, NULL);
+							specs = _nm_utils_strv_to_slist ((char **) new->pdata, FALSE);
+
+							specs_joined = nm_match_spec_join (specs);
+
+							g_key_file_set_value (keyfile, group, base_key, specs_joined);
+						}
+					} else {
+						if (is_string_list)
+							g_key_file_remove_key (keyfile, group, base_key, NULL);
+						else
+							g_key_file_set_value (keyfile, group, base_key, "");
+					}
+				} else {
+					/* For any other settings we don't support extending the option with +/-.
+					 * Just drop the key. */
+				}
 				continue;
 			}

--- a/src/nm-config.h
+++ b/src/nm-config.h
@ -49,6 +49,7 @@ G_BEGIN_DECLS
 #define NM_CONFIG_KEYFILE_LIST_SEPARATOR ','

 #define NM_CONFIG_KEYFILE_GROUPPREFIX_CONNECTION            "connection"
+#define NM_CONFIG_KEYFILE_GROUPPREFIX_TEST_APPEND_STRINGLIST ".test-append-stringlist"

 #define NM_CONFIG_KEYFILE_GROUP_MAIN                        "main"
 #define NM_CONFIG_KEYFILE_GROUP_LOGGING                     "logging"
--- a/src/tests/config/conf.d/00-overrides.conf
+++ b/src/tests/config/conf.d/00-overrides.conf
@ -1,9 +1,16 @@
 [main]
 dhcp=dhcpcd

+no-auto-default=spec1,spec2
+ignore-carrier=\s space1 \s
+
 [logging]
 domains=PLATFORM,DNS,WIFI

+[appendable-test]
+non-appendable-key1+=i-will-be-dropped
+non-appendable-key2-=i-will-be-dropped
+
 [order]
 a=0
 b=0
@ -32,7 +39,7 @@ ord.key08=B-1.3.08
 ord.key09=B-1.3.09


-[append]
+[.test-append-stringlist.1]
 val1=a,b

 val2-=VAL2
--- a/src/tests/config/conf.d/10-more.conf
+++ b/src/tests/config/conf.d/10-more.conf
@ -1,5 +1,12 @@
 [main]
 extra=hello
+
+no-auto-default-=spec1
+no-auto-default+=spec3
+
+ignore-carrier+=\sspace2\t	
+
+[.test-append-stringlist.0]
 new+=something

 [connectivity]
@ -29,5 +36,5 @@ ord.key09=C-2.3.09
 ord.ovw01=C-0.1.ovw01


-[append]
+[.test-append-stringlist.1]
 val1-=b
--- a/src/tests/config/conf.d/90-last.conf
+++ b/src/tests/config/conf.d/90-last.conf
@ -4,5 +4,5 @@ plugins+=one,two
 [order]
 a=90

-[append]
+[.test-append-stringlist.1]
 val1+=c,a
--- a/src/tests/config/test-config.c
+++ b/src/tests/config/test-config.c
@ -283,6 +283,7 @@ test_config_confdir (void)
 	NMConfig *config;
 	const char **plugins;
 	char *value;
+	GSList *specs;

 	config = setup_config (NULL, SRCDIR "/NetworkManager.conf", SRCDIR "/conf.d", NULL);

@ -305,7 +306,23 @@ test_config_confdir (void)
 	g_assert_cmpstr (value, ==, "hello");
 	g_free (value);

-	value = nm_config_data_get_value (nm_config_get_data_orig (config), "main", "new", NM_CONFIG_GET_VALUE_NONE);
+	value = nm_config_data_get_value (nm_config_get_data_orig (config), "main", "no-auto-default", NM_CONFIG_GET_VALUE_TYPE_SPEC);
+	specs = nm_match_spec_split (value);
+	g_free (value);
+	g_assert_cmpint (g_slist_length (specs), ==, 2);
+	g_assert_cmpstr (g_slist_nth_data (specs, 0), ==, "spec2");
+	g_assert_cmpstr (g_slist_nth_data (specs, 1), ==, "spec3");
+	g_slist_free_full (specs, g_free);
+
+	value = nm_config_data_get_value (nm_config_get_data_orig (config), "main", "ignore-carrier", NM_CONFIG_GET_VALUE_TYPE_SPEC);
+	specs = nm_match_spec_split (value);
+	g_free (value);
+	g_assert_cmpint (g_slist_length (specs), ==, 2);
+	g_assert_cmpstr (g_slist_nth_data (specs, 0), ==, "  space1  ");
+	g_assert_cmpstr (g_slist_nth_data (specs, 1), ==, " space2\t");
+	g_slist_free_full (specs, g_free);
+
+	value = nm_config_data_get_value (nm_config_get_data_orig (config), NM_CONFIG_KEYFILE_GROUPPREFIX_TEST_APPEND_STRINGLIST".0", "new", NM_CONFIG_GET_VALUE_NONE);
 	g_assert_cmpstr (value, ==, "something"); /* not ",something" */
 	g_free (value);

@ -319,6 +336,13 @@ test_config_confdir (void)
 	g_assert_cmpstr (value, ==, "0");
 	g_free (value);

+	g_assert (!nm_config_data_has_value (nm_config_get_data_orig (config), "appendable-test", "non-appendable-key1", NM_CONFIG_GET_VALUE_RAW));
+	g_assert (!nm_config_data_has_value (nm_config_get_data_orig (config), "appendable-test", "non-appendable-key1+", NM_CONFIG_GET_VALUE_RAW));
+	g_assert (!nm_config_data_has_value (nm_config_get_data_orig (config), "appendable-test", "non-appendable-key1-", NM_CONFIG_GET_VALUE_RAW));
+	g_assert (!nm_config_data_has_value (nm_config_get_data_orig (config), "appendable-test", "non-appendable-key2", NM_CONFIG_GET_VALUE_RAW));
+	g_assert (!nm_config_data_has_value (nm_config_get_data_orig (config), "appendable-test", "non-appendable-key2+", NM_CONFIG_GET_VALUE_RAW));
+	g_assert (!nm_config_data_has_value (nm_config_get_data_orig (config), "appendable-test", "non-appendable-key2-", NM_CONFIG_GET_VALUE_RAW));
+
 #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); \
@ -336,23 +360,23 @@ test_config_confdir (void)
 	ASSERT_GET_CONN_DEFAULT (config, "ord.key09", "C-2.1.09");
 	ASSERT_GET_CONN_DEFAULT (config, "ord.ovw01", "C-0.1.ovw01");

-	value = nm_config_data_get_value (nm_config_get_data_orig (config), "append", "val1", NM_CONFIG_GET_VALUE_NONE);
+	value = nm_config_data_get_value (nm_config_get_data_orig (config), NM_CONFIG_KEYFILE_GROUPPREFIX_TEST_APPEND_STRINGLIST".1", "val1", NM_CONFIG_GET_VALUE_NONE);
 	g_assert_cmpstr (value, ==, "a,c");
 	g_free (value);

-	value = nm_config_data_get_value (nm_config_get_data_orig (config), "append", "val2", NM_CONFIG_GET_VALUE_NONE);
+	value = nm_config_data_get_value (nm_config_get_data_orig (config), NM_CONFIG_KEYFILE_GROUPPREFIX_TEST_APPEND_STRINGLIST".1", "val2", NM_CONFIG_GET_VALUE_NONE);
 	g_assert_cmpstr (value, ==, "VAL2");
 	g_free (value);

-	value = nm_config_data_get_value (nm_config_get_data_orig (config), "append", "val3", NM_CONFIG_GET_VALUE_NONE);
+	value = nm_config_data_get_value (nm_config_get_data_orig (config), NM_CONFIG_KEYFILE_GROUPPREFIX_TEST_APPEND_STRINGLIST".1", "val3", NM_CONFIG_GET_VALUE_NONE);
 	g_assert_cmpstr (value, ==, NULL);
 	g_free (value);

-	value = nm_config_data_get_value (nm_config_get_data_orig (config), "append", "val4", NM_CONFIG_GET_VALUE_NONE);
+	value = nm_config_data_get_value (nm_config_get_data_orig (config), NM_CONFIG_KEYFILE_GROUPPREFIX_TEST_APPEND_STRINGLIST".1", "val4", NM_CONFIG_GET_VALUE_NONE);
 	g_assert_cmpstr (value, ==, "vb,vb");
 	g_free (value);

-	value = nm_config_data_get_value (nm_config_get_data_orig (config), "append", "val5", NM_CONFIG_GET_VALUE_NONE);
+	value = nm_config_data_get_value (nm_config_get_data_orig (config), NM_CONFIG_KEYFILE_GROUPPREFIX_TEST_APPEND_STRINGLIST".1", "val5", NM_CONFIG_GET_VALUE_NONE);
 	g_assert_cmpstr (value, ==, "VAL5");
 	g_free (value);