Date: Sat, 7 Sep 2013 19:52:22 +0100 From: Mark R V Murray <mark@grondar.org> To: =?iso-8859-1?Q?Dag-Erling_Sm=F8rgrav?= <des@des.no> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mark Murray <markm@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: <C0253B44-6D0B-4234-8448-DA08468CC663@grondar.org> In-Reply-To: <86bo44iipx.fsf@nine.des.no> References: <201309071415.r87EFDMv025499@svn.freebsd.org> <86bo44iipx.fsf@nine.des.no>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
On 7 Sep 2013, at 19:34, Dag-Erling Smørgrav <des@des.no> wrote:
> I didn't see anything that deviated from the plan we agreed upon in
> Cambridge.
That was the idea! :-D
> 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.
Indeed.
> 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 tuts.
Apologies for making your life hard; that was unintended. I'll be working on
this code some more; I'll fix those long lines then.
> (joking aside - barring an overriding reason, we should strive to always
> conform to style(9))
Indeed; while this may not be a perfect lurch in that direction, the uintN_t
changes were a style(9) nod.
> 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.
Correct; it was a mess and I tidied it up. Having 256-bit blocks bought
us nothing; the intent is to use the natural key and block sizes of the
AES and SHA256 building-blocks.
> 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.
Good point. I'll do that.
> 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.
Of course!
M
--
Mark R V Murray
[-- Attachment #2 --]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.20 (Darwin)
Comment: GPGTools - http://gpgtools.org
iQCVAwUBUit15t58vKOKE6LNAQpZDgP9HYhprygJEJAW1vLDcW3HXaa/xBgtwdwv
nqUK10AAm9ak9DnrxLWFlNzG91Wh+npq71aNHMo2nCyTv+Q8ytVWttniDPYSNTw5
orpi7ShdVd2+RzKiK/JRyYpQ3FWBIiMuLsIyL66/SjIyMwK7z53SlCLsEVxPds4R
z8wPryWtmA4=
=HoEg
-----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?C0253B44-6D0B-4234-8448-DA08468CC663>