From owner-svn-src-head@FreeBSD.ORG Sat Sep 7 18:34:33 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id B9FE4EA4; Sat, 7 Sep 2013 18:34:33 +0000 (UTC) (envelope-from des@des.no) Received: from smtp.des.no (smtp.des.no [194.63.250.102]) by mx1.freebsd.org (Postfix) with ESMTP id 40D362DFE; Sat, 7 Sep 2013 18:34:32 +0000 (UTC) Received: from nine.des.no (smtp.des.no [194.63.250.102]) by smtp-int.des.no (Postfix) with ESMTP id CD9484BBC; Sat, 7 Sep 2013 18:34:31 +0000 (UTC) Received: by nine.des.no (Postfix, from userid 1001) id DC240292A1; Sat, 7 Sep 2013 20:34:34 +0200 (CEST) From: =?utf-8?Q?Dag-Erling_Sm=C3=B8rgrav?= To: Mark Murray 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... References: <201309071415.r87EFDMv025499@svn.freebsd.org> Date: Sat, 07 Sep 2013 20:34:34 +0200 In-Reply-To: <201309071415.r87EFDMv025499@svn.freebsd.org> (Mark Murray's message of "Sat, 7 Sep 2013 14:15:13 +0000 (UTC)") Message-ID: <86bo44iipx.fsf@nine.des.no> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (berkeley-unix) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 07 Sep 2013 18:34:33 -0000 Mark Murray 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) 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