Requesting review of "Eliminate some pointless gymnastics in the config parser."

[Eric S. Raymond]

Hal Murray <hmurray at megapathdsl.net>:
> 
> esr at thyrsus.com said:
> > About eight hours ago I removed some code that looked so stupid that I now
> > wonder if it was serving some purpose I don't understand. 
> 
> I don't know of any reason for the old code.
> 
> Your change looks sane to me.  I don't see how to fully test it.  The iburst 
> case seems to still work.

Thanks for the review.  With iburst and prefer known good I think it's a safe
bet that the handling of the other flags is also correct.

FYI, I'm cleaning up the config parser

(a) as usual, to make the code smaller and tighter.

(b) to eliminate some ordering constraints on attribute settings.

(c) as preparation for implementing the new rule language.

(d) because by bundling per-node variables set by the parser into a
struct peerctl I can make adding new options (such as, say, "speed")
much less painful. There are a couple of places where that lets me
replace error-prone parallel copies of a bunch of state variables with
a struct copy, though I won't be able to fully exploit that until
Daniel's protocol-machine branch is merged.

Since it might be of interest, I will note that eventually I will
replace the peer node members corresponding to the struct peerctl
members with a struct peerctl. This will express in declaration the
fact that these are control variables not modified after the peer
block is initialized.  It would be better if C had a
"single-assignment" modifier analogous to const, but alas...

There is even a minor space optimization available by declaring
portions of the struct as a union so that control variables only used
on refclocks can share storage with those used in network peers.
But more important than the optimization is the ability to eliminate
smelly constructions like using a member named "ttl" (originally
named for the network case) to hold subtype information for refclocks.

That is, we want to change this:

/*
 * Read-only control knobs for a peer structure.
 * Packaging these makes context copies a bit more succinct.
 */
struct peer_ctl {
	int		flags;
	uint8_t		minpoll;
	uint8_t		maxpoll;
	uint32_t	ttl;
	keyid_t		peerkey;
};

to something more like this:

struct peer_ctl {
	bool		iburst:1;
	bool		burst:1;
	bool		prefer:1;
	bool		preempt:1;
	bool		xleave:1;
	uint8_t		minpoll;
	uint8_t		maxpoll;
	union {
		struct {
			uint8_t		version;
			uint32_t	ttl;
			keyid_t		peerkey;
		} peer;
		struct {
			uint32_t	subtype;
		} clock
	}
};

and then (a) change all references to "ttl" in refclock-land to
"clock.subtype" and (b) eliminate error-brone magic flag masks in
favor of bitfield references.  Enough little gains in expressiveness
like this can add up to a large improvement in readability.
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>