config: fix evaluation of no-auto-default setting

We used to merge the spec list for no-auto-default from keyfile with the
content of the state file. Since the addition of the "except:" spec this
is wrong.
For example, if the user configured:

  no-auto-default=except:mac:11:11:11:11:11

and statefile contained "11:11:11:11:11" and "22:22:22:22:22", we would
wrongly not match "11:11:11:11:11". The two lists must be kept separate,
so that devices that are blocked by internal decision always match.

This separation is also clearer. Now the spec list is devided into a
part that comes from from user configuration, and a part that comes from
internal decision.

3 files changed, 97 insertions, 82 deletions

diff --git a/src/nm-config-data.c b/src/nm-config-data.c
index 73c36312f3..f093292564 100644
--- a/src/nm-config-data.c
+++ b/src/nm-config-data.c
@@ -60,6 +60,7 @@ typedef struct {
 	struct {
 		char **arr;
 		GSList *specs;
+		GSList *specs_config;
 	} no_auto_default;
 
 	GSList *ignore_carrier;
@@ -145,12 +146,17 @@ nm_config_data_get_no_auto_default (const NMConfigData *self)
 	return (const char *const*) NM_CONFIG_DATA_GET_PRIVATE (self)->no_auto_default.arr;
 }
 
-const GSList *
-nm_config_data_get_no_auto_default_list (const NMConfigData *self)
+gboolean
+nm_config_data_get_no_auto_default_for_device (const NMConfigData *self, NMDevice *device)
 {
-	g_return_val_if_fail (self, NULL);
+	NMConfigDataPrivate *priv;
+
+	g_return_val_if_fail (NM_IS_CONFIG_DATA (self), FALSE);
+	g_return_val_if_fail (NM_IS_DEVICE (device), FALSE);
 
-	return NM_CONFIG_DATA_GET_PRIVATE (self)->no_auto_default.specs;
+	priv = NM_CONFIG_DATA_GET_PRIVATE (self);
+	return    nm_device_spec_match_list (device, priv->no_auto_default.specs)
+	       || nm_device_spec_match_list (device, priv->no_auto_default.specs_config);
 }
 
 const char *
@@ -310,12 +316,21 @@ _keyfile_a_contains_all_in_b (GKeyFile *kf_a, GKeyFile *kf_b)
 	return TRUE;
 }
 
+static gboolean
+_slist_str_equals (GSList *a, GSList *b)
+{
+	while (a && b && g_strcmp0 (a->data, b->data) == 0) {
+		a = a->next;
+		b = b->next;
+	}
+	return !a && !b;
+}
+
 NMConfigChangeFlags
 nm_config_data_diff (NMConfigData *old_data, NMConfigData *new_data)
 {
 	NMConfigChangeFlags changes = NM_CONFIG_CHANGE_NONE;
 	NMConfigDataPrivate *priv_old, *priv_new;
-	GSList *spec_old, *spec_new;
 
 	g_return_val_if_fail (NM_IS_CONFIG_DATA (old_data), NM_CONFIG_CHANGE_NONE);
 	g_return_val_if_fail (NM_IS_CONFIG_DATA (new_data), NM_CONFIG_CHANGE_NONE);
@@ -336,13 +351,8 @@ nm_config_data_diff (NMConfigData *old_data, NMConfigData *new_data)
 	    || g_strcmp0 (nm_config_data_get_connectivity_response (old_data), nm_config_data_get_connectivity_response (new_data)))
 		changes |= NM_CONFIG_CHANGE_CONNECTIVITY;
 
-	spec_old = priv_old->no_auto_default.specs;
-	spec_new = priv_new->no_auto_default.specs;
-	while (spec_old && spec_new && strcmp (spec_old->data, spec_new->data) == 0) {
-		spec_old = spec_old->next;
-		spec_new = spec_new->next;
-	}
-	if (spec_old || spec_new)
+	if (   !_slist_str_equals (priv_old->no_auto_default.specs, priv_new->no_auto_default.specs)
+	    || !_slist_str_equals (priv_old->no_auto_default.specs_config, priv_new->no_auto_default.specs_config))
 		changes |= NM_CONFIG_CHANGE_NO_AUTO_DEFAULT;
 
 	if (g_strcmp0 (nm_config_data_get_dns_mode (old_data), nm_config_data_get_dns_mode (new_data)))
@@ -371,9 +381,6 @@ get_property (GObject *object,
 	case PROP_CONFIG_DESCRIPTION:
 		g_value_set_string (value, nm_config_data_get_config_description (self));
 		break;
-	case PROP_NO_AUTO_DEFAULT:
-		g_value_take_boxed (value, g_strdupv ((char **) nm_config_data_get_no_auto_default (self)));
-		break;
 	case PROP_CONNECTIVITY_URI:
 		g_value_set_string (value, nm_config_data_get_connectivity_uri (self));
 		break;
@@ -397,7 +404,6 @@ set_property (GObject *object,
 {
 	NMConfigData *self = NM_CONFIG_DATA (object);
 	NMConfigDataPrivate *priv = NM_CONFIG_DATA_GET_PRIVATE (self);
-	guint i;
 
 	/* This type is immutable. All properties are construct only. */
 	switch (prop_id) {
@@ -413,12 +419,24 @@ set_property (GObject *object,
 			priv->keyfile = nm_config_create_keyfile ();
 		break;
 	case PROP_NO_AUTO_DEFAULT:
-		priv->no_auto_default.arr = g_strdupv (g_value_get_boxed (value));
-		if (!priv->no_auto_default.arr)
-			priv->no_auto_default.arr = g_new0 (char *, 1);
-		for (i = 0; priv->no_auto_default.arr[i]; i++)
-			priv->no_auto_default.specs = g_slist_prepend (priv->no_auto_default.specs, priv->no_auto_default.arr[i]);
-		priv->no_auto_default.specs = g_slist_reverse (priv->no_auto_default.specs);
+		{
+			char ** value_arr = g_value_get_boxed (value);
+			guint i, j = 0;
+
+			priv->no_auto_default.arr = g_new (char *, g_strv_length (value_arr) + 1);
+			priv->no_auto_default.specs = NULL;
+
+			for (i = 0; value_arr && value_arr[i]; i++) {
+				if (   *value_arr[i]
+				    && nm_utils_hwaddr_valid (value_arr[i], -1)
+				    && _nm_utils_strv_find_first (value_arr, i, value_arr[i]) < 0) {
+					priv->no_auto_default.arr[j++] = g_strdup (value_arr[i]);
+					priv->no_auto_default.specs = g_slist_prepend (priv->no_auto_default.specs, g_strdup_printf ("mac:%s", value_arr[i]));
+				}
+			}
+			priv->no_auto_default.arr[j++] = NULL;
+			priv->no_auto_default.specs = g_slist_reverse (priv->no_auto_default.specs);
+		}
 		break;
 	default:
 		G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
@@ -443,7 +461,8 @@ finalize (GObject *gobject)
 	g_free (priv->connectivity.uri);
 	g_free (priv->connectivity.response);
 
-	g_slist_free (priv->no_auto_default.specs);
+	g_slist_free_full (priv->no_auto_default.specs, g_free);
+	g_slist_free_full (priv->no_auto_default.specs_config, g_free);
 	g_strfreev (priv->no_auto_default.arr);
 
 	g_free (priv->dns_mode);
@@ -496,6 +515,8 @@ constructed (GObject *object)
 	priv->ignore_carrier = nm_config_get_device_match_spec (priv->keyfile, "main", "ignore-carrier");
 	priv->assume_ipv6ll_only = nm_config_get_device_match_spec (priv->keyfile, "main", "assume-ipv6ll-only");
 
+	priv->no_auto_default.specs_config = nm_config_get_device_match_spec (priv->keyfile, "main", "no-auto-default");
+
 	G_OBJECT_CLASS (nm_config_data_parent_class)->constructed (object);
 }
 
@@ -589,7 +610,7 @@ nm_config_data_class_init (NMConfigDataClass *config_class)
 	    (object_class, PROP_NO_AUTO_DEFAULT,
 	     g_param_spec_boxed (NM_CONFIG_DATA_NO_AUTO_DEFAULT, "", "",
 	                         G_TYPE_STRV,
-	                         G_PARAM_READWRITE |
+	                         G_PARAM_WRITABLE |
 	                         G_PARAM_CONSTRUCT_ONLY |
 	                         G_PARAM_STATIC_STRINGS));
 
diff --git a/src/nm-config-data.h b/src/nm-config-data.h
index 78b04ec8c9..14dfcf6300 100644
--- a/src/nm-config-data.h
+++ b/src/nm-config-data.h
@@ -86,7 +86,7 @@ const guint nm_config_data_get_connectivity_interval (const NMConfigData *config
 const char *nm_config_data_get_connectivity_response (const NMConfigData *config_data);
 
 const char *const*nm_config_data_get_no_auto_default (const NMConfigData *config_data);
-const GSList *    nm_config_data_get_no_auto_default_list (const NMConfigData *config_data);
+gboolean          nm_config_data_get_no_auto_default_for_device (const NMConfigData *self, NMDevice *device);
 
 const char *nm_config_data_get_dns_mode (const NMConfigData *self);
 const char *nm_config_data_get_rc_manager (const NMConfigData *self);
diff --git a/src/nm-config.c b/src/nm-config.c
index 473eabd95f..c377aee989 100644
--- a/src/nm-config.c
+++ b/src/nm-config.c
@@ -230,94 +230,99 @@ nm_config_get_configure_and_quit (NMConfig *config)
 /************************************************************************/
 
 static char **
-no_auto_default_merge_from_file (const char *no_auto_default_file, const char *const* no_auto_default)
+no_auto_default_from_file (const char *no_auto_default_file)
 {
-	GPtrArray *updated;
+	GPtrArray *no_auto_default_new;
 	char **list;
-	int i, j;
+	guint i;
 	char *data;
 
-	updated = g_ptr_array_new ();
-	if (no_auto_default) {
-		for (i = 0; no_auto_default[i]; i++)
-			g_ptr_array_add (updated, g_strdup (no_auto_default[i]));
-	}
+	no_auto_default_new = g_ptr_array_new ();
 
 	if (   no_auto_default_file
 	    && g_file_get_contents (no_auto_default_file, &data, NULL, NULL)) {
 		list = g_strsplit (data, "\n", -1);
 		for (i = 0; list[i]; i++) {
-			if (!*list[i])
+			if (   *list[i]
+			    && nm_utils_hwaddr_valid (list[i], -1)
+			    && _nm_utils_strv_find_first (list, i, list[i]) < 0)
+				g_ptr_array_add (no_auto_default_new, list[i]);
+			else
 				g_free (list[i]);
-			else {
-				for (j = 0; j < updated->len; j++) {
-					if (!strcmp (list[i], updated->pdata[j]))
-						break;
-				}
-				if (j == updated->len)
-					g_ptr_array_add (updated, list[i]);
-				else
-					g_free (list[i]);
-			}
 		}
 		g_free (list);
 		g_free (data);
 	}
 
-	g_ptr_array_add (updated, NULL);
-	return (char **) g_ptr_array_free (updated, FALSE);
+	g_ptr_array_add (no_auto_default_new, NULL);
+	return (char **) g_ptr_array_free (no_auto_default_new, FALSE);
+}
+
+static gboolean
+no_auto_default_to_file (const char *no_auto_default_file, const char *const*no_auto_default, GError **error)
+{
+	GString *data;
+	gboolean success;
+	guint i;
+
+	data = g_string_new ("");
+	for (i = 0; no_auto_default && no_auto_default[i]; i++) {
+		g_string_append (data, no_auto_default[i]);
+		g_string_append_c (data, '\n');
+	}
+	success = g_file_set_contents (no_auto_default_file, data->str, data->len, error);
+	g_string_free (data, TRUE);
+	return success;
 }
 
 gboolean
 nm_config_get_no_auto_default_for_device (NMConfig *self, NMDevice *device)
 {
-	NMConfigData *config_data;
-
 	g_return_val_if_fail (NM_IS_CONFIG (self), FALSE);
-	g_return_val_if_fail (NM_IS_DEVICE (device), FALSE);
 
-	config_data = NM_CONFIG_GET_PRIVATE (self)->config_data;
-	return nm_device_spec_match_list (device, nm_config_data_get_no_auto_default_list (config_data));
+	return nm_config_data_get_no_auto_default_for_device (NM_CONFIG_GET_PRIVATE (self)->config_data, device);
 }
 
 void
 nm_config_set_no_auto_default_for_device (NMConfig *self, NMDevice *device)
 {
 	NMConfigPrivate *priv = NM_CONFIG_GET_PRIVATE (self);
-	char *current;
-	GString *updated;
 	GError *error = NULL;
-	char **no_auto_default;
 	NMConfigData *new_data = NULL;
+	const char *hw_address;
+	const char *const*no_auto_default_current;
+	GPtrArray *no_auto_default_new = NULL;
+	guint i;
 
 	g_return_if_fail (NM_IS_CONFIG (self));
 	g_return_if_fail (NM_IS_DEVICE (device));
 
-	if (nm_config_get_no_auto_default_for_device (self, device))
-		return;
+	hw_address = nm_device_get_hw_address (device);
 
-	updated = g_string_new (NULL);
-	if (g_file_get_contents (priv->no_auto_default_file, &current, NULL, NULL)) {
-		g_string_append (updated, current);
-		g_free (current);
-		if (updated->str[updated->len - 1] != '\n')
-			g_string_append_c (updated, '\n');
+	no_auto_default_current = nm_config_data_get_no_auto_default (priv->config_data);
+
+	if (_nm_utils_strv_find_first ((char **) no_auto_default_current, -1, hw_address) >= 0) {
+		/* @hw_address is already blocked. We don't have to update our in-memory representation.
+		 * Maybe we should write to no_auto_default_file anew, but let's save that too. */
+		return;
 	}
 
-	g_string_append (updated, nm_device_get_hw_address (device));
-	g_string_append_c (updated, '\n');
+	no_auto_default_new = g_ptr_array_new ();
+	for (i = 0; no_auto_default_current && no_auto_default_current[i]; i++)
+		g_ptr_array_add (no_auto_default_new, (char *) no_auto_default_current[i]);
+	g_ptr_array_add (no_auto_default_new, (char *) hw_address);
+	g_ptr_array_add (no_auto_default_new, NULL);
 
-	if (!g_file_set_contents (priv->no_auto_default_file, updated->str, updated->len, &error)) {
+	if (!no_auto_default_to_file (priv->no_auto_default_file, (const char *const*) no_auto_default_new->pdata, &error)) {
 		nm_log_warn (LOGD_SETTINGS, "Could not update no-auto-default.state file: %s",
 		             error->message);
 		g_error_free (error);
 	}
 
-	g_string_free (updated, TRUE);
+	new_data = nm_config_data_new_update_no_auto_default (priv->config_data, (const char *const*) no_auto_default_new->pdata);
 
-	no_auto_default = no_auto_default_merge_from_file (priv->no_auto_default_file, nm_config_data_get_no_auto_default (priv->config_data));
-	new_data = nm_config_data_new_update_no_auto_default (priv->config_data, (const char *const*) no_auto_default);
-	g_strfreev (no_auto_default);
+	/* unref no_auto_default_set here. Note that _set_config_data() probably invalidates the content of the array. */
+	g_ptr_array_unref (no_auto_default_new);
 
 	_set_config_data (self, new_data);
 }
@@ -867,9 +872,7 @@ init_sync (GInitable *initable, GCancellable *cancellable, GError **error)
 	GKeyFile *keyfile;
 	char *config_main_file = NULL;
 	char *config_description = NULL;
-	char **no_auto_default;
-	GSList *no_auto_default_orig_list;
-	GPtrArray *no_auto_default_orig;
+	gs_strfreev char **no_auto_default = NULL;
 
 	if (priv->config_dir) {
 		/* Object is already initialized. */
@@ -929,19 +932,10 @@ init_sync (GInitable *initable, GCancellable *cancellable, GError **error)
 
 	priv->configure_and_quit = nm_config_keyfile_get_boolean (keyfile, "main", "configure-and-quit", FALSE);
 
-	no_auto_default_orig_list = nm_config_get_device_match_spec (keyfile, "main", "no-auto-default");
-
-	no_auto_default_orig = _nm_utils_copy_slist_to_array (no_auto_default_orig_list, NULL, NULL);
-	g_ptr_array_add (no_auto_default_orig, NULL);
-	no_auto_default = no_auto_default_merge_from_file (priv->no_auto_default_file, (const char *const *) no_auto_default_orig->pdata);
-	g_ptr_array_unref (no_auto_default_orig);
-
-	g_slist_free_full (no_auto_default_orig_list, g_free);
+	no_auto_default = no_auto_default_from_file (priv->no_auto_default_file);
 
 	priv->config_data_orig = nm_config_data_new (config_main_file, config_description, (const char *const*) no_auto_default, keyfile);
 
-	g_strfreev (no_auto_default);
-
 	priv->config_data = g_object_ref (priv->config_data_orig);
 
 	g_free (config_main_file);