Re: [PATCH] add AllowRoaming station property

[Denis Kenzior <denkenz@gmail.com>]

Hi Pedro,

On 10/18/23 07:27, Pedro André wrote:
> From: Pedro Andre <peda@bang-olufsen.dk>
> 
> This adds an AllowRoaming property to the station struct that makes it
> possible to indicate to the station/interface that it should not go
> into roaming state (when set to false). This property defaults to true
> (i.e. it defaults to the normal iwd behaviour that allows roaming).
> 

Is it possible for you to share the actual use case for this?  It would help to 
understand what problem this is trying to solve.  One problem I see here is that 
we might still want to honor AP directed roaming.

> ---
>   src/station.c | 40 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
> 

<snip>

> +static struct l_dbus_message *station_property_set_allow_roaming(
> +					struct l_dbus *dbus,
> +					struct l_dbus_message *message,
> +					struct l_dbus_message_iter *new_value,
> +					l_dbus_property_complete_cb_t complete,
> +					void *user_data)
> +{
> +	struct station *station = user_data;
> +	bool roaming;
> +
> +	if (!l_dbus_message_iter_get_variant(new_value, "b", &roaming))
> +		return dbus_error_invalid_args(message);
> +
> +	l_debug("Setting allow_roaming %s", roaming ? "true" : "false");
> +
> +	station->allow_roaming = roaming;

So what happens if we're in the middle of a roam?

Also, what happens if this is reset to true and our rssi is already below the 
roam threshold?

> +
> +	return l_dbus_message_new_method_return(message);
> +}
> +
>   void station_foreach(station_foreach_func_t func, void *user_data)
>   {
>   	const struct l_queue_entry *entry;

<snip>

> @@ -4484,6 +4522,8 @@ static void station_setup_interface(struct l_dbus_interface *interface)
>   					station_property_get_scanning, NULL);
>   	l_dbus_interface_property(interface, "State", 0, "s",
>   					station_property_get_state, NULL);
> +	l_dbus_interface_property(interface, "AllowRoaming", 0, "b",
> +				station_property_get_allow_roaming, station_property_set_allow_roaming);

doc/coding-style.txt, Section 'Background', paragraph 2.

Also, there's missing documentation for this property.  And it seems to overlap 
rather significantly with "DisableRoamingScan" setting.  What's the plan there? 
And since we're adding a new API, can we at least add an auto test?

Regards,
-Denis