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

[Mark R V Murray]

On 7 Sep 2013, at 19:34, Dag-Erling Smørgrav <des at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 353 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20130907/9f2417c3/attachment.sig>