Re: [PATH] Support resolvconf

[Roy Marples <roy marples name>]

On Sat, 2008-08-23 at 03:24 +0200, Michael Biebl wrote:
> I gave it a try (on Debian), but it didn't out of the box.
> If I activate a normal (i.e. no vpn) connection, priv->vpn_config will
> be NULL.
> Thus in rewrite_resolv_conf(line 445), dispatch_resolvconf is called
> with an empty config. The results is that in dispatch_resolvconf(line
> 377) the resolvconf entry will be *removed* and as TRUE is returned,
> rewrite_resolv_conf will return at this point.
> 
> I applied the attached patch to get it working for me. I haven't tested
> VPN connections.

This one should work. We should either have a vpn_config or
device_config when writing resolv.conf

New patch attached.

+       /* If resolvconf is available, use it.
+        * resolvconf only needs what the interface has provided, so
don't merge */
+       if (priv->vpn_config &&
+           dispatch_resolvconf (priv->vpn_config, iface, TRUE, error))
+               return TRUE;
+       if (dispatch_resolvconf (priv->device_config, iface, FALSE,
error))
+               return TRUE;


> I'm not sure if openresolv is completely compatible to (Debian)
> resolvconf. In Debian, if /etc/resolv.conf is *not* a symlink to
> /etc/resolvconf/run/resolv.conf, it means that resolvconf is disabled.
> I guess we should add a check at nm-named-manager.c:349, to return FALSE
> in that case, so NM will update /etc/resolv.conf

That is incorrect. Debians resolvconf program makes no such assumption.
However, the libc helper they ship with checks that /etc/resolv.conf is
a symlink to the actual resolv.conf file it updates - however it still
proceeds as normal. Here's the relevant part of their libc helper

report_warning() { echo "$0: Warning: $*" >&2 ; }

if [ ! -L ${ETC}/resolv.conf ] || [ ! "$(readlink ${ETC}/resolv.conf)" =
"$DYNAMICRSLVCNFFILE" ] ; then
	case "$REPORT_ABSENT_SYMLINK" in
		y|Y|yes|YES|Yes)
			report_warning "${ETC}/resolv.conf is not a symbolic link to
$DYNAMICRSLVCNFFILE"
			;;
	esac
	# Old name for the variable was '...ALTERED...' so accept that for now
	case "$REPORT_ALTERED_SYMLINK" in
		y|Y|yes|YES|Yes)
			report_warning "${ETC}/resolv.conf is not a symbolic link to
$DYNAMICRSLVCNFFILE"
			;;
	esac
fi

As you can see, it just warns but still proceeds.

Thanks

Roy

Index: src/named-manager/nm-named-manager.c
===================================================================
--- src/named-manager/nm-named-manager.c	(revision 4002)
+++ src/named-manager/nm-named-manager.c	(working copy)
@@ -38,6 +38,7 @@
 #include "nm-ip4-config.h"
 #include "nm-utils.h"
 #include "NetworkManagerSystem.h"
+#include "NetworkManagerUtils.h"
 
 #ifdef HAVE_SELINUX
 #include <selinux/selinux.h>
@@ -47,6 +48,8 @@
 #define RESOLV_CONF "/etc/resolv.conf"
 #endif
 
+#define ADDR_BUF_LEN 50
+
 G_DEFINE_TYPE(NMNamedManager, nm_named_manager, G_TYPE_OBJECT)
 
 #define NM_NAMED_MANAGER_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), \
@@ -115,6 +118,65 @@
 	}
 }
 
+static void
+get_resolv_conf(NMIP4Config *cfg, char ***searches, char ***nameservers)
+{
+	int num_domains;
+	int num_searches;
+	int num_nameservers;
+	int i;
+	GPtrArray *array;
+
+	/* Some DHCP servers like to put multiple search paths into the domain
+	 * option as the domain-search option wasn't around in the first RFC.
+	 * We should try and support these old servers as best we can. */
+
+	num_domains = nm_ip4_config_get_num_domains (cfg);
+	num_searches = nm_ip4_config_get_num_searches (cfg);
+
+	/* Searches */
+	if (num_searches > 0) {
+		array = g_ptr_array_sized_new (num_searches + 1);
+		for (i = 0; i < num_searches; i++)
+			g_ptr_array_add (array, g_strdup (nm_ip4_config_get_search (cfg, i)));
+
+		g_ptr_array_add (array, NULL);
+		*searches = (char **) g_ptr_array_free (array, FALSE);
+	} else if (num_domains > 0) {
+		array = g_ptr_array_sized_new (num_domains + 1);
+		for (i = 0; i < num_domains; i++)
+			g_ptr_array_add (array, g_strdup (nm_ip4_config_get_domain (cfg, i)));
+
+		g_ptr_array_add (array, NULL);
+		*searches = (char **) g_ptr_array_free (array, FALSE);
+	}
+
+	/* Name servers */
+	num_nameservers = nm_ip4_config_get_num_nameservers (cfg);
+	if (num_nameservers > 0) {
+		array = g_ptr_array_sized_new (num_nameservers + 1);
+		for (i = 0; i < num_nameservers; i++) {
+			struct in_addr addr;
+			char *buf;
+
+			addr.s_addr = nm_ip4_config_get_nameserver (cfg, i);
+			buf = g_malloc0 (ADDR_BUF_LEN);
+			if (!buf)
+				continue;
+
+			if (inet_ntop (AF_INET, &addr, buf, ADDR_BUF_LEN))
+				g_ptr_array_add (array, buf);
+			else
+				nm_warning ("%s: error converting IP4 address 0x%X",
+						  __func__, ntohl (addr.s_addr));
+		}
+
+		g_ptr_array_add (array, NULL);
+		*nameservers = (char **) g_ptr_array_free (array, FALSE);
+	}
+}
+
+
 #if defined(TARGET_SUSE)
 /**********************************/
 /* SUSE */
@@ -164,98 +226,89 @@
 }
 
 static gboolean
-update_resolv_conf (const char *iface,
-				const char *domain,
-				char **searches,
-				char **nameservers,
-				GError **error)
+dispatch_netconfig (NMIP4Config *cfg, const char *iface, GError **error)
 {
 	gint fd;
 	char *str;
+	char **searches, **nameservers;
 
 	fd = run_netconfig (error);
 	if (fd < 0)
 		return FALSE;
 
+	get_resolv_conf (cfg, &searches, &nameservers);
+
 	write_to_netconfig (fd, "INTERFACE", iface);
 
 	if (searches) {
 		str = g_strjoinv (" ", searches);
 		write_to_netconfig (fd, "DNSDOMAIN", str);
 		g_free (str);
+		g_strfreev (searches);
 	}
 
 	if (nameservers) {
 		str = g_strjoinv (" ", nameservers);
 		write_to_netconfig (fd, "DNSSERVERS", str);
 		g_free (str);
+		g_strfreev (nameservers);
 	}
 
 	close (fd);
 
 	return TRUE;
 }
+#endif
 
-#else
-/**********************************/
-/* Generic */
 
 static gboolean
-update_resolv_conf (const char *iface,
-				const char *domain,
-				char **searches,
-				char **nameservers,
-				GError **error)
+write_resolv_conf (FILE *f, NMIP4Config *cfg, gboolean do_domain, GError **error)
 {
-	const char *tmp_resolv_conf = RESOLV_CONF ".tmp";
 	char *domain_str = NULL;
 	char *searches_str = NULL;
 	char *nameservers_str = NULL;
-	FILE *f;
+	const char *domain;
+	char **searches = NULL;
+	char **nameservers = NULL;
+	int i;
+	gboolean retval = FALSE;
 
-	if ((f = fopen (tmp_resolv_conf, "w")) == NULL) {
+	if (fprintf (f, "%s","# generated by NetworkManager\n") < 0) {
 		g_set_error (error,
 				   NM_NAMED_MANAGER_ERROR,
 				   NM_NAMED_MANAGER_ERROR_SYSTEM,
-				   "Could not open " RESOLV_CONF ": %s\n",
+				   "Could not write " RESOLV_CONF ": %s\n",
 				   g_strerror (errno));
 		return FALSE;
 	}
 
-	if (fprintf (f, "%s","# generated by NetworkManager, do not edit!\n\n") < 0) {
-		g_set_error (error,
-				   NM_NAMED_MANAGER_ERROR,
-				   NM_NAMED_MANAGER_ERROR_SYSTEM,
-				   "Could not write " RESOLV_CONF ": %s\n",
-				   g_strerror (errno));
-		fclose (f);
-		return FALSE;
+	get_resolv_conf (cfg, &searches, &nameservers);
+
+	if (do_domain) {
+		domain = nm_ip4_config_get_domain (cfg, 0);
+		domain_str = g_strconcat ("domain ", domain, "\n", NULL);
 	}
 
-	if (domain)
-		domain_str = g_strconcat ("domain ", domain, "\n\n", NULL);
-
 	if (searches) {
 		char *tmp_str;
 
 		tmp_str = g_strjoinv (" ", searches);
-		searches_str = g_strconcat ("search ", tmp_str, "\n\n", NULL);
+		searches_str = g_strconcat ("search ", tmp_str, "\n", NULL);
 		g_free (tmp_str);
 	}
 
 	if (nameservers) {
 		GString *str;
 		int num;
-		int i;
 
 		str = g_string_new ("");
 		num = g_strv_length (nameservers);
 
 		for (i = 0; i < num; i++) {
 			if (i == 3) {
+				g_string_append (str, "# ");
+				g_string_append (str, _("NOTE: the libc resolver may not support more than 3 nameservers."));
 				g_string_append (str, "\n# ");
-				g_string_append (str, _("NOTE: the glibc resolver does not support more than 3 nameservers."));
-				g_string_append (str, "\n# ");
 				g_string_append (str, _("The nameservers listed below may not be recognized."));
 				g_string_append_c (str, '\n');
 			}
@@ -268,20 +321,87 @@
 		nameservers_str = g_string_free (str, FALSE);
 	}
 
-	if (fprintf (f, "%s%s%s\n",
-	             domain_str ? domain_str : "",
+	if (fprintf (f, "%s%s%s",
+		     domain_str ? domain_str : "",
 	             searches_str ? searches_str : "",
-	             nameservers_str ? nameservers_str : "") < 0) {
+	             nameservers_str ? nameservers_str : "") != -1)
+		retval = TRUE;
+
+	g_strfreev (searches);
+	g_strfreev (nameservers);
+	g_free (domain_str);
+	g_free (searches_str);
+	g_free (nameservers_str);
+
+	return retval;
+}
+
+
+static gboolean
+dispatch_resolvconf (NMIP4Config *cfg, const char *iface, gboolean do_domain, GError **error)
+{
+	char **paths, **path, *tmp, *cmd;
+	char *resolvconf = NULL;
+	FILE *f;
+	gboolean retval = FALSE;
+
+	/* See if resolvconf is installed */
+	if ((tmp = getenv ("PATH"))) {
+		paths = g_strsplit (tmp, ":", 0);
+		for (path = paths; *path; path++) {
+			resolvconf = g_strconcat (*path, "/resolvconf", NULL);
+			if (g_file_test (resolvconf, G_FILE_TEST_IS_EXECUTABLE))
+				break;
+			g_free (resolvconf);
+			resolvconf = NULL;
+		}
+		g_free (paths);
+		if (resolvconf == NULL)
+			return FALSE;
+	}
+
+	cmd = g_strconcat (resolvconf, cfg ? " -a " : " -d ", iface, NULL);
+	if (cfg) {
+		nm_info ("(%s): writing resolv.conf to %s", iface, resolvconf);
+		if ((f = popen (cmd, "w")) == NULL)
+			g_set_error (error,
+				     NM_NAMED_MANAGER_ERROR,
+				     NM_NAMED_MANAGER_ERROR_SYSTEM,
+				     "Could not write to %s: %s\n",
+				     resolvconf,
+				     g_strerror (errno));
+		else {
+			retval = write_resolv_conf (f, cfg, do_domain, error);
+			pclose (f);
+		}
+	} else {
+		nm_info ("(%s): removing resolv.conf from %s", iface, resolvconf);
+		if (nm_spawn_process (cmd) == 0)
+			retval = TRUE;
+	}
+
+	g_free (cmd);
+	g_free (resolvconf);
+
+	return retval;
+}
+
+static gboolean
+update_resolv_conf (NMIP4Config *cfg, const char *iface, GError **error)
+{
+	const char *tmp_resolv_conf = RESOLV_CONF ".tmp";
+	FILE *f;
+
+	if ((f = fopen (tmp_resolv_conf, "w")) == NULL) {
 		g_set_error (error,
 				   NM_NAMED_MANAGER_ERROR,
 				   NM_NAMED_MANAGER_ERROR_SYSTEM,
-				   "Could not write to " RESOLV_CONF ": %s\n",
+				   "Could not open " RESOLV_CONF ": %s\n",
 				   g_strerror (errno));
+		return FALSE;
 	}
 
-	g_free (domain_str);
-	g_free (searches_str);
-	g_free (nameservers_str);
+	write_resolv_conf (f, cfg, FALSE, error);
 
 	if (fclose (f) < 0) {
 		if (*error == NULL) {
@@ -305,9 +425,7 @@
 
 	return *error ? FALSE : TRUE;
 }
-#endif
 
-#define ADDR_BUF_LEN 50
 
 static gboolean
 rewrite_resolv_conf (NMNamedManager *mgr, const char *iface, GError **error)
@@ -315,21 +433,21 @@
 	NMNamedManagerPrivate *priv;
 	NMIP4Config *composite;
 	GSList *iter;
-	GPtrArray *array;
-	const char *domain = NULL;
-	char **searches = NULL;
-	char **nameservers = NULL;
-	int num_domains;
-	int num_searches;
-	int num_nameservers;
-	int i;
-	gboolean success;
+	gboolean success = FALSE;
 
 	g_return_val_if_fail (error != NULL, FALSE);
 	g_return_val_if_fail (*error == NULL, FALSE);
 
 	priv = NM_NAMED_MANAGER_GET_PRIVATE (mgr);
 
+	/* If resolvconf is available, use it.
+	 * resolvconf only needs what the interface has provided, so don't merge */
+	if (priv->vpn_config &&
+	    dispatch_resolvconf (priv->vpn_config, iface, TRUE, error))
+		return TRUE;
+	if (dispatch_resolvconf (priv->device_config, iface, FALSE, error))
+		return TRUE;
+
 	/* Construct the composite config from all the currently active IP4Configs */
 	composite = nm_ip4_config_new ();
 
@@ -348,79 +466,13 @@
 		merge_one_ip4_config (composite, config);
 	}
 
-	/* ISC DHCP 3.1 provides support for the domain-search option. This is the
-	 * correct way for a DHCP server to provide a domain search list. Wedging
-	 * multiple domains into the domain-name option is a horrible hack.
-	 *
-	 * So, we handle it like this (as proposed by Andrew Pollock at
-	 * http://bugs.debian.org/465158):
-	 *
-	 * - if the domain-search option is present in the data received via DHCP,
-	 *   use it in favour of the domain-name option for setting the search
-	 *   directive in /etc/resolv.conf
-	 *
-	 * - if the domain-name option is present in the data received via DHCP, use
-	 *   it to set the domain directive in /etc/resolv.conf
-	 *   (this is handled in compute_domain() below)
-	 *
-	 * - if only the domain-name option is present in the data received via DHCP
-	 *   (and domain-search is not), for backwards compatibility, set the search
-	 *   directive in /etc/resolv.conf to the specified domain names
-	 */
+#ifdef TARGET_SUSE
+	success = dispatch_netconfig (composite, iface, error);
+#endif
 
-	num_domains = nm_ip4_config_get_num_domains (composite);
-	num_searches = nm_ip4_config_get_num_searches (composite);
+	if (success == FALSE)
+		success = update_resolv_conf (composite, iface, error);
 
-	/* Domain */
-	if (num_domains > 0)
-		domain = nm_ip4_config_get_domain (composite, 0);
-
-	/* Searches */
-	if ((num_searches == 0)  && (num_domains > 0)) {
-		array = g_ptr_array_sized_new (num_domains + 1);
-		for (i = 0; i < num_domains; i++)
-			g_ptr_array_add (array, g_strdup (nm_ip4_config_get_domain (composite, i)));
-
-		g_ptr_array_add (array, NULL);
-		searches = (char **) g_ptr_array_free (array, FALSE);
-	} else if (num_searches > 0) {
-		array = g_ptr_array_sized_new (num_searches + 1);
-		for (i = 0; i < num_searches; i++)
-			g_ptr_array_add (array, g_strdup (nm_ip4_config_get_search (composite, i)));
-
-		g_ptr_array_add (array, NULL);
-		searches = (char **) g_ptr_array_free (array, FALSE);
-	}
-
-	/* Name servers */
-	num_nameservers = nm_ip4_config_get_num_nameservers (composite);
-	if (num_nameservers > 0) {
-		array = g_ptr_array_sized_new (num_nameservers + 1);
-		for (i = 0; i < num_nameservers; i++) {
-			struct in_addr addr;
-			char *buf;
-
-			addr.s_addr = nm_ip4_config_get_nameserver (composite, i);
-			buf = g_malloc0 (ADDR_BUF_LEN);
-			if (!buf)
-				continue;
-
-			if (inet_ntop (AF_INET, &addr, buf, ADDR_BUF_LEN))
-				g_ptr_array_add (array, buf);
-			else
-				nm_warning ("%s: error converting IP4 address 0x%X",
-						  __func__, ntohl (addr.s_addr));
-		}
-
-		g_ptr_array_add (array, NULL);
-		nameservers = (char **) g_ptr_array_free (array, FALSE);
-	}
-
-	success = update_resolv_conf (iface, domain, searches, nameservers, error);
-
-	g_strfreev (searches);
-	g_strfreev (nameservers);
-
 	if (success)
 		nm_system_update_dns ();