[PATCH 2/2] NTPD: Allow stratum 1 behavior

Jean-Christophe Dubois jcd at tribudubois.net
Mon Oct 4 06:25:52 UTC 2010


le lundi 4 octobre 2010 Denys Vlasenko a écrit
> On Sunday 03 October 2010 18:24, Jean-Christophe DUBOIS wrote:
> > Allow busybox to act as a stratum 1 server if no peer is
> > specified on the command line.
> > 
> > This allow busybox to act as a time reference in an isolated
> > network where it is not possible to reach a real stratum
> > server through the IP network.
> > 
> > For this feature to work correctly the busybox system needs
> > to be synchronized to a real time source through other means
> > like for example a GPS device.
> 
> Why do you want this to be a config option? It can be just
> a permanently enabled feature, right?

Well I was not sure everybody would:
- agree with the feature (is it right to claim by default you are a stratum 
1?)
- need the feature
- happy with the small code it is adding

So making it optional was allowing people to add the feature only if they 
needed it.

> > --- a/networking/ntpd.c
> > +++ b/networking/ntpd.c
> > @@ -1763,6 +1763,11 @@ recv_and_process_client_pkt(void /*int fd*/)
> > 
> >  	msg.m_stratum = G.stratum;
> >  	msg.m_ppoll = G.poll_exp;
> >  	msg.m_precision_exp = G_precision_exp;
> > 
> > +#ifdef CONFIG_FEATURE_NTPD_STRATUM_1
> > +	if (G.stratum == 1) {
> > +		G.reftime = gettime1900d(); /* sets G.cur_time too */
> > +	}
> > +#endif
> > 
> >       /* this time was obtained between poll() and recv() */
> >       msg.m_rectime = d_to_lfp(G.cur_time);
> >       msg.m_xmttime = d_to_lfp(gettime1900d()); /* this instant */
> 
> Well, calling gettime1900d() twice is wasteful.

True.
 
> Wait...
> 
> First gettime1900d() is not needed! Check the only one existing callsite
> of recv_and_process_client_pkt(): at that point, current time is
> already in G.cur_time.

True, my bad.

> So you can simply do:
> 
> 	if (G.peer_cnt == 0) {
> 		/* we have no peers: "stratum 1 server" mode */
> 		G.reftime = G.cur_time;
> 	}

OK

> 
> >  	ntp_init(argv);
> > 
> > -	/* it is useless to continue if no peer is defined */
> > -	/* if in server mode, we will never sync */
> > -	/* if in client mode, we will never sync */
> > 
> >  	if (G.peer_cnt == 0) {
> > 
> > +#ifdef CONFIG_FEATURE_NTPD_STRATUM_1
> > +		/*
> > +		 * No peer is defined on the command line. So this is
> > +		 * an isolated system and we will pretend being a
> > +		 * stratum 1 assuming our local clock is correctly
> > +		 * synchronized through another mean (like GPS).
> > +		 */
> > +		G.stratum = 1;
> > +#else
> > +		/*
> > +		 * it is useless to continue if no peer is defined
> > +		 * if in server mode, we will never sync
> > +		 * if in client mode, we will never sync
> > +		 */
> > 
> >  		bb_error_msg("no peer defined");
> >  		return -EINVAL;
> > 
> > +#endif
> 
> Why the check is not in ntp_init()? Why return -EINVAL? Usual error code is
> 1, not -something. Can use bb_error_msg_and_die().

OK. I am not used to these BB specific functions yet. Anyway your patch doesn't 
use it as the stratum 1 mode is now default if no peer argument on the command 
line.
 
> Here is the commit which implements this:
> 
> http://git.busybox.net/busybox/commit/?id=d678257c26e0993efc48ac4433d153e1e
> 9dfc954
> 
> Thanks!



More information about the busybox mailing list