Lockclock internalization patch

[Richard Laager]

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?

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.

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

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.

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.

----

+ * 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.

----

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.

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.

-- 
Richard