[PATCH] [RFC] Use timeouts in DHCP client daemons

[Daniel Gnoutcheff <daniel gnoutcheff name>]

OK, here's my suggestion with how to deal with lazy DHCP servers, 
finally codified as a patch. :) Please be kind, this is my first patch 
ever :D

This moves responsibility for DHCP timeouts down to the DHCP client 
daemons. Thus, dhclient now has the freedom to take its time when it 
finds a functioning but lazy DHCP server. When a timeout is specified in 
the corresponding nm_dhcp_client_start parameter, that value is passed 
along to the dhcp client daemon. Otherwise, I let dhclient/dhcpcd use 
their defaults (usually 60 seconds)

I've tested out the dhclient half of this patch, and I've been running 
n-m with this patch for about a day now. It certainly has helped, 
dealing with this network is less annoying now. :)

My main concerns with my patch as it stands:

- When no timeout is supplied to nm_dhcp_client_start, I let dhclient 
respect the "timeout" value specified (possibly by the user) in 
dhclient.conf. Otherwise, it gets overridden. Is this going to 
confuse/annoy users trying to customize the timeout?

- My changes for dhcpcd have not been tested at all, not even 
compile-tested. I can test it, but I first wanted to ping this list to 
see how people feel about my approach.

Thanks in advance for any comments!

Have a great one,
Daniel

diff --git a/src/dhcp-manager/nm-dhcp-dhclient.c b/src/dhcp-manager/nm-dhcp-dhclient.c
index 478ac29..b62353f 100644
--- a/src/dhcp-manager/nm-dhcp-dhclient.c
+++ b/src/dhcp-manager/nm-dhcp-dhclient.c
@@ -287,10 +287,14 @@ out:
 #define DHCP_HOSTNAME_TAG "send host-name"
 #define DHCP_HOSTNAME_FORMAT DHCP_HOSTNAME_TAG " \"%s\"; # added by NetworkManager"
 
+#define DHCP_TIMEOUT_TAG "timeout"
+#define DHCP_TIMEOUT_FORMAT DHCP_TIMEOUT_TAG " %u; # added by NetworkManager"
+
 static gboolean
 merge_dhclient_config (NMDHCPDevice *device,
                        NMSettingIP4Config *s_ip4,
                        guint8 *anycast_addr,
+                       guint32 timeout,
                        const char *contents,
                        const char *orig,
                        GError **error)
@@ -326,6 +330,10 @@ merge_dhclient_config (NMDHCPDevice *device,
 			    && !strncmp (*line, DHCP_HOSTNAME_TAG, strlen (DHCP_HOSTNAME_TAG)))
 				ignore = TRUE;
 
+			if (timeout 
+			    && !strncmp (*line, DHCP_TIMEOUT_TAG, strlen (DHCP_TIMEOUT_TAG)))
+				ignore = TRUE;
+
 			if (!ignore) {
 				g_string_append (new_contents, *line);
 				g_string_append_c (new_contents, '\n');
@@ -380,6 +388,9 @@ merge_dhclient_config (NMDHCPDevice *device,
 		                        anycast_addr[4], anycast_addr[5]);
 	}
 
+	if (timeout)
+		g_string_append_printf (new_contents, DHCP_TIMEOUT_FORMAT "\n", timeout);
+
 	if (g_file_set_contents (device->conf_file, new_contents->str, -1, error))
 		success = TRUE;
 
@@ -396,7 +407,8 @@ merge_dhclient_config (NMDHCPDevice *device,
 static gboolean
 create_dhclient_config (NMDHCPDevice *device,
                         NMSettingIP4Config *s_ip4,
-                        guint8 *dhcp_anycast_addr)
+                        guint8 *dhcp_anycast_addr,
+                        guint32 timeout)
 {
 	char *orig = NULL, *contents = NULL;
 	GError *error = NULL;
@@ -436,7 +448,7 @@ create_dhclient_config (NMDHCPDevice *device,
 
 out:
 	error = NULL;
-	if (merge_dhclient_config (device, s_ip4, dhcp_anycast_addr, contents, orig, &error))
+	if (merge_dhclient_config (device, s_ip4, dhcp_anycast_addr, timeout, contents, orig, &error))
 		success = TRUE;
 	else {
 		nm_warning ("%s: error creating dhclient configuration: %s",
@@ -463,7 +475,8 @@ GPid
 nm_dhcp_client_start (NMDHCPDevice *device,
                       const char *uuid,
                       NMSettingIP4Config *s_ip4,
-                      guint8 *dhcp_anycast_addr)
+                      guint8 *dhcp_anycast_addr,
+                      guint32 timeout)
 {
 	GPtrArray *dhclient_argv = NULL;
 	GPid pid = 0;
@@ -487,7 +500,7 @@ nm_dhcp_client_start (NMDHCPDevice *device,
 		goto out;
 	}
 
-	if (!create_dhclient_config (device, s_ip4, dhcp_anycast_addr))
+	if (!create_dhclient_config (device, s_ip4, dhcp_anycast_addr, timeout))
 		goto out;
 
 	/* Kill any existing dhclient bound to this interface */
diff --git a/src/dhcp-manager/nm-dhcp-dhcpcd.c b/src/dhcp-manager/nm-dhcp-dhcpcd.c
index a8d929a..12ddc8e 100644
--- a/src/dhcp-manager/nm-dhcp-dhcpcd.c
+++ b/src/dhcp-manager/nm-dhcp-dhcpcd.c
@@ -68,12 +68,14 @@ GPid
 nm_dhcp_client_start (NMDHCPDevice *device,
                       const char *uuid,
                       NMSettingIP4Config *s_ip4,
-                      guint8 *dhcp_anycast_addr)
+                      guint8 *dhcp_anycast_addr,
+                      guint32 timeout)
 {
 	GPtrArray *argv = NULL;
 	GPid pid = 0;
 	GError *error = NULL;
 	char *pid_contents = NULL;
+	char *timeout_str = NULL;
 
 	if (!g_file_test (DHCP_CLIENT_PATH, G_FILE_TEST_EXISTS)) {
 		nm_warning (DHCP_CLIENT_PATH " does not exist.");
@@ -107,6 +109,13 @@ nm_dhcp_client_start (NMDHCPDevice *device,
 	g_ptr_array_add (argv, (gpointer) "-c");	/* Set script file */
 	g_ptr_array_add (argv, (gpointer) ACTION_SCRIPT_PATH );
 
+	if (timeout) {	/* Set timeout, if one was specified */
+		timeout_str = g_strdup_printf ("%u", timeout);
+
+		g_ptr_array_add (argv, (gpointer) "-t");
+		g_ptr_array>add (argv, (gpointer) timeout_str);
+	}
+
 	g_ptr_array_add (argv, (gpointer) device->iface);
 	g_ptr_array_add (argv, NULL);
 
@@ -121,6 +130,7 @@ nm_dhcp_client_start (NMDHCPDevice *device,
 
 out:
 	g_free (pid_contents);
+	g_free (timeout_str);
 	g_ptr_array_free (argv, TRUE);
 	return pid;
 }
diff --git a/src/dhcp-manager/nm-dhcp-manager.c b/src/dhcp-manager/nm-dhcp-manager.c
index c3ca358..ef06d33 100644
--- a/src/dhcp-manager/nm-dhcp-manager.c
+++ b/src/dhcp-manager/nm-dhcp-manager.c
@@ -47,8 +47,6 @@
 #define NM_DHCP_CLIENT_DBUS_SERVICE "org.freedesktop.nm_dhcp_client"
 #define NM_DHCP_CLIENT_DBUS_IFACE   "org.freedesktop.nm_dhcp_client"
 
-#define NM_DHCP_TIMEOUT   	45 /* DHCP timeout, in seconds */
-
 typedef struct {
 	NMDBusManager * dbus_mgr;
 	GHashTable *	devices;
@@ -63,7 +61,6 @@ G_DEFINE_TYPE (NMDHCPManager, nm_dhcp_manager, G_TYPE_OBJECT)
 
 enum {
 	STATE_CHANGED,
-	TIMEOUT,
 	LAST_SIGNAL
 };
 
@@ -132,16 +129,6 @@ nm_dhcp_manager_class_init (NMDHCPManagerClass *manager_class)
 					  G_TYPE_NONE, 2,
 					  G_TYPE_STRING,
 					  G_TYPE_UCHAR);
-
-	signals[TIMEOUT] =
-		g_signal_new ("timeout",
-					  G_OBJECT_CLASS_TYPE (object_class),
-					  G_SIGNAL_RUN_FIRST,
-					  G_STRUCT_OFFSET (NMDHCPManagerClass, timeout),
-					  NULL, NULL,
-					  g_cclosure_marshal_VOID__STRING,
-					  G_TYPE_NONE, 1,
-					  G_TYPE_STRING);
 }
 
 static gboolean state_is_bound (guint8 state)
@@ -158,15 +145,6 @@ static gboolean state_is_bound (guint8 state)
 
 
 static void
-nm_dhcp_device_timeout_cleanup (NMDHCPDevice * device)
-{
-	if (device->timeout_id) {
-		g_source_remove (device->timeout_id);
-		device->timeout_id = 0;
-	}
-}
-
-static void
 nm_dhcp_device_watch_cleanup (NMDHCPDevice * device)
 {
 	if (device->watch_id) {
@@ -180,8 +158,6 @@ nm_dhcp_device_destroy (NMDHCPDevice *device)
 {
 	int ignored;
 
-	nm_dhcp_device_timeout_cleanup (device);
-
 	if (device->pid)
 		nm_dhcp_client_stop (device, device->pid);
 
@@ -368,12 +344,6 @@ handle_options (NMDHCPManager * manager,
 	if (old_state == new_state)
 		return;
 
-	/* Handle changed device state */
-	if (state_is_bound (new_state)) {
-		/* Cancel the timeout if the DHCP client is now bound */
-		nm_dhcp_device_timeout_cleanup (device);
-	}
-
 	device->state = new_state;
 	nm_info ("DHCP: device %s state changed %s -> %s",
 	         device->iface,
@@ -497,26 +467,6 @@ out:
 }
 
 
-/*
- * nm_dhcp_manager_handle_timeout
- *
- * Called after timeout of a DHCP transaction to notify device of the failure.
- *
- */
-static gboolean
-nm_dhcp_manager_handle_timeout (gpointer user_data)
-{
-	NMDHCPDevice *device = (NMDHCPDevice *) user_data;
-
-	nm_info ("(%s): DHCP transaction took too long, stopping it.", device->iface);
-
-	nm_dhcp_manager_cancel_transaction (device->manager, device->iface);
-
-	g_signal_emit (G_OBJECT (device->manager), signals[TIMEOUT], 0, device->iface);
-
-	return FALSE;
-}
-
 static NMDHCPDevice *
 nm_dhcp_device_new (NMDHCPManager *manager, const char *iface)
 {
@@ -577,7 +527,6 @@ static void dhcp_watch_cb (GPid pid, gint status, gpointer user_data)
 	device->pid = 0;
 
 	nm_dhcp_device_watch_cleanup (device);
-	nm_dhcp_device_timeout_cleanup (device);
 
 	g_signal_emit (G_OBJECT (device->manager), signals[STATE_CHANGED], 0, device->iface, device->state);
 }
@@ -625,12 +574,11 @@ nm_dhcp_manager_begin_transaction (NMDHCPManager *manager,
 		setting = s_ip4 ? g_object_ref (s_ip4) : NULL;
 	}
 
-	if (timeout == 0)
-		timeout = NM_DHCP_TIMEOUT;
-
-	nm_info ("Activation (%s) Beginning DHCP transaction (timeout in %d seconds)",
-	         iface, timeout);
-	device->pid = nm_dhcp_client_start (device, uuid, setting, dhcp_anycast_addr);
+	nm_info ("Activation (%s) Beginning DHCP transaction ", iface);
+	if (timeout)
+		nm_info ("Activation (%s) DHCP timeout forced to %u seconds", iface, timeout);
+	device->pid = nm_dhcp_client_start (device, uuid,
+					 setting, dhcp_anycast_addr, timeout);
 
 	if (setting)
 		g_object_unref (setting);
@@ -638,10 +586,6 @@ nm_dhcp_manager_begin_transaction (NMDHCPManager *manager,
 	if (device->pid == 0)
 		return FALSE;
 
-	/* Set up a timeout on the transaction to kill it after the timeout */
-	device->timeout_id = g_timeout_add_seconds (timeout,
-	                                            nm_dhcp_manager_handle_timeout,
-	                                            device);
 	device->watch_id = g_child_watch_add (device->pid,
 					      (GChildWatchFunc) dhcp_watch_cb,
 					      device);
@@ -726,7 +670,6 @@ nm_dhcp_manager_cancel_transaction_real (NMDHCPDevice *device)
 		device->conf_file = NULL;
 	}
 
-	nm_dhcp_device_timeout_cleanup (device);
 	g_hash_table_remove_all (device->options);
 }
 
diff --git a/src/dhcp-manager/nm-dhcp-manager.h b/src/dhcp-manager/nm-dhcp-manager.h
index 1240084..0297080 100644
--- a/src/dhcp-manager/nm-dhcp-manager.h
+++ b/src/dhcp-manager/nm-dhcp-manager.h
@@ -69,7 +69,6 @@ typedef struct {
 
 	/* Signals */
 	void (*state_changed) (NMDHCPManager *manager, char *iface, NMDHCPState state);
-	void (*timeout)       (NMDHCPManager *manager, char *iface);
 } NMDHCPManagerClass;
 
 typedef struct {
@@ -79,7 +78,6 @@ typedef struct {
         char *			pid_file;
         char *			conf_file;
         char *			lease_file;
-        guint			timeout_id;
         guint			watch_id;
         NMDHCPManager *		manager;
         GHashTable *		options;
@@ -115,7 +113,8 @@ GSList *       nm_dhcp_manager_get_lease_ip4_config (NMDHCPManager *self,
 GPid           nm_dhcp_client_start                 (NMDHCPDevice *device,
                                                      const char *uuid,
                                                      NMSettingIP4Config *s_ip4,
-                                                     guint8 *anycast_addr);
+                                                     guint8 *anycast_addr,
+                                                     guint32 timeout);
 void           nm_dhcp_client_stop                  (NMDHCPDevice *device, pid_t pid);
 
 gboolean       nm_dhcp_client_process_classless_routes (GHashTable *options,
diff --git a/src/nm-device.c b/src/nm-device.c
index 6c0f99c..cca5731 100644
--- a/src/nm-device.c
+++ b/src/nm-device.c
@@ -107,7 +107,6 @@ typedef struct {
 	NMDHCPManager * dhcp_manager;
 	guint32         dhcp_timeout;
 	gulong          dhcp_state_sigid;
-	gulong          dhcp_timeout_sigid;
 	GByteArray *    dhcp_anycast_address;
 	NMDHCP4Config * dhcp4_config;
 
@@ -2348,20 +2347,6 @@ dhcp_state_changed (NMDHCPManager *dhcp_manager,
 	}
 }
 
-static void
-dhcp_timeout (NMDHCPManager *dhcp_manager,
-              const char *iface,
-              gpointer user_data)
-{
-	NMDevice * device = NM_DEVICE (user_data);
-
-	if (strcmp (nm_device_get_ip_iface (device), iface) != 0)
-		return;
-
-	if (nm_device_get_state (device) == NM_DEVICE_STATE_IP_CONFIG)
-		nm_device_activate_schedule_stage4_ip4_config_timeout (device);
-}
-
 gboolean
 nm_device_get_use_dhcp (NMDevice *self)
 {
@@ -2392,10 +2377,6 @@ nm_device_set_use_dhcp (NMDevice *self,
 			                                           "state-changed",
 			                                           G_CALLBACK (dhcp_state_changed),
 			                                           self);
-			priv->dhcp_timeout_sigid = g_signal_connect (priv->dhcp_manager,
-			                                             "timeout",
-			                                             G_CALLBACK (dhcp_timeout),
-			                                             self);
 		}
 	} else {
 		if (priv->dhcp4_config) {
@@ -2407,8 +2388,6 @@ nm_device_set_use_dhcp (NMDevice *self,
 		if (priv->dhcp_manager) {
 			g_signal_handler_disconnect (priv->dhcp_manager, priv->dhcp_state_sigid);
 			priv->dhcp_state_sigid = 0;
-			g_signal_handler_disconnect (priv->dhcp_manager, priv->dhcp_timeout_sigid);
-			priv->dhcp_timeout_sigid = 0;
 			g_object_unref (priv->dhcp_manager);
 			priv->dhcp_manager = NULL;
 		}