Re: [PATCH v2] manager: add support for changing 'PreferredTechnologies' over DBus

Hi Alexandru,

On 7/4/24 11:02 PM, Alexandru Ardelean wrote:
> This change adds support for reading/modifying the 'PreferredTechnologies'
> over DBus.
> This can be extended to 'DefaultAutoConnectTechnologies',
> 'DefaultFavoriteTechnologies' & 'AlwaysConnectedTechnologies' as well.
> However the interest (so far) was in changing the 'PreferredTechnologies'
> order (at runtime) between cellular and wifi.
> Also, 'PreferredTechnologies' was the only that was tested.
> 
> Changing 'PreferredTechnologies' seems to yield the desired result, as the
> switch between cellular/wifi does not need to be super-fast.
> And it changes the routing table (as desired) when cellular is preferred
> over WiFi (and vice-versa).
> 

Can you elaborate?  If setting the property doesn't trigger service re-ordering 
immediately, how does it happen?

> On some boxes that have both cellular & WiFi, it's often desired to prefer
> cellular (over WiFi) in case WiFi is not able to connect to the internet.
> The logic (for this switching) can be determined outside of connman.
> Setting 'PreferredTechnologies' helps in this case.
> ---
> 
> Changelog v1 -> v2:
> * v1: https://lore.kernel.org/connman/[email protected]/
> * removed 'tools/ip6tables-test'
>    * added to .gitignore via https://lore.kernel.org/connman/[email protected]/T/#u
> * added a bit of documentation in 'src/main.conf' for the 'PreferredTechnologies'
>    property now also being accessible over DBus
> 
>   doc/connman.conf.5.in |  2 ++
>   doc/manager-api.txt   | 13 +++++++++
>   include/setting.h     |  2 ++
>   src/main.c            | 22 ++++++++++++++
>   src/main.conf         |  4 +++
>   src/manager.c         | 67 +++++++++++++++++++++++++++++++++++++++++++
>   6 files changed, 110 insertions(+)
> 

<snip>

> diff --git a/doc/manager-api.txt b/doc/manager-api.txt
> index 6eaa0a38..3be9cffc 100644
> --- a/doc/manager-api.txt
> +++ b/doc/manager-api.txt
> @@ -319,3 +319,16 @@ Properties	string State [readonly]
>   			and does not affect ConnMan in any way.
>   
>   			The default value is false.
> +
> +		array{string} PreferredTechnologies [readwrite] [experminental]

Typo 'experminental'

> +
> +			This property is the same one that is defined in
> +			/etc/connman/main.conf. It controls the order of
> +			preferred technologies (e.g. wifi, cellular) at
> +			runtime.
> +			Changing it here will not have an immediate effect.
> +			It's only when connman gets triggered via service changes
> +			that this gets taken into consideration. Or, when it's
> +			triggered via manual connect/disconnect of services.
> +
> +			The default value is defined in /etc/connman/main.conf.

Have you considered looking into how the Online check is implemented?  Perhaps 
if the interface is not Online, it should not be prioritized ahead of one that is?

> diff --git a/src/main.c b/src/main.c
> index f5da979b..7858b967 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -1122,6 +1122,28 @@ unsigned int *connman_setting_get_uint_list(const char *key)
>   	return NULL;
>   }
>   
> +int connman_setting_set_uint_list(const char *key, const unsigned int *lst,
> +				  int len)
> +{
> +	unsigned int *new_list;
> +
> +	new_list = g_try_new0(unsigned int, len + 1);

General advice is that for small allocations g_try_new0 is not necessary.

> +	if (!new_list)
> +		return -1;
> +
> +	memcpy(new_list, lst, sizeof(unsigned int) * len);
> +
> +	if (g_str_equal(key, CONF_PREFERRED_TECHS)) {
> +		g_free(connman_settings.preferred_techs);
> +		connman_settings.preferred_techs = new_list;
> +		return 0;
> +	}
> +
> +	g_free(new_list);
> +
> +	return -1;
> +}
> +
>   unsigned int connman_timeout_input_request(void)
>   {
>   	return connman_settings.timeout_inputreq;

<snip>

> diff --git a/src/manager.c b/src/manager.c
> index 892d3a42..62f34c50 100644
> --- a/src/manager.c
> +++ b/src/manager.c
> @@ -34,6 +34,27 @@
>   static bool connman_state_idle;
>   static dbus_bool_t sessionmode;
>   
> +static const char *tech_property_names[] = {
> +	"PreferredTechnologies",
> +	NULL,
> +};

How would this be extended in the future?


<snip>

> @@ -64,6 +86,13 @@ static DBusMessage *get_properties(DBusConnection *conn,
>   					DBUS_TYPE_BOOLEAN,
>   					&sessionmode);
>   
> +	for (i = 0; tech_property_names[i]; i++) {
> +		const char *name = tech_property_names[i];
> +		unsigned int *lst = connman_setting_get_uint_list(name);
> +		connman_dbus_dict_append_array(&dict, name, DBUS_TYPE_STRING,
> +						append_tech_list, lst);

This logic seems pretty specific to PreferredTechnologies?

> +	}
> +
>   	connman_dbus_dict_close(&array, &dict);
>   
>   	return reply;
> @@ -110,6 +139,44 @@ static DBusMessage *set_property(DBusConnection *conn,
>   
>   		dbus_message_iter_get_basic(&value, &sessionmode);
>   
> +	} else if (g_str_equal(name, "PreferredTechnologies")) {
> +		unsigned int techs[MAX_CONNMAN_SERVICE_TYPES] = {};
> +		DBusMessageIter entry;
> +		int cnt;
> +
> +		if (type != DBUS_TYPE_ARRAY)
> +			return __connman_error_invalid_arguments(msg);
> +
> +		dbus_message_iter_recurse(&value, &entry);
> +
> +		cnt = 0;
> +		while (dbus_message_iter_get_arg_type(&entry) == DBUS_TYPE_STRING) {
> +			enum connman_service_type type;
> +			const char *val;
> +			dbus_message_iter_get_basic(&entry, &val);
> +			dbus_message_iter_next(&entry);
> +			int i;
> +
> +			if (!val[0])
> +				continue;
> +
> +			type = __connman_service_string2type(val);
> +			if (type == CONNMAN_SERVICE_TYPE_UNKNOWN)
> +				return __connman_error_invalid_arguments(msg);
> +
> +			if (cnt >= MAX_CONNMAN_SERVICE_TYPES)
> +				return __connman_error_invalid_arguments(msg);
> +
> +			/* check for duplicates */
> +			for (i = 0; i < cnt; i++) {
> +				if (type == techs[i])
> +					return __connman_error_invalid_arguments(msg);
> +			}

There's a few lines > 80 chars.  Have you run this through checkpatch?

> +
> +			techs[cnt++] = type;
> +		}
> +
> +		connman_setting_set_uint_list(name, techs, cnt);
>   	} else
>   		return __connman_error_invalid_property(msg);
>   

Regards,
-Denis