code review 1666045: Add crypto/rand windows implement, please review it.

Add crypto/rand windows implement, please review it.

[vcc, 2010-06-28 06:07:45]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com, vcc),

I'd like you to review this change.

[rsc1, 2010-06-28 21:27:58]

Thanks for working on this.
Could you please complete a CLA as described at
http://golang.org/doc/contribute.html#copyright ?

http://codereview.appspot.com/1666045/diff/2001/3002
File src/pkg/crypto/rand/rand_windows.go (right):

http://codereview.appspot.com/1666045/diff/2001/3002#newcode18
src/pkg/crypto/rand/rand_windows.go:18: 
can drop blank lines here

http://codereview.appspot.com/1666045/diff/2001/3002#newcode32
src/pkg/crypto/rand/rand_windows.go:32: func CryptGenRandom(ph uint32, l uint32,
pb []byte) (ok bool, errno int) {
I don't think these should be public (capitalized).
Also, it's kind of odd to return ok bool and errno int.
Seems like you could just return (errno int) or, better, (err os.Error).

http://codereview.appspot.com/1666045/diff/2001/3002#newcode34
src/pkg/crypto/rand/rand_windows.go:34: ok = bool(r0 != 0)
r0 != 0 is already a bool
and errno is already zero.

ok = r0 != 0
if !ok {
    errno = int(e1)
}
return

http://codereview.appspot.com/1666045/diff/2001/3002#newcode80
src/pkg/crypto/rand/rand_windows.go:80: _, errno := CryptAcquireContext(&ph,
r.name, "", PROV_RSA_FULL, 0)
Does this really pass "/dev/urandom" to the Windows kernel?
Should be checking all these errors.

[r, 2010-06-28 21:31:51]

http://codereview.appspot.com/1666045/diff/2001/3002
File src/pkg/crypto/rand/rand_windows.go (right):

http://codereview.appspot.com/1666045/diff/2001/3002#newcode21
src/pkg/crypto/rand/rand_windows.go:21: PROV_RSA_FULL = 1
are these values worth being public names?

http://codereview.appspot.com/1666045/diff/2001/3002#newcode34
src/pkg/crypto/rand/rand_windows.go:34: ok = bool(r0 != 0)
why not just

if r0 == 0 {
   errno = int(e1)
}

[brainman, 2010-06-28 23:47:31]

Thanks for helping us here.

http://codereview.appspot.com/1666045/diff/2001/3002
File src/pkg/crypto/rand/rand_windows.go (right):

http://codereview.appspot.com/1666045/diff/2001/3002#newcode24
src/pkg/crypto/rand/rand_windows.go:24: var (
I suggest you move all these winapi and consts to syscall/syscall_windows.go.
This way all the code could be generated automatically, like all other apis.
This way no one need to look at this code mangled with uintptr and unsafe. This
way it will be right next to others like it, if we decide to change
syscall.Syscall interface, or venture to port it to different windows os or
something.

http://codereview.appspot.com/1666045/diff/2001/3002#newcode78
src/pkg/crypto/rand/rand_windows.go:78: func (r *devReader) Read(b []byte) (n
int, err os.Error) {
I don't know much about CryptAPI ro generating random matter. How can we check
that we're generating good random data?

http://codereview.appspot.com/1666045/diff/2001/3002#newcode81
src/pkg/crypto/rand/rand_windows.go:81: if uint32(errno) == NTE_BAD_KEYSET {
I'm second to Russ comment - you must always check for errors.

[vcc, 2010-07-29 14:45:44]

*** Abandoned ***