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

On Thu, Jul 11, 2024 at 11:22 PM Denis Kenzior <[email protected]> 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/[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