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] inet: Prevent glib source from being removed
twice (Daniel Wagner)
2. Re: [PATCH] service: Add EnableOnlineCheck config option
(Daniel Wagner)
3. Re: [PATCH] service: Add EnableOnlineCheck config option
(Daniel Wagner)
4. Re: [RFC] wifi: make max connection retries configurable
(Daniel Wagner)
5. Re: [RFC] wifi: make max connection retries configurable
(M?ns Rullg?rd)
6. Re: [PATCH] inet: Prevent glib source from being removed
twice (Slava Monich)
7. [PATCH] inet: Prevent glib source from being removed twice
(Slava Monich)
----------------------------------------------------------------------
Message: 1
Date: Wed, 1 Feb 2017 08:06:28 +0100
From: Daniel Wagner <[email protected]>
To: Slava Monich <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] inet: Prevent glib source from being removed
twice
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
On 01/31/2017 10:21 AM, Slava Monich wrote:
> On 31/01/17 09:29, Daniel Wagner wrote:
>>
>>> @@ -2262,7 +2265,7 @@ static int inet_rtnl_recv(GIOChannel *chan,
>>> gpointer user_data)
>>> rtnl_data->callback(h, rtnl_data->user_data);
>>> - inet_rtnl_cleanup(rtnl_data);
>>> + return 1;
>>> }
>> So we do not cleanup it here, instead we give back 1. Does this stand
>> for error or success? If it should be an error I would suggest to go
>> with -1 as with the rest in that function. On success just fall
>> through, but I guess you added return 1 on purpose, no?
>
> You may argue that this is unnecessary.
>
> 1. If we call inet_rtnl_cleanup here and return zero, the caller
> (inet_rtnl_event) would return TRUE telling glib to not remove the
> source, which is fine. Everything is cool.
> 2. With this change, inet_rtnl_recv returns non-zero, the caller
> (inet_rtnl_event) zeros watch_id, calls inet_rtnl_cleanup and returns
> FALSE telling glib to remove the source. The result is the same.
inet_rtnl_event() checks for ret != 0 that means we wouldn't clean up
anything in 2), no?
> You can think of it as an attempt to simplify the interface between
> inet_rtnl_event and inet_rtnl_recv - clean things up if the latter
> returns non-zero, do nothing if it returns zero.
>
> Good catch. Do you want to remove this change? Strictly speaking it
> doesn't fix anything.
Right, I just stumbled over it because it stands out. Starring a bit
longer out the code I see
icmpv6_nd_event() does 'if (ret == 0) return TRUE;'
inet_rtnl_event() does 'if (ret_!= 0) return TRUE;'
Maybe it's a good time to make those all path the same and then we add a
comment saying what the return value means. Maybe adding an enum or
using error codes. What do you think?
Thanks,
Daniel
------------------------------
Message: 2
Date: Wed, 1 Feb 2017 08:09:31 +0100
From: Daniel Wagner <[email protected]>
To: Lukasz Nowak <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] service: Add EnableOnlineCheck config option
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
Hi Lukasz,
>> and we either add wispr.c an version for --disable-wispr in a #ifdef
>> block or maybe even better have a wispr-disabled.c (better name) which
>> contains just the READY to ONLINE transistion. And the configure
>> script just adds one or the other to the build.
>
> If that's the decision, I can code it this way, no problem, in the next few
> days.
We have already parts like this. So it is consistent and actually makes
--disable-wispr work. So yes, let's go this way. Thanks a for doing the
work!
>> BTW, there are also couple more of the __connman_wispr_start() calls
>> in the service.c file. That needs also to be addressed.
>
> If we provide a new implementation of __connman_wispr_start() which always
> calls
> __connman_service_ipconfig_indicate_state(CONNMAN_SERVICE_STATE_ONLINE)
> all the cases will be handled.
>
> As far as I can see there is only other scenario when __connman_wispr_start()
> is called is when "Nameservers.Configuration" is changed.
>
> There are also two other places, but they are really the same scenario:
> - from redo_wispr() which can only happen when a previous
> __connman_wispr_start() fails, which will not happen after the change
> - from __connman_wpad_start(), which can only called in the same scenario,
> when a service transitions to READY state
Okay, so we get a consistent wispr use after all :)
Thanks,
Daniel
------------------------------
Message: 3
Date: Wed, 1 Feb 2017 08:16:35 +0100
From: Daniel Wagner <[email protected]>
To: Ingo Albrecht <[email protected]>
Cc: [email protected], 'Marcel Holtmann' <[email protected]>,
Slava Monich <[email protected]>
Subject: Re: [PATCH] service: Add EnableOnlineCheck config option
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
Hi Ingo
On 01/31/2017 08:29 PM, Ingo Albrecht wrote:
>> Hi Lukasz,
>>
>> On 01/26/2017 06:51 PM, Lukasz Nowak wrote:
>>> From: Lukasz Nowak <lnowak at tycoint.com>
>>>
>>> Global config option, which allows to enable/disable (enabled by default)
>>> use of http get in wispr to transition a default service from READY to
>>> ONLINE state.
>>
>> Isn't
>>
>> ./configure --disable-wispr
>>
>> good enough?
>>
>> Thanks,
>> Daniel
>
> Hi,
>
> no it isn't.
Lukasz has stepped up to get --disable-wispr doing what it is supposed
to do. So soon you should be able to turn it off for real.
> In fact the online check as it is done so far (default enabled, no
> option to turn it off, no mention of it in the manpage, no privacy
> policy available for the nginx server replying on how it cycles logs)
> can quickly get this project into trouble. The current implementation
> clearly violates privacy laws (EU-wide for starters).
These are good points.
> You clearly should not only implement a user-configurable option for
> it, but also default it to off (default off gets you a consent of the
> user to the use of the online check service).
I still hope to convince Marcel, that I prefer adding Salva's patches
which allow the user to set the server address for the online check.
> I can give you more input on the why, if you require it. But this
> case is _very_ clear.
Thanks,
Daniel
------------------------------
Message: 4
Date: Wed, 1 Feb 2017 08:21:06 +0100
From: Daniel Wagner <[email protected]>
To: M?ns Rullg?rd <[email protected]>
Cc: [email protected], Patrik Flykt <[email protected]>
Subject: Re: [RFC] wifi: make max connection retries configurable
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
On 01/31/2017 11:00 AM, M?ns Rullg?rd wrote:
>> On 01/29/2017 08:42 PM, M?ns Rullg?rd wrote:
>>> What about making it a per-connection setting stored along with ssid and
>>> psk?
>>
>> Sorry for the delay. Okay that avoid the global setting. If you don't
>> insist having a D-Bus option for this then I am fine with it. I say
>> that because Marcel wont be too happy if we add something like this to
>> the D-Bus API. But having it in the config file sounds like a good
>> compromise to me.
>
> Well, in our system a per-connection setting would need to be accessible
> over d-bus.
So that means we have three options:
1) connman.conf, global knob
2) config file no D-Bus API
3) D-Bus API
All of them have their downside. Patrik what is your stand?
------------------------------
Message: 5
Date: Wed, 01 Feb 2017 10:32:52 +0000
From: M?ns Rullg?rd <[email protected]>
To: Daniel Wagner <[email protected]>
Cc: [email protected], Patrik Flykt <[email protected]>
Subject: Re: [RFC] wifi: make max connection retries configurable
Message-ID: <[email protected]>
Content-Type: text/plain; charset=iso-8859-1
Daniel Wagner <[email protected]> writes:
> On 01/31/2017 11:00 AM, M?ns Rullg?rd wrote:
>>> On 01/29/2017 08:42 PM, M?ns Rullg?rd wrote:
>>>> What about making it a per-connection setting stored along with ssid and
>>>> psk?
>>>
>>> Sorry for the delay. Okay that avoid the global setting. If you don't
>>> insist having a D-Bus option for this then I am fine with it. I say
>>> that because Marcel wont be too happy if we add something like this to
>>> the D-Bus API. But having it in the config file sounds like a good
>>> compromise to me.
>>
>> Well, in our system a per-connection setting would need to be accessible
>> over d-bus.
>
> So that means we have three options:
>
> 1) connman.conf, global knob
> 2) config file no D-Bus API
> 3) D-Bus API
>
> All of them have their downside. Patrik what is your stand?
To me, 2) has the distinct downside of not being useful.
--
M?ns Rullg?rd
------------------------------
Message: 6
Date: Wed, 1 Feb 2017 12:46:40 +0200
From: Slava Monich <[email protected]>
To: Daniel Wagner <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] inet: Prevent glib source from being removed
twice
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
On 01/02/17 09:06, Daniel Wagner wrote:
> On 01/31/2017 10:21 AM, Slava Monich wrote:
>> On 31/01/17 09:29, Daniel Wagner wrote:
>>>
>>>> @@ -2262,7 +2265,7 @@ static int inet_rtnl_recv(GIOChannel *chan,
>>>> gpointer user_data)
>>>> rtnl_data->callback(h, rtnl_data->user_data);
>>>> - inet_rtnl_cleanup(rtnl_data);
>>>> + return 1;
>>>> }
>>> So we do not cleanup it here, instead we give back 1. Does this stand
>>> for error or success? If it should be an error I would suggest to go
>>> with -1 as with the rest in that function. On success just fall
>>> through, but I guess you added return 1 on purpose, no?
>>
>> You may argue that this is unnecessary.
>>
>> 1. If we call inet_rtnl_cleanup here and return zero, the caller
>> (inet_rtnl_event) would return TRUE telling glib to not remove the
>> source, which is fine. Everything is cool.
>> 2. With this change, inet_rtnl_recv returns non-zero, the caller
>> (inet_rtnl_event) zeros watch_id, calls inet_rtnl_cleanup and returns
>> FALSE telling glib to remove the source. The result is the same.
>
> inet_rtnl_event() checks for ret != 0 that means we wouldn't clean up
> anything in 2), no?
>
>> You can think of it as an attempt to simplify the interface between
>> inet_rtnl_event and inet_rtnl_recv - clean things up if the latter
>> returns non-zero, do nothing if it returns zero.
>>
>> Good catch. Do you want to remove this change? Strictly speaking it
>> doesn't fix anything.
>
> Right, I just stumbled over it because it stands out. Starring a bit
> longer out the code I see
>
> icmpv6_nd_event() does 'if (ret == 0) return TRUE;'
> inet_rtnl_event() does 'if (ret_!= 0) return TRUE;'
>
YES, you are absolutely right! icmpv6_event, icmpv6_rs_event and
icmpv6_nd_event check for (ret == 0), but inet_rtnl_event checks for
(ret != 0). This code is cleverly structured to confuse the reader. The
first three create the impression of consistency and then a sudden twist
of logic catches the reader off guard. Good lesson. Always expect the
unexpected!
Actually, I only caught one such case with gdb (that was icmpv6_rs_event
and __connman_inet_ipv6_stop_recv_rs), the other cases were extrapolated
from that experience.
> Maybe it's a good time to make those all path the same and then we add
> a comment saying what the return value means. Maybe adding an enum or
> using error codes. What do you think?
I'll submit another patch with inet_rtnl_event doing 'if (ret == 0)
return TRUE' just as it's done in all other places. Hopefully, I get the
logic right this time. As for the other improvements, the whole thing is
so much not my style that I don't feel myself to be in a position to
propose anything. It can all be done in so many difference ways. It's up
to you.
> Thanks,
> Daniel
>
Cheers,
-Slava
------------------------------
Message: 7
Date: Wed, 1 Feb 2017 15:37:01 +0200
From: Slava Monich <[email protected]>
To: [email protected]
Subject: [PATCH] inet: Prevent glib source from being removed twice
Message-ID: <[email protected]>
If GIOFunc callback returns FALSE, the event source is removed
by glib. We either call g_source_remove or return FALSE from the
callback. If we do both, the source is getting removed twice which
glib considers a critical error.
---
src/inet.c | 52 ++++++++++++++++++++++++++++------------------------
1 file changed, 28 insertions(+), 24 deletions(-)
diff --git a/src/inet.c b/src/inet.c
index 553b57a..99151b0 100644
--- a/src/inet.c
+++ b/src/inet.c
@@ -1252,13 +1252,12 @@ static gboolean rs_timeout_cb(gpointer user_data)
return FALSE;
}
-static int icmpv6_recv(int fd, gpointer user_data)
+static int icmpv6_recv(int fd, struct xs_cb_data *data)
{
struct msghdr mhdr;
struct iovec iov;
unsigned char chdr[CMSG_BUF_LEN];
unsigned char buf[1540];
- struct xs_cb_data *data = user_data;
struct nd_router_advert *hdr;
struct sockaddr_in6 saddr;
ssize_t len;
@@ -1280,7 +1279,6 @@ static int icmpv6_recv(int fd, gpointer user_data)
len = recvmsg(fd, &mhdr, 0);
if (len < 0) {
cb(NULL, 0, data->user_data);
- xs_cleanup(data);
return -errno;
}
@@ -1291,7 +1289,6 @@ static int icmpv6_recv(int fd, gpointer user_data)
return 0;
cb(hdr, len, data->user_data);
- xs_cleanup(data);
return len;
}
@@ -1299,18 +1296,21 @@ static int icmpv6_recv(int fd, gpointer user_data)
static gboolean icmpv6_event(GIOChannel *chan, GIOCondition cond, gpointer
data)
{
int fd, ret;
+ struct xs_cb_data *xs_data = data;
DBG("");
if (cond & (G_IO_NVAL | G_IO_HUP | G_IO_ERR))
- return FALSE;
+ goto cleanup;
fd = g_io_channel_unix_get_fd(chan);
- ret = icmpv6_recv(fd, data);
+ ret = icmpv6_recv(fd, xs_data);
if (ret == 0)
return TRUE;
- return FALSE;
+cleanup:
+ xs_cleanup(xs_data);
+ return TRUE;
}
/* Adapted from RFC 1071 "C" Implementation Example */
@@ -1667,13 +1667,12 @@ void __connman_inet_ipv6_stop_recv_rs(void *context)
xs_cleanup(context);
}
-static int icmpv6_rs_recv(int fd, gpointer user_data)
+static int icmpv6_rs_recv(int fd, struct xs_cb_data *data)
{
struct msghdr mhdr;
struct iovec iov;
unsigned char chdr[CMSG_BUF_LEN];
unsigned char buf[1540];
- struct xs_cb_data *data = user_data;
struct nd_router_solicit *hdr;
struct sockaddr_in6 saddr;
ssize_t len;
@@ -1712,17 +1711,20 @@ static gboolean icmpv6_rs_event(GIOChannel *chan,
GIOCondition cond,
gpointer data)
{
int fd, ret;
+ struct xs_cb_data *xs_data = data;
DBG("");
if (cond & (G_IO_NVAL | G_IO_HUP | G_IO_ERR))
- return FALSE;
+ goto cleanup;
fd = g_io_channel_unix_get_fd(chan);
- ret = icmpv6_rs_recv(fd, data);
+ ret = icmpv6_rs_recv(fd, xs_data);
if (ret == 0)
return TRUE;
+cleanup:
+ xs_data->watch_id = 0;
return FALSE;
}
@@ -1797,13 +1799,12 @@ static gboolean ns_timeout_cb(gpointer user_data)
return FALSE;
}
-static int icmpv6_nd_recv(int fd, gpointer user_data)
+static int icmpv6_nd_recv(int fd, struct xs_cb_data *data)
{
struct msghdr mhdr;
struct iovec iov;
unsigned char chdr[CMSG_BUF_LEN];
unsigned char buf[1540];
- struct xs_cb_data *data = user_data;
struct nd_neighbor_advert *hdr;
struct sockaddr_in6 saddr;
ssize_t len;
@@ -1825,7 +1826,6 @@ static int icmpv6_nd_recv(int fd, gpointer user_data)
len = recvmsg(fd, &mhdr, 0);
if (len < 0) {
cb(NULL, 0, &data->addr.sin6_addr, data->user_data);
- xs_cleanup(data);
return -errno;
}
@@ -1844,7 +1844,6 @@ static int icmpv6_nd_recv(int fd, gpointer user_data)
return 0;
cb(hdr, len, &data->addr.sin6_addr, data->user_data);
- xs_cleanup(data);
return len;
}
@@ -1853,18 +1852,21 @@ static gboolean icmpv6_nd_event(GIOChannel *chan,
GIOCondition cond,
gpointer data)
{
int fd, ret;
+ struct xs_cb_data *xs_data = data;
DBG("");
if (cond & (G_IO_NVAL | G_IO_HUP | G_IO_ERR))
- return FALSE;
+ goto cleanup;
fd = g_io_channel_unix_get_fd(chan);
- ret = icmpv6_nd_recv(fd, data);
+ ret = icmpv6_nd_recv(fd, xs_data);
if (ret == 0)
return TRUE;
- return FALSE;
+cleanup:
+ xs_cleanup(xs_data);
+ return TRUE;
}
int __connman_inet_ipv6_do_dad(int index, int timeout_ms,
@@ -2188,9 +2190,8 @@ static gboolean inet_rtnl_timeout_cb(gpointer user_data)
return FALSE;
}
-static int inet_rtnl_recv(GIOChannel *chan, gpointer user_data)
+static int inet_rtnl_recv(GIOChannel *chan, struct inet_rtnl_cb_data
*rtnl_data)
{
- struct inet_rtnl_cb_data *rtnl_data = user_data;
struct __connman_inet_rtnl_handle *rth = rtnl_data->rtnl;
struct nlmsghdr *h = NULL;
struct sockaddr_nl nladdr;
@@ -2272,17 +2273,20 @@ static gboolean inet_rtnl_event(GIOChannel *chan,
GIOCondition cond,
gpointer user_data)
{
int ret;
+ struct inet_rtnl_cb_data *rtnl_data = user_data;
DBG("");
if (cond & (G_IO_NVAL | G_IO_HUP | G_IO_ERR))
- return FALSE;
+ goto cleanup;
- ret = inet_rtnl_recv(chan, user_data);
- if (ret != 0)
+ ret = inet_rtnl_recv(chan, rtnl_data);
+ if (ret == 0)
return TRUE;
- return FALSE;
+cleanup:
+ inet_rtnl_cleanup(rtnl_data);
+ return TRUE;
}
int __connman_inet_rtnl_talk(struct __connman_inet_rtnl_handle *rtnl,
--
1.9.1
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 16, Issue 1
**************************************
