Re: [PATCH 3/8] station: add debug method GetNetworks

[Denis Kenzior <denkenz@gmail.com>]

Hi James,

On 8/10/22 18:16, James Prestwood wrote:
> This gets all networks but includes individual entries for each
> BSS.
> ---
>   src/station.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 94 insertions(+)
> 

You really should start with a commit in doc/ describing this API.

> +static void station_append_bss_list(struct l_dbus_message_builder *builder,
> +					const struct l_queue_entry *entry)
> +{
> +	for (; entry; entry = entry->next) {
> +		struct scan_bss *bss = entry->data;
> +		int32_t rssi = bss->signal_strength / 100;
> +
> +		l_dbus_message_builder_enter_array(builder, "{sv}");
> +
> +		dbus_append_dict_basic(builder, "Frequency", 'u',
> +						&bss->frequency);
> +		dbus_append_dict_basic(builder, "RSSI", 'i',
> +						&rssi);
> +		dbus_append_dict_basic(builder, "Rank", 'i', &bss->rank);

Rank is a uint16.

> +
> +		station_append_byte_array(builder, "Address", bss->addr, 6);

Are you sure you don't want Address in string form?

> +		station_append_byte_array(builder, "MDE", bss->mde, 3);
> +
> +		l_dbus_message_builder_leave_array(builder);
> +	}
> +}
> +
> +static struct l_dbus_message *station_debug_get_networks(struct l_dbus *dbus,
> +						struct l_dbus_message *message,
> +						void *user_data)
> +{
> +	struct station *station = user_data;
> +	struct l_dbus_message *reply =
> +				l_dbus_message_new_method_return(message);
> +	struct l_dbus_message_builder *builder =
> +				l_dbus_message_builder_new(reply);
> +	const struct l_queue_entry *entry;
> +
> +	l_dbus_message_builder_enter_array(builder, "{sv}");
> +
> +	for (entry = l_queue_get_entries(station->networks_sorted); entry;
> +							entry = entry->next) {
> +		const struct network *network = entry->data;
> +
> +		l_dbus_message_builder_enter_dict(builder, "sv");
> +		l_dbus_message_builder_append_basic(builder, 's',
> +						network_get_ssid(network));

Using the SSID as the key is not a good idea.  You can have multiple network 
types with the same SSID.  This should probably just be an array of some sort. 
Or maybe a dict where the key is the .Network object path.  Something like 
a(oa{sv}) or a{oa{sv}}.

> +		l_dbus_message_builder_enter_variant(builder, "aa{sv}");
> +		l_dbus_message_builder_enter_array(builder, "a{sv}");
> +
> +		station_append_bss_list(builder,
> +					network_bss_list_get_entries(network));
> +
> +		l_dbus_message_builder_leave_array(builder);
> +		l_dbus_message_builder_leave_variant(builder);
> +		l_dbus_message_builder_leave_dict(builder);
> +	}
> +
> +	/* Hidden Networks. Use an empty string for the SSID */
> +	l_dbus_message_builder_enter_dict(builder, "sv");
> +	l_dbus_message_builder_append_basic(builder, 's', "");
> +	l_dbus_message_builder_enter_variant(builder, "aa{sv}");
> +	l_dbus_message_builder_enter_array(builder, "a{sv}");
> +	station_append_bss_list(builder,
> +			l_queue_get_entries(station->hidden_bss_list_sorted));
> +	l_dbus_message_builder_leave_array(builder);
> +	l_dbus_message_builder_leave_variant(builder);
> +	l_dbus_message_builder_leave_dict(builder);
> +
> +	l_dbus_message_builder_leave_array(builder);
> +
> +	l_dbus_message_builder_finalize(builder);
> +	l_dbus_message_builder_destroy(builder);
> +
> +	return reply;
> +}
> +
>   static void station_setup_debug_interface(
>   					struct l_dbus_interface *interface)
>   {

Regards,
-Denis