code review 9687046: nacl: regularize documentation of box and secretbox.

nacl: regularize documentation of box and secretbox.

[sfrithjof, 2013-05-31 22:10:37]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go.crypto

[gobot, 2013-06-03 02:38:37]

R=agl (assigned by minux)

[agl1, 2013-06-03 14:09:08]

NACK.

I'm afraid that I think this is a net loss. I've no idea what the "new part of
out" means.

[sfrithjof, 2013-06-06 09:05:57]

It probably would be a net loss. But the return values should be documented
somehow. Maybe there is a better wording.

Also I think it should be documented, that the capacity of the out argument has
to be big enough to hold message (and poly1305.TagSize more bytes).
Otherwise append might reallocate changing only the local copy of the reference
to out. E.g. http://play.golang.org/p/YGbsHyBjND

[sfrithjof, 2013-06-06 09:20:05]

Another point is that the test cases don't use the argument out by always using
the pattern
    box = Seal(box[:0], message, &nonce, &key)

Would you agree with such changes?

[agl, 2013-06-06 14:47:23]

On Thu, Jun 6, 2013 at 5:06 AM,  <sfrithjof@gmail.com> wrote:
> Also I think it should be documented, that the capacity of the out
> argument has to be big enough to hold message (and poly1305.TagSize more
> bytes).
> Otherwise append might reallocate changing only the local copy of the
> reference to out. E.g. http://play.golang.org/p/YGbsHyBjND

The functions are designed to work like append() - they take a
(possibly nil) buffer and return the updated value. That value may
alias the argument, if there was sufficient capacity, or may be a
fresh buffer.

Perhaps the arguments are confusingly named?


Cheers

AGL

[sfrithjof, 2013-06-06 19:03:01]

The arguments are fine; I just managed to confuse myself.
Still, if I understand correctly this program
http://play.golang.org/p/SRq2AqA4Dz
should print 'FirstPart SecondPart', but it prints 'SecondPart'.

Best, Frithjof


On Thu, Jun 6, 2013 at 4:47 PM, Adam Langley <agl@chromium.org> wrote:

> On Thu, Jun 6, 2013 at 5:06 AM,  <sfrithjof@gmail.com> wrote:
> > Also I think it should be documented, that the capacity of the out
> > argument has to be big enough to hold message (and poly1305.TagSize more
> > bytes).
> > Otherwise append might reallocate changing only the local copy of the
> > reference to out. E.g. http://play.golang.org/p/YGbsHyBjND
>
> The functions are designed to work like append() - they take a
> (possibly nil) buffer and return the updated value. That value may
> alias the argument, if there was sufficient capacity, or may be a
> fresh buffer.
>
> Perhaps the arguments are confusingly named?
>
>
> Cheers
>
> AGL
>

[agl, 2013-06-06 20:32:00]

On Thu, Jun 6, 2013 at 3:03 PM, Frithjof Schulze <sfrithjof@gmail.com> wrote:
> The arguments are fine; I just managed to confuse myself.
> Still, if I understand correctly this program
> http://play.golang.org/p/SRq2AqA4Dz
> should print 'FirstPart SecondPart', but it prints 'SecondPart'.

I've just looked over the secretbox code again and it's clearly nonsense.
Sorry!

Will be fixed with https://codereview.appspot.com/10099043


Cheers

AGL