On Sat, Oct 22, 2011 at 04:35:42PM +0000, Christos Zoulas wrote: > > 1) + memset(r, 0, sizeof(r)); needs (*r)
Indeed. > 2) The code around the above memset has whitespace issues. There are a lot of KNF issues in general. I'll fix these in a final pass before I check anything in, and send another patch so folks can confirm that they've been dealt with. I tried not to make non-whitespace KNF changes to the externally sourced code (rngtest.c, which is Greg Rose's "fips140.c", and the ctr_drbg code, which is hardly modified at all from the original source distribution) to make integrating any later changes easier. But perhaps there won't be any. Should I re-indent all that code and otherwise KNF it? > 3) Why do we have loops that have both a count sentinel and the list pointer > sentinel? Shouldn't/couldn't those always be synced? See below. > 4) What's the hardcoded 16 in the name compares? I'll fix. > 5) sizeof(type) in memcpy() should be sizeof(*dst Same bug as #1, but with worse consequences. Oops. > 6) Isn't it possible to use the list foreach macros instead of open-coding? Regarding this, and #3, I made a deliberate effort to _not_ convert all the rnd.c code from open coded loops over the queue.h datastructures to _FOREACH at this juncture. A few of the instances I checked exited the loop in ways or had intentional side-effects that would have meant more extensive code changes to use _FOREACH and I did not want to risk new and exciting bugs from a wholesale rototill. I am thinking it would be best to stabilize my changes, do the other work in rnd.c I had slated for my "second step", then address this kind of stylistic issue. Is that OK? > 7) instead of printfs shouldn't we use aprint? I am not sure. The documentation says aprint is for autoconfig only. Is it really intended to be used later in the kernel run or by code outside sys/dev or sys/*/*/dev? Thanks for looking at the patch! I know it's rather unwieldy. Thor
