| Message ID | 20240705040204.58337-1-alex@shruggie.ro (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] manager: add support for changing 'PreferredTechnologies' over DBus | expand |
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/20240704204732.56553-1-alex@shruggie.ro/ > * removed 'tools/ip6tables-test' > * added to .gitignore via https://lore.kernel.org/connman/20240705035028.57697-1-alex@shruggie.ro/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
On Thu, Jul 11, 2024 at 11:22 PM Denis Kenzior <denkenz@gmail.com> wrote: > > 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? Hello, Apologies for the slow reply. I got wrapped up in tons of work. For the context (in which this was being run), just setting the properties would be sufficient. There would be various triggering/re-retriggering of services such that cellular would be preferred over WiFi (and vice-versa). (This is valid in our case). In the meantime (since doing this patch), I also found (in the main branch) the support for secondary gateways, which is actually what we'd want. In connman (up to 1.42), if a ppp0 interface would come up, it would setup an interface (default) route, which would not allow something like "ping -I wlan0 8.8.8.8" We've been a implementing a "ping -I ppp0" & "ping -I wlan0" type of logic, to be able to change the primary route (in this case via PreferredTechnologies) in case WiFi is connected, but not able to reach a certain address (via ping). The secondary gateway stuff, makes this patch not-needed. It's only for connman 1.42 (or older) that this is needed (to be order to prioritize WiFi over GSM/LTE and vice-versa). Thank you :) And apologies for the noise Alex > > > 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/20240704204732.56553-1-alex@shruggie.ro/ > > * removed 'tools/ip6tables-test' > > * added to .gitignore via https://lore.kernel.org/connman/20240705035028.57697-1-alex@shruggie.ro/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
diff --git a/doc/connman.conf.5.in b/doc/connman.conf.5.in index 3eb240f5..c8ce8eb6 100644 --- a/doc/connman.conf.5.in +++ b/doc/connman.conf.5.in @@ -101,6 +101,8 @@ with state 'ready' or with a non-preferred type; a service of a preferred technology type in state 'online' will get the default route when compared to either a non-preferred type or a preferred type further down in the list. +This property can be read and set over DBus as well, on +the 'net.connman.Manager' DBus interface. .TP .BI NetworkInterfaceBlacklist= interface\fR[,...] List of blacklisted network interfaces separated by ",". 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] + + 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. diff --git a/include/setting.h b/include/setting.h index 920e6754..6a575dc4 100644 --- a/include/setting.h +++ b/include/setting.h @@ -33,6 +33,8 @@ unsigned int connman_setting_get_uint(const char *key); char *connman_setting_get_string(const char *key); char **connman_setting_get_string_list(const char *key); unsigned int *connman_setting_get_uint_list(const char *key); +int connman_setting_set_uint_list(const char *key, const unsigned int *lst, + int len); unsigned int connman_timeout_input_request(void); unsigned int connman_timeout_browser_launch(void); 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); + 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; diff --git a/src/main.conf b/src/main.conf index 5357edb8..80b4fb83 100644 --- a/src/main.conf +++ b/src/main.conf @@ -60,6 +60,10 @@ # of a preferred technology type in state 'online' will get # the default route when compared to either a non-preferred # type or a preferred type further down in the list. +# This property is also configurable via DBus through the +# 'net.connman.Manager' & SetProperty method. +# The GetProperties DBus method will list this property +# and its current value. # PreferredTechnologies = # List of blacklisted network interfaces separated by ",". 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, +}; + +static void append_tech_list(DBusMessageIter *iter, void *user_data) +{ + unsigned int *tech_list = user_data; + int i; + + if (!tech_list) + return; + + for (i = 0; tech_list[i]; i++) { + const char *s = __connman_service_type2string(tech_list[i]); + if (!s) + continue; + dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &s); + } +} + static DBusMessage *get_properties(DBusConnection *conn, DBusMessage *msg, void *data) { @@ -41,6 +62,7 @@ static DBusMessage *get_properties(DBusConnection *conn, DBusMessageIter array, dict; dbus_bool_t offlinemode; const char *str; + int i; DBG("conn %p", conn); @@ -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); + } + 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); + } + + techs[cnt++] = type; + } + + connman_setting_set_uint_list(name, techs, cnt); } else return __connman_error_invalid_property(msg);