wifi: fix and improve handling of Wi-Fi scanning state

In NMSupplicantInterface, we determine whether we currently are scanning
both on the "scanning" supplicant state and the "Scanning" property.

Extend that. If we currently are scanning and are about to clear the
scanning state, then pretend to still scan as long as we are still
initializing BSS instances. What otherwise happens is that we declare
that we finished scanning, but the NMWifiAP instances are not yet ready.
The result is, that `nmcli device wifi` will already start printing the
scan list, when we didn't yet fully process all access points.

Now, _notify_maybe_scanning() will delay switching the scanning state to
disabled, as long as we have BSS initializing (bss_initializing_lst_head).

Also, ignore the "ScanDone" signal. It's redundant to the "Scanning"
property anyway.

Also, only set priv->last_scan_msec when we switch the scanning state
off. That is the right (and only) place where the last-scan timestamp
needs updating.

3 files changed, 72 insertions, 81 deletions

diff --git a/src/devices/wifi/nm-device-wifi.c b/src/devices/wifi/nm-device-wifi.c
index ed9d5c3df8..9ea16f25d7 100644
--- a/src/devices/wifi/nm-device-wifi.c
+++ b/src/devices/wifi/nm-device-wifi.c
@@ -165,17 +165,10 @@ static void supplicant_iface_bss_changed_cb (NMSupplicantInterface *iface,
                                              gboolean is_present,
                                              NMDeviceWifi *self);
 
-static void supplicant_iface_scan_done_cb (NMSupplicantInterface * iface,
-                                           NMDeviceWifi * self);
-
 static void supplicant_iface_wps_credentials_cb (NMSupplicantInterface *iface,
                                                  GVariant *credentials,
                                                  NMDeviceWifi *self);
 
-static void supplicant_iface_notify_scanning_cb (NMSupplicantInterface * iface,
-                                                 GParamSpec * pspec,
-                                                 NMDeviceWifi * self);
-
 static void supplicant_iface_notify_current_bss (NMSupplicantInterface *iface,
                                                  GParamSpec *pspec,
                                                  NMDeviceWifi *self);
@@ -184,6 +177,10 @@ static void supplicant_iface_notify_p2p_available (NMSupplicantInterface *iface,
                                                    GParamSpec *pspec,
                                                    NMDeviceWifi *self);
 
+static void _requested_scan_set (NMDeviceWifi *self, gboolean value);
+
+static void periodic_update (NMDeviceWifi *self);
+
 static void request_wireless_scan (NMDeviceWifi *self,
                                    gboolean periodic,
                                    gboolean force_if_scanning,
@@ -220,6 +217,7 @@ _notify_scanning (NMDeviceWifi *self)
 {
 	NMDeviceWifiPrivate *priv = NM_DEVICE_WIFI_GET_PRIVATE (self);
 	gboolean scanning;
+	gboolean last_scan_changed = FALSE;
 
 	scanning =    priv->sup_iface
 	           && nm_supplicant_interface_get_scanning (priv->sup_iface);
@@ -229,7 +227,36 @@ _notify_scanning (NMDeviceWifi *self)
 
 	_LOGD (LOGD_WIFI, "wifi-scan: scanning-state: %s", scanning ? "scanning" : "idle");
 	priv->is_scanning = scanning;
-	_notify (self, PROP_SCANNING);
+
+	if (   !scanning
+	    || priv->last_scan_msec == 0) {
+		last_scan_changed = TRUE;
+		priv->last_scan_msec = nm_utils_get_monotonic_timestamp_msec ();
+	}
+
+	schedule_scan (self, TRUE);
+
+	nm_gobject_notify_together (self,
+	                            PROP_SCANNING,
+	                              last_scan_changed
+	                            ? PROP_LAST_SCAN
+	                            : PROP_0);
+
+	if (!priv->is_scanning) {
+		_requested_scan_set (self, FALSE);
+		if (nm_device_get_state (NM_DEVICE (self)) == NM_DEVICE_STATE_ACTIVATED) {
+			/* Run a quick update of current AP when coming out of a scan */
+			periodic_update (self);
+		}
+	}
+}
+
+static void
+supplicant_iface_notify_scanning_cb (NMSupplicantInterface *iface,
+                                     GParamSpec *pspec,
+                                     NMDeviceWifi *self)
+{
+	_notify_scanning (self);
 }
 
 static gboolean
@@ -280,10 +307,6 @@ supplicant_interface_acquire_cb (NMSupplicantManager *supplicant_manager,
 	                  G_CALLBACK (supplicant_iface_bss_changed_cb),
 	                  self);
 	g_signal_connect (priv->sup_iface,
-	                  NM_SUPPLICANT_INTERFACE_SCAN_DONE,
-	                  G_CALLBACK (supplicant_iface_scan_done_cb),
-	                  self);
-	g_signal_connect (priv->sup_iface,
 	                  NM_SUPPLICANT_INTERFACE_WPS_CREDENTIALS,
 	                  G_CALLBACK (supplicant_iface_wps_credentials_cb),
 	                  self);
@@ -1236,7 +1259,12 @@ _nm_device_wifi_request_scan (NMDeviceWifi *self,
 
 	last_scan = nm_supplicant_interface_get_last_scan (priv->sup_iface);
 	if (   last_scan > 0
-	    && (nm_utils_get_monotonic_timestamp_msec () - last_scan) < 10 * NM_UTILS_MSEC_PER_SEC) {
+	    && nm_utils_get_monotonic_timestamp_msec () < last_scan + (10 * NM_UTILS_MSEC_PER_SEC)) {
+		/* FIXME: we really should not outright reject a scan request in this case. We should
+		 * ensure to start a scan request soon, possibly with rate limiting. And there is no
+		 * need to tell the caller that we aren't going to scan...
+		 *
+		 * Same above, if we are currently scanning... */
 		g_dbus_method_invocation_return_error_literal (invocation,
 		                                               NM_DEVICE_ERROR,
 		                                               NM_DEVICE_ERROR_NOT_ALLOWED,
@@ -1505,21 +1533,6 @@ schedule_scan (NMDeviceWifi *self, gboolean backoff)
 	}
 }
 
-static void
-supplicant_iface_scan_done_cb (NMSupplicantInterface *iface,
-                               NMDeviceWifi *self)
-{
-	NMDeviceWifiPrivate *priv = NM_DEVICE_WIFI_GET_PRIVATE (self);
-
-	_LOGD (LOGD_WIFI, "wifi-scan: scan-done callback");
-
-	priv->last_scan_msec = nm_utils_get_monotonic_timestamp_msec ();
-	_notify (self, PROP_LAST_SCAN);
-	schedule_scan (self, TRUE);
-
-	_requested_scan_set (self, FALSE);
-}
-
 /****************************************************************************
  * WPA Supplicant control stuff
  *
@@ -2212,19 +2225,6 @@ supplicant_iface_assoc_cb (NMSupplicantInterface *iface,
 }
 
 static void
-supplicant_iface_notify_scanning_cb (NMSupplicantInterface *iface,
-                                     GParamSpec *pspec,
-                                     NMDeviceWifi *self)
-{
-	_notify_scanning (self);
-
-	/* Run a quick update of current AP when coming out of a scan */
-	if (   !NM_DEVICE_WIFI_GET_PRIVATE (self)->is_scanning
-	    && nm_device_get_state (NM_DEVICE (self)) == NM_DEVICE_STATE_ACTIVATED)
-		periodic_update (self);
-}
-
-static void
 supplicant_iface_notify_current_bss (NMSupplicantInterface *iface,
                                      GParamSpec *pspec,
                                      NMDeviceWifi *self)
diff --git a/src/supplicant/nm-supplicant-interface.c b/src/supplicant/nm-supplicant-interface.c
index ec1afa25ec..b72962804f 100644
--- a/src/supplicant/nm-supplicant-interface.c
+++ b/src/supplicant/nm-supplicant-interface.c
@@ -59,7 +59,6 @@ enum {
 	STATE,                   /* change in the interface's state */
 	BSS_CHANGED,             /* a new BSS appeared, was updated, or was removed. */
 	PEER_CHANGED,            /* a new Peer appeared, was updated, or was removed */
-	SCAN_DONE,               /* wifi scan is complete */
 	WPS_CREDENTIALS,         /* WPS credentials received */
 	GROUP_STARTED,           /* a new Group (interface) was created */
 	GROUP_FINISHED,          /* a Group (interface) has been finished */
@@ -451,6 +450,22 @@ _notify_maybe_scanning (NMSupplicantInterface *self)
 	if (priv->scanning_cached == scanning)
 		return;
 
+	if (   !scanning
+	    && !c_list_is_empty (&priv->bss_initializing_lst_head)) {
+		/* we would change state to indicate we no longer scan. However,
+		 * we still have BSS instances to be initialized. Delay the
+		 * state change further. */
+		return;
+	}
+
+	_LOGT ("scanning: %s", scanning ? "yes" : "no");
+
+	if (!scanning)
+		priv->last_scan_msec = nm_utils_get_monotonic_timestamp_msec ();
+	else {
+		/* while we are scanning, we set the timestamp to -1. */
+		priv->last_scan_msec = -1;
+	}
 	priv->scanning_cached = scanning;
 	_notify (self, PROP_SCANNING);
 }
@@ -514,6 +529,9 @@ _bss_info_changed_emit (NMSupplicantInterface *self,
                         NMSupplicantBssInfo *bss_info,
                         gboolean is_present)
 {
+	_LOGT ("BSS %s %s",
+	       bss_info->bss_path->str,
+	       is_present ? "updated" : "deleted");
 	g_signal_emit (self,
 	               signals[BSS_CHANGED],
 	               0,
@@ -744,6 +762,8 @@ _bss_info_get_all_cb (GVariant *result,
 	_bss_info_properties_changed (self, bss_info, properties, TRUE);
 
 	_starting_check_ready (self);
+
+	_notify_maybe_scanning (self);
 }
 
 static void
@@ -1083,6 +1103,8 @@ set_state_down (NMSupplicantInterface *self,
 	_remove_network (self);
 
 	nm_clear_pointer (&priv->current_bss, nm_ref_string_unref);
+
+	_notify_maybe_scanning (self);
 }
 
 static void
@@ -1105,9 +1127,6 @@ set_state (NMSupplicantInterface *self, NMSupplicantInterfaceState new_state)
 	       nm_supplicant_interface_state_to_string (new_state),
 	       nm_supplicant_interface_state_to_string (priv->state));
 
-	if (old_state == NM_SUPPLICANT_INTERFACE_STATE_SCANNING)
-		priv->last_scan_msec = nm_utils_get_monotonic_timestamp_msec ();
-
 	priv->state = new_state;
 
 	_emit_signal_state (self,
@@ -1137,16 +1156,9 @@ nm_supplicant_interface_get_scanning (NMSupplicantInterface *self)
 gint64
 nm_supplicant_interface_get_last_scan (NMSupplicantInterface *self)
 {
-	NMSupplicantInterfacePrivate *priv;
-
 	g_return_val_if_fail (NM_IS_SUPPLICANT_INTERFACE (self), FALSE);
 
-	priv = NM_SUPPLICANT_INTERFACE_GET_PRIVATE (self);
-
-	/* returns -1 if we are currently scanning. */
-	return   priv->scanning_cached
-	       ? -1
-	       : priv->last_scan_msec;
+	return NM_SUPPLICANT_INTERFACE_GET_PRIVATE (self)->last_scan_msec;
 }
 
 #define MATCH_PROPERTY(p, n, v, t) (!strcmp (p, n) && g_variant_is_of_type (v, t))
@@ -1777,13 +1789,8 @@ _properties_changed_main (NMSupplicantInterface *self,
 		g_variant_unref (v_v);
 	}
 
-	if (nm_g_variant_lookup (properties, "Scanning", "b", &v_b)) {
-		if (priv->scanning_cached != (!!v_b)) {
-			if (priv->scanning_cached)
-				priv->last_scan_msec = nm_utils_get_monotonic_timestamp_msec ();
-			priv->scanning_cached = v_b;
-		}
-	}
+	if (nm_g_variant_lookup (properties, "Scanning", "b", &v_b))
+		priv->scanning_property = v_b;
 
 	if (nm_g_variant_lookup (properties, "Ifname", "&s", &v_s)) {
 		if (nm_utils_strdup_reset (&priv->ifname, v_s))
@@ -1870,6 +1877,8 @@ _properties_changed_main (NMSupplicantInterface *self,
 
 	if (do_set_state)
 		set_state (self, priv->supp_state);
+
+	_notify_maybe_scanning (self);
 }
 
 static void
@@ -2717,16 +2726,6 @@ _signal_handle (NMSupplicantInterface *self,
 		if (!priv->is_ready_main)
 			return;
 
-		if (nm_streq (signal_name, "ScanDone")) {
-			priv->last_scan_msec = nm_utils_get_monotonic_timestamp_msec ();
-			_LOGT ("ScanDone signal received");
-			if (priv->state > NM_SUPPLICANT_INTERFACE_STATE_STARTING) {
-				nm_assert (priv->state < NM_SUPPLICANT_INTERFACE_STATE_DOWN);
-				g_signal_emit (self, signals[SCAN_DONE], 0);
-			}
-			return;
-		}
-
 		if (nm_streq (signal_name, "BSSAdded")) {
 			if (!g_variant_is_of_type (parameters, G_VARIANT_TYPE ("(oa{sv})")))
 				return;
@@ -2874,11 +2873,11 @@ _signal_cb (GDBusConnection *connection,
 	NMSupplicantInterfacePrivate *priv = NM_SUPPLICANT_INTERFACE_GET_PRIVATE (self);
 
 	priv->starting_pending_count++;
-
 	_signal_handle (self, signal_interface_name, signal_name, parameters);
-
 	priv->starting_pending_count--;
 	_starting_check_ready (self);
+
+	_notify_maybe_scanning (self);
 }
 
 /*****************************************************************************/
@@ -3003,6 +3002,7 @@ nm_supplicant_interface_init (NMSupplicantInterface * self)
 
 	priv->state = NM_SUPPLICANT_INTERFACE_STATE_STARTING;
 	priv->supp_state = NM_SUPPLICANT_INTERFACE_STATE_INVALID;
+	priv->last_scan_msec = -1;
 
 	c_list_init (&self->supp_lst);
 
@@ -3298,14 +3298,6 @@ nm_supplicant_interface_class_init (NMSupplicantInterfaceClass *klass)
 	                  NULL, NULL, NULL,
 	                  G_TYPE_NONE, 2, G_TYPE_POINTER, G_TYPE_BOOLEAN);
 
-	signals[SCAN_DONE] =
-	    g_signal_new (NM_SUPPLICANT_INTERFACE_SCAN_DONE,
-	                  G_OBJECT_CLASS_TYPE (object_class),
-	                  G_SIGNAL_RUN_LAST,
-	                  0,
-	                  NULL, NULL, NULL,
-	                  G_TYPE_NONE, 0);
-
 	signals[WPS_CREDENTIALS] =
 	    g_signal_new (NM_SUPPLICANT_INTERFACE_WPS_CREDENTIALS,
 	                  G_OBJECT_CLASS_TYPE (object_class),
diff --git a/src/supplicant/nm-supplicant-interface.h b/src/supplicant/nm-supplicant-interface.h
index 8964a4754f..cfc898d80f 100644
--- a/src/supplicant/nm-supplicant-interface.h
+++ b/src/supplicant/nm-supplicant-interface.h
@@ -71,7 +71,6 @@ typedef enum {
 #define NM_SUPPLICANT_INTERFACE_STATE                   "state"
 #define NM_SUPPLICANT_INTERFACE_BSS_CHANGED             "bss-changed"
 #define NM_SUPPLICANT_INTERFACE_PEER_CHANGED            "peer-changed"
-#define NM_SUPPLICANT_INTERFACE_SCAN_DONE               "scan-done"
 #define NM_SUPPLICANT_INTERFACE_WPS_CREDENTIALS         "wps-credentials"
 #define NM_SUPPLICANT_INTERFACE_GROUP_STARTED           "group-started"
 #define NM_SUPPLICANT_INTERFACE_GROUP_FINISHED          "group-finished"