Re: [PATCH 03/15] service: Leverage 'service_set_pac' function.

On Mar 4, 2025, at 9:06 AM, Denis Kenzior <[email protected]> wrote:
> On 2/21/25 9:56 AM, Grant Erickson wrote:
>> Leverage 'service_set_pac' at the three call sites the proxy
>> auto-configuration (PAC) URL is currently set or cleared for a network
>> service.
>> Frequently, a spurious, misconfigured, or otherwise incorrect PAC URL
>> is the source of connectivity problems, particularly for online
>> reachability checks. Consequently, being able to log all such
>> mutations can be helpful in triaging such problems.
>> ---
>>  src/service.c | 17 ++++-------------
>>  1 file changed, 4 insertions(+), 13 deletions(-)
>> diff --git a/src/service.c b/src/service.c
>> index c48959a7940f..da851d681bc7 100644
>> --- a/src/service.c
>> +++ b/src/service.c
>> @@ -770,8 +770,7 @@ static int service_load(struct connman_service *service)
>>   str = g_key_file_get_string(keyfile,
>>   service->identifier, "Proxy.URL", NULL);
>>   if (str) {
>> - g_free(service->pac);
>> - service->pac = str;
>> + service_set_pac(service, str, false);
> 
> Does this leak str?

I do not believe so, no. service_set_pac now handles the same:

    g_free(service->pac);

    if (pac && strlen(pac) > 0)
        service->pac = g_strstrip(g_strdup(pac));
    else
        service->pac = NULL;

except now instead of being individually duplicated and scattered around to several call sites, it is all handled in one with service_set_pac.

> Also, Kernel Coding Style prefers that braces are not used for single statement blocks.  Quoting:
> "Do not unnecessarily use braces where a single statement will do.
> 
> if (condition)
>        action();
> "
> 
> See https://www.kernel.org/doc/html/latest/process/coding-style.html

I will address this.

Thanks,

Grant

-- 
Principal
Nuovations

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