Re: [PATCH v4 5/9] wispr: Refactor 'wispr_portal_web_result' to leverage 'GError'.

Hi Grant,

On 3/24/25 11:59 PM, Grant Erickson wrote:
> 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, 71 insertions(+), 34 deletions(-)
> 

<snip>

> @@ -1085,15 +1078,15 @@ static bool wispr_portal_web_result(const GError *error, GWebResult *result,
>   
>   		goto done;

This part seems fishy.  Previous version of this function had 
wispr_portal_context_unref() call after the 'done' label...

>   	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:
> @@ -1102,12 +1095,55 @@ static bool wispr_portal_web_result(const GError *error, GWebResult *result,
>   				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;

But now you have a simple return at the end of the function.  Which by the way 
is discouraged and should have been picked up by the compiler or static analysis.

> +}
> +
> +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);

We still use 80 character lines, so please break up accordingly.

> +
> +		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;
>   
> @@ -1117,6 +1153,7 @@ done:
>   	 * maintaining a weak reference to it.
>   	 */
>   	wispr_portal_context_unref(wp_context);
> +

Please don't introduce spurious whitespace changes and keep the patches minimal.

>   	return false;
>   }
>   

<snip>

> @@ -1629,7 +1666,7 @@ 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));

 > 80 chars again

>   
>   	wispr_portal_context_unref(wp_context);
>   

Regards,
-Denis