Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.01.org/mailman/listinfo/connman
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. Re: [PATCH] gweb: fix segfault with musl v1.1.21 (Daniel Wagner)
2. Re: connman installs duplicate routes? (Daniel Wagner)
3. Re: [PATCH] wispr: Prevent a crash when freeing wispr routes
(Daniel Wagner)
4. Re: [PATCH] service: Fix autoconnect after rejection
(Daniel Wagner)
5. Re: Possibility to view/change (edit) wireless network
properties (e.g. passphrase) (Daniel Wagner)
6. Re: [PATCH] timezone : Fix timezone without subpath
(Daniel Wagner)
7. Re: [PATCH] network: Fix disconnect_callback crash on error
(Daniel Wagner)
8. Re: [PATCH] Force BSS expiration (Daniel Wagner)
----------------------------------------------------------------------
Message: 1
Date: Wed, 5 Jun 2019 10:07:47 +0200
From: Daniel Wagner <[email protected]>
To: Nicola Lunghi <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] gweb: fix segfault with musl v1.1.21
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Nicola,
On Thu, May 23, 2019 at 08:20:21AM +0100, Nicola Lunghi wrote:
> In musl > 1.1.21 freeaddrinfo() implementation changed and
> was causing a segmentation fault on recent Yocto using musl.
Patch applied.
Thanks,
Daniel
------------------------------
Message: 2
Date: Wed, 5 Jun 2019 11:20:53 +0200
From: Daniel Wagner <[email protected]>
To: David Weidenkopf <[email protected]>
Cc: "[email protected]" <[email protected]>
Subject: Re: connman installs duplicate routes?
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi David,
On Thu, May 23, 2019 at 11:23:19PM +0000, David Weidenkopf wrote:
> Hi, I am using connman 1.35. If I enable both a ethernet and wifi
> service, the default route is selected as you would expect. However,
> multiple routes will be created for the LAN.
For every Service which is in either in online or ready state ConnMan
will add a route to the routing table.
> ~# ip route show
> default via 192.168.16.1 dev eth0
> 1.0.0.1 via 192.168.16.1 dev eth0
> 1.0.0.1 via 192.168.16.1 dev wifi0
> 1.1.1.1 via 192.168.16.1 dev eth0
> 1.1.1.1 via 192.168.16.1 dev wifi0
> 192.168.16.0/24 dev wifi0 proto kernel scope link src 192.168.16.40
> 192.168.16.0/24 dev eth0 proto kernel scope link src 192.168.16.29
> 192.168.16.1 dev eth0 scope link
> 192.168.16.1 dev wifi0 scope link
>
> This output is after ethernet has been disconnected and been
> reconnected. The default route is eth0 as expected. However, there
> are two LAN routes with the wifi0 route being first and being
> selected. It seems that connman should either remove the wifi0 route
> or set a metric on it.
The routes are added by the kernel and not by ConnMan. It seems it is
not possible to change the metric of an existing route [1], instead
ConnMan would need to delete the route set by the kernel and set a new
one with a metric.
> The impact is that LAN traffic is sent over wifi0 instead of eth0. Using
>
> SingleConnectedTechnology=true
>
> in the configuration works around this but seems to be a discouraged
> setting. Also, it means that wifi is not connected when ethernet is.
Yes, that's correct. Though I haven't really understood why
you want to have the Ethernet and WiFi link into the same network up
and running.
> What is the best way to configure/use connman when ethernet and wifi
> are present?
As usual this depends on what you want to achieve.
> Is this a defect?
Yes and no. I agree it would be good to add the metric to the
links. But then it is only important if you have two links to the same
network, IIUC.
Thanks,
Daniel
[1] http://lkml.iu.edu/hypermail/linux/net/0504.3/0017.html
------------------------------
Message: 3
Date: Wed, 5 Jun 2019 11:45:34 +0200
From: Daniel Wagner <[email protected]>
To: Yasser <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] wispr: Prevent a crash when freeing wispr routes
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Yasser,
On Fri, May 24, 2019 at 12:11:51PM +0200, Yasser wrote:
> I have had the connman crash when free_wispr_routes() is called. It happens
> when there are lots of wireless networks around but none with good signal
> strength.
> As a fix, I use g_slist_nth_data() to get the wispr_route and check it
> before using it.
This commit message needs updated. We follow loosly the Linux kernel
style recommendation on how to submit patches. There is a good section
on how to write good commit messages.
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
e.g. in your example it would start with 'Use g_slist_nth_data()...'.
> diff --git a/src/wispr.c b/src/wispr.c
> index 0ab041e..546660a 100644
> --- a/src/wispr.c
> +++ b/src/wispr.c
> @@ -128,28 +128,30 @@ static void connman_wispr_message_init(struct
> connman_wispr_message *msg)
> static void free_wispr_routes(struct connman_wispr_portal_context
> *wp_context)
> {
> while (wp_context->route_list) {
> - struct wispr_route *route = wp_context->route_list->data;
> -
> - DBG("free route to %s if %d type %d", route->address,
> - route->if_index, wp_context->type);
> + struct wispr_route *route =
> g_slist_nth_data(wp_context->route_list, 0);
g_slist_nth_data() is just returning a pointer to the first element of
the list. So this is not needed.
> + if (route) {
The real change is the check here. But I would like to understand why
we need to check if the pointer is not NULL. This change seems to be a
workaround. Why do we get in a noise environment this problem. Sounds
like we have some sort of race going on. Could you eloborate a bit on
this?
Thanks,
Daniel
------------------------------
Message: 4
Date: Wed, 5 Jun 2019 12:49:33 +0200
From: Daniel Wagner <[email protected]>
To: Yasser <[email protected]>
Cc: [email protected], Jussi Laakkonen <[email protected]>
Subject: Re: [PATCH] service: Fix autoconnect after rejection
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Yasser,
On Mon, May 27, 2019 at 11:02:06AM +0200, Yasser wrote:
> We have had an issue with autoconnect where if the association failed,
> connman would not autoconnect. It happened when there were lots of
> wireless networks around but none with good signal strength. To fix
> this, we schedule another autoconnect attempt after a delay.
See the other email on my comment on the commit message.
IIRC, the reason why we don't have this in ConnMan already, is the
case, where you want to connect to a network with outdated
credentials. In this case your account could be blocked. Or at least
that is what I remember.
> diff --git a/src/service.c b/src/service.c
> index 0c5e647..d740848 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -38,6 +38,7 @@
> #include "connman.h"
>
> #define CONNECT_TIMEOUT 120
> +#define AUTO_CONNECT_FAILURE_DELAY 10
>
> #define VPN_AUTOCONNECT_TIMEOUT_DEFAULT 1
> #define VPN_AUTOCONNECT_TIMEOUT_STEP 30
> @@ -3918,8 +3919,21 @@ static void service_complete(struct
> connman_service *service)
> {
> reply_pending(service, EIO);
>
> - if (service->connect_reason != CONNMAN_SERVICE_CONNECT_REASON_USER)
> - do_auto_connect(service, service->connect_reason);
> + if (service->connect_reason != CONNMAN_SERVICE_CONNECT_REASON_USER) {
> + if(service->state == CONNMAN_SERVICE_STATE_FAILURE &&
> +
> connman_network_get_associating(service->network) == true) {
Don't test for bools (drop the '== true'). That said I think this is
rather strange to be in the failure path and test if the network
abstraction thinks it is in associating. Actually, this only works
because network.c:set_disconnect() is just reseting the state at the
end of the function. Depending on this ordering is asking for trouble.
2c1a0ac2dd8d ("service: Implement a do_auto_connect() to include VPNs
in autoconnect") added the initial do_auto_connect() to
service_complete(). Your changes indicate you are only looking at WiFi
connectivity. I really would like to have the input from Jussi here. I
don't want to mess up the recent work on VPN and autoconnect.
> + /*
> + * If association failed, schedule next autoconnect
> + * attempt after <AUTO_CONNECT_FAILURE_DELAY> seconds
> + *
> + */
> + g_timeout_add_seconds(AUTO_CONNECT_FAILURE_DELAY,
> + do_auto_connect,
> + GUINT_TO_POINTER(service->connect_reason));
> + } else {
> + do_auto_connect(service, service->connect_reason);
> + }
> + }
>
> g_get_current_time(&service->modified);
> service_save(service);
Thanks,
Daniel
------------------------------
Message: 5
Date: Wed, 5 Jun 2019 13:13:30 +0200
From: Daniel Wagner <[email protected]>
To: [email protected]
Cc: "[email protected]" <[email protected]>
Subject: Re: Possibility to view/change (edit) wireless network
properties (e.g. passphrase)
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Tormen,
On Sun, May 26, 2019 at 01:29:23PM +0200, [email protected] wrote:
> I wanted to kindly ask you if you could shed some light on this:
>
> https://github.com/andrew-bibb/cmst/issues/202
>
> Because I am lost and I don't get it :(
>
> There should be a way that a wireless profile connman uses can be
> viewed or edited through a GUI, or not?
ConnMan exposes via D-Bus some information about the underlying
network configuration. On purpose not everything has been exposed via
the D-Bus API. The design idea here is not to expose unnecessary
things, keep it as simple as possible. While this works for some
users, not everyone is happy with it. That is why we added additional
ways to configure and provision ConnMan. While this works, it is far
from being perfect.
I suppose something like this should be considered for a ConnMan 2.x
version.
> This is really useful e.g. to check "auto-connect" (potentially toggle
> that off if you forgot to do that when creating the wireless profile).
>
> The wireless password is something to be discussed. Ideally I as
> connman user could choose to expose the passwords (e.g. shared key wpa2)
> through it's API as well so that a tool like cmst can show me
> the current setting (including passwords) used by connman.
Currently, ConnMan doesn't allow to read back the password via
D-Bus. The only reason why ConnMan stores the password is because
wpa_supplicant doesn't store it itself. If wpa_supplicant could save a
configuration ConnMan wouldn't store the password. All other daemons
are able to make a configuration persistent.
> I am still new to connman. I know that you can use it with iwd and
> wpa_supplicant. But ideally I would expect that connman uses a wireless
> configuration that is agonostic to the actual tool used to do the
> wireless setup. Is that the case?
Yes, ConnMan is designed to abstract the network configuration.
> And if yes, then what do you think:
> (a) Should there be a possibility for cmst to view and/or edit this
> wireless profile?
I can't speak for cmst, but I am really glad someones is working on
UIs for ConnMan!
> (b) Does connman already offer interfaces (APIs?) for cmst to do so?
> And if yes, which ones?
We have all API's documented in the doc folder. This includes how you
can do provisioning (read configuration) of services.
Thanks,
Daniel
------------------------------
Message: 6
Date: Wed, 5 Jun 2019 13:36:00 +0200
From: Daniel Wagner <[email protected]>
To: Yasser <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] timezone : Fix timezone without subpath
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Yasser,
On Mon, May 27, 2019 at 11:12:58AM +0200, Yasser wrote:
> A small patch to fix the handling of timezones with and without subpath.
Patch applied after updating the commit message.
>
> diff --git a/src/timezone.c b/src/timezone.c
> index 8e91267..0538191 100644
> --- a/src/timezone.c
> +++ b/src/timezone.c
> @@ -187,7 +187,11 @@ static char *find_origin(void *src_map, struct
> stat *src_st,
> subpath, d->d_name);
>
> if (compare_file(src_map, src_st, pathname) == 0) {
> - str = g_strdup_printf("%s/%s",
> + if (!subpath)
> + str = g_strdup_printf("%s",
> + d->d_name);
I changed this to g_strdup().
> + else
> + str = g_strdup_printf("%s/%s",
> subpath, d->d_name);
> closedir(dir);
> return str;
Thanks,
Daniel
------------------------------
Message: 7
Date: Wed, 5 Jun 2019 13:41:25 +0200
From: Daniel Wagner <[email protected]>
To: Yasser <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] network: Fix disconnect_callback crash on error
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Yasser,
On Mon, May 27, 2019 at 11:32:58AM +0200, Yasser wrote:
> We had a crash in connman_network_set_connected() when
> disconnect_callback() was called but the value of wifi->network was
> not checked. This has already been fixed for wifi.c. However
> connman_network_set_connected() is called by other components where
> the network parameter is not checked beforehand so we thought it might
> be a good idea to verify in the function itself.
(commit message should be written differently)
So this code is not needed after all? I am relunctant to add checks
which are not necessary and will paper over problems.
>
> diff --git a/src/network.c b/src/network.c
> index 56fe24f..0486fad 100644
> --- a/src/network.c
> +++ b/src/network.c
> @@ -1675,6 +1675,9 @@ void connman_network_set_error(struct
> connman_network *network,
> int connman_network_set_connected(struct connman_network *network,
> bool connected)
> {
> + if (!network)
> + return 0;
> +
> DBG("network %p connected %d/%d connecting %d associating %d",
> network, network->connected, connected, network->connecting,
> network->associating);
BTW, the other patch I just applied was corrupted. This one looks also
whitespace damaged.
Thanks,
Daniel
------------------------------
Message: 8
Date: Wed, 5 Jun 2019 14:19:44 +0200
From: Daniel Wagner <[email protected]>
To: Yasser <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] Force BSS expiration
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Yasser,
On Mon, May 27, 2019 at 12:05:11PM +0200, Yasser wrote:
> We were having a problem with our wifi scanning, where the list of
> wifi available would become empty and would not be repopulated until
> after a long delay. Researching the problem it seemed that it was
> related to BSS expiration age. There were already some people who had
> faced the same issue, so inspired by this we developed the following
> patch which allows us to set the BSS expiration age to match ConnMan
> long scanning interval to avoid the loss of networks during a long
> interval between two scans.
Hopefully this fixes the problem you describe. I could image this
might need a bit more of tweaking but let's try this out.
Again, I was not happy with the commit message and changed it to
"""
wifi: Set the BSS expiration time
Always set the BSSExpireAge. The list of available networks can become
empty and it is not repopulated until after a long delay. Set the BSS
expiration age to match ConnMan long scanning interval to avoid the
loss of networks during a long interval between two scans.
"""
And also the patch is corrupted:
patching file gsupplicant/gsupplicant.h
patch: **** malformed patch at line 97:
g_supplicant_interface_connect(GSupplicantInterface *interface,
> diff --git a/gsupplicant/gsupplicant.h b/gsupplicant/gsupplicant.h
> index bfb52db..08d6b9e 100644
> --- a/gsupplicant/gsupplicant.h
> +++ b/gsupplicant/gsupplicant.h
> @@ -267,7 +267,8 @@ int
> g_supplicant_interface_connect(GSupplicantInterface *interface,
> int g_supplicant_interface_disconnect(GSupplicantInterface *interface,
> GSupplicantInterfaceCallback callback,
> void *user_data);
> -
> +int g_supplicant_interface_set_bss_expiration_age(GSupplicantInterface
> *interface,
> + unsigned int
> bss_expiration_age);
e,g. whitespace damage
> int g_supplicant_interface_set_apscan(GSupplicantInterface *interface,
> unsigned int ap_scan);
>
> diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
> index 6052f7b..fe6ad48 100644
> --- a/gsupplicant/supplicant.c
> +++ b/gsupplicant/supplicant.c
> @@ -981,6 +981,46 @@ static void interface_capability(const char *key,
> DBusMessageIter *iter,
> key, dbus_message_iter_get_arg_type(iter));
> }
>
> +struct g_supplicant_bss_expiration_age
> +{
> + GSupplicantInterface *interface;
> + unsigned int bss_expiration_age;
> +};
Since bss_expiration_age is the only thing we need in the callback
this helper structure is not needed.
> +
> +static void set_bss_expiration_age(DBusMessageIter *iter, void *user_data)
> +{
> + struct g_supplicant_bss_expiration_age *data = user_data;
> + unsigned int bss_expiration_age = data->bss_expiration_age;
> +
> + dbus_free(data);
> + dbus_message_iter_append_basic(iter, DBUS_TYPE_UINT32,
> &bss_expiration_age);
> +}
> +
> +int g_supplicant_interface_set_bss_expiration_age(GSupplicantInterface
> *interface,
> + unsigned int
> bss_expiration_age)
> +{
> + struct g_supplicant_bss_expiration_age *data;
> + int ret;
> +
> + data = dbus_malloc0(sizeof(*data));
> +
> + if (!data)
> + return -ENOMEM;
> +
> + data->bss_expiration_age = bss_expiration_age;
> + data->interface = interface;
> +
> + ret = supplicant_dbus_property_set(interface->path,
> + SUPPLICANT_INTERFACE ".Interface",
> + "BSSExpireAge", DBUS_TYPE_UINT32_AS_STRING,
> + set_bss_expiration_age, NULL, data, NULL);
> + if (ret < 0)
> + dbus_free(data);
> +
> + return ret;
> +}
I've added a modified version from the code above.
> +
> +
> struct set_apscan_data
> {
> unsigned int ap_scan;
> diff --git a/plugins/wifi.c b/plugins/wifi.c
> index 910b739..57b63e2 100644
> --- a/plugins/wifi.c
> +++ b/plugins/wifi.c
> @@ -1522,6 +1522,7 @@ static void interface_create_callback(int result,
> void *user_data)
> {
> struct wifi_data *wifi = user_data;
> + char * bgscan_range_max;
>
> DBG("result %d ifname %s, wifi %p", result,
> g_supplicant_interface_get_ifname(interface),
> @@ -1537,6 +1538,13 @@ static void interface_create_callback(int result,
> wifi->interface_ready = true;
> finalize_interface_creation(wifi);
> }
> + /* Force the BSS expiration age to match ConnMan long scanning
> interval to avoid the loss of networks during a long interval between
> two scannings. */
> + if ((bgscan_range_max = strrchr(BGSCAN_DEFAULT,':')) != NULL &&
> +
> g_supplicant_interface_set_bss_expiration_age(interface,
> strtol(bgscan_range_max + 1, (char**)NULL, 10) + 10) >= 0) {
> + DBG("bss expiration age successfully updated");
> + } else {
> + DBG("bss expiration age update has failed");
> + }
> }
Not sure if this is complete correct. BGSCAN_DEFAULT is only used when
background scaning is enabled. I think you should not use
BGSCAN_DEFAULT. Instead you need to check the scan interval first and
use that info.
Thanks,
Daniel
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 44, Issue 1
**************************************
