NetworkManager

Merge lp:~phablet-team/network-manager/lp1461593-wily into lp:~network-manager/network-manager/ubuntu

  1. lp1461593-wily
  2. Merge into ubuntu
Proposed by Tony Espy
Status: Merged
Approved by: Mathieu Trudel-Lapierre
Approved revision: 969
Merged at revision: 968
Proposed branch: lp:~phablet-team/network-manager/lp1461593-wily
Merge into: lp:~network-manager/network-manager/ubuntu
Prerequisite: lp:~phablet-team/network-manager/lp1435776-wily
Diff against target: 331 lines (+223/-10)
6 files modified
debian/changelog (+10/-0)
debian/patches/0001-wwan-add-support-for-using-oFono-as-a-modem-manager.patch (+16/-9)
debian/patches/add_ofono_settings_support.patch (+5/-1)
debian/patches/lp1461593-add-modem-reconnect-delay-to-policy.patch (+117/-0)
debian/patches/lp1461593-add-nm-settings-connection-reset-retries-methods.patch (+73/-0)
debian/patches/series (+2/-0)
To merge this branch: bzr merge lp:~phablet-team/network-manager/lp1461593-wily
Reviewer Review Type Date Requested Status
Mathieu Trudel-Lapierre Approve
Review via email: mp+264894@code.launchpad.net

Description of the change

This change adds a 5s delay into NM_POLICY's re-connect logic, specifically so that the retry_limit of the associated connection isn't exhausted immediately whenever the connection drops, which then used to result in a 5m timer being set.

There are two changes...

1. The afore-mentioned 5m timer is now reduced to 30s for modem devices. In theory, connections shouldn't be getting disabled anymore due to the above fix, so this is a just-in-case change.

2. The low-level NM_MODEM_OFONO code wasn't setting the modem state to DISCONNECTING when disconnect() was called. This is now done, and in the associated callback ( disconnect_cb ) the new state is now determined by the ofono cached properties ( online && powered && attached ).

To post a comment you must log in.
Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

Ofono bits probably should be folded in to the main patch; anything that touches the core NM code should be split out by itself, so that we can upstream just that independently from any ofono support code.

If you want, I could possibly do the patch mangling.

review: Needs Fixing
lp:~phablet-team/network-manager/lp1461593-wily updated
967. By Tony Espy

debian/patches/add_ofono_settings_support.patch: remove code
that added APN, USERNAME and PASSWORD to NM_SETTING_GSM object.
NM doesn't actually need access to these settings, and USERNAME/
PASSWORD can cause issues with NM's secrets needed logic.

968. By Tony Espy

debian/patches/0001-wwan-add-support-for-using-oFono-as-a-modem-manager.patch,
debian/patches/lp1461593-add-nm-settings-connection-reset-retries-methods.patch,
debian/patches/add_ofono_settings_support.patch,
debian/patches/lp1461593-add-modem-reconnect-delay-to-policy.patch: More changes
to NMModemOfono's modem_state handling. Added get/set_reset_retries_timeout
methods to NMSettingsConnection, and use the set method to lower the timeout for
ofono connections to 30s. Finally added a 5s delay to NM_POLICY's activation
logic triggered when a modem device is disconnected. This allows modem time to
settle and NM to process the resulting DBus state changes. (LP: #1461593)

Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

Looks fine overall, though I've added some nitpicks inline. The extra delay/retries patches will need careful testing in a silo to make sure they don't break ModemManager, but they do look harmless.

review: Needs Fixing
lp:~phablet-team/network-manager/lp1461593-wily updated
969. By Tony Espy

Updated new patches for DEP-3, and re-ordered ofono patches.

Revision history for this message
Tony Espy (awe) wrote :

Updated patches for DEP-3, and re-ordered based on inline comments.

Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'debian/changelog'
--- debian/changelog 2015-07-17 15:50:46 +0000
+++ debian/changelog 2015-07-17 15:50:46 +0000
@@ -5,6 +5,16 @@
5 NM doesn't actually need access to these settings, and USERNAME/5 NM doesn't actually need access to these settings, and USERNAME/
6 PASSWORD can cause issues with NM's secrets needed logic.6 PASSWORD can cause issues with NM's secrets needed logic.
77
8 * debian/patches/0001-wwan-add-support-for-using-oFono-as-a-modem-manager.patch,
9 debian/patches/lp1461593-add-nm-settings-connection-reset-retries-methods.patch,
10 debian/patches/add_ofono_settings_support.patch,
11 debian/patches/lp1461593-add-modem-reconnect-delay-to-policy.patch: More changes
12 to NMModemOfono's modem_state handling. Added get/set_reset_retries_timeout
13 methods to NMSettingsConnection, and use the set method to lower the timeout for
14 ofono connections to 30s. Finally added a 5s delay to NM_POLICY's activation
15 logic triggered when a modem device is disconnected. This allows modem time to
16 settle and NM to process the resulting DBus state changes. (LP: #1461593)
17
8 -- Tony Espy <espy@canonical.com> Wed, 15 Jul 2015 15:35:17 -040018 -- Tony Espy <espy@canonical.com> Wed, 15 Jul 2015 15:35:17 -0400
919
10network-manager (0.9.10.0-4ubuntu18) wily; urgency=medium20network-manager (0.9.10.0-4ubuntu18) wily; urgency=medium
1121
=== modified file 'debian/patches/0001-wwan-add-support-for-using-oFono-as-a-modem-manager.patch'
--- debian/patches/0001-wwan-add-support-for-using-oFono-as-a-modem-manager.patch 2015-04-13 04:34:55 +0000
+++ debian/patches/0001-wwan-add-support-for-using-oFono-as-a-modem-manager.patch 2015-07-17 15:50:46 +0000
@@ -304,7 +304,7 @@
304===================================================================304===================================================================
305--- /dev/null305--- /dev/null
306+++ network-manager-0.9.10.0/src/devices/wwan/nm-modem-ofono.c306+++ network-manager-0.9.10.0/src/devices/wwan/nm-modem-ofono.c
307@@ -0,0 +1,1140 @@307@@ -0,0 +1,1147 @@
308+/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */308+/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
309+/* NetworkManager -- Network link manager309+/* NetworkManager -- Network link manager
310+ *310+ *
@@ -414,7 +414,8 @@
414+{414+{
415+ SimpleDisconnectContext *ctx = (SimpleDisconnectContext*) user_data;415+ SimpleDisconnectContext *ctx = (SimpleDisconnectContext*) user_data;
416+ NMModemOfono *self = ctx->self;416+ NMModemOfono *self = ctx->self;
417+ NMModemState state = nm_modem_get_state (NM_MODEM (self));417+ NMModemOfonoPrivate *priv = NM_MODEM_OFONO_GET_PRIVATE (self);
418+ NMModemState state;
418+ GError *error = NULL;419+ GError *error = NULL;
419+420+
420+ nm_log_dbg (LOGD_MB, "in %s", __func__);421+ nm_log_dbg (LOGD_MB, "in %s", __func__);
@@ -429,10 +430,15 @@
429+430+
430+ simple_disconnect_context_free (ctx);431+ simple_disconnect_context_free (ctx);
431+432+
432+ if (state != NM_MODEM_STATE_SEARCHING)433+ /* No need to re-read contexts in this case... */
433+ nm_modem_set_state (NM_MODEM (self),434+
434+ NM_MODEM_STATE_REGISTERED,435+ if (priv->modem_online && priv->gprs_powered && priv->gprs_attached)
435+ nm_modem_state_to_string (NM_MODEM_STATE_REGISTERED));436+ state = NM_MODEM_STATE_REGISTERED;
437+ else
438+ state = NM_MODEM_STATE_SEARCHING;
439+
440+ nm_modem_set_state (NM_MODEM (self), state,
441+ nm_modem_state_to_string (state));
436+}442+}
437+443+
438+static void444+static void
@@ -449,6 +455,10 @@
449+ ctx->self = g_object_ref (self);455+ ctx->self = g_object_ref (self);
450+ ctx->warn = warn;456+ ctx->warn = warn;
451+457+
458+ nm_modem_set_state (NM_MODEM (self),
459+ NM_MODEM_STATE_DISCONNECTING,
460+ nm_modem_state_to_string (NM_MODEM_STATE_DISCONNECTING));
461+
452+ g_value_init (&value, G_TYPE_BOOLEAN);462+ g_value_init (&value, G_TYPE_BOOLEAN);
453+ g_value_set_boolean (&value, FALSE);463+ g_value_set_boolean (&value, FALSE);
454+464+
@@ -493,18 +503,15 @@
493+{503+{
494+ NMModemOfonoPrivate *priv = NM_MODEM_OFONO_GET_PRIVATE (self);504+ NMModemOfonoPrivate *priv = NM_MODEM_OFONO_GET_PRIVATE (self);
495+ NMModemState new_state;505+ NMModemState new_state;
496+ NMDeviceStateReason reason;
497+506+
498+ if (new_enabled == priv->enabled)507+ if (new_enabled == priv->enabled)
499+ return;508+ return;
500+509+
501+ if (new_enabled) {510+ if (new_enabled) {
502+ new_state = NM_MODEM_STATE_REGISTERED;511+ new_state = NM_MODEM_STATE_REGISTERED;
503+ reason = NM_DEVICE_STATE_REASON_NONE;
504+ ofono_read_contexts (self);512+ ofono_read_contexts (self);
505+ } else {513+ } else {
506+ new_state = NM_MODEM_STATE_SEARCHING;514+ new_state = NM_MODEM_STATE_SEARCHING;
507+ reason = NM_DEVICE_STATE_REASON_MODEM_NO_CARRIER;
508+ }515+ }
509+516+
510+ nm_modem_set_state (NM_MODEM (self),517+ nm_modem_set_state (NM_MODEM (self),
511518
=== modified file 'debian/patches/add_ofono_settings_support.patch'
--- debian/patches/add_ofono_settings_support.patch 2015-07-17 15:50:46 +0000
+++ debian/patches/add_ofono_settings_support.patch 2015-07-17 15:50:46 +0000
@@ -524,7 +524,7 @@
524===================================================================524===================================================================
525--- /dev/null525--- /dev/null
526+++ b/src/settings/plugins/ofono/plugin.c526+++ b/src/settings/plugins/ofono/plugin.c
527@@ -0,0 +1,738 @@527@@ -0,0 +1,742 @@
528+/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */528+/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
529+529+
530+/* Ofono modem settings service530+/* Ofono modem settings service
@@ -677,6 +677,10 @@
677+677+
678+ /* add the new connection */678+ /* add the new connection */
679+ exported = nm_ofono_connection_new (context);679+ exported = nm_ofono_connection_new (context);
680+
681+ /* Lower disabled timer to 30s */
682+ nm_settings_connection_set_reset_retries_timeout (NM_SETTINGS_CONNECTION (exported), 30);
683+
680+ setting = nm_connection_get_setting_connection (NM_CONNECTION (exported));684+ setting = nm_connection_get_setting_connection (NM_CONNECTION (exported));
681+ g_object_set (setting, NM_SETTING_CONNECTION_AUTOCONNECT, TRUE, NULL);685+ g_object_set (setting, NM_SETTING_CONNECTION_AUTOCONNECT, TRUE, NULL);
682+ if (exported) {686+ if (exported) {
683687
=== added file 'debian/patches/lp1461593-add-modem-reconnect-delay-to-policy.patch'
--- debian/patches/lp1461593-add-modem-reconnect-delay-to-policy.patch 1970-01-01 00:00:00 +0000
+++ debian/patches/lp1461593-add-modem-reconnect-delay-to-policy.patch 2015-07-17 15:50:46 +0000
@@ -0,0 +1,117 @@
1Author: Tony Espy <espy@canonical.com>
2Subject: Introduce NMPolicy delay for modem activate retries
3
4This patch introduces a 5s delay between retry activations
5for modem devices. Otherwise, the activation attempts can
6flood the modem all at once, and then trigger the reset_retries
7timeout ( which defaults to 300s ).
8
9Bug: https://bugs.launchpad.net/ubuntu/+source/network-manager/+bug/1461593
10
11---
12
13Index: b/src/nm-policy.c
14===================================================================
15--- a/src/nm-policy.c
16+++ b/src/nm-policy.c
17@@ -1229,7 +1229,7 @@ sleeping_changed (NMManager *manager, GP
18 }
19
20 static void
21-schedule_activate_check (NMPolicy *policy, NMDevice *device)
22+schedule_activate_check (NMPolicy *policy, NMDevice *device, guint delay)
23 {
24 NMPolicyPrivate *priv = NM_POLICY_GET_PRIVATE (policy);
25 ActivateData *data;
26@@ -1258,7 +1258,12 @@ schedule_activate_check (NMPolicy *polic
27 data = g_malloc0 (sizeof (ActivateData));
28 data->policy = policy;
29 data->device = g_object_ref (device);
30- data->autoactivate_id = g_idle_add (auto_activate_device, data);
31+
32+ if (delay)
33+ data->autoactivate_id = g_timeout_add_seconds (delay, auto_activate_device, data);
34+ else
35+ data->autoactivate_id = g_idle_add (auto_activate_device, data);
36+
37 priv->pending_activation_checks = g_slist_append (priv->pending_activation_checks, data);
38 }
39
40@@ -1438,6 +1443,7 @@ device_state_changed (NMDevice *device,
41 NMIP4Config *ip4_config;
42 NMIP6Config *ip6_config;
43 NMSettingConnection *s_con = NULL;
44+ guint delay = 0;
45
46 switch (new_state) {
47 case NM_DEVICE_STATE_FAILED:
48@@ -1455,7 +1461,7 @@ device_state_changed (NMDevice *device,
49
50 nm_settings_connection_set_autoconnect_blocked_reason (connection, NM_DEVICE_STATE_REASON_NO_SECRETS);
51 } else if (tries > 0) {
52- nm_log_dbg (LOGD_DEVICE, "Connection '%s' failed to autoconnect; %d tries left",
53+ nm_log_info (LOGD_DEVICE, "Connection '%s' failed to autoconnect; %d tries left",
54 nm_connection_get_id (NM_CONNECTION (connection)), tries);
55 nm_settings_connection_set_autoconnect_retries (connection, tries - 1);
56 }
57@@ -1466,9 +1472,14 @@ device_state_changed (NMDevice *device,
58 /* Schedule a handler to reset retries count */
59 if (!priv->reset_retries_id) {
60 gint32 retry_time = nm_settings_connection_get_autoconnect_retry_time (connection);
61+ gint32 actual_time = MAX (0, retry_time - nm_utils_get_monotonic_timestamp_s ());
62+
63+ nm_log_info (LOGD_DEVICE, "Disabling autoconnect for connection '%s'; setting retry of %d.",
64+ nm_connection_get_id (NM_CONNECTION (connection)), actual_time);
65+
66
67 g_warn_if_fail (retry_time != 0);
68- priv->reset_retries_id = g_timeout_add_seconds (MAX (0, retry_time - nm_utils_get_monotonic_timestamp_s ()), reset_connections_retries, policy);
69+ priv->reset_retries_id = g_timeout_add_seconds (actual_time, reset_connections_retries, policy);
70 }
71 }
72 nm_connection_clear_secrets (NM_CONNECTION (connection));
73@@ -1532,7 +1543,18 @@ device_state_changed (NMDevice *device,
74 update_routing_and_dns (policy, FALSE);
75
76 /* Device is now available for auto-activation */
77- schedule_activate_check (policy, device);
78+
79+ if (nm_device_get_device_type (device) == NM_DEVICE_TYPE_MODEM)
80+ delay = 5;
81+
82+ if (connection)
83+ nm_log_info (LOGD_DEVICE, "Connection '%s' disconnected, scheduling activate_check in %u seconds.",
84+ nm_connection_get_id (NM_CONNECTION (connection)), delay);
85+ else
86+ nm_log_info (LOGD_DEVICE, "Device has no connection; scheduling activate_check in %u seconds.",
87+ delay);
88+
89+ schedule_activate_check (policy, device, delay);
90 break;
91
92 case NM_DEVICE_STATE_PREPARE:
93@@ -1642,13 +1664,13 @@ device_autoconnect_changed (NMDevice *de
94 gpointer user_data)
95 {
96 if (nm_device_get_autoconnect (device))
97- schedule_activate_check ((NMPolicy *) user_data, device);
98+ schedule_activate_check ((NMPolicy *) user_data, device, 0);
99 }
100
101 static void
102 device_recheck_auto_activate (NMDevice *device, gpointer user_data)
103 {
104- schedule_activate_check (NM_POLICY (user_data), device);
105+ schedule_activate_check (NM_POLICY (user_data), device, 0);
106 }
107
108 typedef struct {
109@@ -1845,7 +1867,7 @@ schedule_activate_all (NMPolicy *policy)
110 const GSList *iter;
111
112 for (iter = nm_manager_get_devices (priv->manager); iter; iter = g_slist_next (iter))
113- schedule_activate_check (policy, NM_DEVICE (iter->data));
114+ schedule_activate_check (policy, NM_DEVICE (iter->data), 0);
115 }
116
117 static void
0118
=== added file 'debian/patches/lp1461593-add-nm-settings-connection-reset-retries-methods.patch'
--- debian/patches/lp1461593-add-nm-settings-connection-reset-retries-methods.patch 1970-01-01 00:00:00 +0000
+++ debian/patches/lp1461593-add-nm-settings-connection-reset-retries-methods.patch 2015-07-17 15:50:46 +0000
@@ -0,0 +1,73 @@
1Author: Tony Espy <espy@canonical.com>
2Subject: Add new NMSettingsConnection reset_retries get/set methods
3
4This patch adds get/set methods to NMSettingsConnection for the
5reset_retries_timeout. This allows sub-classes to override the
6default setting ( 300s ).
7
8Bug: https://bugs.launchpad.net/ubuntu/+source/network-manager/+bug/1461593
9
10---
11Index: network-manager-0.9.10.0/src/settings/nm-settings-connection.c
12===================================================================
13--- network-manager-0.9.10.0.orig/src/settings/nm-settings-connection.c
14+++ network-manager-0.9.10.0/src/settings/nm-settings-connection.c
15@@ -132,6 +132,7 @@ typedef struct {
16
17 int autoconnect_retries;
18 gint32 autoconnect_retry_time;
19+ int reset_retries_timeout;
20 NMDeviceStateReason autoconnect_blocked_reason;
21
22 } NMSettingsConnectionPrivate;
23@@ -1922,6 +1923,19 @@ nm_settings_connection_get_autoconnect_r
24 return NM_SETTINGS_CONNECTION_GET_PRIVATE (connection)->autoconnect_retries;
25 }
26
27+int
28+nm_settings_connection_get_reset_retries_timeout (NMSettingsConnection *connection)
29+{
30+ return NM_SETTINGS_CONNECTION_GET_PRIVATE (connection)->reset_retries_timeout;
31+}
32+
33+void
34+nm_settings_connection_set_reset_retries_timeout (NMSettingsConnection *connection,
35+ int timeout)
36+{
37+ NM_SETTINGS_CONNECTION_GET_PRIVATE (connection)->reset_retries_timeout = timeout;
38+}
39+
40 void
41 nm_settings_connection_set_autoconnect_retries (NMSettingsConnection *connection,
42 int retries)
43@@ -1932,7 +1946,7 @@ nm_settings_connection_set_autoconnect_r
44 if (retries)
45 priv->autoconnect_retry_time = 0;
46 else
47- priv->autoconnect_retry_time = nm_utils_get_monotonic_timestamp_s () + AUTOCONNECT_RESET_RETRIES_TIMER;
48+ priv->autoconnect_retry_time = nm_utils_get_monotonic_timestamp_s () + priv->reset_retries_timeout;
49 }
50
51 void
52@@ -2039,6 +2053,7 @@ nm_settings_connection_init (NMSettingsC
53
54 priv->autoconnect_retries = AUTOCONNECT_RETRIES_DEFAULT;
55 priv->autoconnect_blocked_reason = NM_DEVICE_STATE_REASON_NONE;
56+ priv->reset_retries_timeout = AUTOCONNECT_RESET_RETRIES_TIMER;
57
58 g_signal_connect (self, NM_CONNECTION_SECRETS_CLEARED, G_CALLBACK (secrets_cleared_cb), NULL);
59 g_signal_connect (self, NM_CONNECTION_CHANGED, G_CALLBACK (changed_cb), GUINT_TO_POINTER (TRUE));
60Index: network-manager-0.9.10.0/src/settings/nm-settings-connection.h
61===================================================================
62--- network-manager-0.9.10.0.orig/src/settings/nm-settings-connection.h
63+++ network-manager-0.9.10.0/src/settings/nm-settings-connection.h
64@@ -156,6 +156,9 @@ void nm_settings_connection_reset_autoco
65
66 gint32 nm_settings_connection_get_autoconnect_retry_time (NMSettingsConnection *connection);
67
68+int nm_settings_connection_get_reset_retries_timeout (NMSettingsConnection *connection);
69+void nm_settings_connection_set_reset_retries_timeout (NMSettingsConnection *connection, int timeout);
70+
71 NMDeviceStateReason nm_settings_connection_get_autoconnect_blocked_reason (NMSettingsConnection *connection);
72 void nm_settings_connection_set_autoconnect_blocked_reason (NMSettingsConnection *connection,
73 NMDeviceStateReason reason);
074
=== modified file 'debian/patches/series'
--- debian/patches/series 2015-07-14 18:08:41 +0000
+++ debian/patches/series 2015-07-17 15:50:46 +0000
@@ -63,7 +63,9 @@
63lp1099983_ignore-p2p-wifi-devices.patch63lp1099983_ignore-p2p-wifi-devices.patch
64prioritize_3g_later.patch64prioritize_3g_later.patch
650001-wwan-add-support-for-using-oFono-as-a-modem-manager.patch650001-wwan-add-support-for-using-oFono-as-a-modem-manager.patch
66lp1461593-add-nm-settings-connection-reset-retries-methods.patch
66add_ofono_settings_support.patch67add_ofono_settings_support.patch
68lp1461593-add-modem-reconnect-delay-to-policy.patch
6769
68# killswitch70# killswitch
69ignore_rfkill_if_urfkill_is_present.patch71ignore_rfkill_if_urfkill_is_present.patch

Subscribers

People subscribed via source and target branches