Re: Getting NM to re-try DHCP

[Jirka Klimes <jklimes redhat com>]

On Thursday 26 of May 2011 20:22:26 Dan Williams wrote:
> > 
> > A patch doing that is in the attachment.
> 
> A few concerns with the patch; it might get more complicated because of
> them.  First, if the connection is deleted before the idle handler runs,
> we'll be left with garbage.  So we'd need to do g_object_weak_ref() on
> the connection object, cache the idle ID, and run g_source_remove() on
> that idle ID from the weak ref callback.  Next, in
> reset_connection_retries() we'd want to remove that weak ref when
> freeing the ResetRetriesData structure.
> 
> Second, if the connection gets activated again manually by the user
> while we're waiting the 5 minutes then we should g_source_remove() the
> idle ID.  That gets more complicated.
> 
> Perhaps a simpler approach would be to have a single, global idle
> handler in the NMPolicy that runs over the connection list and resets
> the retries as appropriate?  First, in device_state_changed() we could
> attach the current seconds-since-epoch time to the object using
> g_object_set_data() when its # retries reaches 0, and then if no reset
> idle handler was scheduled, schedule one for 300 seconds later.
> (otherwise allow the existing idle handler to run earlier since
> presumably it was scheduled by an earlier failed connection we want to
> reset).
> 
> Then, when the reset idle handler does run, iterate over each
> connection, save the earliest reset timestamp, and reschedule the idle
> handler for that timestamp + 300 seconds.  During this iteration of
> course we reset the retries count for every connection that has a reset
> timestamp earlier than now.
> 
> If the connection gets activated, nm-policy.c's signal handlers can
> listen for that and clear out the invalid timestamp data too.
> 
> If that would all work, that would allow us to avoid doing all the
> alloc/dealloc of a custom data structure, plus we only have to manage
> one idle handler.  Plus we don't have to care too  much about stuff like
> connection deletion and activation happening before our idle handler
> runs since those will be easier to deal with.
> 
> Thoughts?
> 
> Dan

Please find the reworked patch is the attachment.

Jirka

From 4fe7ec86e19f75d5a8c2e13edc69fe38b33068c5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ji=C5=99=C3=AD=20Klime=C5=A1?= <jklimes redhat com>
Date: Fri, 17 Jun 2011 12:43:28 +0200
Subject: [PATCH] policy: remove "invalid mark" for failed connections after 5
 mins
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If there is a temporary connection failure (e.g. due to unavailable DHCP), the
connection is marked as invalid after several retries. Reset the flag after
5 mins to allow next auto-reconnection.

Signed-off-by: Jiří Klimeš <jklimes redhat com>
---
 src/nm-policy.c |   44 ++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/src/nm-policy.c b/src/nm-policy.c
index 3eead41..ac180f2 100644
--- a/src/nm-policy.c
+++ b/src/nm-policy.c
@@ -66,12 +66,16 @@ struct NMPolicy {
 
 	HostnameThread *lookup;
 
+	gint reset_retries_id;  /* idle handler for resetting the retries count */
+
 	char *orig_hostname; /* hostname at NM start time */
 	char *cur_hostname;  /* hostname we want to assign */
 };
 
 #define RETRIES_TAG "autoconnect-retries"
 #define RETRIES_DEFAULT	4
+#define RESET_RETRIES_TIMESTAMP_TAG "reset-retries-timestamp-tag"
+#define RESET_RETRIES_TIMER 300
 
 static NMDevice *
 get_best_ip4_device (NMManager *manager, NMActRequest **out_req)
@@ -870,6 +874,37 @@ schedule_activate_check (NMPolicy *policy, NMDevice *device, guint delay_seconds
 	}
 }
 
+static gboolean
+reset_connections_retries (gpointer user_data)
+{
+	NMPolicy *policy = (NMPolicy *) user_data;
+	GSList *connections, *iter;
+	time_t con_stamp, min_stamp, now;
+
+	policy->reset_retries_id = 0;
+
+	min_stamp = now = time (NULL);
+	connections = nm_settings_get_connections (policy->settings);
+	for (iter = connections; iter; iter = g_slist_next (iter)) {
+		con_stamp = GPOINTER_TO_SIZE (g_object_get_data (G_OBJECT (iter->data), RESET_RETRIES_TIMESTAMP_TAG));
+		if (con_stamp == 0)
+			continue;
+		if (con_stamp + RESET_RETRIES_TIMER <= now) {
+			set_connection_auto_retries (NM_CONNECTION (iter->data), RETRIES_DEFAULT);
+			g_object_set_data (G_OBJECT (iter->data), RESET_RETRIES_TIMESTAMP_TAG, GSIZE_TO_POINTER (0));
+			continue;
+		}
+		if (con_stamp < min_stamp)
+			min_stamp = con_stamp;
+	}
+	g_slist_free (connections);
+
+	/* Schedule the handler again if there are some stamps left */
+	if (min_stamp != now)
+		policy->reset_retries_id = g_timeout_add_seconds (RESET_RETRIES_TIMER - (now - min_stamp), reset_connections_retries, policy);
+	return FALSE;
+}
+
 static NMConnection *
 get_device_connection (NMDevice *device)
 {
@@ -914,8 +949,13 @@ device_state_changed (NMDevice *device,
 				set_connection_auto_retries (connection, tries - 1);
 			}
 
-			if (get_connection_auto_retries (connection) == 0)
+			if (get_connection_auto_retries (connection) == 0) {
 				nm_log_info (LOGD_DEVICE, "Marking connection '%s' invalid.", nm_connection_get_id (connection));
+				/* Schedule a handler to reset retries count */
+				g_object_set_data (G_OBJECT (connection), RESET_RETRIES_TIMESTAMP_TAG, GSIZE_TO_POINTER ((gsize) time (NULL)));
+				if (!policy->reset_retries_id)
+					policy->reset_retries_id = g_timeout_add_seconds (RESET_RETRIES_TIMER, reset_connections_retries, policy);
+			}
 			nm_connection_clear_secrets (connection);
 		}
 		schedule_activate_check (policy, device, 3);
@@ -938,7 +978,7 @@ device_state_changed (NMDevice *device,
 		update_routing_and_dns (policy, FALSE);
 		break;
 	case NM_DEVICE_STATE_DISCONNECTED:
-		/* Clear INVALID_TAG when carrier on. If cable was unplugged
+		/* Reset RETRIES_TAG when carrier on. If cable was unplugged
 		 * and plugged again, we should try to reconnect */
 		if (reason == NM_DEVICE_STATE_REASON_CARRIER && old_state == NM_DEVICE_STATE_UNAVAILABLE)
 			reset_retries_all (policy->settings, device);
-- 
1.7.5.2