Merge lp:~phablet-team/network-manager/lp1480877-wifi-rm-dup-scan-signals into lp:~network-manager/network-manager/ubuntu

Proposed by Tony Espy on 2015-09-17
Status: Merged
Approved by: Mathieu Trudel-Lapierre on 2015-09-25
Approved revision: 997
Merged at revision: 998
Proposed branch: lp:~phablet-team/network-manager/lp1480877-wifi-rm-dup-scan-signals
Merge into: lp:~network-manager/network-manager/ubuntu
Diff against target: 202 lines (+121/-33)
2 files modified
debian/changelog (+11/-0)
debian/patches/0002-wifi-cull-the-scan-list-before-signalling-ScanDone-b.patch (+110/-33)
To merge this branch: bzr merge lp:~phablet-team/network-manager/lp1480877-wifi-rm-dup-scan-signals
Reviewer Review Type Date Requested Status
Mathieu Trudel-Lapierre 2015-09-17 Needs Information on 2015-09-24
Manuel de la Peña (community) 2015-09-18 Approve on 2015-09-23
Review via email: mp+271575@code.launchpad.net

Description of the change

This change fixes a problem where duplicate (2-3x) AccessPoint 'LastSeen' PropertiesChanged signals are generated each time a WiFi scan completes. This can be seen by running:

dbus-monitor --system --profile "type='signal',sender='org.freedesktop.NetworkManager'"

...and waiting for a scan to occur. You'll see 2-3 PropsChanged for each AccessPoint after a scan. From then on, you'll only see changes for AccessPoint/0 until the next scan happens.

The change makes cull_scan_list the function responsible for updating an AP's last-seen property, and ensures that scan_done_cb is the only function that calls cull_scan_list. It also removes the bss_updated_cb function, as its only purpose was to update the last-seen property and schedule yet another scanlist cull.

To post a comment you must log in.
Mathieu Trudel-Lapierre (cyphermox) wrote :

Have you discussed this with the upstream developers?

The suggested changes look reasonable, but there may be unforseen consequences to changing the code this way. Have you made sure that scan were still running in a reasonable fashion, even if a scan request failed, for example? It seems to me (from memory) that this would be one case where we might expect beacons to possibly schedule a new scan.

I much less intrusive change would be to simply remove the last-seen update on bss-updated -- it still makes sense to update last-seen for every scan results, otherwise it defeats its purpose... Still, there can be cases where scans fail, or when that value won't be up to date enough.

review: Needs Information
Tony Espy (awe) wrote :

My changes have no effect on whether or not scans are scheduled.

For the gory details see:

https://bugs.launchpad.net/ubuntu-rtm/+source/network-manager/+bug/1480877/comments/41

I checked, and the change that introduced DUP signals in first place was a location services fix ( see: 0002-wifi-cull-the-scan-list-before-signalling-ScanDone-b.patch ), which hasn't been up-streamed, so my changes aren't really directly applicable.

I've test three different phones, and a laptop running wily, and the only time bss_updated_cb is ever fired is after a scan completes. As scan_done_cb always calls cull_scan_list directly, which in turn updates all of the last-seen properties of the know AccessPoints, the removal of bss_updated_cb is basically a no-op.

The removal of the schedule_scan_list call from new_bss_cb again is also predicated on the fact that new_bss_cb only ever gets triggered when a scan happens. As it was the last function that used schedule_scanlist_cull, I removed it and the associated functions/priv member.

Manuel de la Peña (mandel) wrote :

This should not affect the location service and fixes the redundant signals.

review: Approve
Mathieu Trudel-Lapierre (cyphermox) wrote :

Have you made sure that this did not break the signal level change notifications, and that we can still see signal level changes in the indicator on desktop?

review: Needs Information
Tony Espy (awe) wrote :

If you mean, signal changes for the currently connected AP ( AccessPoint/0 ), then yes, I've verified that this doesn't break signal strength changes for the current AP.

These signal strength updates are handled by periodic_update_cb, which fires every six seconds when connected to an AP. This callback in turn calls a function periodic_update which reads the signal strength of the connected AP via NMPlaform and then sets the AP property, which fires a DBus signal.

That said, I've only verified this behavior directly on a phone (krillin stable #25) running the 0.9.10 version of this patch. If you want me to verify it on a wily desktop before we build the silo, I will do so...

Tony Espy (awe) wrote :

Just tested on my Thinkpad T410s running wily. It has an Intel WiFi adapter and is running iwlwifi.

Confirmed that DBus signal strength PropertiesChanged signals for AccessPoint/0 are still fired periodically when associated with an access point. Also confirmed that the indicator sees these signals and responds by changing the WiFi icon according to signal strength.

Testing with version 1.0.4-0ubuntu4~awe3 from my PPA:

https://launchpad.net/~awe/+archive/ubuntu/ppa/+packages

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2015-09-10 18:53:27 +0000
3+++ debian/changelog 2015-09-17 22:30:05 +0000
4@@ -1,3 +1,14 @@
5+network-manager (1.0.4-0ubuntu4) UNRELEASED; urgency=medium
6+
7+ [ Tony Espy ]
8+ * d/p/0002-wifi-cull-the-scan-list-before-signalling-ScanDone-b.patch:
9+ Fix duplicate 'LastSeen' PropertiesChanged signals being generated
10+ after every scan. The 'Last-Seen' property is now only updated when
11+ a scan finishes, and schedule_scanlist_cull is no longer triggered
12+ new_bss_cb or updated_bss_cb.
13+
14+ -- Mathieu Trudel-Lapierre <mathieu-tl@ubuntu.com> Thu, 17 Sep 2015 17:03:06 -0400
15+
16 network-manager (1.0.4-0ubuntu3) wily; urgency=medium
17
18 * New upstream NM release post-feature-freeze. (LP: #1493934)
19
20=== modified file 'debian/patches/0002-wifi-cull-the-scan-list-before-signalling-ScanDone-b.patch'
21--- debian/patches/0002-wifi-cull-the-scan-list-before-signalling-ScanDone-b.patch 2015-08-20 01:07:00 +0000
22+++ debian/patches/0002-wifi-cull-the-scan-list-before-signalling-ScanDone-b.patch 2015-09-17 22:30:05 +0000
23@@ -14,33 +14,70 @@
24 src/devices/wifi/nm-device-wifi.c | 35 +++++++++++++++++++++++++++++------
25 1 file changed, 29 insertions(+), 6 deletions(-)
26
27-Index: b/src/devices/wifi/nm-device-wifi.c
28+Index: network-manager-1.0.4/src/devices/wifi/nm-device-wifi.c
29 ===================================================================
30---- a/src/devices/wifi/nm-device-wifi.c
31-+++ b/src/devices/wifi/nm-device-wifi.c
32-@@ -177,6 +177,10 @@ static void supplicant_iface_notify_scan
33+--- network-manager-1.0.4.orig/src/devices/wifi/nm-device-wifi.c
34++++ network-manager-1.0.4/src/devices/wifi/nm-device-wifi.c
35+@@ -123,7 +123,6 @@ struct _NMDeviceWifiPrivate {
36+ gint32 scheduled_scan_time;
37+ guint8 scan_interval; /* seconds */
38+ guint pending_scan_id;
39+- guint scanlist_cull_id;
40+ gboolean requested_scan;
41+
42+ NMSupplicantManager *sup_mgr;
43+@@ -160,11 +159,6 @@ static void supplicant_iface_new_bss_cb
44+ GVariant *properties,
45+ NMDeviceWifi * self);
46+
47+-static void supplicant_iface_bss_updated_cb (NMSupplicantInterface *iface,
48+- const char *object_path,
49+- GHashTable *properties,
50+- NMDeviceWifi *self);
51+-
52+ static void supplicant_iface_bss_removed_cb (NMSupplicantInterface *iface,
53+ const char *object_path,
54+ NMDeviceWifi *self);
55+@@ -177,7 +171,7 @@ static void supplicant_iface_notify_scan
56 GParamSpec * pspec,
57 NMDeviceWifi * self);
58
59-+static void remove_outstanding_scanlist_cull(NMDeviceWifi *self);
60-+
61+-static void schedule_scanlist_cull (NMDeviceWifi *self);
62 +static gboolean cull_scan_list(NMDeviceWifi *self);
63-+
64- static void schedule_scanlist_cull (NMDeviceWifi *self);
65
66 static gboolean request_wireless_scan (gpointer user_data);
67-@@ -1607,14 +1611,22 @@ supplicant_iface_scan_done_cb (NMSupplic
68+
69+@@ -252,10 +246,6 @@ supplicant_interface_acquire (NMDeviceWi
70+ G_CALLBACK (supplicant_iface_new_bss_cb),
71+ self);
72+ g_signal_connect (priv->sup_iface,
73+- NM_SUPPLICANT_INTERFACE_BSS_UPDATED,
74+- G_CALLBACK (supplicant_iface_bss_updated_cb),
75+- self);
76+- g_signal_connect (priv->sup_iface,
77+ NM_SUPPLICANT_INTERFACE_BSS_REMOVED,
78+ G_CALLBACK (supplicant_iface_bss_removed_cb),
79+ self);
80+@@ -287,11 +277,6 @@ supplicant_interface_release (NMDeviceWi
81+ _LOGD (LOGD_WIFI_SCAN, "reset scanning interval to %d seconds",
82+ priv->scan_interval);
83+
84+- if (priv->scanlist_cull_id) {
85+- g_source_remove (priv->scanlist_cull_id);
86+- priv->scanlist_cull_id = 0;
87+- }
88+-
89+ if (priv->sup_iface) {
90+ remove_supplicant_interface_error_handler (self);
91+
92+@@ -1607,14 +1592,17 @@ supplicant_iface_scan_done_cb (NMSupplic
93
94 _LOGD (LOGD_WIFI_SCAN, "scan %s", success ? "successful" : "failed");
95
96 - g_signal_emit (self, signals[SCAN_DONE], 0, NULL);
97 -
98 - schedule_scan (self, success);
99-+ /* Make sure that scheduled and not yet run scanlist cull
100-+ * requests are aborted.
101-+ */
102-+ remove_outstanding_scanlist_cull (self);
103-
104+-
105 /* Ensure that old APs get removed, which otherwise only
106 * happens when there are new BSSes.
107 */
108@@ -56,7 +93,16 @@
109
110 if (priv->requested_scan) {
111 priv->requested_scan = FALSE;
112-@@ -1786,8 +1798,10 @@ cull_scan_list (NMDeviceWifi *self)
113+@@ -1761,8 +1749,6 @@ cull_scan_list (NMDeviceWifi *self)
114+ GSList *elt;
115+ guint32 removed = 0, total = 0;
116+
117+- priv->scanlist_cull_id = 0;
118+-
119+ _LOGD (LOGD_WIFI_SCAN, "checking scan list for outdated APs");
120+
121+ /* Walk the access point list and remove any access points older than
122+@@ -1786,8 +1772,10 @@ cull_scan_list (NMDeviceWifi *self)
123 * supplicant in the last scan.
124 */
125 if ( nm_ap_get_supplicant_path (ap)
126@@ -68,27 +114,58 @@
127
128 last_seen = nm_ap_get_last_seen (ap);
129 if (!last_seen || last_seen + prune_interval_s < boottime_now)
130-@@ -1824,13 +1838,22 @@ cull_scan_list (NMDeviceWifi *self)
131+@@ -1824,17 +1812,6 @@ cull_scan_list (NMDeviceWifi *self)
132 }
133
134 static void
135 -schedule_scanlist_cull (NMDeviceWifi *self)
136-+remove_outstanding_scanlist_cull (NMDeviceWifi *self)
137- {
138- NMDeviceWifiPrivate *priv = NM_DEVICE_WIFI_GET_PRIVATE (self);
139-
140- /* Cull the scan list after the last request for it has come in */
141- if (priv->scanlist_cull_id)
142- g_source_remove (priv->scanlist_cull_id);
143-+}
144-+
145-+static void
146-+schedule_scanlist_cull (NMDeviceWifi *self)
147-+{
148-+ NMDeviceWifiPrivate *priv = NM_DEVICE_WIFI_GET_PRIVATE (self);
149-+
150-+ remove_outstanding_scanlist_cull(self);
151-+
152- priv->scanlist_cull_id = g_timeout_add_seconds (4, (GSourceFunc) cull_scan_list, self);
153+-{
154+- NMDeviceWifiPrivate *priv = NM_DEVICE_WIFI_GET_PRIVATE (self);
155+-
156+- /* Cull the scan list after the last request for it has come in */
157+- if (priv->scanlist_cull_id)
158+- g_source_remove (priv->scanlist_cull_id);
159+- priv->scanlist_cull_id = g_timeout_add_seconds (4, (GSourceFunc) cull_scan_list, self);
160+-}
161+-
162+-static void
163+ supplicant_iface_new_bss_cb (NMSupplicantInterface *iface,
164+ const char *object_path,
165+ GVariant *properties,
166+@@ -1863,36 +1840,6 @@ supplicant_iface_new_bss_cb (NMSupplican
167+ g_object_unref (ap);
168+ } else
169+ _LOGW (LOGD_WIFI_SCAN, "invalid AP properties received");
170+-
171+- /* Remove outdated access points */
172+- schedule_scanlist_cull (self);
173+-}
174+-
175+-static void
176+-supplicant_iface_bss_updated_cb (NMSupplicantInterface *iface,
177+- const char *object_path,
178+- GHashTable *properties,
179+- NMDeviceWifi *self)
180+-{
181+- NMDeviceState state;
182+- NMAccessPoint *ap;
183+-
184+- g_return_if_fail (self != NULL);
185+- g_return_if_fail (object_path != NULL);
186+- g_return_if_fail (properties != NULL);
187+-
188+- /* Ignore new APs when unavailable or unamnaged */
189+- state = nm_device_get_state (NM_DEVICE (self));
190+- if (state <= NM_DEVICE_STATE_UNAVAILABLE)
191+- return;
192+-
193+- /* Update the AP's last-seen property */
194+- ap = get_ap_by_supplicant_path (self, object_path);
195+- if (ap)
196+- nm_ap_update_last_seen (ap);
197+-
198+- /* Remove outdated access points */
199+- schedule_scanlist_cull (self);
200 }
201
202+ static void

Subscribers

People subscribed via source and target branches