code review 4126041: crypto/cipher: add resync open to OCFB mode.

crypto/cipher: add resync open to OCFB mode.

OpenPGP changed its OCFB mode for more modern packets (for example, the
MDC symmetrically encrypted packet). This change adds a bool to
determine which mode is used.

[agl1, 2011-01-29 21:17:50]

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

I'd like you to review this change.

[r, 2011-01-29 23:10:20]

http://codereview.appspot.com/4126041/diff/2001/src/pkg/crypto/cipher/ocfb.go
File src/pkg/crypto/cipher/ocfb.go (right):

http://codereview.appspot.com/4126041/diff/2001/src/pkg/crypto/cipher/ocfb.go...
src/pkg/crypto/cipher/ocfb.go:74: // incorrect key is detected then nil is
returned. resync determines if the
Please begin your sentences with a capital letter, at least in doc comments.  In
the comment above, too.  If you're adamant that resync be lower case to avoid
confusion (although I wouldn't be), rephrase.

[agl1, 2011-01-30 12:13:34]

http://codereview.appspot.com/4126041/diff/2001/src/pkg/crypto/cipher/ocfb.go
File src/pkg/crypto/cipher/ocfb.go (right):

http://codereview.appspot.com/4126041/diff/2001/src/pkg/crypto/cipher/ocfb.go...
src/pkg/crypto/cipher/ocfb.go:74: // incorrect key is detected then nil is
returned. resync determines if the
On 2011/01/29 23:10:20, r wrote:
> Please begin your sentences with a capital letter, at least in doc comments. 
In
> the comment above, too.  If you're adamant that resync be lower case to avoid
> confusion (although I wouldn't be), rephrase.

Done.

[bradfitz, 2011-02-02 17:45:00]

https://codereview.appspot.com/4126041/diff/4002/src/pkg/crypto/cipher/ocfb.go
File src/pkg/crypto/cipher/ocfb.go (right):

https://codereview.appspot.com/4126041/diff/4002/src/pkg/crypto/cipher/ocfb.g...
src/pkg/crypto/cipher/ocfb.go:20: func NewOCFBEncrypter(block Block, randData
[]byte, resync bool) (Stream, []byte) {
I'd personally rather see a specific type here rather than a bool for resync.

Reading code, I'd rather see a callsite with ocfb.Resync rather than a "true"
and have to go lookup what that nameless parameter was.  It also leaves open the
possibility for more than 2 modes, as a minor bonus.

*shrug*

[bradfitz, 2011-02-02 17:45:16]

LGTM

[rsc, 2011-02-02 20:33:52]

> Reading code, I'd rather see a callsite with ocfb.Resync rather than a
> "true" and have to go lookup what that nameless parameter was.  It also
> leaves open the possibility for more than 2 modes, as a minor bonus.
>
> *shrug*

Agreed.  I try to avoid bool params for exactly this reason.
But agreed about the shrug too.

Russ

[agl1, 2011-02-03 14:16:53]

On Wed, Feb 2, 2011 at 3:33 PM, Russ Cox <rsc@golang.org> wrote:
> Agreed.  I try to avoid bool params for exactly this reason.
> But agreed about the shrug too.

Shrug's aside, it's a good idea. Have done.



AGL

[agl1, 2011-02-03 14:17:51]

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

crypto/cipher: add resync open to OCFB mode.

OpenPGP changed its OCFB mode for more modern packets (for example, the
MDC symmetrically encrypted packet). This change adds a bool to
determine which mode is used.

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