Send connman mailing list submissions to
connman@lists.01.org
To subscribe or unsubscribe via email, send a message with subject or
body 'help' to
connman-requ...@lists.01.org
You can reach the person managing the list at
connman-ow...@lists.01.org
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. Re: [PATCH] clock: Add TimeSynced signal emitted when the system time has
been synced
(Daniel Wagner)
2. RE: [PATCH] clock: Add TimeSynced signal emitted when the system time has
been synced
(VAUTRIN Emmanuel (Canal Plus Prestataire))
----------------------------------------------------------------------
Date: Wed, 27 Jan 2021 09:35:14 +0100
From: Daniel Wagner <w...@monom.org>
Subject: Re: [PATCH] clock: Add TimeSynced signal emitted when the
system time has been synced
To: "VAUTRIN Emmanuel (Canal Plus Prestataire)"
<emmanuel.vaut...@cpexterne.org>
Cc: "connman@lists.01.org" <connman@lists.01.org>
Message-ID: <20210127083514.tylzmrkyzzmn4...@beryllium.lan>
Content-Type: text/plain; charset=us-ascii
Hi Emmanuel,
On Thu, Jan 21, 2021 at 04:35:38PM +0000, VAUTRIN Emmanuel (Canal Plus
Prestataire) wrote:
> Hello,
>
> Finally, after running several campaigns, I faced (rarely) no time update
> after updating the timeservers lists.
> It seems to work better with this second version.
>
> From ced5a1988f5b0e7a44a278b0e270577e6b427f88 Mon Sep 17 00:00:00 2001
> From: Emmanuel VAUTRIN <emmanuel.vaut...@cpexterne.org>
> Date: Thu, 7 Jan 2021 17:27:39 +0100
> Subject: [PATCH v2] clock: Implement Clock API time synchronization
>
> Implement Clock API TimeserverSynced property, which indicates if the
> current system time is synced via NTP servers.
> Trigger a time synchronization when the TimeUpdate property is set to
> auto.
> Fix time synchronization when starting with an empty Timeservers list.
This really needs to be a separate patch.
> Emit a Time PropertyChanged signal on NTP synchronization success.
> ---
> doc/clock-api.txt | 5 +++++
> src/clock.c | 15 +++++++++++++++
> src/connman.h | 3 +++
> src/timeserver.c | 43 +++++++++++++++++++++++++++++++++++++++++--
> 4 files changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/doc/clock-api.txt b/doc/clock-api.txt
> index 6818f5a8..48d29970 100644
> --- a/doc/clock-api.txt
> +++ b/doc/clock-api.txt
> @@ -85,3 +85,8 @@ Properties uint64 Time [readonly or readwrite]
> [experimental]
>
> This list of servers is used when TimeUpdates is set
> to auto.
> +
> + boolean TimeserverSynced [readonly] [experimental]
> +
> + This value indicates if the current system time
> + is synced via NTP servers.
Add something like
'Only when the Time property is set to auto and the system time is in
sync with an NTP server this property is set to true'
> diff --git a/src/clock.c b/src/clock.c
The D-Bus bits look good.
> diff --git a/src/connman.h b/src/connman.h
> index e51d94d0..50d52bbe 100644
> --- a/src/connman.h
> +++ b/src/connman.h
> @@ -452,6 +452,9 @@ GSList *__connman_timeserver_get_all(struct
> connman_service *service);
> void __connman_timeserver_sync(struct connman_service *service);
> void __connman_timeserver_conf_update(struct connman_service *service);
>
> +bool __connman_timeserver_is_synced(void);
> +void __connman_timeserver_set_synced(bool status);
> +
> enum __connman_dhcpv6_status {
> CONNMAN_DHCPV6_STATUS_FAIL = 0,
> CONNMAN_DHCPV6_STATUS_SUCCEED = 1,
> diff --git a/src/timeserver.c b/src/timeserver.c
> index 5f0aca68..2675113b 100644
> --- a/src/timeserver.c
> +++ b/src/timeserver.c
> @@ -29,6 +29,7 @@
> #include <stdlib.h>
> #include <gweb/gresolv.h>
> #include <netdb.h>
> +#include <sys/time.h>
>
> #include "connman.h"
>
> @@ -40,6 +41,7 @@ static GSList *ts_list = NULL;
> static char *ts_current = NULL;
> static int ts_recheck_id = 0;
> static int ts_backoff_id = 0;
> +static bool ts_is_synced = false;
>
> static GResolv *resolv = NULL;
> static int resolv_id = 0;
> @@ -55,8 +57,24 @@ static void ntp_callback(bool success, void *user_data)
> {
> DBG("success %d", success);
>
> + __connman_timeserver_set_synced(success);
> if (!success)
> sync_next();
Use early exit to avoid big indents:
if (!success) {
sync_next();
return;
}
if (!gettimeofday(...)) {
connman_warn(...);
return;
}
....
> + else {
> + struct timeval tv;
> +
> + if (gettimeofday(&tv, NULL) == 0) {
> + dbus_uint64_t timestamp;
> +
> + timestamp = tv.tv_sec;
> + connman_dbus_property_changed_basic(
> + CONNMAN_MANAGER_PATH,
> + CONNMAN_CLOCK_INTERFACE, "Time",
> + DBUS_TYPE_UINT64, ×tamp);
> + } else {
> + connman_warn("Failed to get current time");
> + }
> + }
> }
>
> static void save_timeservers(char **servers)
> @@ -337,6 +355,8 @@ static void ts_reset(struct connman_service *service)
> if (!resolv)
> return;
>
> + __connman_timeserver_set_synced(false);
> +
> /*
> * Before we start creating the new timeserver list we must stop
> * any ongoing ntp query and server resolution.
> @@ -366,6 +386,8 @@ static void ts_reset(struct connman_service *service)
>
> __connman_service_timeserver_changed(service, timeservers_list);
>
> + ts_service = service;
> +
> if (!timeservers_list) {
> DBG("No timeservers set.");
> return;
> @@ -373,7 +395,6 @@ static void ts_reset(struct connman_service *service)
>
> ts_recheck_enable();
>
> - ts_service = service;
This looks very suspicious. You need to explain why you need to change
this as separate patch. I know why I put the ts_service assignment after
the timeserver_list check. ts_service points to the currently used
service to probe for NTP. If the timeserver_list is empty, there can't
be any service we probe.
> timeserver_sync_start();
> }
>
> @@ -394,6 +415,24 @@ void __connman_timeserver_conf_update(struct
> connman_service *service)
> }
>
>
> +bool __connman_timeserver_is_synced(void)
> +{
> + return ts_is_synced;
> +}
> +
> +void __connman_timeserver_set_synced(bool status)
> +{
> + if (ts_is_synced != status) {
> + dbus_bool_t is_synced;
> +
> + ts_is_synced = status;
> + is_synced = status;
> + connman_dbus_property_changed_basic(CONNMAN_MANAGER_PATH,
> + CONNMAN_CLOCK_INTERFACE, "TimeserverSynced",
> + DBUS_TYPE_BOOLEAN, &is_synced);
> + }
Again use early exit
if (ts_is_synced == status)
return
...
> +}
> +
> static int timeserver_start(struct connman_service *service)
> {
> char **nameservers;
> @@ -467,7 +506,7 @@ int __connman_timeserver_system_set(char **servers)
> save_timeservers(servers);
>
> service = connman_service_get_default();
> - __connman_timeserver_conf_update(service);
> + ts_reset(service);
>
> return 0;
Apart my style nitpicks (early exit) and the ts_service thing the patch
looks good.
Thanks,
Daniel
------------------------------
Date: Wed, 27 Jan 2021 10:19:10 +0000
From: "VAUTRIN Emmanuel (Canal Plus Prestataire)"
<emmanuel.vaut...@cpexterne.org>
Subject: RE: [PATCH] clock: Add TimeSynced signal emitted when the
system time has been synced
To: Daniel Wagner <w...@monom.org>
Cc: "connman@lists.01.org" <connman@lists.01.org>
Message-ID: <pr1pr02mb4794f44abada1b3e7dba74db93...@pr1pr02mb4794.eur
prd02.prod.outlook.com>
Content-Type: text/plain; charset="iso-8859-1"
Thank you Daniel,
So, to sum up, I shall separate 2 patches
* Time PropertyChanged signal implementation.
* the ts_service source code move
And follow your code recommendations on the main patch and deliver the 3
patches, is it correct?
B.R.,
Emmanuel
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list -- connman@lists.01.org
To unsubscribe send an email to connman-le...@lists.01.org
------------------------------
End of connman Digest, Vol 63, Issue 11
***************************************
