From 332420ffecfb396b7855f72dcbc067306a862339 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Jun 2014 20:35:08 +0200 Subject: [PATCH 01/10] core: fix activation of slave when master is not active, but device exists NM fails to activate a slave if the master device already exists but has not active connection. One way to reproduce, create a bond master/slave configuration and ensure that the master device exists (e.g. by activating the bond, and killing NM without taking down the device, or externally via `ip link add`). If you try to activate the slave it will fail with the following message (in nmcli): "Error: Connection activation failed: The active connection on MASTER is not a valid master for 'SLAVE'" although MASTER is not active. This also triggers the following assertion: #0 0x0000003370c504e9 in g_logv () from /lib64/libglib-2.0.so.0 #1 0x0000003370c5063f in g_log () from /lib64/libglib-2.0.so.0 #2 0x000000000047646a in is_compatible_with_slave (master=0x0, slave=slave@entry=0xc4aa60) at nm-manager.c:2193 #3 0x000000000047e289 in ensure_master_active_connection (self=self@entry=0xc8d150, subject=0x7f23b80059e0, connection=connection@entry=0xc4aa60, device=device@entry=0xcac380, master_connection=master_connection@entry=0x0, master_device=master_device@entry=0xc9e800, error=error@entry=0x7fffa5cc4958) at nm-manager.c:2395 #4 0x000000000047eb4a in _internal_activate_device (self=self@entry=0xc8d150, active=active@entry=0xcc33b0, error=error@entry=0x7fffa5cc4958) at nm-manager.c:2665 #5 0x000000000047ecf2 in _internal_activate_generic (self=self@entry=0xc8d150, active=active@entry=0xcc33b0, error=error@entry=0x7fffa5cc4958) at nm-manager.c:2712 #6 0x000000000047ef2b in _internal_activation_auth_done (active=0xcc33b0, success=, error_desc=0x0, user_data1=0xc8d150, user_data2=) at nm-manager.c:2848 #7 0x0000000000466fa1 in auth_done (chain=0xcef020, error=0x0, unused=, user_data=) at nm-active-connection.c:603 #8 0x00000000004753da in auth_chain_finish (user_data=0xcef020) at nm-manager-auth.c:88 #9 0x0000003370c492a6 in g_main_context_dispatch () from /lib64/libglib-2.0.so.0 #10 0x0000003370c49628 in g_main_context_iterate.isra () from /lib64/libglib-2.0.so.0 #11 0x0000003370c49a3a in g_main_loop_run () from /lib64/libglib-2.0.so.0 #12 0x0000000000429e65 in main (argc=1, argv=0x7fffa5cc4e48) at main.c:678 Signed-off-by: Thomas Haller --- src/nm-manager.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 1b7c87e25d..00799ffd67 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -2390,9 +2390,8 @@ ensure_master_active_connection (NMManager *self, /* If we're passed a connection and a device, we require that connection * be already activated on the device, eg returned from find_master(). */ - if (master_connection) - g_assert (device_connection == master_connection); - else if (!is_compatible_with_slave (device_connection, connection)) { + g_assert (!master_connection || master_connection == device_connection); + if (device_connection && !is_compatible_with_slave (device_connection, connection)) { g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_DEPENDENCY_FAILED, "The active connection on %s is not a valid master for '%s'", nm_device_get_iface (master_device), @@ -2404,6 +2403,7 @@ ensure_master_active_connection (NMManager *self, if ( (master_state == NM_DEVICE_STATE_ACTIVATED) || nm_device_is_activating (master_device)) { /* Device already using master_connection */ + g_assert (device_connection); return NM_ACTIVE_CONNECTION (nm_device_get_act_request (master_device)); } From 524fc6d454e063cd84b004d23e2a0ccc163b8588 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Jun 2014 13:46:25 +0200 Subject: [PATCH 02/10] core: preserve reason on device deactivation while pre-down (fix tearing down slave when deactivating master) When delaying the deactivation of a device during dispatcher-pre-down, we must preseve the reason to pass it on. This is especially important, because nm_device_slave_notify_release() checks for the reason, and does not deactivate the slave if no reason is given. This error caused slaves the be left up when deactivating the master. Also update the call to nm_device_slave_notify_release() to ensure we have a valid state reason when configuring the slave. This would have pointed out the issue and would even work around it. Regression introduced by commit d00e2147de148bcec1d32023bda8a90dab552a18. Signed-off-by: Thomas Haller --- src/devices/nm-device.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index b1bf887ebf..7fca6efb4e 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -217,6 +217,7 @@ typedef struct { gpointer act_source6_func; guint recheck_assume_id; guint dispatcher_id; + NMDeviceStateReason dispatcher_pre_down_reason; /* Link stuff */ guint link_connected_id; @@ -831,8 +832,12 @@ nm_device_release_one_slave (NMDevice *dev, NMDevice *slave, gboolean configure) reason = NM_DEVICE_STATE_REASON_NONE; else if (priv->state == NM_DEVICE_STATE_FAILED) reason = NM_DEVICE_STATE_REASON_DEPENDENCY_FAILED; - else + else if (priv->state_reason != NM_DEVICE_STATE_REASON_NONE) reason = priv->state_reason; + else { + g_warn_if_reached (); + reason = NM_DEVICE_STATE_REASON_UNKNOWN; + } nm_device_slave_notify_release (info->slave, reason); free_slave_info (info); @@ -6530,7 +6535,7 @@ dispatcher_pre_down_done (guint call_id, gpointer user_data) g_return_if_fail (call_id == priv->dispatcher_id); priv->dispatcher_id = 0; - nm_device_queue_state (self, NM_DEVICE_STATE_DISCONNECTED, NM_DEVICE_STATE_REASON_NONE); + nm_device_queue_state (self, NM_DEVICE_STATE_DISCONNECTED, priv->dispatcher_pre_down_reason); } static void @@ -6709,6 +6714,7 @@ _set_state_full (NMDevice *device, nm_act_request_get_connection (req), device); } else { + priv->dispatcher_pre_down_reason = reason; if (!nm_dispatcher_call (DISPATCHER_ACTION_PRE_DOWN, nm_act_request_get_connection (req), device, From 851be22146828fdb6aa18c83ff0d08fa64310522 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Jun 2014 14:04:42 +0200 Subject: [PATCH 03/10] core/trivial: move code Signed-off-by: Thomas Haller --- src/devices/nm-device.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 7fca6efb4e..6c04187d76 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -5268,6 +5268,17 @@ nm_device_get_ip6_config (NMDevice *self) /****************************************************************/ +static void +dispatcher_cleanup (NMDevice *self) +{ + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + + if (priv->dispatcher_id) { + nm_dispatcher_call_cancel (priv->dispatcher_id); + priv->dispatcher_id = 0; + } +} + static void ip_check_pre_up_done (guint call_id, gpointer user_data) { @@ -6538,17 +6549,6 @@ dispatcher_pre_down_done (guint call_id, gpointer user_data) nm_device_queue_state (self, NM_DEVICE_STATE_DISCONNECTED, priv->dispatcher_pre_down_reason); } -static void -dispatcher_cleanup (NMDevice *self) -{ - NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - - if (priv->dispatcher_id) { - nm_dispatcher_call_cancel (priv->dispatcher_id); - priv->dispatcher_id = 0; - } -} - static gboolean ip_config_valid (NMDeviceState state) { From afecbf1f661a775f6164c469f392f549483ffe52 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Jun 2014 14:20:14 +0200 Subject: [PATCH 04/10] device: refactor by combining dispatcher callback functions Signed-off-by: Thomas Haller --- src/devices/nm-device.c | 62 ++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 6c04187d76..4d70f24595 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -216,8 +216,11 @@ typedef struct { guint act_source6_id; gpointer act_source6_func; guint recheck_assume_id; - guint dispatcher_id; - NMDeviceStateReason dispatcher_pre_down_reason; + struct { + guint call_id; + NMDeviceState post_state; + NMDeviceStateReason post_state_reason; + } dispatcher; /* Link stuff */ guint link_connected_id; @@ -5273,39 +5276,51 @@ dispatcher_cleanup (NMDevice *self) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - if (priv->dispatcher_id) { - nm_dispatcher_call_cancel (priv->dispatcher_id); - priv->dispatcher_id = 0; + if (priv->dispatcher.call_id) { + nm_dispatcher_call_cancel (priv->dispatcher.call_id); + priv->dispatcher.call_id = 0; + priv->dispatcher.post_state = NM_DEVICE_STATE_UNKNOWN; + priv->dispatcher.post_state_reason = NM_DEVICE_STATE_REASON_NONE; } } static void -ip_check_pre_up_done (guint call_id, gpointer user_data) +dispatcher_complete_proceed_state (guint call_id, gpointer user_data) { NMDevice *self = NM_DEVICE (user_data); NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - g_return_if_fail (call_id == priv->dispatcher_id); + g_return_if_fail (call_id == priv->dispatcher.call_id); - priv->dispatcher_id = 0; - nm_device_queue_state (self, NM_DEVICE_STATE_SECONDARIES, NM_DEVICE_STATE_REASON_NONE); + priv->dispatcher.call_id = 0; + nm_device_queue_state (self, priv->dispatcher.post_state, + priv->dispatcher.post_state_reason); + priv->dispatcher.post_state = NM_DEVICE_STATE_UNKNOWN; + priv->dispatcher.post_state_reason = NM_DEVICE_STATE_REASON_NONE; } +/****************************************************************/ + static void ip_check_pre_up (NMDevice *self) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - g_warn_if_fail (priv->dispatcher_id == 0); + if (priv->dispatcher.call_id != 0) { + g_warn_if_reached (); + dispatcher_cleanup (self); + } + priv->dispatcher.post_state = NM_DEVICE_STATE_SECONDARIES; + priv->dispatcher.post_state_reason = NM_DEVICE_STATE_REASON_NONE; if (!nm_dispatcher_call (DISPATCHER_ACTION_PRE_UP, nm_device_get_connection (self), self, - ip_check_pre_up_done, + dispatcher_complete_proceed_state, self, - &priv->dispatcher_id)) { + &priv->dispatcher.call_id)) { /* Just proceed on errors */ - ip_check_pre_up_done (0, self); + dispatcher_complete_proceed_state (0, self); } } @@ -6537,18 +6552,6 @@ nm_device_cleanup (NMDevice *self, NMDeviceStateReason reason) /***********************************************************/ -static void -dispatcher_pre_down_done (guint call_id, gpointer user_data) -{ - NMDevice *self = NM_DEVICE (user_data); - NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - - g_return_if_fail (call_id == priv->dispatcher_id); - - priv->dispatcher_id = 0; - nm_device_queue_state (self, NM_DEVICE_STATE_DISCONNECTED, priv->dispatcher_pre_down_reason); -} - static gboolean ip_config_valid (NMDeviceState state) { @@ -6714,15 +6717,16 @@ _set_state_full (NMDevice *device, nm_act_request_get_connection (req), device); } else { - priv->dispatcher_pre_down_reason = reason; + priv->dispatcher.post_state = NM_DEVICE_STATE_DISCONNECTED; + priv->dispatcher.post_state_reason = reason; if (!nm_dispatcher_call (DISPATCHER_ACTION_PRE_DOWN, nm_act_request_get_connection (req), device, - dispatcher_pre_down_done, + dispatcher_complete_proceed_state, device, - &priv->dispatcher_id)) { + &priv->dispatcher.call_id)) { /* Just proceed on errors */ - dispatcher_pre_down_done (0, device); + dispatcher_complete_proceed_state (0, device); } } break; From c8e79533337b817bbb049914de272dcc7034669d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 15 Jun 2014 10:05:28 +0200 Subject: [PATCH 05/10] dispatcher/trivial: rename variables for script directory to NMD_SCRIPT_DIR_* --- callouts/nm-dispatcher-api.h | 6 +++--- callouts/nm-dispatcher.c | 6 +++--- src/nm-dispatcher.c | 14 +++++++------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/callouts/nm-dispatcher-api.h b/callouts/nm-dispatcher-api.h index eb6b33fe0c..03e40f7752 100644 --- a/callouts/nm-dispatcher-api.h +++ b/callouts/nm-dispatcher-api.h @@ -20,9 +20,9 @@ #include -#define NMD_SCRIPT_DIR NMCONFDIR "/dispatcher.d" -#define NMD_PRE_UP_DIR NMD_SCRIPT_DIR "/pre-up.d" -#define NMD_PRE_DOWN_DIR NMD_SCRIPT_DIR "/pre-down.d" +#define NMD_SCRIPT_DIR_DEFAULT NMCONFDIR "/dispatcher.d" +#define NMD_SCRIPT_DIR_PRE_UP NMD_SCRIPT_DIR_DEFAULT "/pre-up.d" +#define NMD_SCRIPT_DIR_PRE_DOWN NMD_SCRIPT_DIR_DEFAULT "/pre-down.d" /* dbus-glib types for dispatcher call return value */ #define DISPATCHER_TYPE_RESULT (dbus_g_type_get_struct ("GValueArray", G_TYPE_STRING, G_TYPE_UINT, G_TYPE_STRING, G_TYPE_INVALID)) diff --git a/callouts/nm-dispatcher.c b/callouts/nm-dispatcher.c index 93998225d1..680715d1cd 100644 --- a/callouts/nm-dispatcher.c +++ b/callouts/nm-dispatcher.c @@ -426,12 +426,12 @@ find_scripts (const char *str_action) if ( strcmp (str_action, NMD_ACTION_PRE_UP) == 0 || strcmp (str_action, NMD_ACTION_VPN_PRE_UP) == 0) - dirname = NMD_PRE_UP_DIR; + dirname = NMD_SCRIPT_DIR_PRE_UP; else if ( strcmp (str_action, NMD_ACTION_PRE_DOWN) == 0 || strcmp (str_action, NMD_ACTION_VPN_PRE_DOWN) == 0) - dirname = NMD_PRE_DOWN_DIR; + dirname = NMD_SCRIPT_DIR_PRE_DOWN; else - dirname = NMD_SCRIPT_DIR; + dirname = NMD_SCRIPT_DIR_DEFAULT; if (!(dir = g_dir_open (dirname, 0, &error))) { g_message ("Failed to open dispatcher directory '%s': (%d) %s", diff --git a/src/nm-dispatcher.c b/src/nm-dispatcher.c index 336b91f7ed..2ddbf49000 100644 --- a/src/nm-dispatcher.c +++ b/src/nm-dispatcher.c @@ -221,7 +221,7 @@ dispatcher_results_process (guint request_id, GPtrArray *results) if (!validate_element (request_id, tmp, G_TYPE_STRING, i, 0)) continue; script = g_value_get_string (tmp); - if (!script || strncmp (script, NMD_SCRIPT_DIR "/", STRLEN (NMD_SCRIPT_DIR "/"))) + if (!script || strncmp (script, NMD_SCRIPT_DIR_DEFAULT "/", STRLEN (NMD_SCRIPT_DIR_DEFAULT "/"))) continue; /* Result */ @@ -240,11 +240,11 @@ dispatcher_results_process (guint request_id, GPtrArray *results) if (result == DISPATCH_RESULT_SUCCESS) { nm_log_dbg (LOGD_DISPATCH, "(%u) %s succeeded", request_id, - script + STRLEN (NMD_SCRIPT_DIR "/")); + script + STRLEN (NMD_SCRIPT_DIR_DEFAULT "/")); } else { nm_log_warn (LOGD_DISPATCH, "(%u) %s failed (%s): %s", request_id, - script + STRLEN (NMD_SCRIPT_DIR "/"), + script + STRLEN (NMD_SCRIPT_DIR_DEFAULT "/"), dispatch_result_to_string (result), err ? err : ""); } @@ -379,7 +379,7 @@ _dispatcher_call (DispatcherAction action, info->user_data = user_data; info->idle_id = g_idle_add (dispatcher_idle_cb, info); } - nm_log_dbg (LOGD_DISPATCH, "(%u) ignoring request; no scripts in " NMD_SCRIPT_DIR, reqid); + nm_log_dbg (LOGD_DISPATCH, "(%u) ignoring request; no scripts in " NMD_SCRIPT_DIR_DEFAULT, reqid); success = TRUE; goto done; } @@ -633,9 +633,9 @@ typedef struct { } Monitor; static Monitor monitors[3] = { - { NMD_SCRIPT_DIR, NULL, TRUE }, - { NMD_PRE_UP_DIR, NULL, TRUE }, - { NMD_PRE_DOWN_DIR, NULL, TRUE } + { NMD_SCRIPT_DIR_DEFAULT, NULL, TRUE }, + { NMD_SCRIPT_DIR_PRE_UP, NULL, TRUE }, + { NMD_SCRIPT_DIR_PRE_DOWN, NULL, TRUE } }; static void From b317e79b5ba36de27a3c525e96455206f3b27da9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Jun 2014 12:52:56 +0200 Subject: [PATCH 06/10] dispatcher: improve debug logging for dispatcher callouts - ensure, that dispatcher_results_process() logs a line even if no scripts were run. This way we alyways know when the callout returns. - log a line when cancelling a dispatcher call Signed-off-by: Thomas Haller --- src/nm-dispatcher.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/src/nm-dispatcher.c b/src/nm-dispatcher.c index 2ddbf49000..73b2041dc2 100644 --- a/src/nm-dispatcher.c +++ b/src/nm-dispatcher.c @@ -203,6 +203,12 @@ dispatcher_results_process (guint request_id, GPtrArray *results) g_return_if_fail (results != NULL); + if (results->len == 0) { + nm_log_dbg (LOGD_DISPATCH, "(%u) succeeded but no scripts invoked", + request_id); + return; + } + for (i = 0; i < results->len; i++) { GValueArray *item = g_ptr_array_index (results, i); GValue *tmp; @@ -221,8 +227,10 @@ dispatcher_results_process (guint request_id, GPtrArray *results) if (!validate_element (request_id, tmp, G_TYPE_STRING, i, 0)) continue; script = g_value_get_string (tmp); - if (!script || strncmp (script, NMD_SCRIPT_DIR_DEFAULT "/", STRLEN (NMD_SCRIPT_DIR_DEFAULT "/"))) - continue; + if (!script) + script = "(unknown)"; + else if (!strncmp (script, NMD_SCRIPT_DIR_DEFAULT "/", STRLEN (NMD_SCRIPT_DIR_DEFAULT "/"))) + script += STRLEN (NMD_SCRIPT_DIR_DEFAULT "/"), /* Result */ tmp = g_value_array_get_nth (item, 1); @@ -240,11 +248,11 @@ dispatcher_results_process (guint request_id, GPtrArray *results) if (result == DISPATCH_RESULT_SUCCESS) { nm_log_dbg (LOGD_DISPATCH, "(%u) %s succeeded", request_id, - script + STRLEN (NMD_SCRIPT_DIR_DEFAULT "/")); + script); } else { nm_log_warn (LOGD_DISPATCH, "(%u) %s failed (%s): %s", request_id, - script + STRLEN (NMD_SCRIPT_DIR_DEFAULT "/"), + script, dispatch_result_to_string (result), err ? err : ""); } @@ -356,15 +364,21 @@ _dispatcher_call (DispatcherAction action, /* All actions except 'hostname' require a device */ if (action == DISPATCHER_ACTION_HOSTNAME) { - nm_log_dbg (LOGD_DISPATCH, "(%u) dispatching action '%s'", - reqid, action_to_string (action)); + nm_log_dbg (LOGD_DISPATCH, "(%u) dispatching action '%s'%s", + reqid, action_to_string (action), + blocking + ? " (blocking)" + : (callback ? " (with callback)" : "")); } else { g_return_val_if_fail (NM_IS_DEVICE (device), FALSE); - nm_log_dbg (LOGD_DISPATCH, "(%u) (%s) dispatching action '%s'", + nm_log_dbg (LOGD_DISPATCH, "(%u) (%s) dispatching action '%s'%s", reqid, vpn_iface ? vpn_iface : nm_device_get_iface (device), - action_to_string (action)); + action_to_string (action), + blocking + ? " (blocking)" + : (callback ? " (with callback)" : "")); } /* VPN actions require at least an IPv4 config (for now) */ @@ -620,10 +634,13 @@ nm_dispatcher_call_cancel (guint call_id) * DispatcherInfo's callback to NULL. */ info = g_hash_table_lookup (requests, GUINT_TO_POINTER (call_id)); - if (info) + g_return_if_fail (info); + + if (info && info->callback) { + nm_log_dbg (LOGD_DISPATCH, "(%u) cancelling dispatcher callback action", + call_id); info->callback = NULL; - else - g_return_if_reached (); + } } typedef struct { From 10780592c82beae87d82120d346326a255279412 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Jun 2014 18:08:59 +0200 Subject: [PATCH 07/10] dispatcher/trivial: move code Signed-off-by: Thomas Haller --- src/nm-dispatcher.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/nm-dispatcher.c b/src/nm-dispatcher.c index 73b2041dc2..2ab867c777 100644 --- a/src/nm-dispatcher.c +++ b/src/nm-dispatcher.c @@ -37,6 +37,18 @@ static gboolean do_dispatch = TRUE; static GHashTable *requests = NULL; +typedef struct { + const char *dir; + GFileMonitor *monitor; + gboolean has_scripts; +} Monitor; + +static Monitor monitors[3] = { + { NMD_SCRIPT_DIR_DEFAULT, NULL, TRUE }, + { NMD_SCRIPT_DIR_PRE_UP, NULL, TRUE }, + { NMD_SCRIPT_DIR_PRE_DOWN, NULL, TRUE } +}; + static void dump_object_to_props (GObject *object, GHashTable *hash) { @@ -643,18 +655,6 @@ nm_dispatcher_call_cancel (guint call_id) } } -typedef struct { - const char *dir; - GFileMonitor *monitor; - gboolean has_scripts; -} Monitor; - -static Monitor monitors[3] = { - { NMD_SCRIPT_DIR_DEFAULT, NULL, TRUE }, - { NMD_SCRIPT_DIR_PRE_UP, NULL, TRUE }, - { NMD_SCRIPT_DIR_PRE_DOWN, NULL, TRUE } -}; - static void dispatcher_dir_changed (GFileMonitor *monitor, GFile *file, From 0c54ab217edcc961cdaa90deb12bfb79ebd70dde Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Jun 2014 18:23:20 +0200 Subject: [PATCH 08/10] dispatcher: skip callouts for empty script directory based on dispatcher action type Before, there was only one combined variable checking whether any dispatcher scripts are present at all. This meant for example, that a call to PRE_UP was still sent out even if no scripts were in pre-up.d/ directory. Optimize this, by distinguishing between the dispatcher type and the script directories. Signed-off-by: Thomas Haller --- src/nm-dispatcher.c | 51 +++++++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/src/nm-dispatcher.c b/src/nm-dispatcher.c index 2ab867c777..0e3b6b1855 100644 --- a/src/nm-dispatcher.c +++ b/src/nm-dispatcher.c @@ -34,21 +34,41 @@ #define CALL_TIMEOUT (1000 * 60 * 10) /* 10 mintues for all scripts */ -static gboolean do_dispatch = TRUE; static GHashTable *requests = NULL; typedef struct { - const char *dir; + const char *const dir; GFileMonitor *monitor; gboolean has_scripts; } Monitor; -static Monitor monitors[3] = { - { NMD_SCRIPT_DIR_DEFAULT, NULL, TRUE }, - { NMD_SCRIPT_DIR_PRE_UP, NULL, TRUE }, - { NMD_SCRIPT_DIR_PRE_DOWN, NULL, TRUE } +enum { + MONITOR_INDEX_DEFAULT, + MONITOR_INDEX_PRE_UP, + MONITOR_INDEX_PRE_DOWN, }; +static Monitor monitors[3] = { + [MONITOR_INDEX_DEFAULT] = { NMD_SCRIPT_DIR_DEFAULT, NULL, TRUE }, + [MONITOR_INDEX_PRE_UP] = { NMD_SCRIPT_DIR_PRE_UP, NULL, TRUE }, + [MONITOR_INDEX_PRE_DOWN] = { NMD_SCRIPT_DIR_PRE_DOWN, NULL, TRUE }, +}; + +static const Monitor* +_get_monitor_by_action (DispatcherAction action) +{ + switch (action) { + case DISPATCHER_ACTION_PRE_UP: + case DISPATCHER_ACTION_VPN_PRE_UP: + return &monitors[MONITOR_INDEX_PRE_UP]; + case DISPATCHER_ACTION_PRE_DOWN: + case DISPATCHER_ACTION_VPN_PRE_DOWN: + return &monitors[MONITOR_INDEX_PRE_DOWN]; + default: + return &monitors[MONITOR_INDEX_DEFAULT]; + } +} + static void dump_object_to_props (GObject *object, GHashTable *hash) { @@ -397,15 +417,16 @@ _dispatcher_call (DispatcherAction action, if (action == DISPATCHER_ACTION_VPN_UP) g_return_val_if_fail (vpn_ip4_config != NULL, FALSE); - if (do_dispatch == FALSE) { + if (!_get_monitor_by_action(action)->has_scripts) { if (blocking == FALSE && (out_call_id || callback)) { info = g_malloc0 (sizeof (*info)); info->request_id = reqid; info->callback = callback; info->user_data = user_data; info->idle_id = g_idle_add (dispatcher_idle_cb, info); - } - nm_log_dbg (LOGD_DISPATCH, "(%u) ignoring request; no scripts in " NMD_SCRIPT_DIR_DEFAULT, reqid); + nm_log_dbg (LOGD_DISPATCH, "(%u) simulate request; no scripts in %s", reqid, _get_monitor_by_action(action)->dir); + } else + nm_log_dbg (LOGD_DISPATCH, "(%u) ignoring request; no scripts in %s", reqid, _get_monitor_by_action(action)->dir); success = TRUE; goto done; } @@ -663,21 +684,15 @@ dispatcher_dir_changed (GFileMonitor *monitor, Monitor *item) { GDir *dir; - guint i; - - /* Default to dispatching on any errors */ - item->has_scripts = TRUE; dir = g_dir_open (item->dir, 0, NULL); if (dir) { item->has_scripts = !!g_dir_read_name (dir); g_dir_close (dir); + } else { + /* Default to dispatching on error opening the directory */ + item->has_scripts = TRUE; } - - /* Recheck all dirs for scripts and update global variable */ - do_dispatch = FALSE; - for (i = 0; i < G_N_ELEMENTS (monitors); i++) - do_dispatch |= monitors[i].has_scripts; } void From c674f26cd1d2ea33e8338a35efb070a56e6fad7f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Jun 2014 18:27:29 +0200 Subject: [PATCH 09/10] dispatcher: for debug logging, truncate the (expected) script name when showing dispatcher results Only truncate the script name to "basename" if the directory is the expected one. Otherwise we print the raw value as returned by the dispatcher service. Signed-off-by: Thomas Haller --- src/nm-dispatcher.c | 47 ++++++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/src/nm-dispatcher.c b/src/nm-dispatcher.c index 0e3b6b1855..64042bbb69 100644 --- a/src/nm-dispatcher.c +++ b/src/nm-dispatcher.c @@ -32,14 +32,15 @@ #include "nm-dbus-glib-types.h" #include "nm-glib-compat.h" -#define CALL_TIMEOUT (1000 * 60 * 10) /* 10 mintues for all scripts */ +#define CALL_TIMEOUT (1000 * 60 * 10) /* 10 minutes for all scripts */ static GHashTable *requests = NULL; typedef struct { - const char *const dir; GFileMonitor *monitor; - gboolean has_scripts; + const char *const dir; + const guint16 dir_len; + char has_scripts; } Monitor; enum { @@ -49,9 +50,10 @@ enum { }; static Monitor monitors[3] = { - [MONITOR_INDEX_DEFAULT] = { NMD_SCRIPT_DIR_DEFAULT, NULL, TRUE }, - [MONITOR_INDEX_PRE_UP] = { NMD_SCRIPT_DIR_PRE_UP, NULL, TRUE }, - [MONITOR_INDEX_PRE_DOWN] = { NMD_SCRIPT_DIR_PRE_DOWN, NULL, TRUE }, +#define MONITORS_INIT_SET(INDEX, SCRIPT_DIR) [INDEX] = { .dir_len = STRLEN (SCRIPT_DIR), .dir = SCRIPT_DIR, .has_scripts = TRUE } + MONITORS_INIT_SET (MONITOR_INDEX_DEFAULT, NMD_SCRIPT_DIR_DEFAULT), + MONITORS_INIT_SET (MONITOR_INDEX_PRE_UP, NMD_SCRIPT_DIR_PRE_UP), + MONITORS_INIT_SET (MONITOR_INDEX_PRE_DOWN, NMD_SCRIPT_DIR_PRE_DOWN), }; static const Monitor* @@ -168,6 +170,7 @@ fill_vpn_props (NMIP4Config *ip4_config, } typedef struct { + DispatcherAction action; guint request_id; DispatcherFunc callback; gpointer user_data; @@ -229,9 +232,10 @@ validate_element (guint request_id, GValue *val, GType expected_type, guint idx, } static void -dispatcher_results_process (guint request_id, GPtrArray *results) +dispatcher_results_process (guint request_id, DispatcherAction action, GPtrArray *results) { guint i; + const Monitor *monitor = _get_monitor_by_action (action); g_return_if_fail (results != NULL); @@ -246,6 +250,7 @@ dispatcher_results_process (guint request_id, GPtrArray *results) GValue *tmp; const char *script, *err; DispatchResult result; + const char *script_validation_msg = ""; if (item->n_values != 3) { nm_log_dbg (LOGD_DISPATCH, "(%u) unexpected number of items in " @@ -259,10 +264,18 @@ dispatcher_results_process (guint request_id, GPtrArray *results) if (!validate_element (request_id, tmp, G_TYPE_STRING, i, 0)) continue; script = g_value_get_string (tmp); - if (!script) + if (!script) { + script_validation_msg = " (path is NULL)"; script = "(unknown)"; - else if (!strncmp (script, NMD_SCRIPT_DIR_DEFAULT "/", STRLEN (NMD_SCRIPT_DIR_DEFAULT "/"))) - script += STRLEN (NMD_SCRIPT_DIR_DEFAULT "/"), + } else if (!strncmp (script, monitor->dir, monitor->dir_len) /* check: prefixed by script directory */ + && script[monitor->dir_len] == '/' && script[monitor->dir_len+1] /* check: with additional "/?" */ + && !strchr (&script[monitor->dir_len+1], '/')) { /* check: and no further '/' */ + /* we expect the script to lie inside monitor->dir. If it does, + * strip the directory name. Otherwise show the full path and a warning. */ + script += monitor->dir_len + 1; + } else + script_validation_msg = " (unexpected path)"; + /* Result */ tmp = g_value_array_get_nth (item, 1); @@ -278,15 +291,15 @@ dispatcher_results_process (guint request_id, GPtrArray *results) if (result == DISPATCH_RESULT_SUCCESS) { - nm_log_dbg (LOGD_DISPATCH, "(%u) %s succeeded", + nm_log_dbg (LOGD_DISPATCH, "(%u) %s succeeded%s", request_id, - script); + script, script_validation_msg); } else { - nm_log_warn (LOGD_DISPATCH, "(%u) %s failed (%s): %s", + nm_log_warn (LOGD_DISPATCH, "(%u) %s failed (%s): %s%s", request_id, script, dispatch_result_to_string (result), - err ? err : ""); + err ? err : "", script_validation_msg); } } } @@ -309,7 +322,7 @@ dispatcher_done_cb (DBusGProxy *proxy, DBusGProxyCall *call, gpointer user_data) if (dbus_g_proxy_end_call (proxy, call, &error, DISPATCHER_TYPE_RESULT_ARRAY, &results, G_TYPE_INVALID)) { - dispatcher_results_process (info->request_id, results); + dispatcher_results_process (info->request_id, info->action, results); free_results (results); } else { g_assert (error); @@ -420,6 +433,7 @@ _dispatcher_call (DispatcherAction action, if (!_get_monitor_by_action(action)->has_scripts) { if (blocking == FALSE && (out_call_id || callback)) { info = g_malloc0 (sizeof (*info)); + info->action = action; info->request_id = reqid; info->callback = callback; info->user_data = user_data; @@ -496,7 +510,7 @@ _dispatcher_call (DispatcherAction action, DISPATCHER_TYPE_RESULT_ARRAY, &results, G_TYPE_INVALID); if (success) { - dispatcher_results_process (reqid, results); + dispatcher_results_process (reqid, action, results); free_results (results); } else { nm_log_warn (LOGD_DISPATCH, "(%u) failed: (%d) %s", reqid, error->code, error->message); @@ -504,6 +518,7 @@ _dispatcher_call (DispatcherAction action, } } else { info = g_malloc0 (sizeof (*info)); + info->action = action; info->request_id = reqid; info->callback = callback; info->user_data = user_data; From 0a7a7f5e4d9eb7a0fa4f986d02965bb181a11904 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 14 Jun 2014 11:41:12 +0200 Subject: [PATCH 10/10] dispatcher: better detection for dispatcher scripts Previously, we would not check the content of the script directory. This meant, that "/etc/NetworkManager/dispatcher.d" almost always contained something, namely the "pre-up.d" and "pre-down.d" directories. Improve that by searching the directories for at least one executable file. Also, debug log the detected state of the dispatcher directories. Signed-off-by: Thomas Haller --- src/nm-dispatcher.c | 46 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 8 deletions(-) diff --git a/src/nm-dispatcher.c b/src/nm-dispatcher.c index 64042bbb69..c51a6cba00 100644 --- a/src/nm-dispatcher.c +++ b/src/nm-dispatcher.c @@ -22,6 +22,7 @@ #include #include #include +#include #include "nm-dispatcher.h" #include "nm-dispatcher-api.h" @@ -38,6 +39,7 @@ static GHashTable *requests = NULL; typedef struct { GFileMonitor *monitor; + const char *const description; const char *const dir; const guint16 dir_len; char has_scripts; @@ -50,10 +52,10 @@ enum { }; static Monitor monitors[3] = { -#define MONITORS_INIT_SET(INDEX, SCRIPT_DIR) [INDEX] = { .dir_len = STRLEN (SCRIPT_DIR), .dir = SCRIPT_DIR, .has_scripts = TRUE } - MONITORS_INIT_SET (MONITOR_INDEX_DEFAULT, NMD_SCRIPT_DIR_DEFAULT), - MONITORS_INIT_SET (MONITOR_INDEX_PRE_UP, NMD_SCRIPT_DIR_PRE_UP), - MONITORS_INIT_SET (MONITOR_INDEX_PRE_DOWN, NMD_SCRIPT_DIR_PRE_DOWN), +#define MONITORS_INIT_SET(INDEX, USE, SCRIPT_DIR) [INDEX] = { .dir_len = STRLEN (SCRIPT_DIR), .dir = SCRIPT_DIR, .description = ("" USE), .has_scripts = TRUE } + MONITORS_INIT_SET (MONITOR_INDEX_DEFAULT, "default", NMD_SCRIPT_DIR_DEFAULT), + MONITORS_INIT_SET (MONITOR_INDEX_PRE_UP, "pre-up", NMD_SCRIPT_DIR_PRE_UP), + MONITORS_INIT_SET (MONITOR_INDEX_PRE_DOWN, "pre-down", NMD_SCRIPT_DIR_PRE_DOWN), }; static const Monitor* @@ -698,16 +700,44 @@ dispatcher_dir_changed (GFileMonitor *monitor, GFileMonitorEvent event_type, Monitor *item) { + const char *name; + char *full_name; GDir *dir; + GError *error = NULL; - dir = g_dir_open (item->dir, 0, NULL); + dir = g_dir_open (item->dir, 0, &error); if (dir) { - item->has_scripts = !!g_dir_read_name (dir); + int errsv = 0; + + item->has_scripts = FALSE; + errno = 0; + while (!item->has_scripts + && (name = g_dir_read_name (dir))) { + full_name = g_build_filename (item->dir, name, NULL); + item->has_scripts = g_file_test (full_name, G_FILE_TEST_IS_EXECUTABLE); + g_free (full_name); + } + errsv = errno; g_dir_close (dir); + if (item->has_scripts) + nm_log_dbg (LOGD_DISPATCH, "dispatcher: %s script directory '%s' has scripts", item->description, item->dir); + else if (errsv == 0) + nm_log_dbg (LOGD_DISPATCH, "dispatcher: %s script directory '%s' has no scripts", item->description, item->dir); + else { + nm_log_dbg (LOGD_DISPATCH, "dispatcher: %s script directory '%s' error reading (%s)", item->description, item->dir, strerror (errsv)); + item->has_scripts = TRUE; + } } else { - /* Default to dispatching on error opening the directory */ - item->has_scripts = TRUE; + if (g_error_matches (error, G_FILE_ERROR, G_FILE_ERROR_NOENT)) { + nm_log_dbg (LOGD_DISPATCH, "dispatcher: %s script directory '%s' does not exist", item->description, item->dir); + item->has_scripts = FALSE; + } else { + nm_log_dbg (LOGD_DISPATCH, "dispatcher: %s script directory '%s' error (%s)", item->description, item->dir, error->message); + item->has_scripts = TRUE; + } + g_error_free (error); } + } void