On 7 Sep 2013, at 19:34, Dag-Erling Smørgrav <d...@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
signature.asc
Description: Message signed with OpenPGP using GPGMail
