[Merge] lp:~phablet-team/network-manager/lp1461593-wily into lp:~network-manager/network-manager/ubuntu
Mathieu Trudel-Lapierre
mathieu.tl at gmail.com
Thu Jul 16 03:20:06 UTC 2015
Review: Needs Fixing
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.
Diff comments:
>
> === 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-15 20:37:00 +0000
> @@ -0,0 +1,105 @@
Looks fine, but please add DEP-3 patch tags[1]. You can compare with the other patches (and if some are missing it, it's an error, which we should fix asap. Mostly just From:, Subject: would be sufficient. Since you wrote the patches, Author/From is probably better than Origin:.
[1] http://dep.debian.net/deps/dep3/
> +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-15 20:37:00 +0000
> @@ -0,0 +1,63 @@
As above, new patches should carry DEP-3 patch tags if possible. In addition, this would be "enforced" if/when we start to use a git flattened workflow.
> +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-15 20:37:00 +0000
> @@ -74,3 +75,4 @@
> git_route_fixes_part1_207ab013.patch
> git_route_fixes_part2_529591d8.patch
> git_avoid_conflict_reinstall_dev_route_e439478c.patch
> +lp1461593-add-modem-reconnect-delay-to-policy.patch
Since this is for ofono stuff, can you reorder it to put it with the other ofono stuff? It probably belongs towards the top of the "ofono" section, along with lp1461593-add-nm-settings-connection-reset-retries-methods.patch.
--
https://code.launchpad.net/~phablet-team/network-manager/lp1461593-wily/+merge/264894
Your team Ubuntu Phablet Team is subscribed to branch lp:~phablet-team/network-manager/lp1435776-wily.
More information about the Ubuntu-reviews
mailing list