[PATCH v5 0/5] Close Two GWeb Request "Bookend" Failure "Holes"

[PATCH v5 0/5] Close Two GWeb Request "Bookend" Failure "Holes"

[Grant Erickson]

There existed, prior to this set of changes, two "holes" in the
finalization of web request sessions that can (for example, in a client
such as WISPr) leave one or both of IPv4 and IPv6 "online" HTTP-based
Internet reachability checks "dangling" and non-renewing such that an
expected active, default network service failover does not occur when
it should.

The first of those two failure "holes" involves an end-of-file (EOF)
condition. In the normal case, there are a series of one or more data
receipts for the web request as headers and, if present, the body of
the request response are fulfilled. When there is no further data to
send, the final receive completes with 'g_io_channel_read_chars'
returning 'G_IO_STATUS_EOF' status and zero (0) bytes read. This should
and does lead to a successful EOF closure of the web request.

However, there is a second EOF case that appears to happen with a
random distribution in the nginx server backing the default "online"
HTTP-based Internet reachability check URLs,
'ipv[46].connman.net/online/status.html'. In that case, the nginx
server appears to periodically do an unexpected and spontaneous remote
connection close after the initial connection but before any data has
been received.

For WISPr, presently, all such failures hit the same error-handling
block which effectively maps such a failure to the effective "null"
'GWEB_HTTP_STATUS_CODE_UNKNOWN' status value. Unfortunately, clients
such as WISPr have no way to distinguish this as an actual failure or
success and so it lands on the case:

	case GWEB_HTTP_STATUS_CODE_UNKNOWN:
		wispr_portal_context_ref(wp_context);
		__connman_agent_request_browser(wp_context->service,
				wispr_portal_browser_reply_cb,
				wp_context->status_url, wp_context);

which does not "bookend" the original, initiating WISPr web request
and leaves the original request "dangling" and non-renewed, eventually
leading to the aforementioned "hole" and network service failover
failure.

To handle this failure EOF case, if GWeb asked for a non-zero amount
of data from 'g_io_channel_read_chars' but received none and has
accumulated no headers thus far upon receiving the status
'G_IO_STATUS_EOF', then GWeb assumes that the remote peer server
unexpectedly closed the connection, and synthesizes a new GError with
the error 'ECONNRESET'. With the addition of the optional, immutable
GError parameter to the GWebResult callback function, clients such as
WISPr can now distinguish between a HTTP non-response in which no HTTP
data was received and one in which HTTP data was received and the
status can be disguished by the HTTP status code.

The second of the two failure "holes" involves the case where
'g_io_channel_read_chars' returns 'G_IO_STATUS_ERROR' status. Prior to
this change, this funneled to the same "hole" as the
'G_IO_STATUS_ERROR' failure with the same consequence to clients such
as WISPr.

To handle this second error case, GWeb now passes a glib GError
pointer to 'g_io_channel_read_chars'. If it is set, GWeb passes the
GError through to the GWebResult callback function. Otherwise, it
synthesizes a GError anew from 'EIO'. As with the failure EOF case,
this allows clients to distinguish and handle such failures and to
successfully "bookend" their initial web request.

With these changes, WISPr now leverages the newly-added
'GWebResultFunc' GError parameter to refactor
'wispr_portal_web_result' into 'wispr_portal_web_result_failure' and
'wispr_portal_web_result_success' with the former handling
non-successful cases and the latter handling successful HTTP status
code cases.

With this change, the second, failure EOF case will now return a
non-null GError instance from 'GWebResultFunc' and will be handled as
a failure by 'wispr_portal_web_result_failure', "bookending" the
original "online" HTTP-based Internet reachability check.

All of the prior HTTP status code cases are handled by
'wispr_portal_web_result_success'.

This approachs works within the boundaries of the following constraints:

  1. The gio/gio.h header cannot be included in either src/wispr.c
     or src/service.c since that throws a bunch of symbol conflicts
     with gdbus/gdbus.h which redefines a number of symbols otherwise
     defined by gio/gio.h.

  2. 'handle_online_check_failure' in src/service.c needs to determine
     when a check failed due to cancellation.

     a. Historically, it looked at 'err == -ECANCELED' from the WISPr
        closure callback.

     b. However, due to (1), using 'g_error_matches(error, G_IO_ERROR,
        G_IO_ERROR_CANCELLED)' instead of 'err == -ECANCELED' cannot be
        used. So, passing 'const GError *error' all the way through will
        not work.

  3. Some HTTP status codes are failures insofar as WISPr is
     concerned. The codes are not important, the error messages are.

  4. WISPr failure messages need to be logged in src/service.c by
     'online_check_log_failure'. A simple null-terminated C string
     will do.

Grant Erickson (5):
  wispr: Refactor 'wispr_portal_web_result' to leverage 'GError'.
  wispr: Document 'wispr_portal_web_result_failure'.
  wispr: Document 'wispr_portal_web_result_success'.
  wispr: Document 'wispr_portal_web_result'.
  wispr: Add an optional 'message' parameter to '__connman_wispr_cb_t'.

 src/connman.h |   3 +-
 src/service.c |  61 +++++++++++------
 src/wispr.c   | 184 +++++++++++++++++++++++++++++++++++++++-----------
 3 files changed, 189 insertions(+), 59 deletions(-)

-- 
2.45.0

[PATCH v5 1/5] wispr: Refactor 'wispr_portal_web_result' to leverage 'GError'.

[Grant Erickson]

Leverages the newly-added GError parameter to the 'GWebResultFunc' to
refactor 'wispr_portal_web_result' into
'wispr_portal_web_result_failure' and
'wispr_portal_web_result_success' with the former handling
non-successful cases and the latter handling successful HTTP status
code cases.

Prior to the introduction of an optional, immutable 'GError' parameter
to 'GWebResultFunc' and this refactor, there existed a "hole" in the
finalization of WISPr IPv4 and IPv6 "online" HTTP-based Internet
reachability checks web request sessions that both could and did leave
such checks "dangling" and non-renewing such that an expected active,
default network service failover does not occur when it should.

The "hole" involves an end-of-file (EOF) condition. In the normal
case, there are a series of one or more data receipts for the web
request as headers of the request response are fulfilled. When there
is no further data to send, the final receive completes a normal EOF
that leads to the successful closure of the web request, typically
with HTTP 200 "OK" in the nominal success case.

However, there is a second EOF case that appears to happen with a
random distribution in the nginx server backing the default "online"
HTTP-based Internet reachability check URLs,
'ipv[46].connman.net/online/status.html'. In that case, the nginx
server appears to periodically do an unexpected and spontaneous remote
connection close after the initial connection but before any data has
been received. Prior to this change, all such failures hit the same
WISPr error-handling block which effectively maps such a failure to the
effective 'GWebResult' "null" GWEB_HTTP_STATUS_CODE_UNKNOWN status
value. Unfortunately, WISPr historically had no way to distinguish
this as an actual failure or success and so it lands on the case:

        case GWEB_HTTP_STATUS_CODE_UNKNOWN:
                wispr_portal_context_ref(wp_context);
                __connman_agent_request_browser(wp_context->service,
                                wispr_portal_browser_reply_cb,
                                wp_context->status_url, wp_context);

which does not "bookend" the original, initiating WISPr web request
and leaves the original request "dangling" and non-renewed, eventually
leading to the aforementioned "hole".

With this change, the second, failure EOF case will now return a
GError instance pointer synthesized from 'ECONNRESET' and will be
handled as a failure by 'wispr_portal_web_result_failure',
"bookending" the original "online" HTTP-based Internet reachability
check.

All of the prior HTTP status code cases are handled by
'wispr_portal_web_result_success'.
---
 src/wispr.c | 105 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 70 insertions(+), 35 deletions(-)

diff --git a/src/wispr.c b/src/wispr.c
index c3d8b13a9610..a21ec8e29aca 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -652,6 +652,9 @@ static void wispr_portal_error(struct connman_wispr_portal_context *wp_context)
  *                              associated with the unsuccessful
  *                              "online" HTTP-based Internet
  *                              reachability check.
+ *  @param[in]      message     A pointer to an immutable null-
+ *                              terminated C string describing the
+ *                              reason for the online check failure.
  *
  *  @sa portal_manage_success_status
  *  @sa wispr_portal_web_result_no_err
@@ -663,7 +666,8 @@ static void wispr_portal_error(struct connman_wispr_portal_context *wp_context)
  */
 static void portal_manage_failure_status(
 			struct connman_wispr_portal_context *wp_context,
-			int err)
+			int err,
+			const char *message)
 {
 	struct connman_service *service = wp_context->service;
 	enum connman_ipconfig_type type = wp_context->type;
@@ -810,8 +814,11 @@ static void wispr_portal_request_portal(
 					wispr_route_request,
 					wp_context, &err);
 
+	DBG("wp_context->request_id %d err %d",
+		wp_context->request_id, err);
+
 	if (wp_context->request_id == 0) {
-		portal_manage_failure_status(wp_context, err);
+		portal_manage_failure_status(wp_context, -err, strerror(-err));
 		wispr_portal_error(wp_context);
 		wispr_portal_context_unref(wp_context);
 	}
@@ -1001,35 +1008,22 @@ static bool wispr_manage_message(GWebResult *result,
 	return false;
 }
 
-static bool wispr_portal_web_result(const GError *error, GWebResult *result,
-		gpointer user_data)
+static void wispr_portal_web_result_failure(const GError *error,
+		GWebResult *result,
+		struct connman_wispr_portal_context *wp_context)
 {
-	struct connman_wispr_portal_context *wp_context = user_data;
-	const char *redirect = NULL;
-	const guint8 *chunk = NULL;
-	const char *str = NULL;
-	guint16 status;
-	gsize length;
-
-	DBG("");
+	portal_manage_failure_status(wp_context, 0, error->message);
 
-	if (wp_context->wispr_result != CONNMAN_WISPR_RESULT_ONLINE) {
-		g_web_result_get_chunk(result, &chunk, &length);
-
-		if (length > 0) {
-			g_web_parser_feed_data(wp_context->wispr_parser,
-								chunk, length);
-			/* read more data */
-			return true;
-		}
-
-		g_web_parser_end_data(wp_context->wispr_parser);
+	free_wispr_routes(wp_context);
+	wp_context->request_id = 0;
+}
 
-		if (wp_context->wispr_msg.message_type >= 0) {
-			if (wispr_manage_message(result, wp_context))
-				goto done;
-		}
-	}
+static void wispr_portal_web_result_success(GWebResult *result,
+		struct connman_wispr_portal_context *wp_context)
+{
+	guint16 status;
+	const char *str = NULL;
+	const char *redirect = NULL;
 
 	status = g_web_result_get_status(result);
 
@@ -1083,17 +1077,17 @@ static bool wispr_portal_web_result(const GError *error, GWebResult *result,
 				redirect, wispr_portal_web_result,
 				wispr_route_request, wp_context, NULL);
 
-		goto done;
+		return;
 	case GWEB_HTTP_STATUS_CODE_BAD_REQUEST:
-		portal_manage_failure_status(wp_context, -EINVAL);
+		portal_manage_failure_status(wp_context, 0, "bad request");
 		break;
 
 	case GWEB_HTTP_STATUS_CODE_NOT_FOUND:
-		portal_manage_failure_status(wp_context, -ENOENT);
+		portal_manage_failure_status(wp_context, 0, "resource not found");
 		break;
 
 	case GWEB_HTTP_STATUS_CODE_REQUEST_TIMEOUT:
-		portal_manage_failure_status(wp_context, -ETIMEDOUT);
+		portal_manage_failure_status(wp_context, 0, "request timeout");
 		break;
 
 	case GWEB_HTTP_STATUS_CODE_HTTP_VERSION_NOT_SUPPORTED:
@@ -1108,6 +1102,46 @@ static bool wispr_portal_web_result(const GError *error, GWebResult *result,
 
 	free_wispr_routes(wp_context);
 	wp_context->request_id = 0;
+}
+
+static bool wispr_portal_web_result(const GError *error, GWebResult *result,
+		gpointer user_data)
+{
+	struct connman_wispr_portal_context *wp_context = user_data;
+	const guint8 *chunk = NULL;
+	gsize length;
+
+	DBG("error %p result %p user_data %p wispr_result %d",
+		error, result, user_data, wp_context->wispr_result);
+
+	if (wp_context->wispr_result != CONNMAN_WISPR_RESULT_ONLINE) {
+		g_web_result_get_chunk(result, &chunk, &length);
+
+		DBG("length %zu", length);
+
+		if (length > 0) {
+			g_web_parser_feed_data(wp_context->wispr_parser,
+								chunk, length);
+			/* read more data */
+			return true;
+		}
+
+		g_web_parser_end_data(wp_context->wispr_parser);
+
+		DBG("wp_context->wispr_msg.message_type %d",
+			wp_context->wispr_msg.message_type);
+
+		if (wp_context->wispr_msg.message_type >= 0) {
+			if (wispr_manage_message(result, wp_context))
+				goto done;
+		}
+	}
+
+	if (error)
+		wispr_portal_web_result_failure(error, result, wp_context);
+	else
+		wispr_portal_web_result_success(result, wp_context);
+
 done:
 	wp_context->wispr_msg.message_type = -1;
 
@@ -1196,7 +1230,7 @@ static void proxy_callback(const char *proxy, void *user_data)
 	if (!proxy) {
 		wispr_log_proxy_failure(wp_context, "No valid proxy");
 
-		portal_manage_failure_status(wp_context, -EINVAL);
+		portal_manage_failure_status(wp_context, 0, "no valid proxy");
 
 		return;
 	}
@@ -1543,7 +1577,7 @@ int __connman_wispr_start(struct connman_service *service,
 free_wp:
 	DBG("err %d wp_context %p", err, wp_context);
 
-	portal_manage_failure_status(wp_context, err);
+	portal_manage_failure_status(wp_context, -err, strerror(-err));
 
 	g_hash_table_remove(wispr_portal_hash, GINT_TO_POINTER(index));
 	return err;
@@ -1629,7 +1663,8 @@ int __connman_wispr_cancel(struct connman_service *service,
 
 	cancel_connman_wispr_portal_context(wp_context);
 
-	portal_manage_failure_status(wp_context, -ECANCELED);
+	portal_manage_failure_status(wp_context, -ECANCELED,
+		strerror(-ECANCELED));
 
 	wispr_portal_context_unref(wp_context);
 
-- 
2.45.0

[PATCH v5 2/5] wispr: Document 'wispr_portal_web_result_failure'.

[Grant Erickson]

Add documentation to the 'wispr_portal_web_result_failure' function.
---
 src/wispr.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/src/wispr.c b/src/wispr.c
index a21ec8e29aca..fd80715ec719 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -657,7 +657,7 @@ static void wispr_portal_error(struct connman_wispr_portal_context *wp_context)
  *                              reason for the online check failure.
  *
  *  @sa portal_manage_success_status
- *  @sa wispr_portal_web_result_no_err
+ *  @sa wispr_portal_web_result_failure
  *  @sa wispr_portal_web_result_err
  *  @sa wispr_portal_web_result
  *
@@ -1008,6 +1008,28 @@ static bool wispr_manage_message(GWebResult *result,
 	return false;
 }
 
+/**
+ *  @brief
+ *    Handle closure and finalization of a web request associated with
+ *    an unsuccessful "online" HTTP-based Internet reachability check.
+ *
+ *  @param[in]      error       A pointer to the immutable GLib GError
+ *                              instance associated the web request
+ *                              failure.
+ *  @param[in]      result      A pointer to the mutable web request
+ *                              result being finalized.
+ *  @param[in,out]  wp_context  A pointer to the mutable WISPr portal
+ *                              detection context associated with the
+ *                              unsuccessful "online" HTTP-based
+ *                              Internet reachability check this is
+ *                              finalizing.
+ *
+ *  @sa wispr_portal_web_result
+ *  @sa wispr_portal_web_result_no_err
+ *
+ *  @private
+ *
+ */
 static void wispr_portal_web_result_failure(const GError *error,
 		GWebResult *result,
 		struct connman_wispr_portal_context *wp_context)
-- 
2.45.0

[PATCH v5 3/5] wispr: Document 'wispr_portal_web_result_success'.

[Grant Erickson]

Add documentation to the 'wispr_portal_web_result_success' function.
---
 src/wispr.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/src/wispr.c b/src/wispr.c
index fd80715ec719..47659b58383c 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -658,7 +658,7 @@ static void wispr_portal_error(struct connman_wispr_portal_context *wp_context)
  *
  *  @sa portal_manage_success_status
  *  @sa wispr_portal_web_result_failure
- *  @sa wispr_portal_web_result_err
+ *  @sa wispr_portal_web_result_success
  *  @sa wispr_portal_web_result
  *
  *  @private
@@ -1025,7 +1025,7 @@ static bool wispr_manage_message(GWebResult *result,
  *                              finalizing.
  *
  *  @sa wispr_portal_web_result
- *  @sa wispr_portal_web_result_no_err
+ *  @sa wispr_portal_web_result_success
  *
  *  @private
  *
@@ -1040,6 +1040,25 @@ static void wispr_portal_web_result_failure(const GError *error,
 	wp_context->request_id = 0;
 }
 
+/**
+ *  @brief
+ *    Handle closure and finalization of a web request associated with
+ *    a successful "online" HTTP-based Internet reachability check.
+ *
+ *  @param[in]      result      A pointer to the mutable web request
+ *                              result being finalized.
+ *  @param[in,out]  wp_context  A pointer to the mutable WISPr portal
+ *                              detection context associated with the
+ *                              successful "online" HTTP-based
+ *                              Internet reachability check this is
+ *                              finalizing.
+ *
+ *  @sa wispr_portal_web_result
+ *  @sa wispr_portal_web_result_failure
+ *
+ *  @private
+ *
+ */
 static void wispr_portal_web_result_success(GWebResult *result,
 		struct connman_wispr_portal_context *wp_context)
 {
-- 
2.45.0

[PATCH v5 4/5] wispr: Document 'wispr_portal_web_result'.

[Grant Erickson]

Add documentation to the 'wispr_portal_web_result' function.
---
 src/wispr.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/src/wispr.c b/src/wispr.c
index 47659b58383c..0cbc8095364f 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -1145,6 +1145,36 @@ static void wispr_portal_web_result_success(GWebResult *result,
 	wp_context->request_id = 0;
 }
 
+/**
+ *  @brief
+ *    Handle the receipt of new data and the potential closure and
+ *    finalization of a web request.
+ *
+ *  This handles the receipt of new data associated with @a result and
+ *  the potential closure and finalization of it, if appropriate.
+ *
+ *  @param[in]      error      An optional pointer to the immutable
+ *                             GLib GError instance associated the web
+ *                             request, if it failed.
+ *  @param[in]      result     A pointer to the mutable web request
+ *                             result with data to process or to be
+ *                             finalized.
+ *  @param[in,out]  user_data  A pointer to the mutable WISPr portal
+ *                             detection context associated with the
+ *                             "online" HTTP-based Internet
+ *                             reachability check this is finalizing.
+ *
+ *  @returns
+ *    True if web request should wait for and process further data;
+ *    otherwise, false.
+ *
+ *  @sa wispr_portal_web_result_failure
+ *  @sa wispr_portal_web_result_success
+ *  @sa wispr_manage_message
+ *
+ *  @private
+ *
+ */
 static bool wispr_portal_web_result(const GError *error, GWebResult *result,
 		gpointer user_data)
 {
-- 
2.45.0

[PATCH v5 5/5] wispr: Add an optional 'message' parameter to '__connman_wispr_cb_t'.

[Grant Erickson]

'message' is an optional pointer to an immutable null- terminated C
string describing the reason for the online check failure.

In the following instances, GWeb (which WISPr utilizes to make an
online reachability check request) can fail before initiating or while
processing a web request:

  1. TCP connection timeout to the remote server peer.
  2. Decoding a chunked response.
  3. Non-successful events (that is, G_IO_NVAL, G_IO_ERR, or G_IO_HUP)
     from 'g_io_add_watch'.
  4. GResolve "host not found" DNS forward resolution failure.
  5. Transport creation failure from 'create_transport'.
  6. Unexpected remote peer disconnect after connect but before data
     transfer.

When an online check request fails, 'online_check_log_failure' in
src/service.c logs the failure in the form:

  "Interface <interface> [ <technology> ] IPv[46] online check to
   <online check URL> failed: <message>"

where <message> is the contents of this newly-added callback
parameter. This avoids 'online_check_log_failure' having to invoke
'strerror' as the message has already been pre-created and -formatted
by the error source. So, for the failures above, the messages would
be:

  1. "connect timeout to <IP address> after <timeout in
     milliseconds>ms".
  2. "Error in chunk decode <error>".
  3. "I/O error".
  4. "could not resolve <host name>".
  5. Output from 'strerror'.
  6. "Connection reset by peer".
---
 src/connman.h |  3 ++-
 src/service.c | 61 +++++++++++++++++++++++++++++++++++----------------
 src/wispr.c   |  4 ++--
 3 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/src/connman.h b/src/connman.h
index 32ba55912ad6..7ebda7af3a55 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -550,7 +550,8 @@ void __connman_wpad_stop(struct connman_service *service);
 typedef void (*__connman_wispr_cb_t) (struct connman_service *service,
 				enum connman_ipconfig_type type,
 				bool success,
-				int err);
+				int err,
+				const char *message);
 
 int __connman_wispr_init(void);
 void __connman_wispr_cleanup(void);
diff --git a/src/service.c b/src/service.c
index c44335d9c962..f73f42eba9b0 100644
--- a/src/service.c
+++ b/src/service.c
@@ -240,7 +240,8 @@ static void service_list_sort(const char *function);
 static void complete_online_check(struct connman_service *service,
 					enum connman_ipconfig_type type,
 					bool success,
-					int err);
+					int err,
+					const char *message);
 static bool service_downgrade_online_state(struct connman_service *service);
 static bool connman_service_is_default(const struct connman_service *service);
 static int start_online_check_if_connected(struct connman_service *service);
@@ -3451,27 +3452,27 @@ static bool handle_online_check_success(struct connman_service *service,
  *                       check failure.
  *  @param[in]  type     The IP configuration type for which
  *                       the online check failed.
- *  @param[in]  err      The error status, in the POSIX domain,
- *                       associated with the online check failure.
+ *  @param[in]  message  A pointer to an immutable null-terminated
+ *                       C string describing the reason for the online
+ *                       check failure.
  *
  */
 static void online_check_log_failure(const struct connman_service *service,
 			enum connman_ipconfig_type type,
-			int err)
+			const char *message)
 {
 	g_autofree char *interface = NULL;
 
 	interface = connman_service_get_interface(service);
 
-	connman_warn("Interface %s [ %s ] %s online check to %s failed: %d: %s",
+	connman_warn("Interface %s [ %s ] %s online check to %s failed: %s",
 		interface,
 		__connman_service_type2string(service->type),
 		__connman_ipconfig_type2string(type),
 		type == CONNMAN_IPCONFIG_TYPE_IPV4 ?
 			connman_setting_get_string("OnlineCheckIPv4URL") :
 			connman_setting_get_string("OnlineCheckIPv6URL"),
-		err,
-		strerror(-err));
+			message);
 }
 
 /**
@@ -3507,6 +3508,10 @@ static void online_check_log_failure(const struct connman_service *service,
  *                                      the failed previously-requested
  *                                      online check. This is expected
  *                                      to be less than zero ('< 0').
+ *  @param[in]      message             A pointer to an immutable null-
+ *                                      terminated C string describing
+ *                                      the reason for the online
+ *                                      check failure.
  *
  *  @returns
  *    True, unconditionally.
@@ -3520,7 +3525,8 @@ static bool handle_oneshot_online_check_failure(
 			enum connman_ipconfig_type type,
 			enum connman_service_state ipconfig_state,
 			struct online_check_state *online_check_state,
-			int err)
+			int err,
+			const char *message)
 {
 	const bool reschedule = true;
 
@@ -3562,6 +3568,10 @@ static bool handle_oneshot_online_check_failure(
  *                                      the failed previously-requested
  *                                      online check. This is expected
  *                                      to be less than zero ('< 0').
+ *  @param[in]      message             A pointer to an immutable null-
+ *                                      terminated C string describing
+ *                                      the reason for the online
+ *                                      check failure.
  *
  *  @returns
  *    True if another online check should be scheduled; otherwise,
@@ -3576,7 +3586,8 @@ static bool handle_continuous_online_check_failure(
 			enum connman_ipconfig_type type,
 			enum connman_service_state ipconfig_state,
 			struct online_check_state *online_check_state,
-			int err)
+			int err,
+			const char *message)
 {
 	bool reschedule = false;
 
@@ -3699,6 +3710,10 @@ static bool handle_continuous_online_check_failure(
  *                                      the failed previously-requested
  *                                      online check. This is expected
  *                                      to be less than zero ('< 0').
+ *  @param[in]      message             A pointer to an immutable null-
+ *                                      terminated C string describing
+ *                                      the reason for the online
+ *                                      check failure.
  *
  *  @returns
  *    True if another online check should be scheduled; otherwise,
@@ -3714,17 +3729,18 @@ static bool handle_online_check_failure(struct connman_service *service,
 				enum connman_service_state ipconfig_state,
 				struct online_check_state *online_check_state,
 				bool oneshot,
-				int err)
+				int err,
+				const char *message)
 {
 	bool reschedule = false;
 
 	DBG("service %p (%s) type %d (%s) state %d (%s) "
-		"one-shot %u err %d (%s)\n",
+		"one-shot %u err %d message %s\n",
 		service,
 		connman_service_get_identifier(service),
 		type, __connman_ipconfig_type2string(type),
 		ipconfig_state, state2string(ipconfig_state),
-		oneshot, err, strerror(-err));
+		oneshot, err, message ? message : "<null>");
 
 	/*
 	 * Regardless of online check mode, if this completion closure
@@ -3740,7 +3756,7 @@ static bool handle_online_check_failure(struct connman_service *service,
 
 	/* Unconditionally log the failure, regardless of online check mode. */
 
-	online_check_log_failure(service, type, err);
+	online_check_log_failure(service, type, message);
 
 	/* Handle the failure according to the online check mode. */
 
@@ -3750,14 +3766,16 @@ static bool handle_online_check_failure(struct connman_service *service,
 						type,
 						ipconfig_state,
 						online_check_state,
-						err);
+						err,
+						message);
 	else
 		reschedule = handle_continuous_online_check_failure(
 						service,
 						type,
 						ipconfig_state,
 						online_check_state,
-						err);
+						err,
+						message);
 
 done:
 	return reschedule;
@@ -3803,6 +3821,9 @@ done:
  *                           to be zero ('0') if @a success is @a true
  *                           and less than zero ('< 0') if @a success
  *                           is @a false.
+ *  @param[in]      message  An optional pointer to an immutable null-
+ *                           terminated C string describing the reason
+ *                           for the online check failure.
  *
  *  @sa cancel_online_check
  *  @sa start_online_check
@@ -3816,7 +3837,8 @@ done:
 static void complete_online_check(struct connman_service *service,
 					enum connman_ipconfig_type type,
 					bool success,
-					int err)
+					int err,
+					const char *message)
 {
 	const bool oneshot = __connman_service_is_online_check_mode(
 		CONNMAN_SERVICE_ONLINE_CHECK_MODE_ONE_SHOT);
@@ -3825,11 +3847,11 @@ static void complete_online_check(struct connman_service *service,
 	bool reschedule = false;
 
 	DBG("service %p (%s) type %d (%s) "
-		"success %u err %d (%s)\n",
+		"success %u err %d message %s\n",
 		service,
 		connman_service_get_identifier(service),
 		type, __connman_ipconfig_type2string(type),
-		success, err, strerror(-err));
+		success, err, message ? message : "<null>");
 
 	if (type == CONNMAN_IPCONFIG_TYPE_IPV4) {
 		online_check_state = &service->online_check_state_ipv4;
@@ -3851,7 +3873,8 @@ static void complete_online_check(struct connman_service *service,
 					 ipconfig_state,
 					 online_check_state,
 					 oneshot,
-					 err);
+					 err,
+					 message);
 
 	DBG("reschedule online check %u", reschedule);
 
diff --git a/src/wispr.c b/src/wispr.c
index 0cbc8095364f..21657f375473 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -672,7 +672,7 @@ static void portal_manage_failure_status(
 	struct connman_service *service = wp_context->service;
 	enum connman_ipconfig_type type = wp_context->type;
 
-	wp_context->cb(service, type, false, err);
+	wp_context->cb(service, type, false, err, message);
 }
 
 /**
@@ -724,7 +724,7 @@ static void portal_manage_success_status(GWebResult *result,
 				&str))
 		connman_info("Client-Timezone: %s", str);
 
-	wp_context->cb(service, type, true, 0);
+	wp_context->cb(service, type, true, 0, NULL);
 }
 
 static bool wispr_route_request(const char *address, int ai_family,
-- 
2.45.0

Re: [PATCH v5 0/5] Close Two GWeb Request "Bookend" Failure "Holes"

[patchwork-bot+connman]

Hello:

This series was applied to connman.git (master)
by Denis Kenzior <denkenz@gmail.com>:

On Tue, 25 Mar 2025 09:27:43 -0700 you wrote:
> There existed, prior to this set of changes, two "holes" in the
> finalization of web request sessions that can (for example, in a client
> such as WISPr) leave one or both of IPv4 and IPv6 "online" HTTP-based
> Internet reachability checks "dangling" and non-renewing such that an
> expected active, default network service failover does not occur when
> it should.
> 
> [...]

Here is the summary with links:
  - [v5,1/5] wispr: Refactor 'wispr_portal_web_result' to leverage 'GError'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=52546061c590
  - [v5,2/5] wispr: Document 'wispr_portal_web_result_failure'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=cfbddc9490f6
  - [v5,3/5] wispr: Document 'wispr_portal_web_result_success'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=7e97e93f57b9
  - [v5,4/5] wispr: Document 'wispr_portal_web_result'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=e09d3137ce8f
  - [v5,5/5] wispr: Add an optional 'message' parameter to '__connman_wispr_cb_t'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=300daeb9829f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html