[PATCH] Respect EnableOnlineCheck globally

[PATCH] Respect EnableOnlineCheck globally

[Страхиња Радић]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

Currently, even while having EnableOnlineCheck=false,
EnableOnlineToReadyTransition=false and Enable6to4=false in the configuration
file, connman seems to make connections to ipv4.connman.net and ipv6.connman.net
in the function wispr_portal_detect in src/wispr.c. This patch makes the
configuration option behave consistently by setting the status_url to
"localhost" if EnableOnlineCheck=false. 

If a user doesn't want connman "pinging home", that should be respected.

---
 src/wispr.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/wispr.c b/src/wispr.c
index c63dc819..d69ed70b 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -916,6 +916,11 @@ static int wispr_portal_detect(struct connman_wispr_portal_context *wp_context)
 		wp_context->status_url = STATUS_URL_IPV6;
 	}
 
+	/* Respect EnableOnlineCheck globally */
+	if (connman_setting_get_bool("EnableOnlineCheck")) {
+		wp_context->status_url = "localhost";
+	}
+
 	for (i = 0; nameservers[i]; i++)
 		g_web_add_nameserver(wp_context->web, nameservers[i]);
 
-- 
2.33.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Respect EnableOnlineCheck globally

[Страхиња Радић]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

On 21/08/28 07:16, Страхиња Радић wrote:
> +	if (connman_setting_get_bool("EnableOnlineCheck")) {

This should read

+       if (!connman_setting_get_bool("EnableOnlineCheck")) {

sorry. I'm sending the modified patch with this email.

---
 src/wispr.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/wispr.c b/src/wispr.c
index c63dc819..7b52914e 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -916,6 +916,11 @@ static int wispr_portal_detect(struct
connman_wispr_portal_context *wp_context)
                wp_context->status_url = STATUS_URL_IPV6;
        }

+       /* Respect EnableOnlineCheck globally */
+       if (!connman_setting_get_bool("EnableOnlineCheck")) {
+               wp_context->status_url = "localhost";
+       }
+
        for (i = 0; nameservers[i]; i++)
                g_web_add_nameserver(wp_context->web, nameservers[i]);

--
2.33.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Respect EnableOnlineCheck globally

[Daniel Wagner]

Hi Страхиња,

On Sat, Aug 28, 2021 at 07:34:27PM +0200, Страхиња Радић wrote:
> On 21/08/28 07:16, Страхиња Радић wrote:
> > +	if (connman_setting_get_bool("EnableOnlineCheck")) {
> 
> This should read
> 
> +       if (!connman_setting_get_bool("EnableOnlineCheck")) {
> 
> sorry. I'm sending the modified patch with this email.
> 
> ---
>  src/wispr.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/wispr.c b/src/wispr.c
> index c63dc819..7b52914e 100644
> --- a/src/wispr.c
> +++ b/src/wispr.c
> @@ -916,6 +916,11 @@ static int wispr_portal_detect(struct
> connman_wispr_portal_context *wp_context)
>                 wp_context->status_url = STATUS_URL_IPV6;
>         }
> 
> +       /* Respect EnableOnlineCheck globally */
> +       if (!connman_setting_get_bool("EnableOnlineCheck")) {
> +               wp_context->status_url = "localhost";
> +       }
> +

This looks like the wrong place to break the online check. We should not
start the online check state machine at all. What about something like:

diff --git a/src/service.c b/src/service.c
index eb74a85d6e32..d04933070c61 100644
--- a/src/service.c
+++ b/src/service.c
@@ -1618,15 +1618,18 @@ static void default_changed(void)
 				connman_setting_get_bool("AllowDomainnameUpdates"))
 			__connman_utsname_set_domainname(service->domainname);
 
-		if (__connman_service_is_connected_state(service,
+		if (connman_setting_get_bool("EnableOnlineCheck")) {
+
+			if (__connman_service_is_connected_state(service,
 						CONNMAN_IPCONFIG_TYPE_IPV4))
-			__connman_service_wispr_start(service,
-						CONNMAN_IPCONFIG_TYPE_IPV4);
+				__connman_service_wispr_start(service,
+							CONNMAN_IPCONFIG_TYPE_IPV4);
 
-		if (__connman_service_is_connected_state(service,
+			if (__connman_service_is_connected_state(service,
 						CONNMAN_IPCONFIG_TYPE_IPV6))
-			__connman_service_wispr_start(service,
-						CONNMAN_IPCONFIG_TYPE_IPV6);
+				__connman_service_wispr_start(service,
+							CONNMAN_IPCONFIG_TYPE_IPV6);
+		}
 
 		/*
 		 * Connect VPN automatically when new default service


This was the only place I found where we call
__connman_service_wispr_start() without the EnableOnlineCheck test.

Could you give this patch a spin?

Daniel

Re: [PATCH] Respect EnableOnlineCheck globally

[Страхиња Радић]

[-- Attachment #1: Type: text/plain, Size: 233 bytes --]

On 21/08/29 07:54, Daniel Wagner wrote:
> This was the only place I found where we call
> __connman_service_wispr_start() without the EnableOnlineCheck test.

There's one more place, src/service.c:3728-3734 (set_property function).


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Respect EnableOnlineCheck globally

[Daniel Wagner]

On Sun, Aug 29, 2021 at 09:32:30PM +0200, Страхиња Радић wrote:
> On 21/08/29 07:54, Daniel Wagner wrote:
> > This was the only place I found where we call
> > __connman_service_wispr_start() without the EnableOnlineCheck test.
> 
> There's one more place, src/service.c:3728-3734 (set_property function).

Good catch! I think we should introduce a helper function for this
instead opqen coding the check 3 times.