code review 5625045: crypto/...: more fixes for bug 2841

crypto/...: more fixes for bug 2841

1) Remove the Reset() member in crypto/aes and crypto/des (and
   document the change).
2) Turn several empty error structures into vars. Any remaining error
   structures are either non-empty, or will probably become so in the
   future.
3) Implement SetWriteDeadline for TLS sockets. At the moment, the TLS
   status cannot be reused after a Write error, which is probably fine
   for most uses.
4) Make crypto/aes and crypto/des return a cipher.Block.

[agl1, 2012-02-03 21:35:04]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2012-02-03 21:48:00]

http://codereview.appspot.com/5625045/diff/5001/src/pkg/crypto/aes/cipher.go
File src/pkg/crypto/aes/cipher.go (right):

http://codereview.appspot.com/5625045/diff/5001/src/pkg/crypto/aes/cipher.go#...
src/pkg/crypto/aes/cipher.go:12: // A Cipher is an instance of AES encryption
using a particular key.
I'd like to get to the point where the ciphers
don't have their own exported types.  So this would be

import "crypto/cipher"

type _cipher struct ...

func NewCipher(key []byte) (cipher.Block, error) {

And then when you run godoc crypto/aes, you just see
that it returns a cipher.Block.  You don't need to know the
method details, since they're the same as the other ciphers.

(Compare to godoc crypto/md5.)

[agl1, 2012-02-03 21:56:29]

http://codereview.appspot.com/5625045/diff/5001/src/pkg/crypto/aes/cipher.go
File src/pkg/crypto/aes/cipher.go (right):

http://codereview.appspot.com/5625045/diff/5001/src/pkg/crypto/aes/cipher.go#...
src/pkg/crypto/aes/cipher.go:12: // A Cipher is an instance of AES encryption
using a particular key.
On 2012/02/03 21:48:00, rsc wrote:
> I'd like to get to the point where the ciphers
> don't have their own exported types.  So this would be
> 
> import "crypto/cipher"
> 
> type _cipher struct ...
> 
> func NewCipher(key []byte) (cipher.Block, error) {
> 
> And then when you run godoc crypto/aes, you just see
> that it returns a cipher.Block.  You don't need to know the
> method details, since they're the same as the other ciphers.

Sorry, I hadn't updated the bug with why I didn't do this.

crypto/cipher imports crypto/aes for testing, but if crypto/aes returns a
cipher.Block, then it needs to import crypto/cipher.

I couldn't think of a very good way around that. Maybe move the tests into
crypto/cipher/test, which can import both? In any case, I punted on it for this
change.

[r, 2012-02-03 22:03:09]

http://codereview.appspot.com/5625045/diff/5001/doc/go1.tmpl
File doc/go1.tmpl (right):

http://codereview.appspot.com/5625045/diff/5001/doc/go1.tmpl#newcode773
doc/go1.tmpl:773: </p>
needs an "updating" sentence, in parallel with the other sections

[rsc, 2012-02-03 22:13:03]

On Fri, Feb 3, 2012 at 16:56,  <agl@golang.org> wrote:
> I couldn't think of a very good way around that. Maybe move the tests
> into crypto/cipher/test, which can import both? In any case, I punted on
> it for this change.

Aha.  We have a mechanism for this, and it's pretty simple.
If you say package cipher_test instead of package cipher,
it builds as a separate package; of course it cannot get
at internals then, but it resolves the cycle.

See CL 5629047, which you are welcome to patch into
yours.

Russ

[agl1, 2012-02-06 16:28:36]

PTAL

http://codereview.appspot.com/5625045/diff/5001/doc/go1.tmpl
File doc/go1.tmpl (right):

http://codereview.appspot.com/5625045/diff/5001/doc/go1.tmpl#newcode773
doc/go1.tmpl:773: </p>
On 2012/02/03 22:03:10, r wrote:
> needs an "updating" sentence, in parallel with the other sections

Done.

[rsc, 2012-02-06 16:34:54]

We should probably write a gofix for the type change, since it is easy.
It can be a later CL, or feel free to file an issue if you'd like someone
else to do it.  Thanks.

http://codereview.appspot.com/5625045/diff/10001/doc/go1.html
File doc/go1.html (right):

http://codereview.appspot.com/5625045/diff/10001/doc/go1.html#newcode873
doc/go1.html:873: Remove the calls to Reset().
s/()//

http://codereview.appspot.com/5625045/diff/10001/doc/go1.tmpl
File doc/go1.tmpl (right):

http://codereview.appspot.com/5625045/diff/10001/doc/go1.tmpl#newcode774
doc/go1.tmpl:774: 

<p>
The cipher-specific types <code>*aes.Cipher</code>, <code>*des.Cipher</code>,
and <code>*des.TripleDESCipher</code> have been removed in favor of
<code>cipher.Block</code>.
</p>

http://codereview.appspot.com/5625045/diff/10001/doc/go1.tmpl#newcode777
doc/go1.tmpl:777: Remove the calls to Reset().
Remove the calls to Reset.  Replace uses of the specific cipher types with
cipher.Block.

http://codereview.appspot.com/5625045/diff/10001/src/pkg/crypto/aes/cipher.go
File src/pkg/crypto/aes/cipher.go (right):

http://codereview.appspot.com/5625045/diff/10001/src/pkg/crypto/aes/cipher.go...
src/pkg/crypto/aes/cipher.go:46: // BlockSize returns the AES block size, 16
bytes.
Please delete the doc comments; no one will see them,
so it's a little confusing to have things directed at users,
like the mention of CBC.

http://codereview.appspot.com/5625045/diff/10001/src/pkg/crypto/des/cipher.go
File src/pkg/crypto/des/cipher.go (right):

http://codereview.appspot.com/5625045/diff/10001/src/pkg/crypto/des/cipher.go...
src/pkg/crypto/des/cipher.go:37: // BlockSize returns the DES block size, 8
bytes.
Same (please delete the doc comments).

http://codereview.appspot.com/5625045/diff/10001/src/pkg/crypto/des/cipher.go...
src/pkg/crypto/des/cipher.go:49: // A TripleDESCipher is an instance of
TripleDES encryption.
s/T/t/

http://codereview.appspot.com/5625045/diff/10001/src/pkg/crypto/des/cipher.go...
src/pkg/crypto/des/cipher.go:54: // NewCipher creates and returns a new
cipher.Block.
// NewTripleDESCipher

[agl1, 2012-02-07 17:58:35]

I've filed http://code.google.com/p/go/issues/detail?id=2905 for the gofix since
this CL is pretty large already.

http://codereview.appspot.com/5625045/diff/10001/doc/go1.html
File doc/go1.html (right):

http://codereview.appspot.com/5625045/diff/10001/doc/go1.html#newcode873
doc/go1.html:873: Remove the calls to Reset().
On 2012/02/06 16:34:54, rsc wrote:
> s/()//

Done.

http://codereview.appspot.com/5625045/diff/10001/doc/go1.tmpl
File doc/go1.tmpl (right):

http://codereview.appspot.com/5625045/diff/10001/doc/go1.tmpl#newcode774
doc/go1.tmpl:774: 
On 2012/02/06 16:34:54, rsc wrote:
> 
> <p>
> The cipher-specific types <code>*aes.Cipher</code>, <code>*des.Cipher</code>,
> and <code>*des.TripleDESCipher</code> have been removed in favor of
> <code>cipher.Block</code>.
> </p>

Done.

http://codereview.appspot.com/5625045/diff/10001/doc/go1.tmpl#newcode777
doc/go1.tmpl:777: Remove the calls to Reset().
On 2012/02/06 16:34:54, rsc wrote:
> Remove the calls to Reset.  Replace uses of the specific cipher types with
> cipher.Block.

Done.

http://codereview.appspot.com/5625045/diff/10001/src/pkg/crypto/aes/cipher.go
File src/pkg/crypto/aes/cipher.go (right):

http://codereview.appspot.com/5625045/diff/10001/src/pkg/crypto/aes/cipher.go...
src/pkg/crypto/aes/cipher.go:46: // BlockSize returns the AES block size, 16
bytes.
On 2012/02/06 16:34:54, rsc wrote:
> Please delete the doc comments; no one will see them,
> so it's a little confusing to have things directed at users,
> like the mention of CBC.

Done.

http://codereview.appspot.com/5625045/diff/10001/src/pkg/crypto/des/cipher.go
File src/pkg/crypto/des/cipher.go (right):

http://codereview.appspot.com/5625045/diff/10001/src/pkg/crypto/des/cipher.go...
src/pkg/crypto/des/cipher.go:37: // BlockSize returns the DES block size, 8
bytes.
On 2012/02/06 16:34:54, rsc wrote:
> Same (please delete the doc comments).

Done.

http://codereview.appspot.com/5625045/diff/10001/src/pkg/crypto/des/cipher.go...
src/pkg/crypto/des/cipher.go:49: // A TripleDESCipher is an instance of
TripleDES encryption.
On 2012/02/06 16:34:54, rsc wrote:
> s/T/t/

Done.

http://codereview.appspot.com/5625045/diff/10001/src/pkg/crypto/des/cipher.go...
src/pkg/crypto/des/cipher.go:54: // NewCipher creates and returns a new
cipher.Block.
On 2012/02/06 16:34:54, rsc wrote:
> // NewTripleDESCipher

Done.

[agl1, 2012-02-11 17:01:19]

poke.

[rsc, 2012-02-13 04:02:28]

LGTM

Very sorry about dropping this.  I thought I'd already replied.
I wrote the gofix over the weekend.

[agl1, 2012-02-13 17:38:54]

*** Submitted as http://code.google.com/p/go/source/detail?r=9d7addec2635 ***

crypto/...: more fixes for bug 2841

1) Remove the Reset() member in crypto/aes and crypto/des (and
   document the change).
2) Turn several empty error structures into vars. Any remaining error
   structures are either non-empty, or will probably become so in the
   future.
3) Implement SetWriteDeadline for TLS sockets. At the moment, the TLS
   status cannot be reused after a Write error, which is probably fine
   for most uses.
4) Make crypto/aes and crypto/des return a cipher.Block.

R=rsc, r
CC=golang-dev
http://codereview.appspot.com/5625045