hi Jukka,

On Wed, Apr 4, 2012 at 11:09 AM, Jukka Rissanen <
[email protected]> wrote:

> Hi Alok,
>
>
> On 04/03/2012 11:50 PM, Alok Barsode wrote:
>
>> From: Alok 
>> Barsode<alok.barsode@linux.**intel.com<[email protected]>
>> >
>>
>> Add default service Gateway to the timeserver query list.
>> Now the list is service timeservers(via DHCP), gateway and global
>> timeservers, in that order.
>> ---
>>  src/timeserver.c |   33 ++++++++++++++++++++++++++++--**---
>>  1 files changed, 28 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/timeserver.c b/src/timeserver.c
>> index 308355f..b81129a 100644
>> --- a/src/timeserver.c
>> +++ b/src/timeserver.c
>> @@ -35,6 +35,7 @@
>>  static GSList *ts_list = NULL;
>>
>>  static char **service_ts = NULL;
>> +static const char *service_gw = NULL;
>>
>
> This is unnecessary initialization, static is NULL anyway. As the other
> variables around it have done initialization so perhaps this is not worth
> fixing.
>
> Actually, do we need to remember service_gw because the
> __connman_timeserver_sync() will fetch it anyway if service is not NULL?
>

we need to cache it , to reconstruct the server list when we dont have the
service pointer. For example when the global timeservers are modified, we
call  __connman_timeserver_sync(NULL). In case like this we dont have the
service pointer to get the gw.

>
>
>>  static GResolv *resolv = NULL;
>>  static int resolv_id = 0;
>> @@ -162,13 +163,14 @@ void __connman_timeserver_sync_**next()
>>   * __connman_timeserver_sync function recreates the timeserver
>>   * list which will be used to determine NTP server for time corrections.
>>   * It must be called everytime the default service changes, the service
>> - * timeserver(s) changes or the global timeserver(s) changes.The service
>> - * settings take priority over the global timeservers.
>> + * timeserver(s) or gatway changes or the global timeserver(s) changes.
>> + * The service settings take priority over the global timeservers.
>>   */
>>  int __connman_timeserver_sync(**struct connman_service *service)
>>  {
>> +       struct connman_network *network;
>>        char **timeservers;
>> -       int i;
>> +       int i, index;
>>
>>        if (resolv == NULL)
>>                return 0;
>> @@ -187,12 +189,25 @@ int __connman_timeserver_sync(**struct
>> connman_service *service)
>>                ts_list = NULL;
>>        }
>>
>> -       if (service != NULL)
>> +       if (service != NULL) {
>>                service_ts = connman_service_get_**timeservers(service);
>>
>> +               network = __connman_service_get_network(**service);
>> +
>> +               index = connman_network_get_index(**network);
>> +
>> +               service_gw = __connman_ipconfig_get_**
>> gateway_from_index(index);
>> +       }
>> +
>> +       /* First add Service Timeservers via DHCP to the list */
>>        for (i=0; service_ts != NULL&&  service_ts[i] != NULL; i++)
>>
>>                ts_list = g_slist_prepend(ts_list,
>> g_strdup(service_ts[i]));
>>
>> +       /* Then add Service Gateway to the list */
>> +       if (service_gw != NULL)
>> +               ts_list = g_slist_prepend(ts_list, g_strdup(service_gw));
>> +
>> +       /* Then add Global Timeservers to the list */
>>        timeservers = load_timeservers();
>>
>>        for (i=0; timeservers != NULL&&  timeservers[i] != NULL; i++)
>>
>> @@ -212,7 +227,8 @@ int __connman_timeserver_sync(**struct
>> connman_service *service)
>>
>>  static int timeserver_start(struct connman_service *service)
>>  {
>> -       char **nameservers = NULL;
>> +       struct connman_network *network;
>> +       char **nameservers;
>>        int i;
>>
>>        DBG("service %p", service);
>> @@ -248,6 +264,13 @@ static int timeserver_start(struct connman_service
>> *service)
>>        /* Cache service timeservers */
>>        service_ts = connman_service_get_**timeservers(service);
>>
>>
> The next lines seem to be useless code because __connman_timeserver_sync()
> will do the same if given service as a parameter. So instead of calling
> __connman_timeserver_sync(**NULL) we should call
> __connman_timeserver_sync(**service) and then we don't need to fetch
> service_gw because that is done in sync() func anyway.


good catch! will fix this.

>
>
>  +       network = __connman_service_get_network(**service);
>> +
>> +       i = connman_network_get_index(**network);
>> +
>> +       /* Cache service Gateway */
>> +       service_gw = __connman_ipconfig_get_**gateway_from_index(i);
>> +
>>        return __connman_timeserver_sync(**NULL);
>>  }
>>
>>
>
> Cheers,
> Jukka
>

Cheers,
Alok.
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman

Reply via email to