Lockclock internalization patch

[Eric S. Raymond]

This is good feedback.  Exactly the sotrt of thing that should come up
in review.

Richard Laager via devel <devel at ntpsec.org>:
> On 1/12/19 8:10 AM, Eric S. Raymond via devel wrote:
> > Please review and test.
> 
> Can you please post this as a merge request, so we can use GitLab
> comment/review/approval processes and automated testing?

I'll do that on the next iteration of this oatch.  I was going to do it for
this one in response to Matt's request, but (a) I think you've identified the
typos he was seeing, and (b) I didn't have a repo for set up for making
MRs yet - I generally push directly.

> If not...
> 
> +* NIST lockclock mode is now a tinker flag rather than a compile-time
> +  option, heading off bad things that can occur if a distro packager
> +  gets careless and enables every compile-time option by default.
> 
> ^ "gets careless and" seems unnecessary. Even if it's true, it's not
> adding anything to the sentence except a scolding.

OK, fixed in my master.

> Typo:
> +/* following variables are non-locjclock case only */

Likewise.

> Bigger picture questions/concerns:
> 
> You have removed the ENABLE_LOCKCLOCK define. This means every build of
> NTPsec has lockclock support. I think that's intentional, but I want to
> highlight that. Refclocks are compile-time conditional, so maybe
> lockclock should be too.

It is intentional. For two reasons:

(1) The bug fix.  We want to close out the possibility that a distro will
compltely break ntpd with a bad compile-time config.

(2) I'm keeping a weather eye forward on the possibility of porting to Go.
(And more willing to talk about it in public now that we're going to have NTS
in C as a strong incentive for potential adopters to not wait on the Go port).

This creates an incentive to start eliminating compile-time options
and not create any new ones.  Go doesn't support that sort of thing
easily - matter of fact if we want to keep the capability to configure
subsets of clock drivers we're going to have to write a custom code
generator to do it.

This is an extra reason to get rid of ENABLE_LOCKCLOCK, and to *not* put
more code within REFCLOCK.  Eventually REFCLOCK is going to have to go away.
The additional code volume is so small by modern standards that I'm
completely unworried anout the additional binary size even on systems
as small as RasPis.

> Perhaps "lockclock" should be a ./waf --refclock= choice. If and only if
> refclock=lockclock and local (since lockclock requires local) are
> selected (including by way of "all"), then #define ENABLE_LOCKCLOCK.
> 
> Alternatively, and this is probably better, set ENABLE_LOCKCLOCK (or
> change it all to ENABLE_LOCAL or something) if and only if
> refclock=local (including by way of "all") is set.

I'd rather not screw around with compile-time options at all at this
point.  In the long and even medium term that will have been wasted effort.

On the other hand, your point about tinker being a kludgy way to set the
runtime option is sound. I'll address that below.

> + * When lockclock is on: (1) If the local clock is the prefer peer, it
> + * will always be enabled, even if declared falseticker,
> 
> Is there a reason that the local clock has to be the prefer peer to
> enable the lockclock behavior? If formerly ENABLE_LOCKCLOCK now "tinker
> lockclock" is set, that triggers at least some of the lockclock
> behaviors, right? That's the entire reason that --enable-lockclock is a
> problem and thus this patch exists.
> 
> So it seems that this condition could be, and should be, eliminated. If
> you set "tinker lockclock", you get the lockclock behaviors, without
> needing to set "prefer" on the "local" refclock.

I don't fully understand the relationship between the localclock
driver and lockclock, so I can't answer your question ("Is there a
reason that the local clock has to be the prefer peer to enable the
lockclock behavior?"). It's the kind of thing Hal Murray might know.

What you are looking at is the simplest, most transparent elimination
of ENABLE_LOCKCLOCK I could come up with.  It fixes the immediate bug.

I'm quite willing for the interface to the rest of the code to be
refined (that is, away from a being a tinker flag) but don't think
that should block application of the fix patch. After all, actual use
of lockclock mode is entirely theoretical at this point.

In fact I think any re-jiggering of how the lockclock global is set
*should* be a separate patch in order to make the modification history
clearer.

> Is "tinker lockclock" the right place to put this? I realize it's easy
> to add one there. But perhaps this should be an option on peers, which
> is only available on "local" (i.e. a fatal error or ignored with a
> warning on others). It probably implies prefer.
>
> This would also address the following issue: if you set "tinker
> lockclock" but don't have a local clock peer, presumably bad things
> happen (like the original bad things causing this patch to have been
> created). Should that be disallowed? If lockclock is moved to be a peer
> option, that scenario is impossible, so you don't need extra code to
> disallow it.

Yes, it did occur to me that it might be better for lockclock to be
set by a localclock driver mode flag.  I even checked that any of the three
binary flags are available for this use in that driver.

I didn't try to implement it this way for one superficial reason and
one deep one.  The superficial reason is that doing so would have
complicated the fix patch

The deep one is that lockclock - whether in its old version as a
compile-time option or its new version as a boolean global - has
global scope.  It messes with *all* clock adjustment, from whatever
source, disabling frequency adjustments - not just adustments coming
in from the localclock driver.

Is it right to set something like that from a driver initalization
routine? I'd need to think about that for a bit.

> As a possible better alternative to all this... What is the difference
> between enabling lockclock behaviors and marking a "local" refclock as
> "true" (or and "true prefer")? If there is no significant/useful
> difference, an even easier option would be to make "true" or "true
> prefer" on a "local" refclock enable the lockclock behavior. If
> practical, this would avoid all the code for a "lockclock" option
> (whether in "tinker" or on the peer). And lockclock behavior may just be
> the obvious implication of setting "true and prefer" on a "local" clock
> anyway, so this may be a more intuitive way to get it.

Hmmm...this is an attractive alternative, but I'd want to be more sure that
this didn't conflict in anyt way with any existing usage of prefer status.
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

My work is funded by the Internet Civil Engineering Institute: https://icei.org
Please visit their site and donate: the civilization you save might be your own.