svn commit: r255362 - in head: share/examples/kld share/examples/kld/random_adaptor sys/conf sys/dev/glxsb sys/dev/hifn sys/dev/random sys/dev/rndtest sys/dev/safe sys/dev/ubsec sys/kern sys/mips/c...

[Dag-Erling Smørgrav]

Mark Murray <markm at FreeBSD.org> writes:
> Log:
>   Bring in some behind-the-scenes development, mainly By Arthur Mesh,
>   the rest by me.
>   
>   o Namespace cleanup; the Yarrow name is now restricted to where it
>     really applies; this is in anticipation of being augmented or
>     replaced by Fortuna in the future. Fortuna is mentioned, but behind
>     #if logic, and is ignorable for now.
>   
>   o The harvest queue is pulled out into its own modules.
>   
>   o Entropy harvesting is emproved, both by being made more conservative,
>     and by separating (a bit!) the sources. Available entropy crumbs are
>     marginally improved.
>   
>   o Selection of sources is made clearer. With recent revelations,
>     this will receive more work in the weeks and months to come.
>   
>   Submitted by:	 Arthur Mesh (partly) <arthurmesh at gmail.com>

The patch is very large, but almost exclusively structural - code that
will be shared between Yarrow and Fortuna and any other entropy mixer we
may implement in the future has been renamed and / or moved away from
Yarrow.

I didn't see anything that deviated from the plan we agreed upon in
Cambridge.

Several random_harvest() calls have been changed to reduce the entropy
estimate - that's a good thing (as long as we don't reduce it to an
unusable level, which I don't think is the case).  I also see that the
network entropy harvesting bug we talked about has been fixed, which is
also a good thing.  As far as I can tell, these are the only changes
which affect the quality of the output.

The renaming part made the patch hard to read - IWBNI it had been
committed separately, but it didn't kill me.  Another factor that
reduces readability is that the patch needlessly unfolds
previously wrapped lines, e.g.

-			if (++random_state.outputblocks >=
-				random_state.gengateinterval) {
+			if (++random_state.outputblocks >= random_state.gengateinterval) {

which doesn't actually change anything but introduces a style bug and
increases the reviewers' workload.  In fact, the patch introduces quite
a few style bugs (including some that I personally aprove of but bde@
may complain about, such as s/u_char/uint8_t/), but that can be
addressed when we get a fresh batch of round tuits.

(joking aside - barring an overriding reason, we should strive to always
conform to style(9))

In Yarrow, buffer sizes are now consistently referred to by BLOCKSIZE
rather than a mix of BLOCKSIZE and (int)sizeof(whatever), which improves
code readability at the cost of patch readability.  However, it appears
that the *meaning* of BLOCKSIZE has changed from bits to bytes, and if I
read the code correctly, it used to be 256 bits but is now 128.

I dislike the use of "pseudo" in sys/dev/random/pseudo_rng.c since it is
easily confused with the P in PRNG and pseudo_rng.c is actually not a
PRNG but rather a collection of fake or dummy RNGs for testing purposes.
Perhaps s/pseudo/dummy/g would be in order.

So, this is a provisional OK from my part.  *However*, I did not review
the new harvesting queue in detail, and a bug there could, in the worst
case, result in all the harvested entropy being discarded and Yarrow
receiving kilobyte upon kilobyte of zeroes; so I'd like to get a second
opinion.

DES
-- 
Dag-Erling Smørgrav - des at des.no