Re: [PATCH 08/15] service: Leverage 'service_set_proxy_method'.

On Mar 4, 2025, at 9:09 AM, Denis Kenzior <[email protected]> wrote:
> On 2/21/25 9:56 AM, Grant Erickson wrote:
>> Leverage 'service_set_proxy_method' for
>> 'connman_service_set_proxy_method', the first of two call sites.
> 
> Could this commit description be clearer?  E.g. mention connman_service_set_proxy_method() instead of 'first of two call sites'?

Will revise.

>> ---
>>  src/service.c | 19 +++++++------------
>>  1 file changed, 7 insertions(+), 12 deletions(-)
>> diff --git a/src/service.c b/src/service.c
>> index e58958635992..5c94c2962de8 100644
>> --- a/src/service.c
>> +++ b/src/service.c
>> @@ -5617,19 +5617,14 @@ static void service_set_proxy_method(struct connman_service *service,
>>  void connman_service_set_proxy_method(struct connman_service *service,
>>   enum connman_service_proxy_method method)
>>  {
>> - DBG("service %p (%s) method %d (%s)",
>> - service, connman_service_get_identifier(service),
>> - method, proxymethod2string(method));
>> -
>> - if (!service || service->hidden)
>> - return;
>> -
>> - service->proxy = method;
>> + const bool donotifier = method != CONNMAN_SERVICE_PROXY_METHOD_AUTO;
>> + void * const context = NULL;
> 
> What's the point to this variable?

As always, I have a preference / habit of making code self-documenting wherever possible. So, rather than seeing “true, NULL” in the parameter list and then having to run through the code to figure out what those are / mean, it’s self-documented.

>>  - proxy_changed(service);
>> -
>> - if (method != CONNMAN_SERVICE_PROXY_METHOD_AUTO)
>> - __connman_notifier_proxy_changed(service);
>> + service_set_proxy_method(service,
>> + method,
>> + donotifier,
>> + NULL,
>> + context);
> 
> Just use NULL here which is clearer?

Will do.

Best,

Grant

-- 
Principal
Nuovations

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