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

[Tony Espy]

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.

[Mathieu Trudel-Lapierre (cyphermox)]

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

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

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

review: Approve

[Mathieu Trudel-Lapierre (cyphermox)]

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

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

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

=== modified file 'debian/changelog'
--- debian/changelog	2015-09-10 18:53:27 +0000
+++ debian/changelog	2015-09-17 22:30:05 +0000
@@ -1,3 +1,14 @@
+network-manager (1.0.4-0ubuntu4) UNRELEASED; urgency=medium
+
+  [ Tony Espy ]
+  * d/p/0002-wifi-cull-the-scan-list-before-signalling-ScanDone-b.patch:
+    Fix duplicate 'LastSeen' PropertiesChanged signals being generated
+    after every scan. The 'Last-Seen' property is now only updated when
+    a scan finishes, and schedule_scanlist_cull is no longer triggered
+    new_bss_cb or updated_bss_cb.
+
+ -- Mathieu Trudel-Lapierre <[e-mail]>  Thu, 17 Sep 2015 17:03:06 -0400
+
 network-manager (1.0.4-0ubuntu3) wily; urgency=medium
 
   * New upstream NM release post-feature-freeze. (LP: #1493934)

=== modified file 'debian/patches/0002-wifi-cull-the-scan-list-before-signalling-ScanDone-b.patch'
--- debian/patches/0002-wifi-cull-the-scan-list-before-signalling-ScanDone-b.patch	2015-08-20 01:07:00 +0000
+++ debian/patches/0002-wifi-cull-the-scan-list-before-signalling-ScanDone-b.patch	2015-09-17 22:30:05 +0000
@@ -14,33 +14,70 @@
  src/devices/wifi/nm-device-wifi.c |   35 +++++++++++++++++++++++++++++------
  1 file changed, 29 insertions(+), 6 deletions(-)
 
-Index: b/src/devices/wifi/nm-device-wifi.c
+Index: network-manager-1.0.4/src/devices/wifi/nm-device-wifi.c
 ===================================================================
---- a/src/devices/wifi/nm-device-wifi.c
-+++ b/src/devices/wifi/nm-device-wifi.c
-@@ -177,6 +177,10 @@ static void supplicant_iface_notify_scan
+--- network-manager-1.0.4.orig/src/devices/wifi/nm-device-wifi.c
++++ network-manager-1.0.4/src/devices/wifi/nm-device-wifi.c
+@@ -123,7 +123,6 @@ struct _NMDeviceWifiPrivate {
+ 	gint32            scheduled_scan_time;
+ 	guint8            scan_interval; /* seconds */
+ 	guint             pending_scan_id;
+-	guint             scanlist_cull_id;
+ 	gboolean          requested_scan;
+ 
+ 	NMSupplicantManager   *sup_mgr;
+@@ -160,11 +159,6 @@ static void supplicant_iface_new_bss_cb
+                                          GVariant *properties,
+                                          NMDeviceWifi * self);
+ 
+-static void supplicant_iface_bss_updated_cb (NMSupplicantInterface *iface,
+-                                             const char *object_path,
+-                                             GHashTable *properties,
+-                                             NMDeviceWifi *self);
+-
+ static void supplicant_iface_bss_removed_cb (NMSupplicantInterface *iface,
+                                              const char *object_path,
+                                              NMDeviceWifi *self);
+@@ -177,7 +171,7 @@ static void supplicant_iface_notify_scan
                                                   GParamSpec * pspec,
                                                   NMDeviceWifi * self);
  
-+static void remove_outstanding_scanlist_cull(NMDeviceWifi *self);
-+
+-static void schedule_scanlist_cull (NMDeviceWifi *self);
 +static gboolean cull_scan_list(NMDeviceWifi *self);
-+
- static void schedule_scanlist_cull (NMDeviceWifi *self);
  
  static gboolean request_wireless_scan (gpointer user_data);
-@@ -1607,14 +1611,22 @@ supplicant_iface_scan_done_cb (NMSupplic
+ 
+@@ -252,10 +246,6 @@ supplicant_interface_acquire (NMDeviceWi
+ 	                  G_CALLBACK (supplicant_iface_new_bss_cb),
+ 	                  self);
+ 	g_signal_connect (priv->sup_iface,
+-	                  NM_SUPPLICANT_INTERFACE_BSS_UPDATED,
+-	                  G_CALLBACK (supplicant_iface_bss_updated_cb),
+-	                  self);
+-	g_signal_connect (priv->sup_iface,
+ 	                  NM_SUPPLICANT_INTERFACE_BSS_REMOVED,
+ 	                  G_CALLBACK (supplicant_iface_bss_removed_cb),
+ 	                  self);
+@@ -287,11 +277,6 @@ supplicant_interface_release (NMDeviceWi
+ 	_LOGD (LOGD_WIFI_SCAN, "reset scanning interval to %d seconds",
+ 	       priv->scan_interval);
+ 
+-	if (priv->scanlist_cull_id) {
+-		g_source_remove (priv->scanlist_cull_id);
+-		priv->scanlist_cull_id = 0;
+-	}
+-
+ 	if (priv->sup_iface) {
+ 		remove_supplicant_interface_error_handler (self);
+ 
+@@ -1607,14 +1592,17 @@ supplicant_iface_scan_done_cb (NMSupplic
  
  	_LOGD (LOGD_WIFI_SCAN, "scan %s", success ? "successful" : "failed");
  
 -	g_signal_emit (self, signals[SCAN_DONE], 0, NULL);
 -
 -	schedule_scan (self, success);
-+	/* Make sure that scheduled and not yet run scanlist cull
-+	 * requests are aborted.
-+	 */
-+	remove_outstanding_scanlist_cull (self);
- 
+-
  	/* Ensure that old APs get removed, which otherwise only
  	 * happens when there are new BSSes.
  	 */
@@ -56,7 +93,16 @@
  
  	if (priv->requested_scan) {
  		priv->requested_scan = FALSE;
-@@ -1786,8 +1798,10 @@ cull_scan_list (NMDeviceWifi *self)
+@@ -1761,8 +1749,6 @@ cull_scan_list (NMDeviceWifi *self)
+ 	GSList *elt;
+ 	guint32 removed = 0, total = 0;
+ 
+-	priv->scanlist_cull_id = 0;
+-
+ 	_LOGD (LOGD_WIFI_SCAN, "checking scan list for outdated APs");
+ 
+ 	/* Walk the access point list and remove any access points older than
+@@ -1786,8 +1772,10 @@ cull_scan_list (NMDeviceWifi *self)
  		 * supplicant in the last scan.
  		 */
  		if (   nm_ap_get_supplicant_path (ap)
@@ -68,27 +114,58 @@
  
  		last_seen = nm_ap_get_last_seen (ap);
  		if (!last_seen || last_seen + prune_interval_s < boottime_now)
-@@ -1824,13 +1838,22 @@ cull_scan_list (NMDeviceWifi *self)
+@@ -1824,17 +1812,6 @@ cull_scan_list (NMDeviceWifi *self)
  }
  
  static void
 -schedule_scanlist_cull (NMDeviceWifi *self)
-+remove_outstanding_scanlist_cull (NMDeviceWifi *self)
- {
- 	NMDeviceWifiPrivate *priv = NM_DEVICE_WIFI_GET_PRIVATE (self);
- 
- 	/* Cull the scan list after the last request for it has come in */
- 	if (priv->scanlist_cull_id)
- 		g_source_remove (priv->scanlist_cull_id);
-+}
-+
-+static void
-+schedule_scanlist_cull (NMDeviceWifi *self)
-+{
-+	NMDeviceWifiPrivate *priv = NM_DEVICE_WIFI_GET_PRIVATE (self);
-+
-+	remove_outstanding_scanlist_cull(self);
-+
- 	priv->scanlist_cull_id = g_timeout_add_seconds (4, (GSourceFunc) cull_scan_list, self);
+-{
+-	NMDeviceWifiPrivate *priv = NM_DEVICE_WIFI_GET_PRIVATE (self);
+-
+-	/* Cull the scan list after the last request for it has come in */
+-	if (priv->scanlist_cull_id)
+-		g_source_remove (priv->scanlist_cull_id);
+-	priv->scanlist_cull_id = g_timeout_add_seconds (4, (GSourceFunc) cull_scan_list, self);
+-}
+-
+-static void
+ supplicant_iface_new_bss_cb (NMSupplicantInterface *iface,
+                              const char *object_path,
+                              GVariant *properties,
+@@ -1863,36 +1840,6 @@ supplicant_iface_new_bss_cb (NMSupplican
+ 		g_object_unref (ap);
+ 	} else
+ 		_LOGW (LOGD_WIFI_SCAN, "invalid AP properties received");
+-
+-	/* Remove outdated access points */
+-	schedule_scanlist_cull (self);
+-}
+-
+-static void
+-supplicant_iface_bss_updated_cb (NMSupplicantInterface *iface,
+-                                 const char *object_path,
+-                                 GHashTable *properties,
+-                                 NMDeviceWifi *self)
+-{
+-	NMDeviceState state;
+-	NMAccessPoint *ap;
+-
+-	g_return_if_fail (self != NULL);
+-	g_return_if_fail (object_path != NULL);
+-	g_return_if_fail (properties != NULL);
+-
+-	/* Ignore new APs when unavailable or unamnaged */
+-	state = nm_device_get_state (NM_DEVICE (self));
+-	if (state <= NM_DEVICE_STATE_UNAVAILABLE)
+-		return;
+-
+-	/* Update the AP's last-seen property */
+-	ap = get_ap_by_supplicant_path (self, object_path);
+-	if (ap)
+-		nm_ap_update_last_seen (ap);
+-
+-	/* Remove outdated access points */
+-	schedule_scanlist_cull (self);
  }
  
+ static void