[Buildroot] [PATCH v4] package/ntp: sntp time sync script

Arnout Vandecappelle arnout at mind.be
Tue Dec 11 13:12:13 UTC 2018


 Hi Matt,

 Sorry to come with some big comments only in v4...

On 02/11/2018 03:52, Matt Weber wrote:
> This patch adds the installation of a startup script if the sntp
> utility is selected as an option. The utility is design to do a
> one time step/slew adjustment of the system time (similar to the
> ntpdate tool http://support.ntp.org/bin/view/Dev/DeprecatingNtpdate).
> One nice benefit over ntpdate is that sntp can run while ntpd is still
> running. However, ntpd may still need to be restarted if the time
> step was large enough.
> 
> The script provides the ability to override the arguments as part of a
> /etc/defaults/sntp file.
> 
> On a local LAN, the initial large step adjustment took less then
> one second to be retrieved and system time updated. If a user already
> has a RTC maintaining the time and the system was powered off for
> a long period of time, the script assumes a slew adjustment when
> +/- 128ms, rather then a time step(jump). This could be further
> tuned by a user with the /etc/defaults/sntp configuration file.
> 
> One NTP pool server is being set as sntp uses all of the servers
> provided when the DNS is resolved as servers to attempt to retrieve
> time from before timing out. It looks like currently that is 4 servers
> per *pool.ntp.org hostname.
> 
> Cc: Oscar Gomez Fuente <oscargomezf at gmail.com>
> Signed-off-by: Matt Weber <matthew.weber at rockwellcollins.com>
> ---
> 
> Major design/observations
> (v2) Originally an attempt was made to use the ntpd in one shot mode
> but it required more complex logic to handle the cases of a timeout
> where the app had to be killed before the actual ntpd was started.
> Similarly bad server addreses or network connection being down had
> to be handled with the timeout case as the app in one shot mode
> wouldn't exit.
> http://lists.ntp.org/pipermail/questions/2011-July/030041.html

 So how does sntp behave when there is no network or the servers can't be reached?

> 

[snip]
> diff --git a/package/ntp/S48sntp b/package/ntp/S48sntp
> new file mode 100755
> index 0000000..cc0948e
> --- /dev/null
> +++ b/package/ntp/S48sntp
> @@ -0,0 +1,42 @@
> +#! /bin/sh
> +
> +NAME=sntp

 Could you make this script follow the convention that was initiated by Carlos
(and now finally committed)? So this should be DAEMON rather than NAME.

> +# sntp uses all the IPs resolved for the hostname (i.e. pool.ntp.org has 4).
> +# It will try each until they either all timeout or time has been set. Thus
> +# default to only providing one NTP pool host.
> +SNTP_SERVERS="pool.ntp.org"

 It would have been nice if this could have instead used /etc/ntp.conf. But
that's pretty hard I guess, so OK.

 Unrelated to this patch: we actually shouldn't do this. Instead, we should ask
for a buildroot.pool.ntp.org domain and use that. Cfr. [1], "Get your vendor zone".

> +# Step if time delta is greater then 128ms, otherwise slew
> +SNTP_ARGS="-Ss -M 128"
> +SNTP_KEY_CACHE="/tmp/kod"
> +
> +# Read config file if it is present.
> +if [ -r /etc/default/$NAME ]
> +then
> +	. /etc/default/$NAME
> +fi

 ... and this should be

# shellcheck source=/dev/null
[ -r "/etc/default/$DAEMON" ] && . "/etc/default/$DAEMON"

and so on.

> +
> +# Create key cache file to prevents warning that file is missing
> +touch $SNTP_KEY_CACHE

 I don't see why you need to do this in the stop path... (in other words, should
be in the start() function).

> +
> +case "$1" in
> +	start)

 This should call a start() function.

> +		echo "Starting $NAME: "
> +		$NAME $SNTP_ARGS -K $SNTP_KEY_CACHE $SNTP_SERVERS

 Use full path to sntp.

> +		[ $? = 0 ] && echo "OK" || echo "FAIL"> +		;;
> +	stop)
> +		echo "Stopping $NAME: OK"

 Move to stop() function, and add a comment that there is nothing to do since no
daemon is started.

> +		;;
> +	restart|reload)
> +		echo "Restarting $NAME: "
> +		$0 stop
> +		sleep 1
> +		$0 start> +		;;
> +	*)
> +		echo "Usage: $0 {start|stop|restart|reload}" >&2
> +		exit 1
> +		;;
> +esac
> +
> +exit 0
> diff --git a/package/ntp/ntp.mk b/package/ntp/ntp.mk
> index af3c1aa..aad1cdd 100644
> --- a/package/ntp/ntp.mk
> +++ b/package/ntp/ntp.mk
> @@ -93,9 +93,18 @@ define NTP_INSTALL_TARGET_CMDS
>  	$(INSTALL) -m 644 package/ntp/ntpd.etc.conf $(TARGET_DIR)/etc/ntp.conf
>  endef
>  
> +# This script will step the time if there is a large difference
> +# before ntpd takes over the necessary slew adjustments

 I think this should be put in the help text of BR2_PACKAGE_NTP_SNTP.


 Regards,
 Arnout

[1] https://www.ntppool.org/nl/vendors.html


> +ifeq ($(BR2_PACKAGE_NTP_SNTP),y)
> +define NTP_INSTALL_INIT_SYSV_SNTP
> +	$(INSTALL) -D -m 755 package/ntp/S48sntp $(TARGET_DIR)/etc/init.d/S48sntp
> +endef
> +endif
> +
>  ifeq ($(BR2_PACKAGE_NTP_NTPD),y)
>  define NTP_INSTALL_INIT_SYSV
>  	$(INSTALL) -D -m 755 package/ntp/S49ntp $(TARGET_DIR)/etc/init.d/S49ntp
> +	$(NTP_INSTALL_INIT_SYSV_SNTP)
>  endef
>  
>  define NTP_INSTALL_INIT_SYSTEMD
> 


More information about the buildroot mailing list