12:43 p.m.
From: Aleksandar Mitev <shondll(a)yahoo.com>
Hi,
Thanks for the lively discussion on this proposed change.
Taking into account all the notes and concerns I think I
came up with a balanced solution:
- regular online check is now governed by a compile time optio
rather than a runtime one in the configration file
- the proposed update to the state machine is disabled by default
leaving the current behaviour in place.
Btw, please advise on a better use of compile time defines (if you
happen to find the current implementation not suiatable) as there
are no usual #ifdef..#endif blocks in the project.
On 9/4/19 5:22 PM, Slava Monich wrote:
>> As of v1.37, when a service reaches the READY state, then the online
>> check is started. If it succeeds, the a transition to ONLINE state
>> happens. You are right in thinking that my proposed change will cause
>> a downgrading to READY again, but you see, according to the state
>> machine, another round of online checking will begin, possibly
>> promoting the service back to ONLINE it it ever succeeds (ex. the
>> Internet disruption was temporary). In general, this is a chance for
>> any of the other services in READY state to take over.
>
> Are you sure that another service WILL actually take over when some
> other service is stuck in the READY state? That's the key point.
I agree. There are many small problems along the road.
Yes, it does happen in our use case (provided the number of failed
online checks is reached).
> If such a handover doesn't happen then this transition back
to READY is
> useless. It will be just generating additional network traffic and
> that's it.
>
> If handover does indeed happen then it's a major change of behavior and
> must be configurable. We (Sailfish OS) consider READY as of the
> "connected" states. We don't need and don't want to automatically
switch
> from WiFi to mobile data if WiFi gets stuck in the READY state. We still
> consider it connected and let the user to turn it off and switch to
> mobile data, if the user notices that it's not actually working.
Okay, understood. If we going to mess with the current state machine we
need to keep a compact mode. I suppose compile time would be good enough?
>> Finally, if a failed online check is not reliable enough to determine
>> Internet reachability, then it can be disabled altogether and replaced
>> with a simple ping check (from a separate service) as in the case of
>> mwan3 tool for Openwrt. However, in that case one will miss the
>> checking of DNS working.
>>
>> Please share more details if you think I am missing smth here.
>>
>
> Have you considered the VPN scenario? When VPN is connected, you may not
> be able to run online check for the transport interface - it won't have
> any routes to anything other than local subnet and the VPN server.
Indeed, VPN can be pretty tricky. This needs a lot of testing.
@Slava: I highly appreciate your feedback!
I haven't used connman with VPN, unfortunately for this is not a use case
in our application. So I believe there are a lot of other projects out there
that have similar setups to ours - simple in nework topology, but needing
a network manager trying its best to keep you connected. Those would really
only benefit from the proposed change.
Best,
Aleksandar
Aleksandar Mitev (3):
Added timeout to socket connections
Added logic to perform perpetual online checks
Defined a compile time config of perpetual online check
README | 10 ++++++
configure.ac | 6 ++++
gweb/gweb.c | 32 +++++++++++++++++++
src/service.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++---
4 files changed, 131 insertions(+), 4 deletions(-)
--
2.17.1
12:43 p.m.
New subject: [PATCH 1/3] Added timeout to socket connections
From: Aleksandar Mitev <shondll(a)yahoo.com>
By default, sockets timeout after the system defined time which is
too much all the timers in connman.
This patch helps make online checks in a deterministic fashion.
---
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;
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;
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
+ */
+ session->timeout_timer = g_timeout_add_seconds_full(G_PRIORITY_DEFAULT_IDLE,
+ socket_timeout, socket_timeout_handler, session, NULL);
+
return 0;
}
--
2.17.1
6:36 a.m.
New subject: [PATCH 1/3] Added timeout to socket connections
Hi Aleksandar,
On Mon, Sep 09, 2019 at 03:43:53PM +0300, aleksandar(a)analyticsfire.com wrote:
From: Aleksandar Mitev <shondll(a)yahoo.com>
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
12:43 p.m.
New subject: [PATCH 2/3] Added logic to perform perpetual online checks
From: Aleksandar Mitev <shondll(a)yahoo.com>
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
+ */
+#define ONLINE_CHECK_REGULAR_INTERVAL 300
+
+static int __connman_service_online_perform_regular(struct connman_service *service,
+ enum connman_ipconfig_type type)
+{
+ GSourceFunc redo_func;
+ guint retry_interval = ONLINE_CHECK_REGULAR_INTERVAL;
+
+ /* no use of double starting the check */
+ 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);
+ __connman_service_online_perform_regular(service, type);
+ service_indicate_state(service);
+ }
+
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
+ */
+ connman_info("Online state reached for %s.\n", service->name);
+ __connman_service_online_perform_regular(service, type);
break;
case CONNMAN_SERVICE_STATE_DISCONNECT:
if (service->state == CONNMAN_SERVICE_STATE_IDLE)
--
2.17.1
6:48 a.m.
New subject: [PATCH 2/3] Added logic to perform perpetual online checks
Hi Aleksandar,
On Mon, Sep 09, 2019 at 03:43:54PM +0300, aleksandar(a)analyticsfire.com wrote:
From: Aleksandar Mitev <shondll(a)yahoo.com>
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
12:43 p.m.
New subject: [PATCH 3/3] Defined a compile time config of perpetual online check
From: Aleksandar Mitev <shondll(a)yahoo.com>
New flag is
--enable-perpetual-online-check
and is disabled by default, leaving the original behaviour
in place. Even if enabled the regular check relies on:
EnableOnlineCheck = true
Otherwise it will have no effect.
---
README | 10 ++++++++++
configure.ac | 6 ++++++
src/service.c | 12 ++++++++++--
3 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/README b/README
index f16b9ec0..1ccf766c 100644
--- a/README
+++ b/README
@@ -231,6 +231,16 @@ For a working system, certain configuration options need to be
enabled:
is selected, ConnMan configures systemd-resolved to do DNS
resolving. The default value is "internal".
+ --enable-perpetual-online-check
+
+ Enable continuously checking for Internet connection.
+
+ If enabled, services will conduct online state check
+ regularly once Online state is reached. When in Online state,
+ but due to lack of internet connectivity, the service is
+ downgraded to a Ready state giving the chance for other
+ configured services to take over.
+ This feature depends on EnableOnlineCheck = true.
Activating debugging
====================
diff --git a/configure.ac b/configure.ac
index ee49a22c..a6868b32 100644
--- a/configure.ac
+++ b/configure.ac
@@ -323,6 +323,12 @@ AC_ARG_ENABLE(selinux, AC_HELP_STRING([--enable-selinux],
[enable_selinux=${enableval}], [enable_selinux="no"])
AM_CONDITIONAL(SELINUX, test "${enable_selinux}" != "no")
+AC_ARG_ENABLE(perpetual-online-check, AC_HELP_STRING([--enable-perpetual-online-check],
+ [enable perpetual online check in the online state]),
+ [enable_perp_check="true"], [enable_perp_check="false"])
+AC_DEFINE_UNQUOTED([PERPETUAL_ONLINE_CHECK_ENABLED], ${enable_perp_check},
+ [enable perpetual online check during the online state])
+
AC_ARG_ENABLE(loopback, AC_HELP_STRING([--disable-loopback],
[disable loopback support]),
[enable_loopback=${enableval}])
diff --git a/src/service.c b/src/service.c
index f86baaa6..128ec1d8 100644
--- a/src/service.c
+++ b/src/service.c
@@ -135,6 +135,7 @@ struct connman_service {
bool hidden_service;
char *config_file;
char *config_entry;
+ bool perpetual_online_check;
};
static bool allow_property_changed(struct connman_service *service);
@@ -6191,7 +6192,8 @@ int __connman_service_ipconfig_indicate_state(struct connman_service
*service,
/* Any change? */
if (old_state == new_state) {
/* continuous online check requires restart of the check at regular intervals */
- if (new_state == CONNMAN_SERVICE_STATE_ONLINE) {
+ if ((new_state == CONNMAN_SERVICE_STATE_ONLINE) &&
+ service->perpetual_online_check ) {
connman_info("Online check continues for %s.\n", service->name);
__connman_service_online_perform_regular(service, type);
service_indicate_state(service);
@@ -6231,7 +6233,8 @@ int __connman_service_ipconfig_indicate_state(struct connman_service
*service,
* ONLINE to ONLINE, there is a special check above
*/
connman_info("Online state reached for %s.\n", service->name);
- __connman_service_online_perform_regular(service, type);
+ if (service->perpetual_online_check)
+ __connman_service_online_perform_regular(service, type);
break;
case CONNMAN_SERVICE_STATE_DISCONNECT:
if (service->state == CONNMAN_SERVICE_STATE_IDLE)
@@ -7164,6 +7167,11 @@ static void update_from_network(struct connman_service *service,
if (!service->network)
service->network = connman_network_ref(network);
+ /* TODO: Perform runtime/config file setting of this flag if needed */
+ service->perpetual_online_check = PERPETUAL_ONLINE_CHECK_ENABLED;
+ connman_info("Perpetual online check for service %s is %s",
+ service->identifier, service->perpetual_online_check ? "Enabled" :
"Disabled");
+
service_list_sort();
}
--
2.17.1
7 a.m.
New subject: [PATCH 3/3] Defined a compile time config of perpetual online check
Hi Aleksandar,
On Mon, Sep 09, 2019 at 03:43:55PM +0300, aleksandar(a)analyticsfire.com wrote:
From: Aleksandar Mitev <shondll(a)yahoo.com>
New flag is
--enable-perpetual-online-check
and is disabled by default, leaving the original behaviour
in place. Even if enabled the regular check relies on:
EnableOnlineCheck = true
Otherwise it will have no effect.
---
README | 10 ++++++++++
configure.ac | 6 ++++++
src/service.c | 12 ++++++++++--
3 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/README b/README
index f16b9ec0..1ccf766c 100644
--- a/README
+++ b/README
@@ -231,6 +231,16 @@ For a working system, certain configuration options need to be
enabled:
is selected, ConnMan configures systemd-resolved to do DNS
resolving. The default value is "internal".
+ --enable-perpetual-online-check
I am not an native English speaker and it took a few seconds to get to
the meaning of perpetual. But that could also be that I am constantly
in fight with languages :)
+
+ Enable continuously checking for Internet connection.
'continuously' is more catching in my ears. Though, not sure about
it. If no one objects we go with it.
+
+ If enabled, services will conduct online state check
+ regularly once Online state is reached. When in Online state,
+ but due to lack of internet connectivity, the service is
+ downgraded to a Ready state giving the chance for other
+ configured services to take over.
+ This feature depends on EnableOnlineCheck = true.
Activating debugging
====================
diff --git a/configure.ac b/configure.ac
index ee49a22c..a6868b32 100644
--- a/configure.ac
+++ b/configure.ac
@@ -323,6 +323,12 @@ AC_ARG_ENABLE(selinux, AC_HELP_STRING([--enable-selinux],
[enable_selinux=${enableval}], [enable_selinux="no"])
AM_CONDITIONAL(SELINUX, test "${enable_selinux}" != "no")
+AC_ARG_ENABLE(perpetual-online-check, AC_HELP_STRING([--enable-perpetual-online-check],
+ [enable perpetual online check in the online state]),
+ [enable_perp_check="true"], [enable_perp_check="false"])
+AC_DEFINE_UNQUOTED([PERPETUAL_ONLINE_CHECK_ENABLED], ${enable_perp_check},
+ [enable perpetual online check during the online state])
+
AC_ARG_ENABLE(loopback, AC_HELP_STRING([--disable-loopback],
[disable loopback support]),
[enable_loopback=${enableval}])
diff --git a/src/service.c b/src/service.c
index f86baaa6..128ec1d8 100644
--- a/src/service.c
+++ b/src/service.c
@@ -135,6 +135,7 @@ struct connman_service {
bool hidden_service;
char *config_file;
char *config_entry;
+ bool perpetual_online_check;
};
static bool allow_property_changed(struct connman_service *service);
@@ -6191,7 +6192,8 @@ int __connman_service_ipconfig_indicate_state(struct
connman_service *service,
/* Any change? */
if (old_state == new_state) {
/* continuous online check requires restart of the check at regular intervals */
- if (new_state == CONNMAN_SERVICE_STATE_ONLINE) {
+ if ((new_state == CONNMAN_SERVICE_STATE_ONLINE) &&
+ service->perpetual_online_check ) {
connman_info("Online check continues for %s.\n", service->name);
__connman_service_online_perform_regular(service, type);
service_indicate_state(service);
@@ -6231,7 +6233,8 @@ int __connman_service_ipconfig_indicate_state(struct
connman_service *service,
* ONLINE to ONLINE, there is a special check above
*/
connman_info("Online state reached for %s.\n", service->name);
- __connman_service_online_perform_regular(service, type);
+ if (service->perpetual_online_check)
+ __connman_service_online_perform_regular(service, type);
break;
case CONNMAN_SERVICE_STATE_DISCONNECT:
if (service->state == CONNMAN_SERVICE_STATE_IDLE)
@@ -7164,6 +7167,11 @@ static void update_from_network(struct connman_service *service,
if (!service->network)
service->network = connman_network_ref(network);
+ /* TODO: Perform runtime/config file setting of this flag if needed */
+ service->perpetual_online_check = PERPETUAL_ONLINE_CHECK_ENABLED;
I think it would be better at just to move the 'continuously' is more catching in
my ears. Though, not sure about
it. If no one objects we go with it.
+
+ If enabled, services will conduct online state check
+ regularly once Online state is reached. When in Online state,
+ but due to lack of internet connectivity, the service is
+ downgraded to a Ready state giving the chance for other
+ configured services to take over.
+ This feature depends on EnableOnlineCheck = true.
Activating debugging
====================
diff --git a/configure.ac b/configure.ac
index ee49a22c..a6868b32 100644
--- a/configure.ac
+++ b/configure.ac
@@ -323,6 +323,12 @@ AC_ARG_ENABLE(selinux, AC_HELP_STRING([--enable-selinux],
[enable_selinux=${enableval}], [enable_selinux="no"])
AM_CONDITIONAL(SELINUX, test "${enable_selinux}" != "no")
+AC_ARG_ENABLE(perpetual-online-check, AC_HELP_STRING([--enable-perpetual-online-check],
+ [enable perpetual online check in the online state]),
+ [enable_perp_check="true"], [enable_perp_check="false"])
+AC_DEFINE_UNQUOTED([PERPETUAL_ONLINE_CHECK_ENABLED], ${enable_perp_check},
+ [enable perpetual online check during the online state])
+
AC_ARG_ENABLE(loopback, AC_HELP_STRING([--disable-loopback],
[disable loopback support]),
[enable_loopback=${enableval}])
diff --git a/src/service.c b/src/service.c
index f86baaa6..128ec1d8 100644
--- a/src/service.c
+++ b/src/service.c
@@ -135,6 +135,7 @@ struct connman_service {
bool hidden_service;
char *config_file;
char *config_entry;
+ bool perpetual_online_check;
};
static bool allow_property_changed(struct connman_service *service);
@@ -6191,7 +6192,8 @@ int __connman_service_ipconfig_indicate_state(struct
connman_service *service,
/* Any change? */
if (old_state == new_state) {
/* continuous online check requires restart of the check at regular intervals */
- if (new_state == CONNMAN_SERVICE_STATE_ONLINE) {
+ if ((new_state == CONNMAN_SERVICE_STATE_ONLINE) &&
+ service->perpetual_online_check ) {
connman_info("Online check continues for %s.\n", service->name);
__connman_service_online_perform_regular(service, type);
service_indicate_state(service);
@@ -6231,7 +6233,8 @@ int __connman_service_ipconfig_indicate_state(struct
connman_service *service,
* ONLINE to ONLINE, there is a special check above
*/
connman_info("Online state reached for %s.\n", service->name);
- __connman_service_online_perform_regular(service, type);
+ if (service->perpetual_online_check)
+ __connman_service_online_perform_regular(service, type);
break;
case CONNMAN_SERVICE_STATE_DISCONNECT:
if (service->state == CONNMAN_SERVICE_STATE_IDLE)
@@ -7164,6 +7167,11 @@ static void update_from_network(struct connman_service *service,
if (!service->network)
service->network = connman_network_ref(network);
+ /* TODO: Perform runtime/config file setting of this flag if needed */
+ service->perpetual_online_check = PERPETUAL_ONLINE_CHECK_ENABLED;
I think it would be better at just to move the
PERPETUAL_ONLINE_CHECK_ENABLED right to the check above. With this the
compiler should be able to remove the code completely if it is
disabled. We can still add the runtime config part if needed. Or
turned into a main.conf config option. This is not too important right
now. The important thing is that we don't change the old behaviour.
+ connman_info("Perpetual online check for service %s is
%s",
+ service->identifier, service->perpetual_online_check ? "Enabled" :
"Disabled");
Please remove this one as well.
BTW, could you also update the doc accordingly? The state diagram
would need an update for this.
Overall, I think this goes into the right direction.
Thanks for your patience!
Daniel
761
days inactive
776
days old
6 comments
2 participants
participants (2)
-
aleksandar＠analyticsfire.com -
Daniel Wagner