dhcp: handle client early exit correctly

When the client	exits it may take a short amount of time for the
dhclient hook script to	deliver	the options to NetworkManager; so
we need	to keep	the client object around a bit (so we know what
NMDHCPClient the options getting delivered are for).  If we don't,
the DHCPManager will dispose of	the DHCPClient object and then
when the options come in, it can't match up the	PID from the
options with the PID of	an existing NMDHCPClient.  So put the
clients	on a removal timer that	keeps them around for a	bit before
we let the manager dispose of them.

Since we're keeping the	PID around too instead of zeroing it when
the client exits (for the reason above), track whether the client
is really dead yet so we don't indiscriminately	kill a random
process	that happens to re-use the PID.

src/dhcp-manager/nm-dhcp-client.c

@@ -41,10 +41,13 @@ typedef struct {
 
 	guchar       state;
 	GPid         pid;
+	gboolean     dead;
 	guint        timeout_id;
 	guint        watch_id;
+	guint32      remove_id;
 	GHashTable * options;
 	gboolean     info_only;
+
 } NMDHCPClientPrivate;
 
 #define NM_DHCP_CLIENT_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), NM_TYPE_DHCP_CLIENT, NMDHCPClientPrivate))
@@ -54,6 +57,7 @@ G_DEFINE_TYPE_EXTENDED (NMDHCPClient, nm_dhcp_client, G_TYPE_OBJECT, G_TYPE_FLAG
 enum {
 	STATE_CHANGED,
 	TIMEOUT,
+	REMOVE,
 	LAST_SIGNAL
 };
 
@@ -206,11 +210,46 @@ daemon_timeout (gpointer user_data)
 	return FALSE;
 }
 
+static gboolean
+signal_remove (gpointer user_data)
+{
+	NMDHCPClient *self = NM_DHCP_CLIENT (user_data);
+
+	NM_DHCP_CLIENT_GET_PRIVATE (self)->remove_id = 0;
+	g_signal_emit (G_OBJECT (self), signals[REMOVE], 0);
+	return FALSE;
+}
+
+static void
+dhcp_client_set_state (NMDHCPClient *self,
+                       NMDHCPState state,
+                       gboolean emit_state,
+                       gboolean remove_now)
+{
+	NMDHCPClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE (self);
+
+	priv->state = state;
+
+	if (emit_state)
+		g_signal_emit (G_OBJECT (self), signals[STATE_CHANGED], 0, priv->state);
+
+	if (state == DHC_END || state == DHC_ABEND) {
+		/* Start the remove signal timer */
+		if (remove_now) {
+			g_signal_emit (G_OBJECT (self), signals[REMOVE], 0);
+		} else {
+			if (!priv->remove_id)
+				priv->remove_id = g_timeout_add_seconds (5, signal_remove, self);
+		}
+	}
+}
+
 static void
 daemon_watch_cb (GPid pid, gint status, gpointer user_data)
 {
 	NMDHCPClient *self = NM_DHCP_CLIENT (user_data);
 	NMDHCPClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE (self);
+	NMDHCPState new_state;
 
 	if (priv->ipv6) {
 		nm_log_info (LOGD_DHCP6, "(%s): DHCPv6 client pid %d exited with status %d",
@@ -223,15 +262,16 @@ daemon_watch_cb (GPid pid, gint status, gpointer user_data)
 	}
 
 	if (!WIFEXITED (status)) {
-		priv->state = DHC_ABEND;
+		new_state = DHC_ABEND;
 		nm_log_warn (LOGD_DHCP, "DHCP client died abnormally");
 	} else
-		priv->state = DHC_END;
+		new_state = DHC_END;
 
 	watch_cleanup (self);
 	timeout_cleanup (self);
+	priv->dead = TRUE;
 
-	g_signal_emit (G_OBJECT (self), signals[STATE_CHANGED], 0, priv->state);
+	dhcp_client_set_state (self, new_state, TRUE, FALSE);
 }
 
 static void
@@ -349,8 +389,9 @@ nm_dhcp_client_stop (NMDHCPClient *self)
 	priv = NM_DHCP_CLIENT_GET_PRIVATE (self);
 
 	/* Kill the DHCP client */
-	if (priv->pid > 0) {
+	if (!priv->dead) {
 		NM_DHCP_CLIENT_GET_CLASS (self)->stop (self);
+		priv->dead = TRUE;
 
 		nm_log_info (LOGD_DHCP, "(%s): canceled DHCP transaction, DHCP client pid %d",
 		             priv->iface, priv->pid);
@@ -359,7 +400,7 @@ nm_dhcp_client_stop (NMDHCPClient *self)
 	/* And clean stuff up */
 
 	priv->pid = -1;
-	priv->state = DHC_END;
+	dhcp_client_set_state (self, DHC_END, FALSE, TRUE);
 
 	g_hash_table_remove_all (priv->options);
 
@@ -530,23 +571,19 @@ nm_dhcp_client_new_options (NMDHCPClient *self,
 		timeout_cleanup (self);
 	}
 
-	priv->state = new_state;
 	if (priv->ipv6) {
 		nm_log_info (LOGD_DHCP6, "(%s): DHCPv6 state changed %s -> %s",
 		            priv->iface,
 		            state_to_string (old_state),
-		            state_to_string (priv->state));
+		            state_to_string (new_state));
 	} else {
 		nm_log_info (LOGD_DHCP4, "(%s): DHCPv4 state changed %s -> %s",
 		            priv->iface,
 		            state_to_string (old_state),
-		            state_to_string (priv->state));
+		            state_to_string (new_state));
 	}
 
-	g_signal_emit (G_OBJECT (self),
-	               signals[STATE_CHANGED],
-	               0,
-	               priv->state);
+	dhcp_client_set_state (self, new_state, TRUE, FALSE);
 }
 
 #define NEW_TAG "new_"
@@ -1089,6 +1126,9 @@ dispose (GObject *object)
 	 * the DHCP client.
 	 */
 
+	if (priv->remove_id)
+		g_source_remove (priv->remove_id);
+
 	g_hash_table_destroy (priv->options);
 	g_free (priv->iface);
 
@@ -1159,5 +1199,14 @@ nm_dhcp_client_class_init (NMDHCPClientClass *client_class)
 					  NULL, NULL,
 					  g_cclosure_marshal_VOID__VOID,
 					  G_TYPE_NONE, 0);
+
+	signals[REMOVE] =
+		g_signal_new ("remove",
+					  G_OBJECT_CLASS_TYPE (object_class),
+					  G_SIGNAL_RUN_FIRST,
+					  G_STRUCT_OFFSET (NMDHCPClientClass, remove),
+					  NULL, NULL,
+					  g_cclosure_marshal_VOID__VOID,
+					  G_TYPE_NONE, 0);
 }
 

src/dhcp-manager/nm-dhcp-client.h

@@ -99,6 +99,7 @@ typedef struct {
 	/* Signals */
 	void (*state_changed) (NMDHCPClient *self, NMDHCPState state);
 	void (*timeout)       (NMDHCPClient *self);
+	void (*remove)        (NMDHCPClient *self);
 } NMDHCPClientClass;
 
 GType nm_dhcp_client_get_type (void);

src/dhcp-manager/nm-dhcp-manager.c

@@ -358,7 +358,7 @@ nm_dhcp_manager_new (const char *client, GError **error)
 	return singleton;
 }
 
-#define STATE_ID_TAG "state-id"
+#define REMOVE_ID_TAG "remove-id"
 #define TIMEOUT_ID_TAG "timeout-id"
 
 static void
@@ -367,7 +367,7 @@ remove_client (NMDHCPManager *self, NMDHCPClient *client)
 	NMDHCPManagerPrivate *priv = NM_DHCP_MANAGER_GET_PRIVATE (self);
 	guint id;
 
-	id = GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (client), STATE_ID_TAG));
+	id = GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (client), REMOVE_ID_TAG));
 	if (id)
 		g_signal_handler_disconnect (client, id);
 
@@ -383,29 +383,16 @@ remove_client (NMDHCPManager *self, NMDHCPClient *client)
 	g_hash_table_remove (priv->clients, client);
 }
 
-static void
-client_state_changed (NMDHCPClient *client, NMDHCPState new_state, gpointer user_data)
-{
-	if (new_state == DHC_ABEND || new_state == DHC_END)
-		remove_client (NM_DHCP_MANAGER (user_data), client);
-}
-
-static void
-client_timeout (NMDHCPClient *client, gpointer user_data)
-{
-	remove_client (NM_DHCP_MANAGER (user_data), client);
-}
-
 static void
 add_client (NMDHCPManager *self, NMDHCPClient *client)
 {
 	NMDHCPManagerPrivate *priv = NM_DHCP_MANAGER_GET_PRIVATE (self);
 	guint id;
 
-	id = g_signal_connect (client, "state-changed", G_CALLBACK (client_state_changed), self);
-	g_object_set_data (G_OBJECT (client), STATE_ID_TAG, GUINT_TO_POINTER (id));
+	id = g_signal_connect_swapped (client, "remove", G_CALLBACK (remove_client), self);
+	g_object_set_data (G_OBJECT (client), REMOVE_ID_TAG, GUINT_TO_POINTER (id));
 
-	id = g_signal_connect (client, "timeout", G_CALLBACK (client_timeout), self);
+	id = g_signal_connect_swapped (client, "timeout", G_CALLBACK (remove_client), self);
 	g_object_set_data (G_OBJECT (client), TIMEOUT_ID_TAG, GUINT_TO_POINTER (id));
 
 	g_hash_table_insert (priv->clients, client, g_object_ref (client));