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

[Denis Kenzior <denkenz@gmail.com>]

Hi Grant,

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?

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

>   	}
>   
>   	service->mdns_config = g_key_file_get_boolean(keyfile,

Regards,
-Denis