Re: [PATCH v3 09/18] gweb: Adopt optional, immutable GError instance for GWeb result errors.

Hi Grant,

On 3/20/25 6:39 PM, Grant Erickson wrote:
> In the following instances, GWeb 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'.
> 
> Rather than trying to overload 'GWEB_HTTP_STATUS_CODE_' error
> enumerations from actual web server responses for these non-response
> or response-in-progress failures, the signature of 'GWebResultFunc' is
> amended to take an optional pointer to an immutable GError instance
> which describes one of the five errors above on a GWeb request
> failure.

This looks so much better, a few nits:

> ---
>   gweb/gweb.c      | 125 +++++++++++++++++++++++++++++++++++------------
>   gweb/gweb.h      |   2 +-
>   src/6to4.c       |   3 +-
>   src/wispr.c      |   5 +-
>   tools/web-test.c |   2 +-
>   tools/wispr.c    |   2 +-
>   6 files changed, 101 insertions(+), 38 deletions(-)
> 

<snip>

> @@ -233,9 +233,14 @@ static inline void call_route_func(struct web_session *session)
>   static gboolean connect_timeout_cb(gpointer user_data)
>   {
>   	struct web_session *session = user_data;
> +	g_autofree char *message = NULL;
> +	GError *local_error = NULL;

Could you use:

g_autoptr(GError) local_error = NULL;

...

> +
> +	message = g_strdup_printf("connect timeout to %s after %ums",
> +		session->address, g_web_get_connect_timeout(session->web));
>   
> -	debug(session->web, "session %p connect timeout after %ums",
> -			session, g_web_get_connect_timeout(session->web));
> +	debug(session->web, "session %p %s",
> +			session, message);
>   
>   	session->connect_timeout = 0;
>   
> @@ -244,7 +249,14 @@ 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);
> +	local_error = g_error_new_literal(G_IO_ERROR,
> +					G_IO_ERROR_TIMED_OUT,
> +					message);
> +
> +	call_result_func(session, local_error);
> +
> +	if (local_error)
> +		g_error_free(local_error);

And avoid this if statement completely.

>   
>   	return G_SOURCE_REMOVE;
>   }
> @@ -1020,6 +1030,8 @@ static int handle_body(struct web_session *session,
>   				const guint8 *buf, gsize len)
>   {
>   	int err;
> +	g_autofree char *message = NULL;
> +	GError *local_error = NULL;

Ditto here

>   
>   	debug(session->web, "[body] length %zu", len);
>   
> @@ -1027,19 +1039,28 @@ static int handle_body(struct web_session *session,
>   		if (len > 0) {
>   			session->result.buffer = buf;
>   			session->result.length = len;
> -			call_result_func(session,
> -				GWEB_HTTP_STATUS_CODE_UNKNOWN);
> +			call_result_func(session, NULL);
>   		}
>   		return 0;
>   	}
>   
>   	err = decode_chunked(session, buf, len);
>   	if (err < 0) {
> -		debug(session->web, "Error in chunk decode %d", err);
> +		message = g_strdup_printf("Error in chunk decode %d", err);
> +
> +		debug(session->web, message);
>   
>   		session->result.buffer = NULL;
>   		session->result.length = 0;
> -		call_result_func(session, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
> +
> +		local_error = g_error_new_literal(G_WEB_ERROR,
> +			G_WEB_ERROR_CHUNK_DECODE,
> +			message);
> +
> +		call_result_func(session, local_error);
> +
> +		if (local_error)
> +			g_error_free(local_error);

Ditto

>   	}
>   
>   	return err;

<snip>

> @@ -1255,6 +1274,7 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
>   	struct web_session *session = user_data;
>   	gsize bytes_read;
>   	GIOStatus status;
> +	GError *local_error = NULL;

And here

>   
>   	/* We received some data or condition, cancel the connect timeout. */
>   
> @@ -1270,7 +1290,14 @@ 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);
> +		local_error = g_error_new_literal(G_IO_ERROR,
> +			g_io_error_from_errno(EIO),
> +			g_strerror(EIO));
> +
> +		call_result_func(session, local_error);
> +
> +		if (local_error)
> +			g_error_free(local_error);

Ditto

>   
>   		return FALSE;
>   	}
> @@ -2288,6 +2315,8 @@ static void handle_resolved_address(struct web_session *session)
>   	struct addrinfo hints;
>   	g_autofree char *port;
>   	int ret;
> +	g_autofree char *message = NULL;
> +	GError *local_error = NULL;
>   
>   	debug(session->web, "address %s", session->address);
>   
> @@ -2303,16 +2332,35 @@ static void handle_resolved_address(struct web_session *session)
>   	port = g_strdup_printf("%u", session->port);
>   	ret = getaddrinfo(session->address, port, &hints, &session->addr);
>   	if (ret != 0 || !session->addr) {
> -		call_result_func(session, GWEB_HTTP_STATUS_CODE_BAD_REQUEST);
> -		return;
> +		message = g_strdup_printf("could not resolve %s: %s",
> +			session->address,
> +			gai_strerror(ret));

Maybe consider dropping 'message' variable entirely and using g_set_error or 
g_error_new?

> +
> +		local_error = g_error_new_literal(G_WEB_ERROR,
> +			G_WEB_ERROR_HOST_NOT_FOUND,
> +			message);
> +
> +		call_result_func(session, local_error);
> +
> +		goto done;
>   	}
>   
>   	call_route_func(session);
>   
> -	if (create_transport(session) < 0) {
> -		call_result_func(session, GWEB_HTTP_STATUS_CODE_CONFLICT);
> -		return;
> +	ret = create_transport(session);
> +	if (ret < 0) {
> +		local_error = g_error_new_literal(G_IO_ERROR,
> +						g_io_error_from_errno(ret),
> +						g_strerror(ret));
> +
> +		call_result_func(session, local_error);
> +
> +		goto done;

nit: The above line is not needed

>   	}
> +
> + done:
> +	if (local_error)
> +		g_error_free(local_error);
>   }
>   
>   static gboolean already_resolved(gpointer data)
> @@ -2329,9 +2377,22 @@ static void resolv_result(GResolvResultStatus status,
>   					char **results, gpointer user_data)
>   {
>   	struct web_session *session = user_data;
> +	g_autofree char *message = NULL;
> +	GError *local_error = NULL;
>   
>   	if (!results || !results[0]) {
> -		call_result_func(session, GWEB_HTTP_STATUS_CODE_NOT_FOUND);
> +		message = g_strdup_printf("could not resolve %s",
> +			session->host);
> +
> +		local_error = g_error_new_literal(G_WEB_ERROR,
> +				G_WEB_ERROR_HOST_NOT_FOUND,
> +				message);
> +

Same here, consider dropping 'message' entirely and using g_set_error or g_error_new

> +		call_result_func(session, local_error);
> +
> +		if (local_error)
> +			g_error_free(local_error);
> +
>   		return;
>   	}
>   

Regards,
-Denis