Date: Sat, 07 Sep 2013 20:34:34 +0200 From: =?utf-8?Q?Dag-Erling_Sm=C3=B8rgrav?= <des@des.no> To: Mark Murray <markm@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: 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... Message-ID: <86bo44iipx.fsf@nine.des.no> In-Reply-To: <201309071415.r87EFDMv025499@svn.freebsd.org> (Mark Murray's message of "Sat, 7 Sep 2013 14:15:13 %2B0000 (UTC)") References: <201309071415.r87EFDMv025499@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Mark Murray <markm@FreeBSD.org> writes:
> Log:
> Bring in some behind-the-scenes development, mainly By Arthur Mesh,
> the rest by me.
>=20=20=20
> 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.
>=20=20=20
> o The harvest queue is pulled out into its own modules.
>=20=20=20
> o Entropy harvesting is emproved, both by being made more conservative,
> and by separating (a bit!) the sources. Available entropy crumbs are
> marginally improved.
>=20=20=20
> o Selection of sources is made clearer. With recent revelations,
> this will receive more work in the weeks and months to come.
>=20=20=20
> Submitted by: Arthur Mesh (partly) <arthurmesh@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 >=3D
- random_state.gengateinterval) {
+ if (++random_state.outputblocks >=3D 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
--=20
Dag-Erling Sm=C3=B8rgrav - des@des.no
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?86bo44iipx.fsf>