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

On Mar 25, 2025, at 7:28 AM, Denis Kenzior <[email protected]> wrote:
> 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.
>> 
>> <snip>
>> 
>> ---
>>  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...

As far as I can see, both success and failure termination paths with or without ‘goto done’ calls, lead to the wispr_portal_context_unref() block:

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;

	/*
	 * Release a reference to the WISPr/portal context to balance the
	 * earlier reference retained to account for ‘g_web_request_get'
	 * maintaining a weak reference to it.
	 */
	wispr_portal_context_unref(wp_context);

	return false;
}

>>   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.

I tried to keep the original block of logic as-is. 

Personally, I’m a fan of single points of exit—so much easier to audit. But, happy to keep whatever style is preferred. It sounds like the preference is to replace the prior ‘goto done’ with ‘return’, correct?

>> + 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.

Surprisingly, checkpatch.pl did not squawk about this. Fixed, regardless.

>> @@ -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

Surprisingly, checkpatch.pl did not squawk about this either. Fixed, regardless.

Best,

Grant

-- 
Principal
Nuovations

[email protected]
https://www.nuovations.com/