Send connman mailing list submissions to
connman@lists.01.org
To subscribe or unsubscribe via email, send a message with subject or
body 'help' to
connman-requ...@lists.01.org
You can reach the person managing the list at
connman-ow...@lists.01.org
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. Re: [PATCH] wifi: Fix connection before disconnection completed
(Daniel Wagner)
2. Re: [PATCH] service: Fix autoconnect after rejection
(Daniel Wagner)
3. RE: [PATCH] wifi: Fix connection before disconnection completed
(VAUTRIN Emmanuel (Canal Plus Prestataire))
4. RE: [PATCH] service: Fix autoconnect after rejection
(VAUTRIN Emmanuel (Canal Plus Prestataire))
5. RE: [PATCH] service: Fix autoconnect after rejection
(Matthijs Vader)
----------------------------------------------------------------------
Date: Mon, 25 Jan 2021 09:45:56 +0100
From: Daniel Wagner <w...@monom.org>
Subject: Re: [PATCH] wifi: Fix connection before disconnection
completed
To: "VAUTRIN Emmanuel (Canal Plus Prestataire)"
<emmanuel.vaut...@cpexterne.org>
Cc: "connman@lists.01.org" <connman@lists.01.org>
Message-ID: <20210125084556.gih6otujkxqjk...@beryllium.lan>
Content-Type: text/plain; charset=iso-8859-1
Hi,
On Mon, Jan 18, 2021 at 01:05:11PM +0000, VAUTRIN Emmanuel (Canal Plus
Prestataire) wrote:
> From 08aefcb3bad90041505acaf95400c5a1ac9c711a Mon Sep 17 00:00:00 2001
> From: Gabriel FORTE <gfo...@wyplay.com>
> Date: Fri, 15 Jan 2021 19:40:00 +0100
> Subject: [PATCH] wifi: Fix connection before disconnection completed
>
> Prevents an input/output error when a network tries to connect before
> the complete disconnection of the former connected one.
> ---
> plugins/wifi.c | 51 +++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 44 insertions(+), 7 deletions(-)
>
> diff --git a/plugins/wifi.c b/plugins/wifi.c
> index fc304e3b..8975a9ff 100644
> --- a/plugins/wifi.c
> +++ b/plugins/wifi.c
> @@ -165,6 +165,11 @@ struct wifi_data {
> int assoc_code;
> };
>
> +struct disconnect_cb_data {
s/disconnect_cb_data/disconnect_data/
> + struct wifi_data *wifi;
> + struct connman_network *network;
> +};
> +
> static GList *iface_list = NULL;
>
> static GList *pending_wifi_device = NULL;
> @@ -2220,18 +2225,32 @@ static int network_connect(struct connman_network
> *network)
> static void disconnect_callback(int result, GSupplicantInterface *interface,
> void *user_data)
> {
> - struct wifi_data *wifi = user_data;
> + struct disconnect_cb_data *cb_data = user_data;
> + struct wifi_data *wifi = cb_data->wifi;
> + struct connman_network *network = cb_data->network;
Generally we try to follow the inverse christmas tree style.
> +
> + g_free(cb_data);
>
> - DBG("result %d supplicant interface %p wifi %p",
> - result, interface, wifi);
> + DBG("result %d supplicant interface %p wifi %p networks: current %p \
> + pending %p disconnected %p", result, interface, wifi,
> + wifi->network, wifi->pending_network, network);
>
> if (result == -ECONNABORTED) {
> DBG("wifi interface no longer available");
> return;
> }
>
> - if (wifi->network && wifi->network != wifi->pending_network)
> - connman_network_set_connected(wifi->network, false);
> + if (network)
Useless, the network pointer wont ever be NULL.
> + connman_network_set_connected(network, false);
> +
> + if (network != wifi->network) {
> + if (network == wifi->pending_network) {
> + wifi->pending_network = NULL;
> + }
> + DBG("current wifi network has changed since disconnection");
> + return;
> + }
> +
> wifi->network = NULL;
>
> wifi->disconnecting = false;
> @@ -2250,6 +2269,7 @@ static int network_disconnect(struct connman_network
> *network)
> struct connman_device *device = connman_network_get_device(network);
> struct wifi_data *wifi;
> int err;
> + struct disconnect_cb_data *cb_data;
>
> DBG("network %p", network);
>
> @@ -2264,8 +2284,16 @@ static int network_disconnect(struct connman_network
> *network)
>
> wifi->disconnecting = true;
>
> + cb_data = g_try_malloc0(sizeof(*cb_data));
Just use g_malloc0((), there is little point in trying to catch failing
small memory allocation. The system will be completely useless before
we ever see this allocation failing. GLib will fail internally for any
small allocation too.
> + if (!cb_data) {
> + connman_error("Failed to allocate cb_data");
> + return -ENOMEM;
> + }
> +
> + cb_data->wifi = wifi;
> + cb_data->network = network;
> err = g_supplicant_interface_disconnect(wifi->interface,
> - disconnect_callback, wifi);
> + disconnect_callback, cb_data);
> if (err < 0)
> wifi->disconnecting = false;
Leaks cb_data.
>
> @@ -2387,9 +2415,18 @@ static bool handle_wps_completion(GSupplicantInterface
> *interface,
>
> if (!wps_ssid || wps_ssid_len != ssid_len ||
> memcmp(ssid, wps_ssid, ssid_len) != 0) {
> + struct disconnect_cb_data *cb_data =
> + g_try_malloc0(sizeof(*cb_data));
> + if (!cb_data) {
> + connman_error("Failed to allocate cb_data");
> + return false;
> + }
> +
> + cb_data->wifi = wifi;
> + cb_data->network = network;
> connman_network_set_associating(network, false);
> g_supplicant_interface_disconnect(wifi->interface,
> - disconnect_callback, wifi);
> + disconnect_callback, cb_data);
> return false;
> }
>
I've fixed the patch accordingly and pushed the patch.
Thanks,
Daniel
------------------------------
Date: Mon, 25 Jan 2021 09:56:13 +0100
From: Daniel Wagner <w...@monom.org>
Subject: Re: [PATCH] service: Fix autoconnect after rejection
To: "VAUTRIN Emmanuel (Canal Plus Prestataire)"
<emmanuel.vaut...@cpexterne.org>
Cc: "connman@lists.01.org" <connman@lists.01.org>
Message-ID: <20210125085613.kodtefbcosgax...@beryllium.lan>
Content-Type: text/plain; charset=us-ascii
Hi Emmanuel,
On Wed, Jan 13, 2021 at 07:00:18PM +0000, VAUTRIN Emmanuel (Canal Plus
Prestataire) wrote:
> Hi Daniel,
>
> I have been able to reproduce this issue, in a wifi environment with only 1
> AP,
> that I turn off during the association step, to set the service in failure
> state. ("State": "failure",
> "Error": "blocked" or "Error": "connect-failed"). In this case, it
> stays blocked in this state.
Yes, this is a design decision. When we get disconnected from an AP we
can't assume it's just because we had a bad connection. The AP could
actively disconnect the station because the password has changed. And if
we keep retry to connect we could get blocked completely. That is why
there is no automatic state transition from the FAILURE state to IDLE.
As this is a recurring topic, I think we could try to be a bit
user friendly as not all AP will do this.
> > 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.
>
> I have followed your recommandation, and propose the following patch, fixing
> it:
> -----------------------------------------------------------------------------------------
> From 9df3909aa487eb1f8c8796d82b0ee09adcead36b Mon Sep 17 00:00:00 2001
> From: Emmanuel VAUTRIN <emmanuel.vaut...@cpexterne.org>
> Date: Wed, 13 Jan 2021 18:19:55 +0100
> Subject: [PATCH] service: Fix autoconnection of a service in failure state
>
> Prevents a service to be blocked in failure state, when its
> autoconnection fails.
> ---
> src/service.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/src/service.c b/src/service.c
> index 5a582cdf..64ee1f44 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -4186,6 +4186,12 @@ static bool auto_connect_service(GList *services,
> return autoconnecting;
> }
>
> + if (service->state == CONNMAN_SERVICE_STATE_FAILURE) {
> + /* Back to idle to unblock next autoconnection. */
> + service->state = CONNMAN_SERVICE_STATE_IDLE;
> + continue;
> + }
What about adding a retry counter and limit the retries? To prevent the
situation above, we could export the retry limit to the D-Bus API. So if
an user has such a network setup where constant retry will result in a
blocked account just try N times. What do you think? Would that work for
you?
Thanks,
Daniel
------------------------------
Date: Mon, 25 Jan 2021 14:28:09 +0000
From: "VAUTRIN Emmanuel (Canal Plus Prestataire)"
<emmanuel.vaut...@cpexterne.org>
Subject: RE: [PATCH] wifi: Fix connection before disconnection
completed
To: Daniel Wagner <w...@monom.org>
Cc: "connman@lists.01.org" <connman@lists.01.org>
Message-ID: <pr1pr02mb479485b477509013e5a0be8e93...@pr1pr02mb4794.eur
prd02.prod.outlook.com>
Content-Type: text/plain; charset="iso-8859-1"
Thank you Daniel for your detailed code review.
------------------------------
Date: Mon, 25 Jan 2021 16:06:51 +0000
From: "VAUTRIN Emmanuel (Canal Plus Prestataire)"
<emmanuel.vaut...@cpexterne.org>
Subject: RE: [PATCH] service: Fix autoconnect after rejection
To: Daniel Wagner <w...@monom.org>
Cc: "connman@lists.01.org" <connman@lists.01.org>
Message-ID: <pr1pr02mb47940e0da1edf82d0a0c9fae93...@pr1pr02mb4794.eur
prd02.prod.outlook.com>
Content-Type: text/plain; charset="iso-8859-1"
All right Daniel,
It is clearer now.
> Yes, this is a design decision. When we get disconnected from an AP we
> can't assume it's just because we had a bad connection. The AP could
> actively disconnect the station because the password has changed. And if
> we keep retry to connect we could get blocked completely. That is why
> there is no automatic state transition from the FAILURE state to IDLE.
I understand that. My main concern was to be able to leave the Failure state.
It has not to be automatic if it is a security limitation.
>As this is a recurring topic, I think we could try to be a bit
>user friendly as not all AP will do this.
> What about adding a retry counter and limit the retries? To prevent the
> situation above, we could export the retry limit to the D-Bus API. So if
> an user has such a network setup where constant retry will result in a
> blocked account just try N times. What do you think? Would that work for
> you?
No problem if you think it is cleaner to require an external intervention.
In this case, I can update our REST api to manage this, for example on
state update signals. I will follow your decision on the best (cleaner / safer)
way
to follow.
B.R.
Emmanuel
------------------------------
Date: Mon, 25 Jan 2021 19:56:41 +0000
From: Matthijs Vader <mva...@victronenergy.com>
Subject: RE: [PATCH] service: Fix autoconnect after rejection
To: "VAUTRIN Emmanuel (Canal Plus Prestataire)"
<emmanuel.vaut...@cpexterne.org>, Daniel Wagner <w...@monom.org>
Cc: "connman@lists.01.org" <connman@lists.01.org>
Message-ID: <as8pr07mb73184a9aef4615fbae576d52c5...@as8pr07mb7318.eur
prd07.prod.outlook.com>
Content-Type: text/plain; charset="us-ascii"
Hello Emmanuel, Daniel, and others,
Emmanuel wrote:
>
> All right Daniel,
>
> It is clearer now.
>
> Daniel wrote:
> > Yes, this is a design decision. When we get disconnected from an AP we
> > can't assume it's just because we had a bad connection. The AP could
> > actively disconnect the station because the password has changed. And if
> > we keep retry to connect we could get blocked completely. That is why
> > there is no automatic state transition from the FAILURE state to IDLE.
>
> I understand that. My main concern was to be able to leave the Failure state.
> It has not to be automatic if it is a security limitation.
>
> > As this is a recurring topic, I think we could try to be a bit
> > user friendly as not all AP will do this.
> > What about adding a retry counter and limit the retries? To prevent the
> > situation above, we could export the retry limit to the D-Bus API. So if
> > an user has such a network setup where constant retry will result in a
> > blocked account just try N times. What do you think? Would that work for
> > you?
>
> No problem if you think it is cleaner to require an external intervention.
> In this case, I can update our REST api to manage this, for example on
> state update signals. I will follow your decision on the best (cleaner /
> safer) way
> to follow.
There is a patch set, dated 2017 that adds a setting to configure the maximum
number of retries [1]. Including an option to set it negative, to retry
indefinitely.
Since then, we now have 10.000s of devices out there, many of which use wifi.
Many of those devices are completely unattended (ie. in an installed system
with no-one watching over it) or at the very least can be considered unattended
compared to what I understood was the original ConnMan use case: a phone. The
difference in perspective between a phone in someone his/her hands vs. an
unattended device with hours of travel to get to, makes quite the difference
for this.
We decided to make it retry the connection forever, which worked out well. At
least much better than the initial situation, where it would stop retrying
after a few attempts.
For details on how we're currently using (a by now perhaps slightly outdated)
ConnMan, please see [2].
Hope this helps, and have a good evening.
Matthijs
[1]
https://lists.01.org/hyperkitty/list/connman@lists.01.org/thread/F5N4TBQKDYGUZPLZMNPQ75O4F2PZ7YTE/
[2]
https://github.com/victronenergy/meta-victronenergy/tree/master/meta-venus/recipes-connectivity/connman/connman
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list -- connman@lists.01.org
To unsubscribe send an email to connman-le...@lists.01.org
------------------------------
End of connman Digest, Vol 63, Issue 9
**************************************
