[PATCH v2 0/3] Fixes wispr refcount and cleanups

[PATCH v2 0/3] Fixes wispr refcount and cleanups

[Daniel Wagner]

Fixes and cleanups from the introduction of the wispr refcounting.

Daniel Wagner (3):
  service: Track online check for IPv4 and IPv6 separately
  wispr: Fix context refcounting in wispr_portal_request_portal()
  wispr: Simplify the IP version check

 src/service.c | 39 +++++++++++++++++++++++++++------------
 src/wispr.c   | 16 ++++++----------
 2 files changed, 33 insertions(+), 22 deletions(-)

-- 
2.37.2

[PATCH v2 1/3] service: Track online check for IPv4 and IPv6 separately

[Daniel Wagner]

The online check is not distinguishing between IPv4 and IPv6 but the
rest of the code assumes we handle them separately.
---
 src/service.c | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/src/service.c b/src/service.c
index 988928e7859e..079c7a6cda84 100644
--- a/src/service.c
+++ b/src/service.c
@@ -137,7 +137,8 @@ struct connman_service {
 	char *pac;
 	bool wps;
 	bool wps_advertizing;
-	guint online_timeout;
+	guint online_timeout_ipv4;
+	guint online_timeout_ipv6;
 	unsigned int online_check_interval_ipv4;
 	unsigned int online_check_interval_ipv6;
 	bool do_split_routing;
@@ -1438,12 +1439,16 @@ static bool check_proxy_setup(struct connman_service *service)
 
 static void cancel_online_check(struct connman_service *service)
 {
-	if (service->online_timeout == 0)
-		return;
-
-	g_source_remove(service->online_timeout);
-	service->online_timeout = 0;
-	connman_service_unref(service);
+	if (service->online_timeout_ipv4) {
+		g_source_remove(service->online_timeout_ipv4);
+		service->online_timeout_ipv4 = 0;
+		connman_service_unref(service);
+	}
+	if (service->online_timeout_ipv6) {
+		g_source_remove(service->online_timeout_ipv6);
+		service->online_timeout_ipv6 = 0;
+		connman_service_unref(service);
+	}
 }
 
 static void start_online_check(struct connman_service *service,
@@ -1461,7 +1466,7 @@ static void start_online_check(struct connman_service *service,
 	online_check_max_interval =
 		connman_setting_get_uint("OnlineCheckMaxInterval");
 
-	if (type != CONNMAN_IPCONFIG_TYPE_IPV4 || check_proxy_setup(service)) {
+	if (type == CONNMAN_IPCONFIG_TYPE_IPV6 || check_proxy_setup(service)) {
 		cancel_online_check(service);
 		__connman_service_wispr_start(service, type);
 	}
@@ -6330,20 +6335,20 @@ static void service_rp_filter(struct connman_service *service,
 static void redo_wispr(struct connman_service *service,
 					enum connman_ipconfig_type type)
 {
-	service->online_timeout = 0;
-	connman_service_unref(service);
-
 	DBG("Retrying %s WISPr for %p %s",
 		__connman_ipconfig_type2string(type),
 		service, service->name);
 
 	__connman_wispr_start(service, type);
+	connman_service_unref(service);
 }
 
 static gboolean redo_wispr_ipv4(gpointer user_data)
 {
 	struct connman_service *service = user_data;
 
+	service->online_timeout_ipv4 = 0;
+
 	redo_wispr(service, CONNMAN_IPCONFIG_TYPE_IPV4);
 
 	return FALSE;
@@ -6353,6 +6358,8 @@ static gboolean redo_wispr_ipv6(gpointer user_data)
 {
 	struct connman_service *service = user_data;
 
+	service->online_timeout_ipv6 = 0;
+
 	redo_wispr(service, CONNMAN_IPCONFIG_TYPE_IPV6);
 
 	return FALSE;
@@ -6365,6 +6372,10 @@ void __connman_service_online_check(struct connman_service *service,
 	GSourceFunc redo_func;
 	unsigned int *interval;
 	enum connman_service_state current_state;
+	int timeout;
+
+	DBG("service %p type %s success %d\n",
+		service, __connman_ipconfig_type2string(type), success);
 
 	if (type == CONNMAN_IPCONFIG_TYPE_IPV4) {
 		interval = &service->online_check_interval_ipv4;
@@ -6393,8 +6404,12 @@ void __connman_service_online_check(struct connman_service *service,
 	DBG("service %p type %s interval %d", service,
 		__connman_ipconfig_type2string(type), *interval);
 
-	service->online_timeout = g_timeout_add_seconds(*interval * *interval,
+	timeout = g_timeout_add_seconds(*interval * *interval,
 				redo_func, connman_service_ref(service));
+	if (type == CONNMAN_IPCONFIG_TYPE_IPV4)
+		service->online_timeout_ipv4 = timeout;
+	else
+		service->online_timeout_ipv6 = timeout;
 
 	/* Increment the interval for the next time, set a maximum timeout of
 	 * online_check_max_interval seconds * online_check_max_interval seconds.
-- 
2.37.2

[PATCH v2 2/3] wispr: Fix context refcounting in wispr_portal_request_portal()

[Daniel Wagner]

The wispr_portal_request_portal() function is expected to read until
there is no data. Hence, the wp_context refcount is supposed to be
hold on while reading.

Furthermore, we should not return early when we read the
X-ConnMan-Status header. Instead we are supposed to go through the
normal return path so that we cleanup any added routing entries. Thus,
we also don't need to update the refcount in this code path as we
handle it at the main return path.

Fixes: 416bfaff9888 ("wispr: Update portal context references")
---
 src/wispr.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/wispr.c b/src/wispr.c
index 9b27af5fff55..a7562e8462f3 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -537,7 +537,8 @@ static bool wispr_route_request(const char *address, int ai_family,
 static void wispr_portal_request_portal(
 		struct connman_wispr_portal_context *wp_context)
 {
-	DBG("");
+	DBG("wp_context %p %s", wp_context,
+		__connman_ipconfig_type2string(wp_context->type));
 
 	wispr_portal_context_ref(wp_context);
 	wp_context->request_id = g_web_request_get(wp_context->web,
@@ -753,7 +754,7 @@ static bool wispr_portal_web_result(GWebResult *result, gpointer user_data)
 		if (length > 0) {
 			g_web_parser_feed_data(wp_context->wispr_parser,
 								chunk, length);
-			wispr_portal_context_unref(wp_context);
+			/* read more data */
 			return true;
 		}
 
@@ -783,8 +784,6 @@ static bool wispr_portal_web_result(GWebResult *result, gpointer user_data)
 		if (g_web_result_get_header(result, "X-ConnMan-Status",
 						&str)) {
 			portal_manage_status(result, wp_context);
-			wispr_portal_context_unref(wp_context);
-			return false;
 		} else {
 			wispr_portal_context_ref(wp_context);
 			__connman_agent_request_browser(wp_context->service,
@@ -996,7 +995,8 @@ int __connman_wispr_start(struct connman_service *service,
 	struct connman_wispr_portal *wispr_portal = NULL;
 	int index, err;
 
-	DBG("service %p", service);
+	DBG("service %p %s", service,
+		__connman_ipconfig_type2string(type));
 
 	if (!wispr_portal_hash)
 		return -EINVAL;
-- 
2.37.2

[PATCH v2 3/3] wispr: Simplify the IP version check

[Daniel Wagner]

No need to be clever. There is IPv4 and IPv6 nothing else.
---
 src/wispr.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/wispr.c b/src/wispr.c
index a7562e8462f3..a4372018a3e1 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -1033,12 +1033,8 @@ int __connman_wispr_start(struct connman_service *service,
 
 	if (type == CONNMAN_IPCONFIG_TYPE_IPV4)
 		wp_context = wispr_portal->ipv4_context;
-	else if (type == CONNMAN_IPCONFIG_TYPE_IPV6)
+	else
 		wp_context = wispr_portal->ipv6_context;
-	else {
-		err = -EINVAL;
-		goto free_wp;
-	}
 
 	/* If there is already an existing context, we wipe it */
 	if (wp_context)
-- 
2.37.2

Re: [PATCH v2 0/3] Fixes wispr refcount and cleanups

[Daniel Wagner]

On Wed, Sep 07, 2022 at 08:52:18PM +0200, Daniel Wagner wrote:
> Fixes and cleanups from the introduction of the wispr refcounting.
> 
> Daniel Wagner (3):
>   service: Track online check for IPv4 and IPv6 separately
>   wispr: Fix context refcounting in wispr_portal_request_portal()
>   wispr: Simplify the IP version check

Patches applied.