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

[Tony Espy]

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 ).

[Mathieu Trudel-Lapierre (cyphermox)]

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]

967. By Tony Espy on 2015-07-15

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 on 2015-07-15

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)

[Mathieu Trudel-Lapierre (cyphermox)]

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]

969. By Tony Espy on 2015-07-17

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

[Tony Espy (awe)]

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

[Mathieu Trudel-Lapierre (cyphermox)]

review: Approve

=== 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 @@
     NM doesn't actually need access to these settings, and USERNAME/
     PASSWORD can cause issues with NM's secrets needed logic.
 
+  * 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)
+
  -- Tony Espy <[e-mail]>  Wed, 15 Jul 2015 15:35:17 -0400
 
 network-manager (0.9.10.0-4ubuntu18) wily; urgency=medium

=== 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 @@
 ===================================================================
 --- /dev/null
 +++ network-manager-0.9.10.0/src/devices/wwan/nm-modem-ofono.c
-@@ -0,0 +1,1140 @@
+@@ -0,0 +1,1147 @@
 +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
 +/* NetworkManager -- Network link manager
 + *
@@ -414,7 +414,8 @@
 +{
 +	SimpleDisconnectContext *ctx = (SimpleDisconnectContext*) user_data;
 +	NMModemOfono *self = ctx->self;
-+	NMModemState state = nm_modem_get_state (NM_MODEM (self));
++	NMModemOfonoPrivate *priv = NM_MODEM_OFONO_GET_PRIVATE (self);
++	NMModemState state;
 +	GError *error = NULL;
 +
 +	nm_log_dbg (LOGD_MB, "in %s", __func__);
@@ -429,10 +430,15 @@
 +
 +	simple_disconnect_context_free (ctx);
 +
-+	if (state != NM_MODEM_STATE_SEARCHING)
-+		nm_modem_set_state (NM_MODEM (self),
-+		                    NM_MODEM_STATE_REGISTERED,
-+		                    nm_modem_state_to_string (NM_MODEM_STATE_REGISTERED));
++	/* No need to re-read contexts in this case... */
++
++	if (priv->modem_online && priv->gprs_powered && priv->gprs_attached)
++		state = NM_MODEM_STATE_REGISTERED;
++	else
++		state = NM_MODEM_STATE_SEARCHING;
++
++	nm_modem_set_state (NM_MODEM (self), state,
++	                    nm_modem_state_to_string (state));
 +}
 +
 +static void
@@ -449,6 +455,10 @@
 +	ctx->self = g_object_ref (self);
 +	ctx->warn = warn;
 +
++	nm_modem_set_state (NM_MODEM (self),
++						NM_MODEM_STATE_DISCONNECTING,
++						nm_modem_state_to_string (NM_MODEM_STATE_DISCONNECTING));
++
 +	g_value_init (&value, G_TYPE_BOOLEAN);
 +	g_value_set_boolean (&value, FALSE);
 +
@@ -493,18 +503,15 @@
 +{
 +	NMModemOfonoPrivate *priv = NM_MODEM_OFONO_GET_PRIVATE (self);
 +	NMModemState new_state;
-+	NMDeviceStateReason reason;
 +
 +	if (new_enabled == priv->enabled)
 +		return;
 +
 +	if (new_enabled) {
 +		new_state = NM_MODEM_STATE_REGISTERED;
-+		reason = NM_DEVICE_STATE_REASON_NONE;
 +		ofono_read_contexts (self);
 +	} else {
 +		new_state = NM_MODEM_STATE_SEARCHING;
-+		reason = NM_DEVICE_STATE_REASON_MODEM_NO_CARRIER;
 +	}
 +
 +	nm_modem_set_state (NM_MODEM (self),

=== 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 @@
 ===================================================================
 --- /dev/null
 +++ b/src/settings/plugins/ofono/plugin.c
-@@ -0,0 +1,738 @@
+@@ -0,0 +1,742 @@
 +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
 +
 +/* Ofono modem settings service
@@ -677,6 +677,10 @@
 +
 +			/* add the new connection */
 +			exported = nm_ofono_connection_new (context);
++
++			/* Lower disabled timer to 30s */
++			nm_settings_connection_set_reset_retries_timeout (NM_SETTINGS_CONNECTION (exported), 30);
++
 +			setting = nm_connection_get_setting_connection (NM_CONNECTION (exported));
 +			g_object_set (setting, NM_SETTING_CONNECTION_AUTOCONNECT, TRUE, NULL);
 +			if (exported) {

=== 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 @@
+Author: Tony Espy <[e-mail]>
+Subject: Introduce NMPolicy delay for modem activate retries
+
+This patch introduces a 5s delay between retry activations
+for modem devices.  Otherwise, the activation attempts can
+flood the modem all at once, and then trigger the reset_retries
+timeout ( which defaults to 300s ).
+
+Bug: https://bugs.launchpad.net/ubuntu/+source/network-manager/+bug/1461593
+
+---
+
+Index: b/src/nm-policy.c
+===================================================================
+--- a/src/nm-policy.c
++++ b/src/nm-policy.c
+@@ -1229,7 +1229,7 @@ sleeping_changed (NMManager *manager, GP
+ }
+ 
+ static void
+-schedule_activate_check (NMPolicy *policy, NMDevice *device)
++schedule_activate_check (NMPolicy *policy, NMDevice *device, guint delay)
+ {
+ 	NMPolicyPrivate *priv = NM_POLICY_GET_PRIVATE (policy);
+ 	ActivateData *data;
+@@ -1258,7 +1258,12 @@ schedule_activate_check (NMPolicy *polic
+ 	data = g_malloc0 (sizeof (ActivateData));
+ 	data->policy = policy;
+ 	data->device = g_object_ref (device);
+-	data->autoactivate_id = g_idle_add (auto_activate_device, data);
++
++	if (delay)
++		data->autoactivate_id = g_timeout_add_seconds (delay, auto_activate_device, data);
++	else
++		data->autoactivate_id = g_idle_add (auto_activate_device, data);
++
+ 	priv->pending_activation_checks = g_slist_append (priv->pending_activation_checks, data);
+ }
+ 
+@@ -1438,6 +1443,7 @@ device_state_changed (NMDevice *device,
+ 	NMIP4Config *ip4_config;
+ 	NMIP6Config *ip6_config;
+ 	NMSettingConnection *s_con = NULL;
++	guint delay = 0;
+ 
+ 	switch (new_state) {
+ 	case NM_DEVICE_STATE_FAILED:
+@@ -1455,7 +1461,7 @@ device_state_changed (NMDevice *device,
+ 
+ 				nm_settings_connection_set_autoconnect_blocked_reason (connection, NM_DEVICE_STATE_REASON_NO_SECRETS);
+ 			} else if (tries > 0) {
+-				nm_log_dbg (LOGD_DEVICE, "Connection '%s' failed to autoconnect; %d tries left",
++				nm_log_info (LOGD_DEVICE, "Connection '%s' failed to autoconnect; %d tries left",
+ 				            nm_connection_get_id (NM_CONNECTION (connection)), tries);
+ 				nm_settings_connection_set_autoconnect_retries (connection, tries - 1);
+ 			}
+@@ -1466,9 +1472,14 @@ device_state_changed (NMDevice *device,
+ 				/* Schedule a handler to reset retries count */
+ 				if (!priv->reset_retries_id) {
+ 					gint32 retry_time = nm_settings_connection_get_autoconnect_retry_time (connection);
++					gint32 actual_time = MAX (0, retry_time - nm_utils_get_monotonic_timestamp_s ());
++
++					nm_log_info (LOGD_DEVICE, "Disabling autoconnect for connection '%s'; setting retry of %d.",
++					             nm_connection_get_id (NM_CONNECTION (connection)), actual_time);
++
+ 
+ 					g_warn_if_fail (retry_time != 0);
+-					priv->reset_retries_id = g_timeout_add_seconds (MAX (0, retry_time - nm_utils_get_monotonic_timestamp_s ()), reset_connections_retries, policy);
++					priv->reset_retries_id = g_timeout_add_seconds (actual_time, reset_connections_retries, policy);
+ 				}
+ 			}
+ 			nm_connection_clear_secrets (NM_CONNECTION (connection));
+@@ -1532,7 +1543,18 @@ device_state_changed (NMDevice *device,
+ 			update_routing_and_dns (policy, FALSE);
+ 
+ 		/* Device is now available for auto-activation */
+-		schedule_activate_check (policy, device);
++
++		if (nm_device_get_device_type (device) == NM_DEVICE_TYPE_MODEM)
++			delay = 5;
++
++		if (connection)
++			nm_log_info (LOGD_DEVICE, "Connection '%s' disconnected, scheduling activate_check in %u seconds.",
++			             nm_connection_get_id (NM_CONNECTION (connection)), delay);
++		else
++			nm_log_info (LOGD_DEVICE, "Device has no connection; scheduling activate_check in %u seconds.",
++			             delay);
++
++		schedule_activate_check (policy, device, delay);
+ 		break;
+ 
+ 	case NM_DEVICE_STATE_PREPARE:
+@@ -1642,13 +1664,13 @@ device_autoconnect_changed (NMDevice *de
+                             gpointer user_data)
+ {
+ 	if (nm_device_get_autoconnect (device))
+-		schedule_activate_check ((NMPolicy *) user_data, device);
++		schedule_activate_check ((NMPolicy *) user_data, device, 0);
+ }
+ 
+ static void
+ device_recheck_auto_activate (NMDevice *device, gpointer user_data)
+ {
+-	schedule_activate_check (NM_POLICY (user_data), device);
++	schedule_activate_check (NM_POLICY (user_data), device, 0);
+ }
+ 
+ typedef struct {
+@@ -1845,7 +1867,7 @@ schedule_activate_all (NMPolicy *policy)
+ 	const GSList *iter;
+ 
+ 	for (iter = nm_manager_get_devices (priv->manager); iter; iter = g_slist_next (iter))
+-		schedule_activate_check (policy, NM_DEVICE (iter->data));
++		schedule_activate_check (policy, NM_DEVICE (iter->data), 0);
+ }
+ 
+ static void

=== 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 @@
+Author: Tony Espy <[e-mail]>
+Subject: Add new NMSettingsConnection reset_retries get/set methods
+
+This patch adds get/set methods to NMSettingsConnection for the
+reset_retries_timeout.  This allows sub-classes to override the
+default setting ( 300s ).
+
+Bug: https://bugs.launchpad.net/ubuntu/+source/network-manager/+bug/1461593
+
+---
+Index: network-manager-0.9.10.0/src/settings/nm-settings-connection.c
+===================================================================
+--- network-manager-0.9.10.0.orig/src/settings/nm-settings-connection.c
++++ network-manager-0.9.10.0/src/settings/nm-settings-connection.c
+@@ -132,6 +132,7 @@ typedef struct {
+ 
+ 	int autoconnect_retries;
+ 	gint32 autoconnect_retry_time;
++	int reset_retries_timeout;
+ 	NMDeviceStateReason autoconnect_blocked_reason;
+ 
+ } NMSettingsConnectionPrivate;
+@@ -1922,6 +1923,19 @@ nm_settings_connection_get_autoconnect_r
+ 	return NM_SETTINGS_CONNECTION_GET_PRIVATE (connection)->autoconnect_retries;
+ }
+ 
++int
++nm_settings_connection_get_reset_retries_timeout (NMSettingsConnection *connection)
++{
++	return NM_SETTINGS_CONNECTION_GET_PRIVATE (connection)->reset_retries_timeout;
++}
++
++void
++nm_settings_connection_set_reset_retries_timeout (NMSettingsConnection *connection,
++                                                  int timeout)
++{
++	NM_SETTINGS_CONNECTION_GET_PRIVATE (connection)->reset_retries_timeout = timeout;
++}
++
+ void
+ nm_settings_connection_set_autoconnect_retries (NMSettingsConnection *connection,
+                                                 int retries)
+@@ -1932,7 +1946,7 @@ nm_settings_connection_set_autoconnect_r
+ 	if (retries)
+ 		priv->autoconnect_retry_time = 0;
+ 	else
+-		priv->autoconnect_retry_time = nm_utils_get_monotonic_timestamp_s () + AUTOCONNECT_RESET_RETRIES_TIMER;
++		priv->autoconnect_retry_time = nm_utils_get_monotonic_timestamp_s () + priv->reset_retries_timeout;
+ }
+ 
+ void
+@@ -2039,6 +2053,7 @@ nm_settings_connection_init (NMSettingsC
+ 
+ 	priv->autoconnect_retries = AUTOCONNECT_RETRIES_DEFAULT;
+ 	priv->autoconnect_blocked_reason = NM_DEVICE_STATE_REASON_NONE;
++	priv->reset_retries_timeout = AUTOCONNECT_RESET_RETRIES_TIMER;
+ 
+ 	g_signal_connect (self, NM_CONNECTION_SECRETS_CLEARED, G_CALLBACK (secrets_cleared_cb), NULL);
+ 	g_signal_connect (self, NM_CONNECTION_CHANGED, G_CALLBACK (changed_cb), GUINT_TO_POINTER (TRUE));
+Index: network-manager-0.9.10.0/src/settings/nm-settings-connection.h
+===================================================================
+--- network-manager-0.9.10.0.orig/src/settings/nm-settings-connection.h
++++ network-manager-0.9.10.0/src/settings/nm-settings-connection.h
+@@ -156,6 +156,9 @@ void nm_settings_connection_reset_autoco
+ 
+ gint32 nm_settings_connection_get_autoconnect_retry_time (NMSettingsConnection *connection);
+ 
++int nm_settings_connection_get_reset_retries_timeout (NMSettingsConnection *connection);
++void nm_settings_connection_set_reset_retries_timeout (NMSettingsConnection *connection, int timeout);
++
+ NMDeviceStateReason nm_settings_connection_get_autoconnect_blocked_reason (NMSettingsConnection *connection);
+ void nm_settings_connection_set_autoconnect_blocked_reason (NMSettingsConnection *connection,
+                                                             NMDeviceStateReason reason);

=== 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 @@
 lp1099983_ignore-p2p-wifi-devices.patch
 prioritize_3g_later.patch
 0001-wwan-add-support-for-using-oFono-as-a-modem-manager.patch
+lp1461593-add-nm-settings-connection-reset-retries-methods.patch
 add_ofono_settings_support.patch
+lp1461593-add-modem-reconnect-delay-to-policy.patch
 
 # killswitch
 ignore_rfkill_if_urfkill_is_present.patch