From a217a742f1207e4451f401280048f64e10cfd5be Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 13 Feb 2014 14:22:26 -0500 Subject: [PATCH 1/3] core: remove some unused code We never pass any delay_seconds value to schedule_activate_check() except "0", so just remove that argument. --- src/nm-policy.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/nm-policy.c b/src/nm-policy.c index 34216c8717..71f53e6afe 100644 --- a/src/nm-policy.c +++ b/src/nm-policy.c @@ -1015,17 +1015,14 @@ auto_activate_device (gpointer user_data) } static ActivateData * -activate_data_new (NMPolicy *policy, NMDevice *device, guint delay_seconds) +activate_data_new (NMPolicy *policy, NMDevice *device) { ActivateData *data; data = g_malloc0 (sizeof (ActivateData)); data->policy = policy; data->device = g_object_ref (device); - if (delay_seconds > 0) - data->autoactivate_id = g_timeout_add_seconds (delay_seconds, auto_activate_device, data); - else - data->autoactivate_id = g_idle_add (auto_activate_device, data); + data->autoactivate_id = g_idle_add (auto_activate_device, data); nm_device_add_pending_action (device, "autoactivate"); @@ -1215,7 +1212,7 @@ sleeping_changed (NMManager *manager, GParamSpec *pspec, gpointer user_data) } static void -schedule_activate_check (NMPolicy *policy, NMDevice *device, guint delay_seconds) +schedule_activate_check (NMPolicy *policy, NMDevice *device) { NMPolicyPrivate *priv = NM_POLICY_GET_PRIVATE (policy); ActivateData *data; @@ -1241,7 +1238,7 @@ schedule_activate_check (NMPolicy *policy, NMDevice *device, guint delay_seconds /* Schedule an auto-activation if there isn't one already for this device */ if (find_pending_activation (priv->pending_activation_checks, device) == NULL) { - data = activate_data_new (policy, device, delay_seconds); + data = activate_data_new (policy, device); priv->pending_activation_checks = g_slist_append (priv->pending_activation_checks, data); } } @@ -1493,7 +1490,7 @@ device_state_changed (NMDevice *device, update_routing_and_dns (policy, FALSE); /* Device is now available for auto-activation */ - schedule_activate_check (policy, device, 0); + schedule_activate_check (policy, device); break; case NM_DEVICE_STATE_PREPARE: @@ -1601,25 +1598,25 @@ device_autoconnect_changed (NMDevice *device, gpointer user_data) { if (nm_device_get_autoconnect (device)) - schedule_activate_check ((NMPolicy *) user_data, device, 0); + schedule_activate_check ((NMPolicy *) user_data, device); } static void wireless_networks_changed (NMDevice *device, GObject *ap, gpointer user_data) { - schedule_activate_check ((NMPolicy *) user_data, device, 0); + schedule_activate_check ((NMPolicy *) user_data, device); } static void nsps_changed (NMDevice *device, GObject *nsp, gpointer user_data) { - schedule_activate_check ((NMPolicy *) user_data, device, 0); + schedule_activate_check ((NMPolicy *) user_data, device); } static void modem_enabled_changed (NMDevice *device, gpointer user_data) { - schedule_activate_check ((NMPolicy *) (user_data), device, 0); + schedule_activate_check ((NMPolicy *) (user_data), device); } typedef struct { @@ -1837,7 +1834,7 @@ schedule_activate_all (NMPolicy *policy) devices = nm_manager_get_devices (priv->manager); for (iter = devices; iter; iter = g_slist_next (iter)) - schedule_activate_check (policy, NM_DEVICE (iter->data), 0); + schedule_activate_check (policy, NM_DEVICE (iter->data)); } static void From 93285054ae5ef290d879a753f88824b5027e9c8a Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 13 Feb 2014 14:25:30 -0500 Subject: [PATCH 2/3] Revert "core: fix warning about pending action "autoactivate"" This change removed the "autoactivate" pending action too soon, creating a window where the device had no pending actions, allowing the manager to declare startup complete while devices were still being activated. This reverts commit a16b7a82538e59af19557e03ae54e2e6afb5a891. --- src/nm-policy.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/nm-policy.c b/src/nm-policy.c index 71f53e6afe..142cf6521a 100644 --- a/src/nm-policy.c +++ b/src/nm-policy.c @@ -937,10 +937,10 @@ typedef struct { static void activate_data_free (ActivateData *data) { - if (data->autoactivate_id) { - nm_device_remove_pending_action (data->device, "autoactivate"); + nm_device_remove_pending_action (data->device, "autoactivate"); + + if (data->autoactivate_id) g_source_remove (data->autoactivate_id); - } g_object_unref (data->device); memset (data, 0, sizeof (*data)); g_free (data); @@ -960,9 +960,8 @@ auto_activate_device (gpointer user_data) policy = data->policy; priv = NM_POLICY_GET_PRIVATE (policy); - priv->pending_activation_checks = g_slist_remove (priv->pending_activation_checks, data); data->autoactivate_id = 0; - nm_device_remove_pending_action (data->device, "autoactivate"); + priv->pending_activation_checks = g_slist_remove (priv->pending_activation_checks, data); // FIXME: if a device is already activating (or activated) with a connection // but another connection now overrides the current one for that device, From 4f0c70e94534abafde6a0459af74b68a7da724d9 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 13 Feb 2014 14:45:25 -0500 Subject: [PATCH 3/3] core: don't recursively schedule an autoactivate check on a device NMPolicy's auto_activate_device() was immediately removing the device from priv->pending_activation_checks, which meant that if nm_manager_activate_connection() had some side effect that would cause schedule_activation_check() to be called again, another auto-activation check could be queued while the first was still in progress (causing a warning). Fix this by not removing the device from the list until the activation attempt is complete. This requires some additional minor changes to correctly handle the possibility of remove_device() being triggered as a side effect of nm_manager_activate_connection(). Also merge activate_data_new() into schedule_activation_check() so that all the "start an auto-activation" code is in one place. --- src/nm-policy.c | 56 ++++++++++++++++++++++--------------------------- 1 file changed, 25 insertions(+), 31 deletions(-) diff --git a/src/nm-policy.c b/src/nm-policy.c index 142cf6521a..90fbc36d8d 100644 --- a/src/nm-policy.c +++ b/src/nm-policy.c @@ -937,12 +937,14 @@ typedef struct { static void activate_data_free (ActivateData *data) { + NMPolicyPrivate *priv = NM_POLICY_GET_PRIVATE (data->policy); + nm_device_remove_pending_action (data->device, "autoactivate"); + priv->pending_activation_checks = g_slist_remove (priv->pending_activation_checks, data); if (data->autoactivate_id) g_source_remove (data->autoactivate_id); g_object_unref (data->device); - memset (data, 0, sizeof (*data)); g_free (data); } @@ -961,7 +963,6 @@ auto_activate_device (gpointer user_data) priv = NM_POLICY_GET_PRIVATE (policy); data->autoactivate_id = 0; - priv->pending_activation_checks = g_slist_remove (priv->pending_activation_checks, data); // FIXME: if a device is already activating (or activated) with a connection // but another connection now overrides the current one for that device, @@ -1013,21 +1014,6 @@ auto_activate_device (gpointer user_data) return G_SOURCE_REMOVE; } -static ActivateData * -activate_data_new (NMPolicy *policy, NMDevice *device) -{ - ActivateData *data; - - data = g_malloc0 (sizeof (ActivateData)); - data->policy = policy; - data->device = g_object_ref (device); - data->autoactivate_id = g_idle_add (auto_activate_device, data); - - nm_device_add_pending_action (device, "autoactivate"); - - return data; -} - static ActivateData * find_pending_activation (GSList *list, NMDevice *device) { @@ -1226,20 +1212,33 @@ schedule_activate_check (NMPolicy *policy, NMDevice *device) if (!nm_device_autoconnect_allowed (device)) return; - /* If the device already has an activation in-progress or waiting for - * authentication, don't start an auto-activation for it. - */ + if (find_pending_activation (priv->pending_activation_checks, device)) + return; + active_connections = nm_manager_get_active_connections (priv->manager); for (iter = active_connections; iter; iter = iter->next) { if (nm_active_connection_get_device (NM_ACTIVE_CONNECTION (iter->data)) == device) return; } - /* Schedule an auto-activation if there isn't one already for this device */ - if (find_pending_activation (priv->pending_activation_checks, device) == NULL) { - data = activate_data_new (policy, device); - priv->pending_activation_checks = g_slist_append (priv->pending_activation_checks, data); - } + nm_device_add_pending_action (device, "autoactivate"); + + data = g_malloc0 (sizeof (ActivateData)); + data->policy = policy; + data->device = g_object_ref (device); + data->autoactivate_id = g_idle_add (auto_activate_device, data); + priv->pending_activation_checks = g_slist_append (priv->pending_activation_checks, data); +} + +static void +clear_pending_activate_check (NMPolicy *policy, NMDevice *device) +{ + NMPolicyPrivate *priv = NM_POLICY_GET_PRIVATE (policy); + ActivateData *data; + + data = find_pending_activation (priv->pending_activation_checks, device); + if (data && data->autoactivate_id) + activate_data_free (data); } static gboolean @@ -1676,15 +1675,10 @@ device_removed (NMManager *manager, NMDevice *device, gpointer user_data) { NMPolicy *policy = (NMPolicy *) user_data; NMPolicyPrivate *priv = NM_POLICY_GET_PRIVATE (policy); - ActivateData *tmp; GSList *iter; /* Clear any idle callbacks for this device */ - tmp = find_pending_activation (priv->pending_activation_checks, device); - if (tmp) { - priv->pending_activation_checks = g_slist_remove (priv->pending_activation_checks, tmp); - activate_data_free (tmp); - } + clear_pending_activate_check (policy, device); /* Clear any signal handlers for this device */ iter = priv->dev_ids;