[PATCH 00/22] Close Two GWeb Request "Bookend" Failure "Holes"

[PATCH 00/22] 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
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 the error
'-ECONNRESET' with the same HTTP status "null" value of
GWEB_HTTP_STATUS_CODE_UNKNOWN. With the addition of the new
'g_web_result_get_err' interface, clients such as WISPr can now
distinguish between a low-level operating system error in which no
HTTP data was received and a success 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 error case, GWeb now passes a glib 'GError' pointer to
'g_io_channel_read_chars'. If it is set, GWeb maps the resulting error
domain/code pairs to negated POSIX domain errors, and default to '-EIO'
if no suitable mapping can be made. 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 GWeb
'g_web_result_get_err' interface to refactor 'wispr_portal_web_result'
into 'wispr_portal_web_result_err' and
'wispr_portal_web_result_no_err' with the former handling
non-successful POSIX error domain cases and the latter handling
successful HTTP status code cases.

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

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

Grant Erickson (22):
  gweb: Leverage 'GWEB_HTTP_STATUS_CODE_UNKNOWN' mnemonic.
  gweb: Refactor 'received_data' into
    'received_data_{finalize,continue}'.
  gweb: Added 'g_web_result_has_headers'.
  gweb: Add documentation to 'g_web_result_has_headers'.
  gweb: Added 'g_web_result_get_err'.
  gweb: Add documentation to 'g_web_result_get_err'.
  gweb: Add an OS err parameter to 'call_result_func'.
  gweb: Add documentation to 'call_result_func'.
  gweb: Add documentation to 'g_web_result_get_status'.
  gweb: Add documentation to 'received_data'.
  gweb: Close web request session finalization 'hole'.
  gweb: Add documentation to 'map_gerror'.
  gweb: Add documentation to 'received_data_{finalize,continue}'.
  gweb: Document 'struct _GWebResult'.
  wispr: Add and leverage 'portal_manage_failure_status'.
  wispr: Refactor 'wispr_portal_web_result' to leverage
    'g_web_result_get_err'.
  wispr: Document 'portal_manage_failure_status'.
  wispr: Document 'wispr_portal_web_result_err'.
  wispr: Document 'wispr_portal_web_result_no_err'.
  wispr: Document 'wispr_portal_web_result'.
  wispr: Fix documentation typo in 'portal_manage_success_status'.
  service: Capture and propagate '__connman_wispr_start' return status.

 gweb/gweb.c   | 444 ++++++++++++++++++++++++++++++++++++++++++++++----
 gweb/gweb.h   |   3 +
 src/service.c |   9 +-
 src/wispr.c   | 236 +++++++++++++++++++++------
 4 files changed, 602 insertions(+), 90 deletions(-)

-- 
2.45.0

[PATCH 01/22] gweb: Leverage 'GWEB_HTTP_STATUS_CODE_UNKNOWN' mnemonic.

[Grant Erickson]

In the GWebResult structure, the 'status' data member is effectively
nullable with the GWEB_HTTP_STATUS_CODE_UNKNOWN mnemonic representing
the "null" value.

Consequently, use the GWEB_HTTP_STATUS_CODE_UNKNOWN mnemonic rather
than zero (0) to make that intent and pattern clear throughout the
code.
---
 gweb/gweb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index ce49e8d3dfc1..8e0b79c52254 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -1189,7 +1189,7 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 
 		str = session->current_header->str;
 
-		if (session->result.status == 0) {
+		if (session->result.status == GWEB_HTTP_STATUS_CODE_UNKNOWN) {
 			unsigned int code;
 
 			if (sscanf(str, "HTTP/%*s %u %*s", &code) == 1)
@@ -2429,7 +2429,7 @@ bool g_web_cancel_request(GWeb *web, guint id)
 guint16 g_web_result_get_status(GWebResult *result)
 {
 	if (!result)
-		return 0;
+		return GWEB_HTTP_STATUS_CODE_UNKNOWN;
 
 	return result->status;
 }
-- 
2.45.0

[PATCH 02/22] gweb: Refactor 'received_data' into 'received_data_{finalize,continue}'.

[Grant Erickson]

The receive-data-and-process-it path for the glib I/O add watch
received data handler was already sufficiently long to warrant its own
function. That now exists and 'received_data_continue'.

While short enough at present, as further error-handling cases are
developed, the finalization logic for an outstanding web request
session warranted its own peer function to
'received_data_continue'. That now exists as 'received_data_finalize'
where error or EOF cases are now handled.
---
 gweb/gweb.c | 88 +++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 62 insertions(+), 26 deletions(-)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index 8e0b79c52254..d4bdbdd641f5 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -1091,37 +1091,22 @@ static void add_header_field(struct web_session *session)
 	}
 }
 
-static gboolean received_data(GIOChannel *channel, GIOCondition cond,
-							gpointer user_data)
+static void received_data_finalize(struct web_session *session)
 {
-	struct web_session *session = user_data;
-	guint8 *ptr = session->receive_buffer;
-	gsize bytes_read;
-	GIOStatus status;
+	const guint16 code = GWEB_HTTP_STATUS_CODE_UNKNOWN;
 
-	cancel_connect_timeout(session);
+	session->transport_watch = 0;
 
-	if (cond & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
-		session->transport_watch = 0;
-		session->result.buffer = NULL;
-		session->result.length = 0;
-		call_result_func(session, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
-		return FALSE;
-	}
-
-	status = g_io_channel_read_chars(channel,
-				(gchar *) session->receive_buffer,
-				session->receive_space - 1, &bytes_read, NULL);
+	session->result.buffer = NULL;
+	session->result.length = 0;
 
-	debug(session->web, "bytes read %zu", bytes_read);
+	call_result_func(session, code);
+}
 
-	if (status != G_IO_STATUS_NORMAL && status != G_IO_STATUS_AGAIN) {
-		session->transport_watch = 0;
-		session->result.buffer = NULL;
-		session->result.length = 0;
-		call_result_func(session, GWEB_HTTP_STATUS_CODE_UNKNOWN);
-		return FALSE;
-	}
+static bool received_data_continue(struct web_session *session,
+				gsize bytes_read)
+{
+	guint8 *ptr = session->receive_buffer;
 
 	session->receive_buffer[bytes_read] = '\0';
 
@@ -1210,6 +1195,57 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 	return TRUE;
 }
 
+static gboolean received_data(GIOChannel *channel, GIOCondition cond,
+							gpointer user_data)
+{
+	struct web_session *session = user_data;
+	gsize bytes_read;
+	GIOStatus status;
+
+	/* We received some data or condition, cancel the connect timeout. */
+
+	cancel_connect_timeout(session);
+
+	/* If there was a low-level I/O condition or error, there is
+	 * nothing more to do; simply fail the request.
+	 */
+
+	if (cond & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
+		session->transport_watch = 0;
+
+		session->result.buffer = NULL;
+		session->result.length = 0;
+
+		call_result_func(session, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
+
+		return FALSE;
+	}
+
+	/* Attempt to read received data from the channel. */
+
+	status = g_io_channel_read_chars(channel,
+				(gchar *) session->receive_buffer,
+				session->receive_space - 1, &bytes_read, NULL);
+
+	debug(session->web, "bytes read %zu status %d", bytes_read,
+		status);
+
+	/* Handle post-channel read errors, which could be either
+	 * G_IO_STATUS_ERROR or G_IO_STATUS_EOF.
+	 */
+	if (status != G_IO_STATUS_NORMAL && status != G_IO_STATUS_AGAIN) {
+		received_data_finalize(session);
+
+		return FALSE;
+	}
+
+	/* Otherwise, continue the session request and process the
+	 * received data.
+	 */
+
+	return received_data_continue(session, bytes_read);
+}
+
 static int bind_to_address(int sk, const char *interface, int family)
 {
 	struct ifaddrs *ifaddr_list, *ifaddr;
-- 
2.45.0

[PATCH 03/22] gweb: Added 'g_web_result_has_headers'.

[Grant Erickson]

Return whether the web session request result has any HTTP headers
and, if requested, the header count.
---
 gweb/gweb.c | 16 ++++++++++++++++
 gweb/gweb.h |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index d4bdbdd641f5..76f01c44b88f 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -2504,6 +2504,22 @@ bool g_web_result_get_header(GWebResult *result,
 	return true;
 }
 
+bool g_web_result_has_headers(const GWebResult *result,
+				guint *count)
+{
+	guint size;
+
+	if (!result)
+		return false;
+
+	size = g_hash_table_size(result->headers);
+
+	if (count)
+		*count = size;
+
+	return size > 0;
+}
+
 struct _GWebParser {
 	gint ref_count;
 	char *begin_token;
diff --git a/gweb/gweb.h b/gweb/gweb.h
index 7c65aebf4698..90cccadbe883 100644
--- a/gweb/gweb.h
+++ b/gweb/gweb.h
@@ -166,6 +166,8 @@ guint16 g_web_result_get_status(GWebResult *result);
 
 bool g_web_result_get_header(GWebResult *result,
 				const char *header, const char **value);
+bool g_web_result_has_headers(const GWebResult *result,
+				guint *count);
 bool g_web_result_get_chunk(GWebResult *result,
 				const guint8 **chunk, gsize *length);
 
-- 
2.45.0

[PATCH 04/22] gweb: Add documentation to 'g_web_result_has_headers'.

[Grant Erickson]

Add documentation to the 'g_web_result_has_headers' function.
---
 gweb/gweb.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index 76f01c44b88f..d79d2d0808cd 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -2504,6 +2504,25 @@ bool g_web_result_get_header(GWebResult *result,
 	return true;
 }
 
+/**
+ *  @brief
+ *    Return whether the web session request result has any HTTP
+ *    headers.
+ *
+ *  @param[in]  result  A pointer to the immutable web session
+ *                      request result for which to determine if there
+ *                      are any HTTP headers.
+ *  @param[out]  count  An optional pointer to mutable storage for the
+ *                      number of HTTP headers associated with @a
+ *                      result.
+ *
+ *  @returns
+ *    True if there are one or more HTTP headers associated with @a
+ *    result; otherwise, false.
+ *
+ *  @sa g_web_result_get_header
+ *
+ */
 bool g_web_result_has_headers(const GWebResult *result,
 				guint *count)
 {
-- 
2.45.0

[PATCH 05/22] gweb: Added 'g_web_result_get_err'.

[Grant Erickson]

Returns the operating system error, if any, associated with the web
session request result.
---
 gweb/gweb.c | 9 +++++++++
 gweb/gweb.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index d79d2d0808cd..6ada0dfa571c 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -55,6 +55,7 @@ enum chunk_state {
 };
 
 struct _GWebResult {
+	int err;
 	guint16 status;
 	const guint8 *buffer;
 	gsize length;
@@ -2462,6 +2463,14 @@ bool g_web_cancel_request(GWeb *web, guint id)
 	return true;
 }
 
+int g_web_result_get_err(const GWebResult *result)
+{
+	if (!result)
+		return -EINVAL;
+
+	return result->err;
+}
+
 guint16 g_web_result_get_status(GWebResult *result)
 {
 	if (!result)
diff --git a/gweb/gweb.h b/gweb/gweb.h
index 90cccadbe883..e9a8eda9f8f7 100644
--- a/gweb/gweb.h
+++ b/gweb/gweb.h
@@ -162,6 +162,7 @@ guint g_web_request_post_file(GWeb *web, const char *url,
 
 bool g_web_cancel_request(GWeb *web, guint id);
 
+int g_web_result_get_err(const GWebResult *result);
 guint16 g_web_result_get_status(GWebResult *result);
 
 bool g_web_result_get_header(GWebResult *result,
-- 
2.45.0

[PATCH 06/22] gweb: Add documentation to 'g_web_result_get_err'.

[Grant Erickson]

Adds documentation to the 'g_web_result_get_err' function and the
associated 'struct _GWebResult' 'err' data member.
---
 gweb/gweb.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index 6ada0dfa571c..72f83d8f27e4 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -55,6 +55,12 @@ enum chunk_state {
 };
 
 struct _GWebResult {
+	/**
+	 *	Operating system error, if any, associated with the request.
+	 *
+	 *	0 on success; otherwise, < 0 (negate to arrive at a POSIX
+	 *	domain error number).
+	 */
 	int err;
 	guint16 status;
 	const guint8 *buffer;
@@ -2463,6 +2469,22 @@ bool g_web_cancel_request(GWeb *web, guint id)
 	return true;
 }
 
+/**
+ *  @brief
+ *    Returns the operating system error, if any, associated with the
+ *    web session request result.
+ *
+ *  @param[in]  result  A pointer to the immutable web session
+ *                      request result for which to return the
+ *                      operating system error.
+ *
+ *  @returns
+ *    0 on success; otherwise, < 0 (negate to arrive at a POSIX
+ *    domain error number).
+ *
+ *  @sa g_web_result_get_status
+ *
+ */
 int g_web_result_get_err(const GWebResult *result)
 {
 	if (!result)
-- 
2.45.0

[PATCH 07/22] gweb: Add an OS err parameter to 'call_result_func'.

[Grant Erickson]

This allows populating the 'struct _GWebResult' 'err' data member for
operating system-related web request session failures and the use of
'g_web_result_get_err' for interested clients, such as WISPr.
---
 gweb/gweb.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index 72f83d8f27e4..f5accf80cbeb 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -164,17 +164,21 @@ static void _debug(GWeb *web, const char *file, const char *caller,
 	va_end(ap);
 }
 
-static inline void call_result_func(struct web_session *session, guint16 status)
+static inline void call_result_func(struct web_session *session,
+					int err, guint16 status)
 {
+	debug(session->web, "session %p err %d status %d result_func %p",
+		session, err, status, session->result_func);
 
 	if (!session->result_func)
 		return;
 
+	session->result.err = err;
+
 	if (status != GWEB_HTTP_STATUS_CODE_UNKNOWN)
 		session->result.status = status;
 
 	session->result_func(&session->result, session->user_data);
-
 }
 
 static inline void call_route_func(struct web_session *session)
@@ -219,7 +223,7 @@ static gboolean connect_timeout_cb(gpointer user_data)
 	session->result.buffer = NULL;
 	session->result.length = 0;
 
-	call_result_func(session, GWEB_HTTP_STATUS_CODE_REQUEST_TIMEOUT);
+	call_result_func(session, -ETIMEDOUT, GWEB_HTTP_STATUS_CODE_REQUEST_TIMEOUT);
 
 	return G_SOURCE_REMOVE;
 }
@@ -960,7 +964,7 @@ static int decode_chunked(struct web_session *session,
 			if (session->chunk_left <= len) {
 				session->result.buffer = ptr;
 				session->result.length = session->chunk_left;
-				call_result_func(session,
+				call_result_func(session, 0,
 					GWEB_HTTP_STATUS_CODE_UNKNOWN);
 
 				len -= session->chunk_left;
@@ -976,7 +980,7 @@ static int decode_chunked(struct web_session *session,
 			/* more data */
 			session->result.buffer = ptr;
 			session->result.length = len;
-			call_result_func(session,
+			call_result_func(session, 0,
 				GWEB_HTTP_STATUS_CODE_UNKNOWN);
 
 			session->chunk_left -= len;
@@ -1002,7 +1006,7 @@ static int handle_body(struct web_session *session,
 		if (len > 0) {
 			session->result.buffer = buf;
 			session->result.length = len;
-			call_result_func(session,
+			call_result_func(session, 0,
 				GWEB_HTTP_STATUS_CODE_UNKNOWN);
 		}
 		return 0;
@@ -1014,7 +1018,7 @@ static int handle_body(struct web_session *session,
 
 		session->result.buffer = NULL;
 		session->result.length = 0;
-		call_result_func(session, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
+		call_result_func(session, 0, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
 	}
 
 	return err;
@@ -1107,7 +1111,7 @@ static void received_data_finalize(struct web_session *session)
 	session->result.buffer = NULL;
 	session->result.length = 0;
 
-	call_result_func(session, code);
+	call_result_func(session, 0, code);
 }
 
 static bool received_data_continue(struct web_session *session,
@@ -1223,7 +1227,7 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 		session->result.buffer = NULL;
 		session->result.length = 0;
 
-		call_result_func(session, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
+		call_result_func(session, -EIO, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
 
 		return FALSE;
 	}
@@ -2257,14 +2261,14 @@ static void handle_resolved_address(struct web_session *session)
 	ret = getaddrinfo(session->address, port, &hints, &session->addr);
 	g_free(port);
 	if (ret != 0 || !session->addr) {
-		call_result_func(session, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
+		call_result_func(session, 0, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
 		return;
 	}
 
 	call_route_func(session);
 
 	if (create_transport(session) < 0) {
-		call_result_func(session, GWEB_HTTP_STATUS_CODE_CONFLICT);
+		call_result_func(session, 0, GWEB_HTTP_STATUS_CODE_CONFLICT);
 		return;
 	}
 }
@@ -2285,7 +2289,7 @@ static void resolv_result(GResolvResultStatus status,
 	struct web_session *session = user_data;
 
 	if (!results || !results[0]) {
-		call_result_func(session, GWEB_HTTP_STATUS_CODE_NOT_FOUND);
+		call_result_func(session, 0, GWEB_HTTP_STATUS_CODE_NOT_FOUND);
 		return;
 	}
 
@@ -2347,6 +2351,7 @@ static guint do_request(GWeb *web, const char *url,
 	debug(web, "host %s", session->host);
 	debug(web, "flags %lu", session->flags);
 	debug(web, "request %s", session->request);
+	debug(web, "result_func %p", func);
 
 	if (type) {
 		session->content_type = g_strdup(type);
-- 
2.45.0

[PATCH 08/22] gweb: Add documentation to 'call_result_func'.

[Grant Erickson]

Add documentation to the 'call_result_func' function.
---
 gweb/gweb.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index f5accf80cbeb..0256d622e00a 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -164,6 +164,34 @@ static void _debug(GWeb *web, const char *file, const char *caller,
 	va_end(ap);
 }
 
+/**
+ *  @brief
+ *    Invoke the closure callback associated with the web session
+ *    request.
+ *
+ *  This closes the specified web session request by invoking the @a
+ *  result_func originally assigned in #do_request when the session
+ *  was first initiated.
+ *
+ *  @param[in]  session
+ *    A pointer to the mutable web session request for which to invoke
+ *    the closure callback.
+ *
+ *  @param[in]  err
+ *    Operating system error to set in the @a session result
+ *    structure.
+ *
+ *  @param[in]  status
+ *    HTTP status code on success to set in the @a session result
+ *    structure. Note that #GWEB_HTTP_STATUS_CODE_UNKNOWN acts as a
+ *    null value such that the status is only set if the value is
+ *    not #GWEB_HTTP_STATUS_CODE_UNKNOWN.
+ *
+ *  @sa do_request
+ *
+ *  @private
+ *
+ */
 static inline void call_result_func(struct web_session *session,
 					int err, guint16 status)
 {
-- 
2.45.0

[PATCH 09/22] gweb: Add documentation to 'g_web_result_get_status'.

[Grant Erickson]

Add documentation to the 'g_web_result_get_status' function.
---
 gweb/gweb.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index 0256d622e00a..ec72cd417a9c 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -2526,6 +2526,21 @@ int g_web_result_get_err(const GWebResult *result)
 	return result->err;
 }
 
+/**
+ *  @brief
+ *    Returns the HTTP status code, if any, associated with the
+ *    web session request result.
+ *
+ *  @param[in]  result  A pointer to the immutable web session
+ *                      request result for which to return the
+ *                      HTTP status code.
+ *
+ *  @returns
+ *    The HTTP status code.
+ *
+ *  @sa g_web_result_get_err
+ *
+ */
 guint16 g_web_result_get_status(GWebResult *result)
 {
 	if (!result)
-- 
2.45.0

[PATCH 10/22] gweb: Add documentation to 'received_data'.

[Grant Erickson]

Add documentation to the 'received_data' function.
---
 gweb/gweb.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index ec72cd417a9c..78d4fb0517ff 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -1234,6 +1234,28 @@ static bool received_data_continue(struct web_session *session,
 	return TRUE;
 }
 
+/**
+ *  @brief
+ *    Handle a glib I/O channel watch received data delegation for a
+ *    web session request.
+ *
+ *  This handles a glib I/O channel received data delegate for the web
+ *  session request associated with @a channel.
+ *
+ *  @param[in,out]  channel    A pointer to the glib channel that
+ *                             received data or a condition(s)/
+ *                             event(s).
+ *  @param[in]      cond       The conditions or events that
+ *                             generated this delegation.
+ *  @param[in,out]  user_data  A pointer to the mutable web session
+ *                             request associated with @a channel.
+ *
+ *  @sa received_data_finalize
+ *  @sa received_data_continue
+ *
+ *  @private
+ *
+ */
 static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 							gpointer user_data)
 {
-- 
2.45.0

[PATCH 11/22] gweb: Close web request session finalization 'hole'.

[Grant Erickson]

There exist 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. 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".

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 have
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 synthesize the error
'-ECONNRESET' with the same HTTP status "null" value of
'GWEB_HTTP_STATUS_CODE_UNKNOWN'. With the addition of the new
'g_web_result_get_err' interface, clients such as WISPr can now
distinguish between a low-level operating system error in which no
HTTP data was received and an operating system success 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 error case, GWeb now passes a glib GError pointer to
'g_io_channel_read_chars'. If it is set, GWeb maps the resulting error
domain/code pairs to negated POSIX domain errors, and default to
'-EIO' if no suitable mapping can be made. As with the failure EOF
case, this allows clients to distinguish and handle such failures and
to successfully "bookend" their initial web request.
---
 gweb/gweb.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 106 insertions(+), 7 deletions(-)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index 78d4fb0517ff..4a402ccfc346 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -1130,8 +1130,67 @@ static void add_header_field(struct web_session *session)
 	}
 }
 
-static void received_data_finalize(struct web_session *session)
+static int map_gerror(const GError *error)
 {
+	int err;
+
+	if (error->domain == G_CONVERT_ERROR) {
+		switch (error->code) {
+		case G_CONVERT_ERROR_NO_MEMORY:
+			err = -ENOMEM;
+			break;
+		case G_CONVERT_ERROR_ILLEGAL_SEQUENCE:
+			err = -EILSEQ;
+			break;
+		case G_CONVERT_ERROR_PARTIAL_INPUT:
+			err = -EBADMSG;
+			break;
+		default:
+			err = -EIO;
+			break;
+		}
+	} else if (error->domain == G_IO_CHANNEL_ERROR) {
+		switch (error->code) {
+		case G_IO_CHANNEL_ERROR_INVAL:
+			err = -EINVAL;
+			break;
+		case G_IO_CHANNEL_ERROR_FBIG:
+			err = -EFBIG;
+			break;
+		case G_IO_CHANNEL_ERROR_IO:
+			err = -EIO;
+			break;
+		case G_IO_CHANNEL_ERROR_ISDIR:
+			err = -EISDIR;
+			break;
+		case G_IO_CHANNEL_ERROR_NOSPC:
+			err = -ENOSPC;
+			break;
+		case G_IO_CHANNEL_ERROR_NXIO:
+			err = -ENXIO;
+			break;
+		case G_IO_CHANNEL_ERROR_OVERFLOW:
+			err = -EOVERFLOW;
+			break;
+		case G_IO_CHANNEL_ERROR_PIPE:
+			err = -EPIPE;
+			break;
+		default:
+			err = -EIO;
+			break;
+		}
+	} else {
+		err = -EIO;
+	}
+
+	return err;
+}
+
+static void received_data_finalize(struct web_session *session,
+				GIOStatus status, gsize bytes_available,
+				gsize bytes_read, const GError *error)
+{
+	int err = 0;
 	const guint16 code = GWEB_HTTP_STATUS_CODE_UNKNOWN;
 
 	session->transport_watch = 0;
@@ -1139,7 +1198,41 @@ static void received_data_finalize(struct web_session *session)
 	session->result.buffer = NULL;
 	session->result.length = 0;
 
-	call_result_func(session, 0, code);
+	/* Handle post-channel read errors, which could be either
+	 * G_IO_STATUS_ERROR or G_IO_STATUS_EOF.
+	 *
+	 * For G_IO_STATUS_ERROR, simply attempt to map the error from
+	 * GError, if available.
+	 *
+	 * For G_IO_STATUS_EOF, this could occur at the end of a nominal,
+	 * successful get. That is, some number of headers, with or
+	 * without a body, termiated by an expected end-of-file (EOF)
+	 * condition. However, G_IO_STATUS_EOF can also happen as a result
+	 * of the remote peer server unexpectedly terminating the
+	 * connection without transferring any data at all. The only
+	 * reasonable recovery for this case it to fail the request,
+	 * synthesizing -ECONNRESET as the error, and to let the client
+	 * request again.
+	 *
+	 * If we asked for a non-zero amount of data but received none and
+	 * have accumulated no headers thus far, then we assume that the
+	 * remote peer server unexpectedly closed the connection;
+	 * otherwise, we assume it is a normal EOF closure.
+	 */
+
+	if (status == G_IO_STATUS_ERROR) {
+		if (error != NULL)
+			err = map_gerror(error);
+		else
+			err = -EIO;
+	} else if (status == G_IO_STATUS_EOF) {
+		if (bytes_available > 0 &&
+				bytes_read == 0 &&
+				!g_web_result_has_headers(&session->result, NULL))
+			err = -ECONNRESET;
+	}
+
+	call_result_func(session, err, code);
 }
 
 static bool received_data_continue(struct web_session *session,
@@ -1260,8 +1353,10 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 							gpointer user_data)
 {
 	struct web_session *session = user_data;
+	const gsize bytes_available = session->receive_space - 1;
 	gsize bytes_read;
 	GIOStatus status;
+	GError *error = NULL;
 
 	/* We received some data or condition, cancel the connect timeout. */
 
@@ -1277,7 +1372,7 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 		session->result.buffer = NULL;
 		session->result.length = 0;
 
-		call_result_func(session, -EIO, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
+		call_result_func(session, -EIO, GWEB_HTTP_STATUS_CODE_UNKNOWN);
 
 		return FALSE;
 	}
@@ -1286,16 +1381,20 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 
 	status = g_io_channel_read_chars(channel,
 				(gchar *) session->receive_buffer,
-				session->receive_space - 1, &bytes_read, NULL);
+				bytes_available, &bytes_read, &error);
 
-	debug(session->web, "bytes read %zu status %d", bytes_read,
-		status);
+	debug(session->web, "bytes read %zu status %d error %p", bytes_read,
+		status, error);
 
 	/* Handle post-channel read errors, which could be either
 	 * G_IO_STATUS_ERROR or G_IO_STATUS_EOF.
 	 */
 	if (status != G_IO_STATUS_NORMAL && status != G_IO_STATUS_AGAIN) {
-		received_data_finalize(session);
+		received_data_finalize(session, status,
+			bytes_available, bytes_read, error);
+
+		if (error != NULL)
+			g_clear_error(&error);
 
 		return FALSE;
 	}
-- 
2.45.0

[PATCH 12/22] gweb: Add documentation to 'map_gerror'.

[Grant Erickson]

Add documentation to the 'map_gerror' function.
---
 gweb/gweb.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index 4a402ccfc346..12fe682cedfb 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -1130,6 +1130,20 @@ static void add_header_field(struct web_session *session)
 	}
 }
 
+/**
+ *  @brief
+ *    Map a glib error into the negated POSIX error domain.
+ *
+ *  This attempts to map the specfied glib error into the negated
+ *  POSIX error domain, defaulting to -EIO for unmapped domain/code
+ *  pairs.
+ *
+ *  @param[in]  error  A pointer to the immutable glib error to map.
+ *
+ *  @returns
+ *    A mapped glib error into the negated POSIX error domain.
+ *
+ */
 static int map_gerror(const GError *error)
 {
 	int err;
-- 
2.45.0

[PATCH 13/22] gweb: Add documentation to 'received_data_{finalize,continue}'.

[Grant Erickson]

Add documentation to the 'received_data_{finalize,continue}' functions.
---
 gweb/gweb.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index 12fe682cedfb..c7af3986df8f 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -1200,6 +1200,43 @@ static int map_gerror(const GError *error)
 	return err;
 }
 
+/**
+ *  @brief
+ *    Finalize a glib I/O channel watch received data delegation for a
+ *    web session request.
+ *
+ *  This finalizes a glib I/O channel received data failure or
+ *  success for @a session. This assumes that @a status is either
+ *  #G_IO_STATUS_ERROR or #G_IO_STATUS_EOF. #G_IO_STATUS_ERROR
+ *  represents an unconditional failure completion. #G_IO_STATUS_EOF
+ *  may represent a successful completion, if it follows one more
+ *  prior received data transfers for @a session. However, it
+ *  represents a failure completion if it occurs prior to the receipt
+ *  of any @a session data.
+ *
+ *  @param[in,out]  session          A pointer to the mutable web
+ *                                   session request to finalize.
+ *  @param[in]      status           The status from the prior call
+ *                                   to #g_io_channel_read_chars used
+ *                                   to determine how to finalize @a
+ *                                   session.
+ *  @param[in]      bytes_available  The number of bytes advertised
+ *                                   to the prior call to
+ *                                   #g_io_channel_read_chars.
+ *  @param[in]      bytes_read       The number of bytes read by the
+ *                                   prior call to
+ *                                   #g_io_channel_read_chars.
+ *  @param[in]      error            An optional pointer to the glib
+ *                                   error instance associated the
+ *                                   prior call to
+ *                                   #g_io_channel_read_chars.
+ *
+ *  @sa received_data_continue
+ *  @sa received_data
+ *
+ *  @private
+ *
+ */
 static void received_data_finalize(struct web_session *session,
 				GIOStatus status, gsize bytes_available,
 				gsize bytes_read, const GError *error)
@@ -1249,6 +1286,23 @@ static void received_data_finalize(struct web_session *session,
 	call_result_func(session, err, code);
 }
 
+/**
+ *  @brief
+ *    Continue a glib I/O channel watch received data delegation for a
+ *    web session request and process the data from it.
+ *
+ *  @param[in,out]  session     A pointer to the mutable web session
+ *                              request to continue and process
+ *                              received data for.
+ *  @param[in]      bytes_read  The number of bytes read by the prior
+ *                              call to #g_io_channel_read_chars.
+ *
+ *  @sa received_data_finalize
+ *  @sa received_data
+ *
+ *  @private
+ *
+ */
 static bool received_data_continue(struct web_session *session,
 				gsize bytes_read)
 {
-- 
2.45.0

[PATCH 14/22] gweb: Document 'struct _GWebResult'.

[Grant Erickson]

Add documentation to the remainder of the 'struct _GWebResult' data
members and to the overall structure.
---
 gweb/gweb.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/gweb/gweb.c b/gweb/gweb.c
index c7af3986df8f..050bb2d3e134 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -54,6 +54,12 @@ enum chunk_state {
 	CHUNK_DATA,
 };
 
+/**
+ *	The opaque structure presented to GWeb clients on received data or
+ *	request closures.
+ *
+ *  @private
+ */
 struct _GWebResult {
 	/**
 	 *	Operating system error, if any, associated with the request.
@@ -62,11 +68,38 @@ struct _GWebResult {
 	 *	domain error number).
 	 */
 	int err;
+
+	/**
+	 *	HTTP status code on success.
+	 */
 	guint16 status;
+
+	/**
+	 *	HTTP body content on success; otherwise, NULL.
+	 */
 	const guint8 *buffer;
+
+	/**
+	 *	HTTP body length on success; otherwise, 0.
+	 */
 	gsize length;
+
+	/**
+	 *	Boolean indicating whether the HTTP response uses HTTP/1.1
+	 *	chunked transfer encoding.
+	 */
 	bool use_chunk;
+
+	/**
+	 *	An optional pointer to a null-terminated C string containing
+	 *	the last HTTP header name added as part of a header key/value
+	 *	pair.
+	 */
 	gchar *last_key;
+
+	/**
+	 *	HTTP headers, on success, keyed by the header name.
+	 */
 	GHashTable *headers;
 };
 
-- 
2.45.0

[PATCH 15/22] wispr: Add and leverage 'portal_manage_failure_status'.

[Grant Erickson]

Consolidates seven common call site patterns into a common function,
reducing the parameters passed at each call site from four to two.
---
 src/wispr.c | 42 +++++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 25 deletions(-)

diff --git a/src/wispr.c b/src/wispr.c
index 9b0e4b24a3db..fc49778fa607 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -633,6 +633,16 @@ static void wispr_portal_error(struct connman_wispr_portal_context *wp_context)
 	wp_context->wispr_result = CONNMAN_WISPR_RESULT_FAILED;
 }
 
+static void portal_manage_failure_status(
+			struct connman_wispr_portal_context *wp_context,
+			int err)
+{
+	struct connman_service *service = wp_context->service;
+	enum connman_ipconfig_type type = wp_context->type;
+
+	wp_context->cb(service, type, false, err);
+}
+
 /**
  *  @brief
  *    Handle a successful "online" HTTP-based Internet reachability
@@ -773,10 +783,7 @@ static void wispr_portal_request_portal(
 					wp_context, &err);
 
 	if (wp_context->request_id == 0) {
-		wp_context->cb(wp_context->service,
-						wp_context->type,
-						false,
-						err);
+		portal_manage_failure_status(wp_context, err);
 		wispr_portal_error(wp_context);
 		wispr_portal_context_unref(wp_context);
 	}
@@ -1049,24 +1056,15 @@ static bool wispr_portal_web_result(GWebResult *result, gpointer user_data)
 
 		goto done;
 	case GWEB_HTTP_STATUS_CODE_BAD_REQUEST:
-		wp_context->cb(wp_context->service,
-				wp_context->type,
-				false,
-				-EINVAL);
+		portal_manage_failure_status(wp_context, -EINVAL);
 		break;
 
 	case GWEB_HTTP_STATUS_CODE_NOT_FOUND:
-		wp_context->cb(wp_context->service,
-				wp_context->type,
-				false,
-				-ENOENT);
+		portal_manage_failure_status(wp_context, -ENOENT);
 		break;
 
 	case GWEB_HTTP_STATUS_CODE_REQUEST_TIMEOUT:
-		wp_context->cb(wp_context->service,
-				wp_context->type,
-				false,
-				-ETIMEDOUT);
+		portal_manage_failure_status(wp_context, -ETIMEDOUT);
 		break;
 
 	case GWEB_HTTP_STATUS_CODE_HTTP_VERSION_NOT_SUPPORTED:
@@ -1169,10 +1167,7 @@ static void proxy_callback(const char *proxy, void *user_data)
 	if (!proxy) {
 		wispr_log_proxy_failure(wp_context, "No valid proxy");
 
-		wp_context->cb(wp_context->service,
-				wp_context->type,
-				false,
-				-EINVAL);
+		portal_manage_failure_status(wp_context, -EINVAL);
 
 		return;
 	}
@@ -1519,7 +1514,7 @@ int __connman_wispr_start(struct connman_service *service,
 free_wp:
 	DBG("err %d wp_context %p", err, wp_context);
 
-	wp_context->cb(wp_context->service, wp_context->type, false, err);
+	portal_manage_failure_status(wp_context, err);
 
 	g_hash_table_remove(wispr_portal_hash, GINT_TO_POINTER(index));
 	return err;
@@ -1605,10 +1600,7 @@ int __connman_wispr_cancel(struct connman_service *service,
 
 	cancel_connman_wispr_portal_context(wp_context);
 
-	wp_context->cb(wp_context->service,
-			wp_context->type,
-			false,
-			-ECANCELED);
+	portal_manage_failure_status(wp_context, -ECANCELED);
 
 	wispr_portal_context_unref(wp_context);
 
-- 
2.45.0

[PATCH 16/22] wispr: Refactor 'wispr_portal_web_result' to leverage 'g_web_result_get_err'.

[Grant Erickson]

Leverages the newly-added GWeb 'g_web_result_get_err' interface to
refactor 'wispr_portal_web_result' into 'wispr_portal_web_result_err'
and 'wispr_portal_web_result_no_err' with the former handling
non-successful POSIX error domain cases and the latter handling
successful HTTP status code cases.

Prior to the introduction of 'g_web_result_get_err' 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
'-ECONNRESET' from 'g_web_result_get_err' and will be handled as a
failure by 'wispr_portal_web_result_err', "bookending" the original
"online" HTTP-based Internet reachability check.

All of the prior HTTP status code cases are handled by
'wispr_portal_web_result_no_err'.
---
 src/wispr.c | 95 ++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 69 insertions(+), 26 deletions(-)

diff --git a/src/wispr.c b/src/wispr.c
index fc49778fa607..d1d49704e5bf 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -782,6 +782,8 @@ 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);
 		wispr_portal_error(wp_context);
@@ -973,38 +975,26 @@ static bool wispr_manage_message(GWebResult *result,
 	return false;
 }
 
-static bool wispr_portal_web_result(GWebResult *result, gpointer user_data)
+static void wispr_portal_web_result_err(GWebResult *result,
+		struct connman_wispr_portal_context *wp_context,
+		int err)
 {
-	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("");
-
-	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;
-		}
+	portal_manage_failure_status(wp_context, err);
 
-		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_no_err(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);
 
-	DBG("status: %03u", status);
+	connman_info("status: %03u", status);
 
 	switch (status) {
 	case GWEB_HTTP_STATUS_CODE_UNKNOWN:
@@ -1073,12 +1063,64 @@ static bool wispr_portal_web_result(GWebResult *result, gpointer user_data)
 				wispr_portal_browser_reply_cb,
 				wp_context->status_url, wp_context);
 		break;
+
 	default:
 		break;
 	}
 
 	free_wispr_routes(wp_context);
 	wp_context->request_id = 0;
+
+done:
+	return;
+}
+
+static bool wispr_portal_web_result(GWebResult *result, gpointer user_data)
+{
+	struct connman_wispr_portal_context *wp_context = user_data;
+	const guint8 *chunk = NULL;
+	int err;
+	gsize length;
+
+	DBG("result %p user_data %p wispr_result %d",
+		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;
+		}
+	}
+
+	/* Check whether there was an operating system error while
+	 * processing the web request associated with the "online"
+	 * HTTP-based Internet reachability check.
+	 */
+
+	err = g_web_result_get_err(result);
+
+	DBG("err %d", err);
+
+	if (err < 0)
+		wispr_portal_web_result_err(result, wp_context, err);
+	else
+		wispr_portal_web_result_no_err(result, wp_context);
+
 done:
 	wp_context->wispr_msg.message_type = -1;
 
@@ -1088,6 +1130,7 @@ done:
 	 * maintaining a weak reference to it.
 	 */
 	wispr_portal_context_unref(wp_context);
+
 	return false;
 }
 
-- 
2.45.0

[PATCH 17/22] wispr: Document 'portal_manage_failure_status'.

[Grant Erickson]

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

diff --git a/src/wispr.c b/src/wispr.c
index d1d49704e5bf..83422c96b50d 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -633,6 +633,34 @@ static void wispr_portal_error(struct connman_wispr_portal_context *wp_context)
 	wp_context->wispr_result = CONNMAN_WISPR_RESULT_FAILED;
 }
 
+/**
+ *  @brief
+ *    Handle an unsuccessful "online" HTTP-based Internet reachability
+ *    check.
+ *
+ *  This handles an unsuccessful (that is, either completed with a
+ *  non-successful operating system error or with a non-HTTP 200 "OK"
+ *  status code) "online" HTTP-based Internet reachability check
+ *  previously-initiated with #__connman_wispr_start.
+ *
+ *  @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
+ *                              handling.
+ *  @param[in]      err         The negated POSIX domain error
+ *                              associated with the unsuccessful
+ *                              "online" HTTP-based Internet
+ *                              reachability check.
+ *
+ *  @sa portal_manage_success_status
+ *  @sa wispr_portal_web_result_no_err
+ *  @sa wispr_portal_web_result_err
+ *  @sa wispr_portal_web_result
+ *
+ *  @private
+ *
+ */
 static void portal_manage_failure_status(
 			struct connman_wispr_portal_context *wp_context,
 			int err)
-- 
2.45.0

[PATCH 18/22] wispr: Document 'wispr_portal_web_result_err'.

[Grant Erickson]

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

diff --git a/src/wispr.c b/src/wispr.c
index 83422c96b50d..0a29303116eb 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -1003,6 +1003,29 @@ 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]      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.
+ *  @param[in]      err         The negated POSIX domain error
+ *                              associated with the unsuccessful
+ *                              "online" HTTP-based Internet
+ *                              reachability check.
+ *
+ *  @sa wispr_portal_web_result
+ *  @sa wispr_portal_web_result_no_err
+ *
+ *  @private
+ *
+ */
 static void wispr_portal_web_result_err(GWebResult *result,
 		struct connman_wispr_portal_context *wp_context,
 		int err)
-- 
2.45.0

[PATCH 19/22] wispr: Document 'wispr_portal_web_result_no_err'.

[Grant Erickson]

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

diff --git a/src/wispr.c b/src/wispr.c
index 0a29303116eb..004227d5f1c7 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -1036,6 +1036,25 @@ static void wispr_portal_web_result_err(GWebResult *result,
 	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_no_err
+ *
+ *  @private
+ *
+ */
 static void wispr_portal_web_result_no_err(GWebResult *result,
 		struct connman_wispr_portal_context *wp_context)
 {
-- 
2.45.0

[PATCH 20/22] wispr: Document 'wispr_portal_web_result'.

[Grant Erickson]

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

diff --git a/src/wispr.c b/src/wispr.c
index 004227d5f1c7..4abb8f071352 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -1145,6 +1145,33 @@ done:
 	return;
 }
 
+/**
+ *  @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]      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_err
+ *  @sa wispr_portal_web_result_no_err
+ *  @sa wispr_manage_message
+ *
+ *  @private
+ *
+ */
 static bool wispr_portal_web_result(GWebResult *result, gpointer user_data)
 {
 	struct connman_wispr_portal_context *wp_context = user_data;
-- 
2.45.0

[PATCH 21/22] wispr: Fix documentation typo in 'portal_manage_success_status'.

[Grant Erickson]

Fix a documentation typo in the 'wp_context' parameter for
'portal_manage_success_status'.
---
 src/wispr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/wispr.c b/src/wispr.c
index 4abb8f071352..49801b015e30 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -685,7 +685,7 @@ static void portal_manage_failure_status(
  *                              Internet reachability check this is
  *                              handling.
  *  @param[in,out]  wp_context  A pointer to the mutable WISPr portal
- *                              detection context associated the
+ *                              detection context associated with the
  *                              successful "online" HTTP-based
  *                              Internet reachability check this is
  *                              handling.
-- 
2.45.0

[PATCH 22/22] service: Capture and propagate '__connman_wispr_start' return status.

[Grant Erickson]

'__connman_wispr_start' has a number of early failure conditions
before the call closure becomes asynchronous. Capture the return
status from '__connman_wispr_start' in '__connman_service_wispr_start'
and propagate it forward, ensuring that 'online_check_active_set' is
NOT called in the failure case.
---
 src/service.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/service.c b/src/service.c
index 0cfb516016b5..48a7d1f72367 100644
--- a/src/service.c
+++ b/src/service.c
@@ -4003,6 +4003,8 @@ static int start_online_check_if_connected(struct connman_service *service)
 int __connman_service_wispr_start(struct connman_service *service,
 					enum connman_ipconfig_type type)
 {
+	int err;
+
 	DBG("service %p (%s) type %d (%s)",
 		service,
 		connman_service_get_identifier(service),
@@ -4034,14 +4036,17 @@ int __connman_service_wispr_start(struct connman_service *service,
 		service->online_check_state_ipv6.interval =
 					online_check_initial_interval;
 
-	__connman_wispr_start(service, type,
+	err = __connman_wispr_start(service, type,
 			online_check_connect_timeout_ms, complete_online_check);
+	if (err < 0)
+		goto done;
 
 	/* Mark the online check state as active. */
 
 	online_check_active_set(service, type);
 
-	return 0;
+done:
+	return err;
 }
 
 /**
-- 
2.45.0