config: cleanup fields in NMGlobalDnsConfig

- consistently set options, searches, domains fields to %NULL,
  if there are no values.

- in nm_global_dns_config_update_checksum(), ensure that we uniquely
  hash values. E.g. a config with "searches[a], options=[b]" should
  hash differently from "searches=[ab], options=[]".

- in nm_global_dns_config_to_dbus(), reuse the sorted domain list.
  We already have it, and it guarantees a consistent ordering of
  fields.

- in global_dns_domain_from_dbus(), fix memleaks if D-Bus strdict
  contains duplicate entries.

3 files changed, 132 insertions, 84 deletions

diff --git a/src/dns/nm-dns-manager.c b/src/dns/nm-dns-manager.c
index 56be5aab35..ea8ce2d805 100644
--- a/src/dns/nm-dns-manager.c
+++ b/src/dns/nm-dns-manager.c
@@ -970,30 +970,36 @@ merge_global_dns_config (NMResolvConfData *rc, NMGlobalDnsConfig *global_conf)
 	const char *const *searches;
 	const char *const *options;
 	const char *const *servers;
-	gint i;
+	guint i;
 
 	if (!global_conf)
 		return FALSE;
 
 	searches = nm_global_dns_config_get_searches (global_conf);
-	options = nm_global_dns_config_get_options (global_conf);
-
-	for (i = 0; searches && searches[i]; i++) {
-		if (domain_is_routing (searches[i]))
-			continue;
-		if (!domain_is_valid (searches[i], FALSE))
-			continue;
-		add_string_item (rc->searches, searches[i], TRUE);
+	if (searches) {
+		for (i = 0; searches[i]; i++) {
+			if (domain_is_routing (searches[i]))
+				continue;
+			if (!domain_is_valid (searches[i], FALSE))
+				continue;
+			add_string_item (rc->searches, searches[i], TRUE);
+		}
 	}
 
-	for (i = 0; options && options[i]; i++)
-		add_string_item (rc->options, options[i], TRUE);
+	options = nm_global_dns_config_get_options (global_conf);
+	if (options) {
+		for (i = 0; options[i]; i++)
+			add_string_item (rc->options, options[i], TRUE);
+	}
 
 	default_domain = nm_global_dns_config_lookup_domain (global_conf, "*");
-	g_assert (default_domain);
+	nm_assert (default_domain);
+
 	servers = nm_global_dns_domain_get_servers (default_domain);
-	for (i = 0; servers && servers[i]; i++)
-		add_string_item (rc->nameservers, servers[i], TRUE);
+	if (servers) {
+		for (i = 0; servers[i]; i++)
+			add_string_item (rc->nameservers, servers[i], TRUE);
+	}
 
 	return TRUE;
 }
diff --git a/src/nm-config-data.c b/src/nm-config-data.c
index 13d6acc2f8..638d5f3cbd 100644
--- a/src/nm-config-data.c
+++ b/src/nm-config-data.c
@@ -54,7 +54,7 @@ struct _NMGlobalDnsConfig {
 	char **searches;
 	char **options;
 	GHashTable *domains;
-	char **domain_list;
+	const char **domain_list;
 	gboolean internal;
 };
 
@@ -705,12 +705,12 @@ nm_config_data_log (const NMConfigData *self,
 
 /*****************************************************************************/
 
-const char *const *
+const char *const*
 nm_global_dns_config_get_searches (const NMGlobalDnsConfig *dns_config)
 {
 	g_return_val_if_fail (dns_config, NULL);
 
-	return (const char *const *) dns_config->searches;
+	return (const char *const*) dns_config->searches;
 }
 
 const char *const *
@@ -718,16 +718,15 @@ nm_global_dns_config_get_options (const NMGlobalDnsConfig *dns_config)
 {
 	g_return_val_if_fail (dns_config, NULL);
 
-	return (const char *const *) dns_config->options;
+	return (const char *const*) dns_config->options;
 }
 
 guint
 nm_global_dns_config_get_num_domains (const NMGlobalDnsConfig *dns_config)
 {
 	g_return_val_if_fail (dns_config, 0);
-	g_return_val_if_fail (dns_config->domains, 0);
 
-	return g_hash_table_size (dns_config->domains);
+	return dns_config->domains ? g_hash_table_size (dns_config->domains) : 0;
 }
 
 NMGlobalDnsDomain *
@@ -737,22 +736,22 @@ nm_global_dns_config_get_domain (const NMGlobalDnsConfig *dns_config, guint i)
 
 	g_return_val_if_fail (dns_config, NULL);
 	g_return_val_if_fail (dns_config->domains, NULL);
-	g_return_val_if_fail (dns_config->domain_list, NULL);
-	g_return_val_if_fail (i < g_strv_length (dns_config->domain_list), NULL);
+	g_return_val_if_fail (i < g_hash_table_size (dns_config->domains), NULL);
+
+	nm_assert (NM_PTRARRAY_LEN (dns_config->domain_list) == g_hash_table_size (dns_config->domains));
 
 	domain = g_hash_table_lookup (dns_config->domains, dns_config->domain_list[i]);
-	g_return_val_if_fail (domain, NULL);
 
+	nm_assert (domain);
 	return domain;
 }
 
 NMGlobalDnsDomain *nm_global_dns_config_lookup_domain (const NMGlobalDnsConfig *dns_config, const char *name)
 {
 	g_return_val_if_fail (dns_config, NULL);
-	g_return_val_if_fail (dns_config->domains, NULL);
 	g_return_val_if_fail (name, NULL);
 
-	return g_hash_table_lookup (dns_config->domains, name);
+	return dns_config->domains ? g_hash_table_lookup (dns_config->domains, name) : NULL;
 }
 
 const char *
@@ -775,6 +774,7 @@ const char *const *
 nm_global_dns_domain_get_options (const NMGlobalDnsDomain *domain)
 {
 	g_return_val_if_fail (domain, NULL);
+
 	return (const char *const *) domain->options;
 }
 
@@ -788,42 +788,59 @@ gboolean
 nm_global_dns_config_is_empty (const NMGlobalDnsConfig *dns_config)
 {
 	g_return_val_if_fail (dns_config, TRUE);
-	g_return_val_if_fail (dns_config->domains, TRUE);
 
-	return    (!dns_config->searches || g_strv_length (dns_config->searches) == 0)
-	       && (!dns_config->options || g_strv_length (dns_config->options) == 0)
-	       && g_hash_table_size (dns_config->domains) == 0;
+	return    !dns_config->searches
+	       && !dns_config->options
+	       && !dns_config->domain_list;
 }
 
 void
 nm_global_dns_config_update_checksum (const NMGlobalDnsConfig *dns_config, GChecksum *sum)
 {
 	NMGlobalDnsDomain *domain;
-	GList *keys, *key;
 	guint i;
+	guint8 v8;
 
 	g_return_if_fail (dns_config);
-	g_return_if_fail (dns_config->domains);
 	g_return_if_fail (sum);
 
-	for (i = 0; dns_config->searches && dns_config->searches[i]; i++)
-		g_checksum_update (sum, (guchar *) dns_config->searches[i], strlen (dns_config->searches[i]));
-	for (i = 0; dns_config->options && dns_config->options[i]; i++)
-		g_checksum_update (sum, (guchar *) dns_config->options[i], strlen (dns_config->options[i]));
+	v8 = NM_HASH_COMBINE_BOOLS (guint8,
+	                            !dns_config->searches,
+	                            !dns_config->options,
+	                            !dns_config->domain_list);
+	g_checksum_update (sum, (guchar *) &v8, 1);
+
+	if (dns_config->searches) {
+		for (i = 0; dns_config->searches[i]; i++)
+			g_checksum_update (sum, (guchar *) dns_config->searches[i], strlen (dns_config->searches[i]) + 1);
+	}
+	if (dns_config->options) {
+		for (i = 0; dns_config->options[i]; i++)
+			g_checksum_update (sum, (guchar *) dns_config->options[i], strlen (dns_config->options[i]) + 1);
+	}
+
+	if (dns_config->domain_list) {
+		for (i = 0; dns_config->domain_list[i]; i++) {
+			domain = g_hash_table_lookup (dns_config->domains, dns_config->domain_list[i]);
+			nm_assert (domain);
 
-	keys = g_list_sort (g_hash_table_get_keys (dns_config->domains), (GCompareFunc) strcmp);
-	for (key = keys; key; key = g_list_next (key)) {
+			v8 = NM_HASH_COMBINE_BOOLS (guint8,
+			                            !domain->servers,
+			                            !domain->options);
+			g_checksum_update (sum, (guchar *) &v8, 1);
 
-		domain = g_hash_table_lookup (dns_config->domains, key->data);
-		g_assert (domain != NULL);
-		g_checksum_update (sum, (guchar *) domain->name, strlen (domain->name));
+			g_checksum_update (sum, (guchar *) domain->name, strlen (domain->name) + 1);
 
-		for (i = 0; domain->servers && domain->servers[i]; i++)
-			g_checksum_update (sum, (guchar *) domain->servers[i], strlen (domain->servers[i]));
-		for (i = 0; domain->options && domain->options[i]; i++)
-			g_checksum_update (sum, (guchar *) domain->options[i], strlen (domain->options[i]));
+			if (domain->servers) {
+				for (i = 0; domain->servers && domain->servers[i]; i++)
+					g_checksum_update (sum, (guchar *) domain->servers[i], strlen (domain->servers[i]) + 1);
+			}
+			if (domain->options) {
+				for (i = 0; domain->options[i]; i++)
+					g_checksum_update (sum, (guchar *) domain->options[i], strlen (domain->options[i]) + 1);
+			}
+		}
 	}
-	g_list_free (keys);
 }
 
 static void
@@ -844,7 +861,8 @@ nm_global_dns_config_free (NMGlobalDnsConfig *dns_config)
 		g_strfreev (dns_config->searches);
 		g_strfreev (dns_config->options);
 		g_free (dns_config->domain_list);
-		g_hash_table_unref (dns_config->domains);
+		if (dns_config->domains)
+			g_hash_table_unref (dns_config->domains);
 		g_free (dns_config);
 	}
 }
@@ -858,12 +876,16 @@ nm_config_data_get_global_dns_config (const NMConfigData *self)
 }
 
 static void
-global_dns_config_update_domain_list (NMGlobalDnsConfig *dns_config)
+global_dns_config_seal_domains (NMGlobalDnsConfig *dns_config)
 {
-	guint length;
+	nm_assert (dns_config);
+	nm_assert (dns_config->domains);
+	nm_assert (!dns_config->domain_list);
 
-	g_free (dns_config->domain_list);
-	dns_config->domain_list = (char **) g_hash_table_get_keys_as_array (dns_config->domains, &length);
+	if (g_hash_table_size (dns_config->domains) == 0)
+		nm_clear_pointer (&dns_config->domains, g_hash_table_unref);
+	else
+		dns_config->domain_list = nm_utils_strdict_get_keys (dns_config->domains, TRUE, NULL);
 }
 
 static NMGlobalDnsConfig *
@@ -892,8 +914,13 @@ load_global_dns (GKeyFile *keyfile, gboolean internal)
 	                                             g_free, (GDestroyNotify) global_dns_domain_free);
 
 	strv = g_key_file_get_string_list (keyfile, group, "searches", NULL, NULL);
-	if (strv)
-		dns_config->searches = _nm_utils_strv_cleanup (strv, TRUE, TRUE, TRUE);
+	if (strv) {
+		_nm_utils_strv_cleanup (strv, TRUE, TRUE, TRUE);
+		if (!strv[0])
+			g_free (strv);
+		else
+			dns_config->searches = strv;
+	}
 
 	strv = g_key_file_get_string_list (keyfile, group, "options", NULL, NULL);
 	if (strv) {
@@ -904,8 +931,12 @@ load_global_dns (GKeyFile *keyfile, gboolean internal)
 			else
 				g_free (strv[i]);
 		}
-		strv[j] = NULL;
-		dns_config->options = strv;
+		if (j == 0)
+			g_free (strv);
+		else {
+			strv[j] = NULL;
+			dns_config->options = strv;
+		}
 	}
 
 	groups = g_key_file_get_groups (keyfile, NULL);
@@ -928,20 +959,23 @@ load_global_dns (GKeyFile *keyfile, gboolean internal)
 				else
 					g_free (strv[i]);
 			}
-			if (j) {
+			if (j == 0)
+				g_free (strv);
+			else {
 				strv[j] = NULL;
 				servers = strv;
 			}
-			else
-				g_free (strv);
 		}
 
 		if (!servers)
 			continue;
 
 		strv = g_key_file_get_string_list (keyfile, groups[g], "options", NULL, NULL);
-		if (strv)
+		if (strv) {
 			options = _nm_utils_strv_cleanup (strv, TRUE, TRUE, TRUE);
+			if (!options[0])
+				nm_clear_g_free (&options);
+		}
 
 		name = strdup (&groups[g][domain_prefix_len]);
 		domain = g_malloc0 (sizeof (NMGlobalDnsDomain));
@@ -963,7 +997,7 @@ load_global_dns (GKeyFile *keyfile, gboolean internal)
 	}
 
 	dns_config->internal = internal;
-	global_dns_config_update_domain_list (dns_config);
+	global_dns_config_seal_domains (dns_config);
 	return dns_config;
 }
 
@@ -972,8 +1006,7 @@ void
 nm_global_dns_config_to_dbus (const NMGlobalDnsConfig *dns_config, GValue *value)
 {
 	GVariantBuilder conf_builder, domains_builder, domain_builder;
-	NMGlobalDnsDomain *domain;
-	GHashTableIter iter;
+	guint i;
 
 	g_variant_builder_init (&conf_builder, G_VARIANT_TYPE ("a{sv}"));
 	if (!dns_config)
@@ -990,27 +1023,30 @@ nm_global_dns_config_to_dbus (const NMGlobalDnsConfig *dns_config, GValue *value
 	}
 
 	g_variant_builder_init (&domains_builder, G_VARIANT_TYPE ("a{sv}"));
+	if (dns_config->domain_list) {
+		for (i = 0; dns_config->domain_list[i]; i++) {
+			NMGlobalDnsDomain *domain;
 
-	g_hash_table_iter_init (&iter, dns_config->domains);
-	while (g_hash_table_iter_next (&iter, NULL, (gpointer *) &domain)) {
+			domain = g_hash_table_lookup (dns_config->domains, dns_config->domain_list[i]);
 
-		g_variant_builder_init (&domain_builder, G_VARIANT_TYPE ("a{sv}"));
+			g_variant_builder_init (&domain_builder, G_VARIANT_TYPE ("a{sv}"));
 
-		if (domain->servers) {
-			g_variant_builder_add (&domain_builder, "{sv}", "servers",
-			                       g_variant_new_strv ((const char *const *) domain->servers, -1));
-		}
-		if (domain->options) {
-			g_variant_builder_add (&domain_builder, "{sv}", "options",
-			                       g_variant_new_strv ((const char *const *) domain->options, -1));
-		}
+			if (domain->servers) {
+				g_variant_builder_add (&domain_builder, "{sv}", "servers",
+				                       g_variant_new_strv ((const char *const *) domain->servers, -1));
+			}
+			if (domain->options) {
+				g_variant_builder_add (&domain_builder, "{sv}", "options",
+				                       g_variant_new_strv ((const char *const *) domain->options, -1));
+			}
 
-		g_variant_builder_add (&domains_builder, "{sv}", domain->name,
-		                       g_variant_builder_end (&domain_builder));
+			g_variant_builder_add (&domains_builder, "{sv}", domain->name,
+			                       g_variant_builder_end (&domain_builder));
+		}
 	}
-
 	g_variant_builder_add (&conf_builder, "{sv}", "domains",
 	                       g_variant_builder_end (&domains_builder));
+
 out:
 	g_value_take_variant (value, g_variant_builder_end (&conf_builder));
 }
@@ -1044,15 +1080,20 @@ global_dns_domain_from_dbus (char *name, GVariant *variant)
 				else
 					g_free (strv[i]);
 			}
-			if (j) {
+			if (j == 0)
+				g_free (strv);
+			else {
 				strv[j] = NULL;
+				g_strfreev (domain->servers);
 				domain->servers = strv;
-			} else
-				g_free (strv);
+			}
 		} else if (   !g_strcmp0 (key, "options")
 		           && g_variant_is_of_type (val, G_VARIANT_TYPE ("as"))) {
 			strv = g_variant_dup_strv (val, NULL);
+			g_strfreev (domain->options);
 			domain->options = _nm_utils_strv_cleanup (strv, TRUE, TRUE, TRUE);
+			if (!domain->options[0])
+				nm_clear_g_free (&domain->options);
 		}
 
 		g_variant_unref (val);
@@ -1111,11 +1152,12 @@ nm_global_dns_config_from_dbus (const GValue *value, GError **error)
 				else
 					g_free (strv[i]);
 			}
-
-			if (strv)
+			if (j == 0)
+				g_free (strv);
+			else {
 				strv[j] = NULL;
-
-			dns_config->options = strv;
+				dns_config->options = strv;
+			}
 		} else if (   !g_strcmp0 (key, "domains")
 		           && g_variant_is_of_type (val, G_VARIANT_TYPE ("a{sv}"))) {
 			NMGlobalDnsDomain *domain;
@@ -1145,7 +1187,7 @@ nm_global_dns_config_from_dbus (const GValue *value, GError **error)
 		return NULL;
 	}
 
-	global_dns_config_update_domain_list (dns_config);
+	global_dns_config_seal_domains (dns_config);
 	return dns_config;
 }
 
diff --git a/src/tests/config/test-config.c b/src/tests/config/test-config.c
index 8398841d39..a6b886ce43 100644
--- a/src/tests/config/test-config.c
+++ b/src/tests/config/test-config.c
@@ -254,7 +254,7 @@ test_config_global_dns (void)
 	NMConfig *config;
 	const NMGlobalDnsConfig *dns;
 	NMGlobalDnsDomain *domain;
-	const char *const *strv;
+	const char *const*strv;
 
 	config = setup_config (NULL, SRCDIR "/NetworkManager.conf", "", NULL,
 	                       "/no/such/dir", "", NULL);