Re: [PATH] Support resolvconf

[Roy Marples <roy marples name>]

On Tue, 2008-08-26 at 22:20 -0400, Dan Williams wrote:
> On Wed, 2008-08-27 at 03:49 +0200, Michael Biebl wrote:
> > Roy Marples wrote:
> > > On Sun, 2008-08-24 at 01:01 +0200, Alexander Sack wrote:
> > 
> > >> For the domain/search tweakage: I feel unsure about this (though I kept
> > >> this part of your patch untouched for now). This is quite a specific
> > >> feature you want to implicitly entrench here. Maybe we can push
> > >> that back until we know if and how we want to implement such a
> > >> feature?
> > > 
> > > There's no real need to push it back.
> > > NM currently folds domain into searches, so there's no need to actually
> > > use domain at all.
> > 
> > Hi Roy,
> > 
> > I agree with Alex here.
> > Could you please split up the patch into two, so we can address those
> > two issues separately.
> > I think the resolvconf part is fine.
> > Dan, if you are ok with it also, I'd be going to commit this, unless you
> > beat me to it ;-)
> 
> It looks OK, except for one change.  Not sure where we left the
> conversation last, but I'm not sure if just because resolvconf is
> installed means that it's actually being used.  Most of the time you can
> have a package installed, but you don't have to enable it.  For the
> moment, I'd suggest either (1) #ifdef-ing the resolvconf specific stuff
> with #ifdef DEBIAN, or (2) adding a configure switch for
> --with-resolv-conf=yes which would #define USE_RESOLVCONF, and then
> protect the resolvconf stuff with #ifdef USE_RESOLVCONF.

New patch attached to enable resolvconf support via configure.
Keeps exiting domain/search logic.
Hunks #10 and #11 are optional, just a condensation of the big comment
and a code optimisation.

> > For the domain/search changes, I'd like to take a separate look.
> 
> Agreed, though I think the behavior change here is just to make searches
> > domain, while right now it's the other way around.  I'm not exactly
> sure whether that's the right thing to do but we should probably be
> following the current standards and doing backwards-compat as the
> fallback unless there's a major problem.

If I have the time, I may investigate a more intrusive patch so that
resolvconf really gets the right interface information instead of the
"best" information so it can work properly and maybe work with the
domain switch for VPNs. Since I first sent my email about this, I've had
a few replies off list asking about it so I believe there is the demand
for it.

Thanks

Roy

Index: src/named-manager/nm-named-manager.c
===================================================================
--- src/named-manager/nm-named-manager.c	(revision 4027)
+++ src/named-manager/nm-named-manager.c	(working copy)
@@ -23,6 +23,7 @@
 
 #include "config.h"
 
+#include <limits.h>
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
@@ -38,6 +39,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 +49,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 +119,7 @@
 	}
 }
 
+
 #if defined(TARGET_SUSE)
 /**********************************/
 /* SUSE */
@@ -157,10 +162,9 @@
 }
 
 static gboolean
-update_resolv_conf (const char *iface,
-				const char *domain,
-				char **searches,
+dispatch_netconfig (char **searches,
 				char **nameservers,
+				const char *iface,
 				GError **error)
 {
 	gint fd;
@@ -188,67 +192,53 @@
 
 	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, const char *domain,
+		   char **searches,
+		   char **nameservers,
+		   GError **error)
 {
-	const char *tmp_resolv_conf = RESOLV_CONF ".tmp";
 	char *domain_str = NULL;
 	char *searches_str = NULL;
 	char *nameservers_str = NULL;
-	FILE *f;
+	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",
-				   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;
 	}
 
 	if (domain)
-		domain_str = g_strconcat ("domain ", domain, "\n\n", NULL);
+		domain_str = g_strconcat ("domain ", domain, "\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');
 			}
@@ -261,27 +251,94 @@
 		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_free (domain_str);
+	g_free (searches_str);
+	g_free (nameservers_str);
+
+	return retval;
+}
+
+#ifdef RESOLVCONF_PATH
+static gboolean
+dispatch_resolvconf (const char *domain,
+		     char **searches,
+		     char **nameservers,
+		     const char *iface,
+		     GError **error)
+{
+	char *cmd;
+	FILE *f;
+	gboolean retval = FALSE;
+
+	if (! g_file_test (RESOLVCONF_PATH, G_FILE_TEST_IS_EXECUTABLE))
+		return FALSE;
+
+	if (domain || searches || nameservers) {
+		cmd = g_strconcat (RESOLVCONF_PATH, " -a ", "NetworkManager", NULL);
+		nm_info ("(%s): writing resolv.conf to %s", iface, RESOLVCONF_PATH);
+		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_PATH,
+				     g_strerror (errno));
+		else {
+			retval = write_resolv_conf (f, domain, searches, nameservers, error);
+			pclose (f);
+		}
+	} else {
+		cmd = g_strconcat (RESOLVCONF_PATH, " -d ", "NetworkManager", NULL);
+		nm_info ("(%s): removing resolv.conf from %s", iface, RESOLVCONF_PATH);
+		if (nm_spawn_process (cmd) == 0)
+			retval = TRUE;
+	}
+
+	g_free (cmd);
+
+	return retval;
+}
+#endif
+
+static gboolean
+update_resolv_conf (const char *domain,
+		    		char **searches,
+				char **nameservers,
+				const char *iface,
+				GError **error)
+{
+	const char *tmp_resolv_conf = RESOLV_CONF ".tmp";
+	char tmp_resolv_conf_realpath [PATH_MAX];
+	FILE *f;
+
+	if (!realpath(tmp_resolv_conf, tmp_resolv_conf_realpath))
+		strcpy(tmp_resolv_conf_realpath, tmp_resolv_conf);
+
+	if ((f = fopen (tmp_resolv_conf_realpath, "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 %s: %s\n",
+				   tmp_resolv_conf_realpath,
 				   g_strerror (errno));
+		return FALSE;
 	}
 
-	g_free (domain_str);
-	g_free (searches_str);
-	g_free (nameservers_str);
+	write_resolv_conf (f, domain, searches, nameservers, error);
 
 	if (fclose (f) < 0) {
 		if (*error == NULL) {
 			g_set_error (error,
 					   NM_NAMED_MANAGER_ERROR,
 					   NM_NAMED_MANAGER_ERROR_SYSTEM,
-					   "Could not close " RESOLV_CONF ": %s\n",
+					   "Could not close %s: %s\n",
+					   tmp_resolv_conf_realpath,
 					   g_strerror (errno));
 		}
 	}
@@ -298,9 +355,7 @@
 
 	return *error ? FALSE : TRUE;
 }
-#endif
 
-#define ADDR_BUF_LEN 50
 
 static gboolean
 rewrite_resolv_conf (NMNamedManager *mgr, const char *iface, GError **error)
@@ -316,7 +371,7 @@
 	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);
@@ -341,25 +396,9 @@
 		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
-	 */
+	/* 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 (composite);
 	num_searches = nm_ip4_config_get_num_searches (composite);
@@ -369,20 +408,20 @@
 		domain = nm_ip4_config_get_domain (composite, 0);
 
 	/* Searches */
-	if ((num_searches == 0)  && (num_domains > 0)) {
+	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);
+	} 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 (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 */
@@ -409,14 +448,24 @@
 		nameservers = (char **) g_ptr_array_free (array, FALSE);
 	}
 
-	success = update_resolv_conf (iface, domain, searches, nameservers, error);
+#ifdef RESOLVCONF_PATH
+	success = dispatch_resolvconf (domain, searches, nameservers, iface, error);
+#endif
 
-	g_strfreev (searches);
-	g_strfreev (nameservers);
+#ifdef TARGET_SUSE
+	if (success == FALSE)
+		success = dispatch_netconfig (domain, searches, nameservers, iface, error);
+#endif
 
+	if (success == FALSE)
+		success = update_resolv_conf (domain, searches, nameservers, iface, error);
+
 	if (success)
 		nm_system_update_dns ();
 
+	g_strfreev (searches);
+	g_strfreev (nameservers);
+
 	return success;
 }
 
Index: configure.in
===================================================================
--- configure.in	(revision 4027)
+++ configure.in	(working copy)
@@ -402,6 +402,38 @@
 fi
 AC_SUBST(DHCP_CLIENT)
 
+# resolvconf support
+AC_ARG_WITH([resolvconf],
+	    AS_HELP_STRING([--enable-resolvconf=yes|no|path], [Enable resolvconf support])
+enable_resolvconf="$enableval",enable_resolvconf=yes)
+# If a full path is given, use that and do not test if it works or not.
+case "${enable_resolvconf}" in
+	/*)
+		RESOLVCONF_PATH="${enable_resolvconf}"
+		AC_MSG_NOTICE(setting resolvconf path to ${RESOLVCONF_PATH})
+		;;
+	no)	AC_MSG_NOTICE(disabling resolvconf support)
+		;;
+	*)
+		AC_MSG_CHECKING(for resolvconf)
+		for path in /sbin /usr/sbin /usr/pkg/sbin /usr/local/sbin; do
+			if test -x "${path}/resolvconf"; then
+				RESOLVCONF_PATH="${path}/resolvconf"
+				break
+			fi
+		done
+		if test -n "${RESOLVCONF_PATH}"; then
+			AC_MSG_RESULT($RESOLVCONF_PATH)
+		else
+			AC_MSG_RESULT(no)
+		fi
+		;;
+esac
+AC_SUBST(RESOLVCONF_PATH)
+if test -n "${RESOLVCONF_PATH}"; then
+	AC_DEFINE_UNQUOTED(RESOLVCONF_PATH, "$RESOLVCONF_PATH", [Define if you have a resolvconf implementation])
+fi
+
 AC_ARG_ENABLE(more-warnings,
 AS_HELP_STRING([--enable-more-warnings], [Maximum compiler warnings]), set_more_warnings="$enableval",set_more_warnings=yes)
 AC_MSG_CHECKING(for more warnings, including -Werror)