diff mbox series

[2/6] station: move BasicServiceSet DBus management into station

Message ID 20240816125214.1162415-2-prestwoj@gmail.com (mailing list archive)
State New
Headers show
Series [1/6] network: add __network_path_append_bss | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
prestwoj/iwd-ci-gitlint success GitLint

Commit Message

James Prestwood Aug. 16, 2024, 12:52 p.m. UTC 
Due to an unnoticed bug after adding the BasicServiceSet object into
network, it became clear that since station already owns the scan_bss
objects it makes sense for it to manage the associated DBus objects
as well. This way network doesn't have to jump through hoops to
determine if the scan_bss object was remove, added, or updated. It
can just manage its list as it did prior.

From the station side this makes things very easy. When scan results
come in we either update or add a new DBus object. And any time a
scan_bss is freed we remove the DBus object.
---
 src/station.c | 89 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 72 insertions(+), 17 deletions(-)

Comments

Denis Kenzior Aug. 19, 2024, 3:22 p.m. UTC | #1 
Hi James,

On 8/16/24 7:52 AM, James Prestwood wrote:
> Due to an unnoticed bug after adding the BasicServiceSet object into
> network, it became clear that since station already owns the scan_bss
> objects it makes sense for it to manage the associated DBus objects
> as well. This way network doesn't have to jump through hoops to
> determine if the scan_bss object was remove, added, or updated. It
> can just manage its list as it did prior.
> 
>  From the station side this makes things very easy. When scan results
> come in we either update or add a new DBus object. And any time a
> scan_bss is freed we remove the DBus object.
> ---
>   src/station.c | 89 +++++++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 72 insertions(+), 17 deletions(-)
> 

<snip>

> @@ -431,6 +429,65 @@ static void station_print_scan_bss(const struct scan_bss *bss)
>   			optional);
>   }
>   
> +static const char *station_get_bss_path(struct station *station,
> +						struct scan_bss *bss)
> +{
> +	enum security security;
> +	char ssid[33];
> +	const char *network_path;
> +
> +	memcpy(ssid, bss->ssid, bss->ssid_len);
> +	ssid[bss->ssid_len] = '\0';
> +
> +	if (scan_bss_get_security(bss, &security) < 0)
> +		return NULL;
> +
> +	network_path = iwd_network_get_path(station, ssid, security);
> +	if (!network_path)
> +		return NULL;
> +
> +	return __network_path_append_bss(network_path, bss);
> +}
> +
> +static bool station_unregister_bss(struct station *station,
> +					struct scan_bss *bss)
> +{
> +	const char *path = station_get_bss_path(station, bss);
> +
> +	if (L_WARN_ON(!path))
> +		return false;
> +
> +	return l_dbus_unregister_object(dbus_get_bus(), path);
> +}
> +
> +static bool station_register_bss(struct station *station, struct scan_bss *bss)
> +{
> +	struct scan_bss *old;
> +	const char *path = station_get_bss_path(station, bss);
> +
> +	if (L_WARN_ON(!path))
> +		return false;
> +
> +	/*
> +	 * If we find this path in the object tree update the data to the new
> +	 * scan_bss pointer, as this one will be freed soon.
> +	 */
> +	old = l_dbus_object_get_data(dbus_get_bus(), path, IWD_BSS_INTERFACE);
> +	if (old)
> +		return l_dbus_object_set_data(dbus_get_bus(), path,
> +						IWD_BSS_INTERFACE, bss);
> +
> +	if (!l_dbus_object_add_interface(dbus_get_bus(), path,
> +						IWD_BSS_INTERFACE, bss))
> +		return false;
> +
> +	if (!l_dbus_object_add_interface(dbus_get_bus(), path,
> +					L_DBUS_INTERFACE_PROPERTIES, bss))
> +		return false;
> +
> +	return true;
> +}
> +
>   /*
>    * Returns the network object the BSS was added to or NULL if ignored.
>    */
> @@ -518,6 +575,7 @@ static struct network *station_add_seen_bss(struct station *station,
>   	}
>   
>   	network_bss_add(network, bss);
> +	station_register_bss(station, bss);

Hmm, you have the network object right above?  Can you utilize that somehow?

>   
>   	return network;
>   }
> @@ -540,7 +598,7 @@ struct bss_expiration_data {
>   	struct scan_bss *connected_bss;
>   	uint64_t now;
>   	const struct scan_freq_set *freqs;
> -	struct l_queue *free_list;
> +	struct station *station;
>   };
>   
>   #define SCAN_RESULT_BSS_RETENTION_TIME (30 * 1000000)
> @@ -562,20 +620,21 @@ static bool bss_free_if_expired(void *data, void *user_data)
>   			bss->time_stamp + SCAN_RESULT_BSS_RETENTION_TIME))
>   		return false;
>   
> -	l_queue_push_head(expiration_data->free_list, bss);
> +	station_unregister_bss(expiration_data->station, bss);
> +
> +	scan_bss_free(bss);
>   
>   	return true;
>   }
>   
>   static void station_bss_list_remove_expired_bsses(struct station *station,
> -					const struct scan_freq_set *freqs,
> -					struct l_queue *free_list)
> +					const struct scan_freq_set *freqs)
>   {
>   	struct bss_expiration_data data = {
>   		.now = l_time_now(),
>   		.connected_bss = station->connected_bss,
>   		.freqs = freqs,
> -		.free_list = free_list,
> +		.station = station,
>   	};
>   
>   	l_queue_foreach_remove(station->bss_list, bss_free_if_expired, &data);
> @@ -823,6 +882,7 @@ static bool station_owe_transition_results(int err, struct l_queue *bss_list,
>   
>   		l_queue_push_tail(station->bss_list, bss);
>   		network_bss_add(network, bss);
> +		station_register_bss(station, bss);

Same comment as above.

>   
>   		continue;
>   
> @@ -951,7 +1011,6 @@ void station_set_scan_results(struct station *station,
>   	const struct l_queue_entry *bss_entry;
>   	struct network *network;
>   	struct process_network_data data;
> -	struct l_queue *free_list = l_queue_new();
>   
>   	l_queue_foreach_remove(new_bss_list, bss_free_if_ssid_not_utf8, NULL);
>   
> @@ -963,7 +1022,7 @@ void station_set_scan_results(struct station *station,
>   	l_queue_destroy(station->autoconnect_list, NULL);
>   	station->autoconnect_list = NULL;
>   
> -	station_bss_list_remove_expired_bsses(station, freqs, free_list);
> +	station_bss_list_remove_expired_bsses(station, freqs);
>   
>   	for (bss_entry = l_queue_get_entries(station->bss_list); bss_entry;
>   						bss_entry = bss_entry->next) {
> @@ -975,12 +1034,8 @@ void station_set_scan_results(struct station *station,
>   			if (old_bss == station->connected_bss)
>   				station->connected_bss = new_bss;
>   
> -			/*
> -			 * The network object is still holding a reference to
> -			 * the BSS. Until we tell network to replace the BSS
> -			 * with a new object, don't free it.
> -			 */
> -			l_queue_push_head(free_list, old_bss);
> +			station_register_bss(station, new_bss);

Why do this here?  Can't you do this step in the loop below where we figure out 
the network object?

> +			scan_bss_free(old_bss);
>   
>   			continue;
>   		}
> @@ -1017,8 +1072,6 @@ void station_set_scan_results(struct station *station,
>   
>   	l_hashmap_foreach_remove(station->networks, process_network, &data);
>   
> -	l_queue_destroy(free_list, bss_free);
> -
>   	station->autoconnect_can_start = trigger_autoconnect;
>   	station_autoconnect_start(station);
>   }
> @@ -2652,6 +2705,7 @@ static void station_update_roam_bss(struct station *station,
>   		l_queue_remove_if(station->bss_list, bss_match, bss);
>   
>   	network_bss_update(network, bss);
> +	station_register_bss(station, bss);

Same here, can we use the network object somehow?

>   	l_queue_push_tail(station->bss_list, bss);
>   
>   	if (old)
> @@ -3160,6 +3214,7 @@ static void station_event_roamed(struct station *station, struct scan_bss *new)
>   	struct scan_bss *stale;
>   
>   	network_bss_update(station->connected_network, new);
> +	station_register_bss(station, new);

And here

>   
>   	/* Remove new BSS if it exists in past scan results */
>   	stale = l_queue_remove_if(station->bss_list, bss_match, new);

Regards,
-Denis
diff mbox series

Patch

diff --git a/src/station.c b/src/station.c
index 06b19db3..25ddce43 100644
--- a/src/station.c
+++ b/src/station.c
@@ -350,8 +350,6 @@  static bool process_network(const void *key, void *data, void *user_data)
 	struct process_network_data *process_data = user_data;
 	struct station *station = process_data->station;
 
-	network_bss_stop_update(network, process_data->freqs);
-
 	if (!network_bss_list_isempty(network)) {
 		bool connected = network == station->connected_network;
 
@@ -431,6 +429,65 @@  static void station_print_scan_bss(const struct scan_bss *bss)
 			optional);
 }
 
+static const char *station_get_bss_path(struct station *station,
+						struct scan_bss *bss)
+{
+	enum security security;
+	char ssid[33];
+	const char *network_path;
+
+	memcpy(ssid, bss->ssid, bss->ssid_len);
+	ssid[bss->ssid_len] = '\0';
+
+	if (scan_bss_get_security(bss, &security) < 0)
+		return NULL;
+
+	network_path = iwd_network_get_path(station, ssid, security);
+	if (!network_path)
+		return NULL;
+
+	return __network_path_append_bss(network_path, bss);
+}
+
+static bool station_unregister_bss(struct station *station,
+					struct scan_bss *bss)
+{
+	const char *path = station_get_bss_path(station, bss);
+
+	if (L_WARN_ON(!path))
+		return false;
+
+	return l_dbus_unregister_object(dbus_get_bus(), path);
+}
+
+static bool station_register_bss(struct station *station, struct scan_bss *bss)
+{
+	struct scan_bss *old;
+	const char *path = station_get_bss_path(station, bss);
+
+	if (L_WARN_ON(!path))
+		return false;
+
+	/*
+	 * If we find this path in the object tree update the data to the new
+	 * scan_bss pointer, as this one will be freed soon.
+	 */
+	old = l_dbus_object_get_data(dbus_get_bus(), path, IWD_BSS_INTERFACE);
+	if (old)
+		return l_dbus_object_set_data(dbus_get_bus(), path,
+						IWD_BSS_INTERFACE, bss);
+
+	if (!l_dbus_object_add_interface(dbus_get_bus(), path,
+						IWD_BSS_INTERFACE, bss))
+		return false;
+
+	if (!l_dbus_object_add_interface(dbus_get_bus(), path,
+					L_DBUS_INTERFACE_PROPERTIES, bss))
+		return false;
+
+	return true;
+}
+
 /*
  * Returns the network object the BSS was added to or NULL if ignored.
  */
@@ -518,6 +575,7 @@  static struct network *station_add_seen_bss(struct station *station,
 	}
 
 	network_bss_add(network, bss);
+	station_register_bss(station, bss);
 
 	return network;
 }
@@ -540,7 +598,7 @@  struct bss_expiration_data {
 	struct scan_bss *connected_bss;
 	uint64_t now;
 	const struct scan_freq_set *freqs;
-	struct l_queue *free_list;
+	struct station *station;
 };
 
 #define SCAN_RESULT_BSS_RETENTION_TIME (30 * 1000000)
@@ -562,20 +620,21 @@  static bool bss_free_if_expired(void *data, void *user_data)
 			bss->time_stamp + SCAN_RESULT_BSS_RETENTION_TIME))
 		return false;
 
-	l_queue_push_head(expiration_data->free_list, bss);
+	station_unregister_bss(expiration_data->station, bss);
+
+	scan_bss_free(bss);
 
 	return true;
 }
 
 static void station_bss_list_remove_expired_bsses(struct station *station,
-					const struct scan_freq_set *freqs,
-					struct l_queue *free_list)
+					const struct scan_freq_set *freqs)
 {
 	struct bss_expiration_data data = {
 		.now = l_time_now(),
 		.connected_bss = station->connected_bss,
 		.freqs = freqs,
-		.free_list = free_list,
+		.station = station,
 	};
 
 	l_queue_foreach_remove(station->bss_list, bss_free_if_expired, &data);
@@ -823,6 +882,7 @@  static bool station_owe_transition_results(int err, struct l_queue *bss_list,
 
 		l_queue_push_tail(station->bss_list, bss);
 		network_bss_add(network, bss);
+		station_register_bss(station, bss);
 
 		continue;
 
@@ -951,7 +1011,6 @@  void station_set_scan_results(struct station *station,
 	const struct l_queue_entry *bss_entry;
 	struct network *network;
 	struct process_network_data data;
-	struct l_queue *free_list = l_queue_new();
 
 	l_queue_foreach_remove(new_bss_list, bss_free_if_ssid_not_utf8, NULL);
 
@@ -963,7 +1022,7 @@  void station_set_scan_results(struct station *station,
 	l_queue_destroy(station->autoconnect_list, NULL);
 	station->autoconnect_list = NULL;
 
-	station_bss_list_remove_expired_bsses(station, freqs, free_list);
+	station_bss_list_remove_expired_bsses(station, freqs);
 
 	for (bss_entry = l_queue_get_entries(station->bss_list); bss_entry;
 						bss_entry = bss_entry->next) {
@@ -975,12 +1034,8 @@  void station_set_scan_results(struct station *station,
 			if (old_bss == station->connected_bss)
 				station->connected_bss = new_bss;
 
-			/*
-			 * The network object is still holding a reference to
-			 * the BSS. Until we tell network to replace the BSS
-			 * with a new object, don't free it.
-			 */
-			l_queue_push_head(free_list, old_bss);
+			station_register_bss(station, new_bss);
+			scan_bss_free(old_bss);
 
 			continue;
 		}
@@ -1017,8 +1072,6 @@  void station_set_scan_results(struct station *station,
 
 	l_hashmap_foreach_remove(station->networks, process_network, &data);
 
-	l_queue_destroy(free_list, bss_free);
-
 	station->autoconnect_can_start = trigger_autoconnect;
 	station_autoconnect_start(station);
 }
@@ -2652,6 +2705,7 @@  static void station_update_roam_bss(struct station *station,
 		l_queue_remove_if(station->bss_list, bss_match, bss);
 
 	network_bss_update(network, bss);
+	station_register_bss(station, bss);
 	l_queue_push_tail(station->bss_list, bss);
 
 	if (old)
@@ -3160,6 +3214,7 @@  static void station_event_roamed(struct station *station, struct scan_bss *new)
 	struct scan_bss *stale;
 
 	network_bss_update(station->connected_network, new);
+	station_register_bss(station, new);
 
 	/* Remove new BSS if it exists in past scan results */
 	stale = l_queue_remove_if(station->bss_list, bss_match, new);