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 1/2] ipconfig: Handle PrefixLength attribute for
IPv4 by D-Bus (Daniel Wagner)
2. Re: [PATCH 1/2] inet: Add connman_inet_compare_ipv6_subnet()
(Daniel Wagner)
3. Re: Is independent control of rfkill possible? (Daniel Wagner)
4. Re: [PATCH] agent: Always set driver for queued message.
(Daniel Wagner)
5. Re: Connman and IWD - Scanning Network - Update? (Daniel Wagner)
6. Re: [PATCH 1/3] Added timeout to socket connections
(Daniel Wagner)
7. Re: [PATCH 2/3] Added logic to perform perpetual online
checks (Daniel Wagner)
----------------------------------------------------------------------
Message: 1
Date: Tue, 24 Sep 2019 08:07:00 +0200
From: Daniel Wagner <[email protected]>
To: Benjamin Cama <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 1/2] ipconfig: Handle PrefixLength attribute for
IPv4 by D-Bus
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Benjamin,
On Tue, Sep 10, 2019 at 05:52:58PM +0200, Benjamin Cama wrote:
> ---
> src/ipaddress.c | 6 +++++-
> src/ipconfig.c | 18 +++++++++++++-----
> 2 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/src/ipaddress.c b/src/ipaddress.c
> index d63d95c3..ebdabbb7 100644
> --- a/src/ipaddress.c
> +++ b/src/ipaddress.c
> @@ -149,7 +149,11 @@ int connman_ipaddress_set_ipv4(struct connman_ipaddress
> *ipaddress,
>
> ipaddress->family = AF_INET;
>
> - ipaddress->prefixlen = connman_ipaddress_calc_netmask_len(netmask);
> + /* check if netmask or prefixlen */
> + if (g_strrstr(netmask, "."))
> + ipaddress->prefixlen =
> connman_ipaddress_calc_netmask_len(netmask);
> + else
> + ipaddress->prefixlen = g_ascii_strtoull(netmask, NULL, 10);
I think we should change connman_ipaddress_set_ipv4() to match
connman_ipaddress_set_ipv6() in regards of the argument
list. Basically, change the netmask argument to prefix len and only
pass that in. And create the netmask here again. That is slightly
suboptimal but I prefer that we keep the both function more
alike.
>
> g_free(ipaddress->local);
> ipaddress->local = g_strdup(address);
> diff --git a/src/ipconfig.c b/src/ipconfig.c
> index 25657733..b5ad1a36 100644
> --- a/src/ipconfig.c
> +++ b/src/ipconfig.c
> @@ -1997,7 +1997,9 @@ int __connman_ipconfig_set_config(struct
> connman_ipconfig *ipconfig,
>
> dbus_message_iter_get_basic(&value, &prefix_length);
>
> - if (prefix_length < 0 || prefix_length > 128)
> + if (ipconfig->type == CONNMAN_IPCONFIG_TYPE_IPV6 &&
> (prefix_length < 0 || prefix_length > 128))
> + return -EINVAL;
> + else if (ipconfig->type == CONNMAN_IPCONFIG_TYPE_IPV4
> && (prefix_length < 0 || prefix_length > 32))
Wrap the two lines. They are a bit too long.
> return -EINVAL;
> } else if (g_str_equal(key, "Netmask")) {
> if (type != DBUS_TYPE_STRING)
> @@ -2071,10 +2073,16 @@ int __connman_ipconfig_set_config(struct
> connman_ipconfig *ipconfig,
>
> ipconfig->method = method;
>
> - if (ipconfig->type == CONNMAN_IPCONFIG_TYPE_IPV4)
> - connman_ipaddress_set_ipv4(ipconfig->address,
> - address, netmask, gateway);
> - else
> + if (ipconfig->type == CONNMAN_IPCONFIG_TYPE_IPV4) {
> + if (prefix_length) {
> + char prefix_length_str[3];
> + sprintf(prefix_length_str, "%d", prefix_length);
calculate the prefix_length from netmaks above whenever the
prefix_length is not set. Then you don't need to handle special case
inse this if block.
BTW, isn't the buffer too short? The string can be "128" which is 4
bytes including the \0 char.
And you should also update the documentation regarding the fact that
prefix length will overwrite your netmask setting.
Thanks,
Daniel
------------------------------
Message: 2
Date: Tue, 24 Sep 2019 08:18:18 +0200
From: Daniel Wagner <[email protected]>
To: Santtu Lakkala <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 1/2] inet: Add connman_inet_compare_ipv6_subnet()
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Santtu,
On Fri, Sep 13, 2019 at 03:30:30PM +0300, Santtu Lakkala wrote:
> Add a helper to check if a IPv6 address is in the local network, similar
> to connman_inet_compare_subnet() for IPv4.
> ---
> include/inet.h | 1 +
> src/inet.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 60 insertions(+)
>
> diff --git a/include/inet.h b/include/inet.h
> index 9c1918f3..fdc2155f 100644
> --- a/include/inet.h
> +++ b/include/inet.h
> @@ -51,6 +51,7 @@ int connman_inet_clear_gateway_address(int index, const
> char *gateway);
> int connman_inet_set_gateway_interface(int index);
> int connman_inet_clear_gateway_interface(int index);
> bool connman_inet_compare_subnet(int index, const char *host);
> +bool connman_inet_compare_ipv6_subnet(int index, const char *host);
> int connman_inet_set_ipv6_address(int index,
> struct connman_ipaddress *ipaddress);
> int connman_inet_clear_ipv6_address(int index,
> diff --git a/src/inet.c b/src/inet.c
> index b128e578..ff32fce2 100644
> --- a/src/inet.c
> +++ b/src/inet.c
> @@ -1116,6 +1116,65 @@ bool connman_inet_compare_subnet(int index, const char
> *host)
> return ((if_addr & netmask_addr) == (host_addr & netmask_addr));
> }
>
> +static bool mem_mask_equal(const void *a, const void *b,
> + const void *mask, size_t n)
> +{
> + size_t i;
> +
> + for (i = 0; i < n; i++) {
> + if ((((unsigned char *)a)[i] ^ ((unsigned char *)b)[i]) &
> + ((unsigned char *)mask)[i])
> + return false;
> + }
Could you please define local variables with the right type at the
beginning of the function and assign them, a, b and mask? This is not
really pleasant to read, e.g.
unsigned char *addr1 = (unsigned char *)a;
...
if (addr1[i] ^ (addr2[i] & mask[i]))
Rest looks good.
Thanks,
Daniel
------------------------------
Message: 3
Date: Tue, 24 Sep 2019 08:21:53 +0200
From: Daniel Wagner <[email protected]>
To: "Messer, David" <[email protected]>
Cc: "[email protected]" <[email protected]>
Subject: Re: Is independent control of rfkill possible?
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi David
On Mon, Sep 16, 2019 at 07:58:43PM +0000, Messer, David wrote:
> Hi,
>
> I have a device with two wi-fi interfaces, one for AP mode and the other for
> STA mode. I would like to control the device so one could operate AP or STA
> individually. The first interface is running hostapd. On the second
> interface, I would like to run connman. The problem I'm running into has to
> do with rfkill. If I unblock the wi-fi radio for AP mode, connman sees this
> event and powers up the wi-fi (STA) technology. Likewise, if I turn off STA
> mode, connman powers down wi-fi technology which then blocks the radio for AP
> mode. Is there any way to get around this?
Did you tell ConnMan to ignore the AP interface, e.g. by connmand -I
wlan0? I haven't played with such a setup but I belive someone
reported that such a setup is possible to get working with ConnMan.
Thanks,
Daniel
------------------------------
Message: 4
Date: Tue, 24 Sep 2019 08:24:11 +0200
From: Daniel Wagner <[email protected]>
To: Jussi Laakkonen <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] agent: Always set driver for queued message.
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Jussi,
On Wed, Sep 18, 2019 at 11:13:47AM +0300, Jussi Laakkonen wrote:
> This addresses the issue of not being able to cancel agent queries in
> case the query timeouts or when component (e.g., a VPN plugin) has
> requested something via agent and the component dies when the query is
> still active. As a result the queries are stacked on top of another if
> the agent creates new one for every query.
>
> It is imperative to always set the driver for the message to be queued
> via agent.c because otherwise sending cancel request will fail. This is
> because the cancel request would be sent to a NULL interface as driver
> is omitted. In this case, following is logged:
>
> connman-vpnd[4835]: src/agent.c:send_cancel_request() send cancel req to
> :1.2951 /org/agent/path/vpnagent iface (null)
>
> dbus-daemon[1553]: [system] Rejected send message, 2 matched rules;
> type="method_call", sender=":1.3387" (uid=0 pid=4835
> comm="/usr/sbin/connman-vpnd -d -n") interface="(unset)" member="Cancel"
> error name="(unset)" requested_reply="0" destination=":1.2951"
> (uid=100000 pid=27084 comm="/usr/bin/agent")
Patch applied. As usual, excellent commit message!
Thanks,
Daniel
------------------------------
Message: 5
Date: Tue, 24 Sep 2019 08:25:40 +0200
From: Daniel Wagner <[email protected]>
To: stef204 <[email protected]>
Cc: [email protected]
Subject: Re: Connman and IWD - Scanning Network - Update?
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi,
On Mon, Sep 09, 2019 at 06:09:26AM -0600, stef204 wrote:
> Hi,
>
> With reference to this
> <https://lists.01.org/pipermail/connman/2018-August/022915.html> and this
> <https://lists.01.org/pipermail/iwd/2018-August/004751.html> are there any
> updates or further progress to achieve scanning networks with ConnMan + IWD?
Sorry for the late response. Anyway, I haven't done anyting at this
front. But I planed to start working on this in the coming weeks. The
summer comes to end so there is more time for evening hackings :)
Thanks,
Daniel
------------------------------
Message: 6
Date: Tue, 24 Sep 2019 08:36:47 +0200
From: Daniel Wagner <[email protected]>
To: [email protected]
Cc: [email protected], Aleksandar Mitev <[email protected]>
Subject: Re: [PATCH 1/3] Added timeout to socket connections
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Aleksandar,
On Mon, Sep 09, 2019 at 03:43:53PM +0300, [email protected] wrote:
> From: Aleksandar Mitev <[email protected]>
>
> By default, sockets timeout after the system defined time which is
> too much all the timers in connman.
I am just wondering if it would make sense to pass down the
information from the calling side to gweb? see TODO down below
> This patch helps make online checks in a deterministic fashion.
This makes absolutely sense!
> ---
> gweb/gweb.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/gweb/gweb.c b/gweb/gweb.c
> index 393afe0a..5743042f 100644
> --- a/gweb/gweb.c
> +++ b/gweb/gweb.c
> @@ -77,6 +77,7 @@ struct web_session {
> GIOChannel *transport_channel;
> guint transport_watch;
> guint send_watch;
> + guint timeout_timer;
s/timeout_timer/timeout/
>
> guint resolv_action;
> guint address_action;
> @@ -164,6 +165,8 @@ static void free_session(struct web_session *session)
>
> web = session->web;
>
> + debug(web, "freeing session %p", session);
> +
> if (session->address_action > 0)
> g_source_remove(session->address_action);
>
> @@ -176,6 +179,9 @@ static void free_session(struct web_session *session)
> if (session->send_watch > 0)
> g_source_remove(session->send_watch);
>
> + if (session->timeout_timer > 0)
> + g_source_remove(session->timeout_timer);
> +
> if (session->transport_channel)
> g_io_channel_unref(session->transport_channel);
>
> @@ -1032,10 +1038,28 @@ static inline int bind_socket(int sk, int index, int
> family)
> return err;
> }
>
> +static gboolean socket_timeout_handler(gpointer user_data)
> +{
> + struct web_session *session = user_data;
> + int fd;
> +
> + if (session->transport_channel) {
> + debug(session->web, "No response, closing the socket");
> + fd = g_io_channel_unix_get_fd(session->transport_channel);
> + g_io_channel_shutdown(session->transport_channel, TRUE, NULL);
> + close(fd);
> + }
> +
> + session->timeout_timer = 0;
> + return FALSE;
> +}
> +
> static int connect_session_transport(struct web_session *session)
> {
> GIOFlags flags;
> int sk;
> + /* TODO: not sure if this is optimal enough */
> + const int socket_timeout = 20;
Not sure either, but hard coded numbers without a real argument why it
should be 20 sounds not too good. We have already some timeouts
defined for the online check. Should we use those numbers?
>
> sk = socket(session->addr->ai_family, SOCK_STREAM | SOCK_CLOEXEC,
> IPPROTO_TCP);
> @@ -1090,6 +1114,14 @@ static int connect_session_transport(struct
> web_session *session)
> G_IO_OUT | G_IO_HUP | G_IO_NVAL | G_IO_ERR,
> send_data, session);
>
> + debug(session->web, "Setting timeout to the socket to %ds",
> socket_timeout);
> + /* TODO: if this is not effective, resort to using mutexes
> + * Assign lower priority to this callback than that of the watch
> functions
> + * as we don't want the timeout to possibly interrupt last moment
> read/writes
> + */
Note the comments style is usually
/*
* asdf adsf
* asdf asdf
*/
About mutexes: the whole code base doesn't use any form of thread
synchronization primitives such as mutex because ConnMan is completely
single threaded. The work is splitet into different state machines. So
synchronization happens by either waiting to get scheduled or canceled
in the job queue.
Thanks,
Daniel
------------------------------
Message: 7
Date: Tue, 24 Sep 2019 08:48:58 +0200
From: Daniel Wagner <[email protected]>
To: [email protected]
Cc: [email protected], Aleksandar Mitev <[email protected]>
Subject: Re: [PATCH 2/3] Added logic to perform perpetual online
checks
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Aleksandar,
On Mon, Sep 09, 2019 at 03:43:54PM +0300, [email protected] wrote:
> From: Aleksandar Mitev <[email protected]>
>
> The feature activates when online state is reached and performs
> regular checks whether online state is still maintained by the
> services (default every 5 minutes). In the case then the
> online check fails the serice state is downgrated to Ready
> to give the chance for a switchover with another service.
> ---
> src/service.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 75 insertions(+), 4 deletions(-)
>
> diff --git a/src/service.c b/src/service.c
> index 3202f26c..f86baaa6 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -3440,17 +3440,23 @@ int __connman_service_reset_ipconfig(struct
> connman_service *service,
> #define ONLINE_CHECK_INITIAL_INTERVAL 1
> #define ONLINE_CHECK_MAX_INTERVAL 12
>
> -void __connman_service_wispr_start(struct connman_service *service,
> +static void reset_online_recheck_interval(struct connman_service *service,
> enum connman_ipconfig_type type)
> {
> - DBG("service %p type %s", service,
> __connman_ipconfig_type2string(type));
> -
> if (type == CONNMAN_IPCONFIG_TYPE_IPV4)
> service->online_check_interval_ipv4 =
> ONLINE_CHECK_INITIAL_INTERVAL;
> else
> service->online_check_interval_ipv6 =
> ONLINE_CHECK_INITIAL_INTERVAL;
> +}
> +
> +void __connman_service_wispr_start(struct connman_service *service,
> + enum connman_ipconfig_type type)
> +{
> + DBG("service %p type %s", service,
> __connman_ipconfig_type2string(type));
> +
> + reset_online_recheck_interval(service, type);
>
> __connman_wispr_start(service, type);
> }
> @@ -6039,18 +6045,64 @@ static gboolean redo_wispr_ipv6(gpointer user_data)
> return FALSE;
> }
>
> +/* When the online state is once reached perform fruther
> + * checks less frequently
> + */
Comment style. Besides that, we usually add this to the commit
message, since comments in the code are often completely outdated.
> +#define ONLINE_CHECK_REGULAR_INTERVAL 300
I guess this would make sense to move up to the begining of the file.
> +
> +static int __connman_service_online_perform_regular(struct connman_service
> *service,
> + enum connman_ipconfig_type type)
If the function is defined as static, you don't need to
prefix. __connman_* symbols are for all code inside the core (src/*.c)
and have a forward definition in src/connman.h.
> +{
> + GSourceFunc redo_func;
> + guint retry_interval = ONLINE_CHECK_REGULAR_INTERVAL;
> +
> + /* no use of double starting the check */
This comment is redundant to the next line. No additional information
in it, so just remove it.
> + if (service->online_timeout != 0)
> + return 0;
> +
> + if (type == CONNMAN_IPCONFIG_TYPE_IPV4) {
> + redo_func = redo_wispr_ipv4;
> + } else {
> + redo_func = redo_wispr_ipv6;
> + }
> + reset_online_recheck_interval(service, type);
> +
> + DBG("service %p (%s) will be rechecked for internet connectivity in
> %ds",
> + service, service ? service->identifier : NULL, retry_interval);
> +
> + service->online_timeout =
> + g_timeout_add_seconds(retry_interval, redo_func,
> connman_service_ref(service));
> +
> + return 0;
> +}
> +
> +static gboolean downgrade_state_handler(gpointer user_data)
> +{
> + struct connman_service *service = user_data;
> +
> + connman_warn("Downgrading service %s for online check did not pass",
> + service ? service->identifier : NULL);
> +
> + downgrade_state(service);
> +
> + return FALSE;
> +}
> +
> int __connman_service_online_check_failed(struct connman_service *service,
> enum connman_ipconfig_type type)
> {
> GSourceFunc redo_func;
> int *interval;
> + enum connman_service_state curr_state;
>
> if (type == CONNMAN_IPCONFIG_TYPE_IPV4) {
> interval = &service->online_check_interval_ipv4;
> redo_func = redo_wispr_ipv4;
> + curr_state = service->state_ipv4;
> } else {
> interval = &service->online_check_interval_ipv6;
> redo_func = redo_wispr_ipv6;
> + curr_state = service->state_ipv6;
> }
>
> DBG("service %p type %s interval %d", service,
> @@ -6064,6 +6116,12 @@ int __connman_service_online_check_failed(struct
> connman_service *service,
> */
> if (*interval < ONLINE_CHECK_MAX_INTERVAL)
> (*interval)++;
> + else {
> + if(curr_state == CONNMAN_SERVICE_STATE_ONLINE) {
> + g_idle_add(downgrade_state_handler, service);
> + return 0;
> + }
> + }
>
> return EAGAIN;
> }
> @@ -6131,8 +6189,16 @@ int __connman_service_ipconfig_indicate_state(struct
> connman_service *service,
> }
>
> /* Any change? */
> - if (old_state == new_state)
> + if (old_state == new_state) {
> + /* continuous online check requires restart of the check at
> regular intervals */
> + if (new_state == CONNMAN_SERVICE_STATE_ONLINE) {
> + connman_info("Online check continues for %s.\n",
> service->name);
This will clutter the log. Since you have already a DBG in the
function below, I would suggest to drop this info here.
> + __connman_service_online_perform_regular(service, type);
> + service_indicate_state(service);
Do we need the service_indicate_state() if nothing has changed?
> + }
> +
> return -EALREADY;
> + }
>
> DBG("service %p (%s) old state %d (%s) new state %d (%s) type %d (%s)",
> service, service ? service->identifier : NULL,
> @@ -6161,6 +6227,11 @@ int __connman_service_ipconfig_indicate_state(struct
> connman_service *service,
> set_mdns(service, service->mdns_config);
> break;
> case CONNMAN_SERVICE_STATE_ONLINE:
> + /* will reach here once, as next time should be from
> + * ONLINE to ONLINE, there is a special check above
> + */
Again, this comment is not really explaining something more in
detail. It just tells us what the next instructions are. So I don't
see any value in adding such comments.
> + connman_info("Online state reached for %s.\n", service->name);
The state machine already prints the state changes. So no need for an
additional info.
Thanks,
Daniel
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 47, Issue 12
***************************************
