Re: NM post-connect hooks?

[Christian Becke <christianbecke web de>]

Dan Williams schrieb:
> So here's how you'd go about doing this.  The dispatcher is currently
> run async and its result is ignored.  First, you need to fix up the
> dispatcher to not return the dbus action method until all the scripts
> have completed.

AFAICT the dispatcher scripts are already run synchronously, or am I wrong?
In nm-dispatcher-action.c: dispatch-scripts, a list of scripts is
iterated, and each script is run with g_spawn_sync.

>  Next, you need to fix up the dispatcher to terminate
> long-running scripts (say, 5 seconds or more) and log that to syslog so
> we know what the problem is.  Next, the NM dispatcher calling code
> should take a callback which is called when the dispatcher is done, or
> NULL to ignore the result.  We'd ignore the result in all cases except
> for a new action you define called "pre-up".
>
> The, you pull the state change to ACTIVATED out of
> nm_device_activate_stage5_ip_config_commit() adn instead, kick off the
> pre-up dispatcher and give it a callback.  When the callback runs, then
> you transition to state ACTIVATED, or if you determine failure of the
> dispatcher run, FAILED.

Attached patch does more or less what you suggested, plus plugging a
pre-down hook into nm_device_deactivate. However, this does not work in
my tests with a wired and a wireless interface (i.e. the hooks are run,
but the link state is not as expected).
According to ifconfig and ping called from a dispatcher script, for the
wired interface the link is up already before
nm_device_activate_stage5_ip_config_commit is called, and it is down
already on pre-down.
With a wireless interface, the interface does have an IP (according to
ifconfig), but ping fails (probably because the route is not set up
yet). On pre-down, the interface is down already.

I believe the IP is assigned in stage 4, when a dhcp client is run
(dhcpcd in my case). It is not clear to me, yet, what brings the
interface down before the pre-down hook is called. Any hints?

Shall I attach the patch to bug #387832?


Christian

diff --git a/callouts/nm-dispatcher-action.c b/callouts/nm-dispatcher-action.c
index b2fba04..fc07ce3 100644
--- a/callouts/nm-dispatcher-action.c
+++ b/callouts/nm-dispatcher-action.c
@@ -89,6 +89,7 @@ nm_dispatcher_action (Handler *obj,
                       GHashTable *connection,
                       GHashTable *connection_props,
                       GHashTable *device_props,
+		      gboolean *success,
                       GError **error);
 
 #include "nm-dispatcher-glue.h"
@@ -378,7 +379,7 @@ construct_envp (const char *uuid, NMIP4Config *ip4_config, NMDHCP4Config *dhcp4_
 	return envp;
 }
 
-static void
+static gboolean
 dispatch_scripts (const char *action,
                   const char *iface,
                   const char *parent_iface,
@@ -392,12 +393,13 @@ dispatch_scripts (const char *action,
 	GSList *scripts = NULL, *iter;
 	GError *error = NULL;
 	char **envp = NULL;
+	gboolean success = TRUE;
 
 	if (!(dir = g_dir_open (NMD_SCRIPT_DIR, 0, &error))) {
 		g_warning ("g_dir_open() could not open '" NMD_SCRIPT_DIR "'.  '%s'",
 		           error->message);
 		g_error_free (error);
-		return;
+		return success;
 	}
 
 	while ((filename = g_dir_read_name (dir))) {
@@ -446,12 +448,17 @@ dispatch_scripts (const char *action,
 		error = NULL;
 		if (g_spawn_sync ("/", argv, envp, 0, child_setup, NULL, NULL, NULL, &status, &error)) {
 			if (WIFEXITED (status)) {
-				if (WEXITSTATUS (status) != 0)
+				if (WEXITSTATUS (status) != 0) {
+					success = FALSE;
 					g_warning ("Script '%s' exited with error status %d.",
 					           (char *) iter->data, WEXITSTATUS (status));
-			} else
+				}
+			} else {
+				success = FALSE;
 				g_warning ("Script '%s' exited abnormally.", (char *) iter->data);
+			}
 		} else {
+			success = FALSE;
 			g_warning ("Could not run script '%s': (%d) %s",
 			           (char *) iter->data, error->code, error->message);
 			g_error_free (error);
@@ -462,6 +469,9 @@ dispatch_scripts (const char *action,
 
 	g_slist_foreach (scripts, (GFunc) g_free, NULL);
 	g_slist_free (scripts);
+
+	g_debug ("%s: returning %s", __func__, (success)? "true" : "false");
+	return success;
 }
 
 static gboolean
@@ -470,6 +480,7 @@ nm_dispatcher_action (Handler *h,
                       GHashTable *connection_hash,
                       GHashTable *connection_props,
                       GHashTable *device_props,
+		      gboolean *success,
                       GError **error)
 {
 	Dispatcher *d = g_object_get_data (G_OBJECT (h), "dispatcher");
@@ -483,6 +494,9 @@ nm_dispatcher_action (Handler *h,
 	NMIP4Config *ip4_config = NULL;
 	GValue *value;
 
+	g_return_val_if_fail (success != NULL, TRUE);
+	*success = TRUE;
+
 	/* Back off the quit timeout */
 	if (d->quit_timeout)
 		g_source_remove (d->quit_timeout);
@@ -560,12 +574,15 @@ nm_dispatcher_action (Handler *h,
 	}
 
 dispatch:
-	dispatch_scripts (action, iface, parent_iface, type, uuid, ip4_config, dhcp4_config);
+	g_debug ("starting dipatcher scripts");
+	*success = dispatch_scripts (action, iface, parent_iface, type, uuid, ip4_config, dhcp4_config);
+	g_debug ("dispatcher scripts done.");
 
 	if (device)
 		g_object_unref (device);
 
 out:
+	g_debug ("%s: returning %s.", __func__, (success)? "true" : "false");
 	return TRUE;
 }
 
diff --git a/callouts/nm-dispatcher.xml b/callouts/nm-dispatcher.xml
index 532ae9f..45c6b80 100644
--- a/callouts/nm-dispatcher.xml
+++ b/callouts/nm-dispatcher.xml
@@ -32,6 +32,13 @@
         </tp:docstring>
       </arg>
 
+      <arg name="success" type="b" direction="out">
+      	<tp:docstring>
+	  TRUE if all scripts exited succesfully, FALSE if at least one script
+	  could not be run or exited with a non-zero exit code.
+	</tp:docstring>
+      </arg>
+
     </method>
   </interface>
 </node>
diff --git a/include/NetworkManager.h b/include/NetworkManager.h
index c8d5074..cda520c 100644
--- a/include/NetworkManager.h
+++ b/include/NetworkManager.h
@@ -365,6 +365,9 @@ typedef enum {
 	/* The supplicant is now available */
 	NM_DEVICE_STATE_REASON_SUPPLICANT_AVAILABLE,
 
+	/* pre-up dispatcher scripts failed */
+	NM_DEVICE_STATE_REASON_PRE_UP_DISPATCHER_FAILED,
+
 	/* Unused */
 	NM_DEVICE_STATE_REASON_LAST = 0xFFFF
 } NMDeviceStateReason;
diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c
index 4934d46..68b48c9 100644
--- a/src/NetworkManagerUtils.c
+++ b/src/NetworkManagerUtils.c
@@ -38,6 +38,24 @@
 #include <netlink/addr.h>
 #include <netinet/in.h>
 
+/* how long to wait for synchronously called dispatcher scripts (in ms) */
+#define NM_UTILS_DISPATCHER_SCRIPT_TIMEOUT 10000
+
+typedef struct
+{
+	NMUtilsDispatcherDoneCallback callback;
+	gpointer data;
+} NMUtilsCallDispatcherNotifyData;
+
+
+void
+nm_utils_call_dispatcher_sync (const char *action,
+                          NMConnection *connection,
+                          NMDevice *device,
+                          const char *vpn_iface,
+			  NMUtilsDispatcherDoneCallback callback,
+			  gpointer userdata);
+
 /*
  * nm_ethernet_address_is_valid
  *
@@ -368,11 +386,36 @@ nm_utils_merge_ip6_config (NMIP6Config *ip6_config, NMSettingIP6Config *setting)
 		nm_ip6_config_set_never_default (ip6_config, TRUE);
 }
 
-void
-nm_utils_call_dispatcher (const char *action,
+static void
+call_dispatcher_notify (DBusGProxy *proxy, DBusGProxyCall *callid, NMUtilsCallDispatcherNotifyData *data)
+{
+	GError *error = NULL;
+	gboolean success = TRUE;
+
+	if (! dbus_g_proxy_end_call (proxy, callid, &error,
+					G_TYPE_BOOLEAN, &success,
+					G_TYPE_INVALID)) {
+		nm_warning ("sync dispatcher scripts: DBus error: %s", error->message);
+		g_error_free (error);
+	} else {
+		if (success)	
+			nm_debug ("sync dispatcher scripts exited successfully.");
+		else
+			nm_warning ("at least one of the dispatcher scripts failed!");
+	}
+
+	g_assert (data != NULL);
+	data->callback (success, data->data);
+
+	g_object_unref (proxy);
+}
+
+static void
+_nm_utils_call_dispatcher (const char *action,
                           NMConnection *connection,
                           NMDevice *device,
-                          const char *vpn_iface)
+                          const char *vpn_iface,
+			  gpointer callback_data)
 {
 	NMDBusManager *dbus_mgr;
 	DBusGProxy *proxy;
@@ -446,12 +489,25 @@ nm_utils_call_dispatcher (const char *action,
 		value_hash_add_object_path (device_props, NMD_DEVICE_PROPS_PATH, nm_device_get_path (device));
 	}
 
-	dbus_g_proxy_call_no_reply (proxy, "Action",
-	                            G_TYPE_STRING, action,
-	                            DBUS_TYPE_G_MAP_OF_MAP_OF_VARIANT, connection_hash,
-	                            DBUS_TYPE_G_MAP_OF_VARIANT, connection_props,
-	                            DBUS_TYPE_G_MAP_OF_VARIANT, device_props,
-	                            G_TYPE_INVALID);
+	if (callback_data == NULL)
+		dbus_g_proxy_call_no_reply (proxy, "Action",
+					    G_TYPE_STRING, action,
+					    DBUS_TYPE_G_MAP_OF_MAP_OF_VARIANT, connection_hash,
+					    DBUS_TYPE_G_MAP_OF_VARIANT, connection_props,
+					    DBUS_TYPE_G_MAP_OF_VARIANT, device_props,
+					    G_TYPE_INVALID);
+	else {
+		g_object_ref (proxy); /* unref'ed in call_dispatcher_notify */
+		dbus_g_proxy_begin_call_with_timeout (proxy, "Action",
+					(DBusGProxyCallNotify) call_dispatcher_notify,
+					callback_data, g_free,
+					NM_UTILS_DISPATCHER_SCRIPT_TIMEOUT,
+					G_TYPE_STRING, action,
+					DBUS_TYPE_G_MAP_OF_MAP_OF_VARIANT, connection_hash,
+					DBUS_TYPE_G_MAP_OF_VARIANT, connection_props,
+					DBUS_TYPE_G_MAP_OF_VARIANT, device_props,
+					G_TYPE_INVALID);
+	}
 
 	g_object_unref (proxy);
 	g_hash_table_destroy (connection_hash);
@@ -460,6 +516,33 @@ nm_utils_call_dispatcher (const char *action,
 	g_object_unref (dbus_mgr);
 }
 
+void
+nm_utils_call_dispatcher (const char *action,
+                          NMConnection *connection,
+                          NMDevice *device,
+                          const char *vpn_iface)
+{
+	_nm_utils_call_dispatcher (action, connection, device, vpn_iface, NULL);
+}
+
+void
+nm_utils_call_dispatcher_sync (const char *action,
+                          NMConnection *connection,
+                          NMDevice *device,
+                          const char *vpn_iface,
+			  NMUtilsDispatcherDoneCallback callback,
+			  gpointer userdata)
+{
+	NMUtilsCallDispatcherNotifyData *data;
+
+	data = g_new0 (NMUtilsCallDispatcherNotifyData, 1);
+	g_return_if_fail (data != NULL);
+	data->callback = callback;
+	data->data = userdata;
+
+	_nm_utils_call_dispatcher (action, connection, device, vpn_iface, data);
+}
+
 gboolean
 nm_match_spec_hwaddr (const GSList *specs, const char *hwaddr)
 {
@@ -578,4 +661,3 @@ nm_utils_do_sysctl (const char *path, const char *value)
 	close (fd);
 	return TRUE;
 }
-
diff --git a/src/NetworkManagerUtils.h b/src/NetworkManagerUtils.h
index 05f3b83..35a077e 100644
--- a/src/NetworkManagerUtils.h
+++ b/src/NetworkManagerUtils.h
@@ -47,6 +47,15 @@ void nm_utils_call_dispatcher (const char *action,
                                NMDevice *device,
                                const char *vpn_iface);
 
+typedef void (*NMUtilsDispatcherDoneCallback) (gboolean success, gpointer userdata);
+
+void nm_utils_call_dispatcher_sync (const char *action,
+                               NMConnection *connection,
+                               NMDevice *device,
+                               const char *vpn_iface,
+			       NMUtilsDispatcherDoneCallback callback,
+			       gpointer userdata);
+
 gboolean nm_match_spec_hwaddr (const GSList *specs, const char *hwaddr);
 
 
diff --git a/src/nm-device.c b/src/nm-device.c
index 0d3481c..35144dd 100644
--- a/src/nm-device.c
+++ b/src/nm-device.c
@@ -1817,14 +1817,8 @@ start_sharing (NMDevice *self)
 	return TRUE;
 }
 
-/*
- * nm_device_activate_stage5_ip_config_commit
- *
- * Commit the IP config on the device
- *
- */
-static gboolean
-nm_device_activate_stage5_ip_config_commit (gpointer user_data)
+static void
+_nm_device_activate_stage5_ip_config_commit (gpointer user_data)
 {
 	NMDevice *self = NM_DEVICE (user_data);
 	NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self);
@@ -1854,9 +1848,6 @@ nm_device_activate_stage5_ip_config_commit (gpointer user_data)
 	g_object_set_data (G_OBJECT (act_request),
 					   NM_ACT_REQUEST_IP6_CONFIG, NULL);
 
-	/* Clear the activation source ID now that this stage has run */
-	activation_source_clear (self, FALSE, 0);
-
 	iface = nm_device_get_iface (self);
 	nm_info ("Activation (%s) Stage 5 of 5 (IP Configure Commit) started...",
 	         iface);
@@ -1900,10 +1891,57 @@ out:
 		g_object_unref (ip4_config);
 	if (ip6_config)
 		g_object_unref (ip6_config);
+}
 
-	return FALSE;
+static void
+pre_up_dispatcher_done_cb (gboolean success, gpointer userdata)
+{
+	NMDevice *self = NM_DEVICE (userdata);
+
+	nm_info ("(%s) pre-up dispatcher scripts done.", nm_device_get_iface (self));
+
+	if (success)
+		_nm_device_activate_stage5_ip_config_commit (userdata);
+	else
+		nm_device_state_changed (self, NM_DEVICE_STATE_FAILED,
+					NM_DEVICE_STATE_REASON_PRE_UP_DISPATCHER_FAILED);
+		
 }
 
+/*
+ * nm_device_activate_stage5_ip_config_commit
+ *
+ * Commit the IP config on the device
+ *
+ */
+static gboolean
+nm_device_activate_stage5_ip_config_commit (gpointer user_data)
+{
+	NMDevice *self = NM_DEVICE (user_data);
+	NMActRequest *act_req;
+	NMConnection *connection = NULL;
+
+	/* Clear the activation source ID now that this stage has run */
+	activation_source_clear (self, FALSE, 0);
+
+	act_req = nm_device_get_act_request (self);
+	if (act_req)
+		connection = nm_act_request_get_connection (act_req);
+	
+	if (! connection) {
+		nm_debug ("(%s) no connection, skipping pre-up dispatcher skripts",
+			nm_device_get_iface (self));
+		_nm_device_activate_stage5_ip_config_commit (user_data);
+		return FALSE;
+	}
+
+	nm_info ("(%s) running pre-up dispatcher scripts", nm_device_get_iface (self));
+
+	nm_utils_call_dispatcher_sync ("pre-up", connection, self, NULL,
+					(NMUtilsDispatcherDoneCallback) pre_up_dispatcher_done_cb,
+					user_data);
+	return FALSE;
+}
 
 /*
  * nm_device_activate_schedule_stage5_ip_config_commit
@@ -2041,14 +2079,8 @@ nm_device_deactivate_quickly (NMDevice *self)
 	return TRUE;
 }
 
-/*
- * nm_device_deactivate
- *
- * Remove a device's routing table entries and IP address.
- *
- */
 static void
-nm_device_deactivate (NMDeviceInterface *device, NMDeviceStateReason reason)
+_nm_device_deactivate (NMDeviceInterface *device, NMDeviceStateReason reason)
 {
 	NMDevice *self = NM_DEVICE (device);
 	NMDeviceStateReason ignored = NM_DEVICE_STATE_REASON_NONE;
@@ -2075,6 +2107,71 @@ nm_device_deactivate (NMDeviceInterface *device, NMDeviceStateReason reason)
 		NM_DEVICE_GET_CLASS (self)->deactivate (self);
 }
 
+typedef struct {
+	NMDeviceInterface *device;
+	NMDeviceStateReason reason;
+} NMDevicePreDownCbData;
+
+static void
+pre_down_dispatcher_done_cb (gboolean success, NMDevicePreDownCbData *data)
+{
+	NMDevice *self;
+
+	g_return_if_fail (data != NULL);
+
+	self = NM_DEVICE (data->device);
+	
+	if (success)
+		nm_info ("(%s) pre-down dispatcher scripts exited successfully.",
+				nm_device_get_iface (self));
+	else
+		nm_warning ("(%s) pre-down dispatcher scripts failed!",
+				nm_device_get_iface (self));
+	
+	_nm_device_deactivate (data->device, data->reason);
+	
+	g_free (data);
+}
+
+/*
+ * nm_device_deactivate
+ *
+ * Remove a device's routing table entries and IP address.
+ *
+ */
+static void
+nm_device_deactivate (NMDeviceInterface *device, NMDeviceStateReason reason)
+{
+	NMDevice *self = NM_DEVICE (device);
+	NMActRequest *act_req;
+	NMConnection *connection = NULL;
+	NMDevicePreDownCbData *data;
+
+	g_return_if_fail (self != NULL);
+
+	act_req = nm_device_get_act_request (self);
+
+	if (act_req)
+		connection = nm_act_request_get_connection (act_req);
+
+	if (! connection) {
+		nm_debug ("(%s) no connection, skipping pre-down dispatcher skripts.",
+				nm_device_get_iface (self));
+		_nm_device_deactivate (device, reason);
+		return;
+	}
+
+	data = g_new0 (NMDevicePreDownCbData, 1);
+	data->device = device;
+	data->reason = reason;
+
+	nm_info ("(%s) running pre-down dispatcher scripts", nm_device_get_iface (self));
+
+	nm_utils_call_dispatcher_sync ("pre-down", connection, self, NULL,
+					(NMUtilsDispatcherDoneCallback) pre_down_dispatcher_done_cb,
+					data);
+}
+
 static gboolean
 device_disconnect (NMDeviceInterface *device,
                    GError **error)