code review 5846059: syscall: Test SCM_CREDENTIALS, SO_PASSCRED on Linux.

syscall: Test SCM_CREDENTIALS, SO_PASSCRED on Linux.

[albert.strasheim, 2012-03-18 07:56:30]

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

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

[bradfitz, 2012-03-18 15:41:05]

You already have a +build linux line, so I'd drop _linux from the filename.
 Or is this really Linux-only?

On Sun, Mar 18, 2012 at 12:56 AM, <fullung@gmail.com> wrote:

> Reviewers: bradfitz, iant,
>
> Message:
> Hello bradfitz@golang.org, iant@golang.org (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> syscall: Test SCM_CREDENTIALS, SO_PASSCRED on Linux.
>
> Please review this at
http://codereview.appspot.com/**5846059/<http://codereview.appspot.com/5846059/>
>
> Affected files:
>  A src/pkg/syscall/creds_linux_**test.go
>
>
>

[albert.strasheim, 2012-03-18 15:58:44]

Hello

On Sun, Mar 18, 2012 at 5:41 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> You already have a +build linux line, so I'd drop _linux from the filename.
>  Or is this really Linux-only?

I was thinking creds_linux_test.go would be for Linux and
creds_test.go would be for the BSDs, which have SCM_CREDS.

I haven't looked to see whether SCM_CREDENTIALS and SCM_CREDS can be
unified in syscall. If so, creds_linux_test.go could just become
creds_test.go.

So for now, the +build is probably redundant. Agree?

Regards

Albert

[bradfitz, 2012-03-18 16:02:23]

LGTM after nits below.

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
File src/pkg/syscall/creds_linux_test.go (right):

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:5: // +build linux
You can keep or drop this.  I don't really mind either way.

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:16: func TestScmCredentials(t *testing.T) {
minor style nit: acronyms in Go should be all the same case.  So
TestSCMCredentials.

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:66: t.Fatal("n should be zero")
Fatalf("WriteMsgUnix n = %d, want 0", n)

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:69: t.Fatalf("oobn should be len(oob)=%d,
not %d", len(oob), oobn)
"WriteMsgUnix oobn = %d, want %d"

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:72: oob2 := make([]byte, len(oob))
I might make this bigger than needed, so ReadMsgUnix has the ability to fail in
other ways (writing too much).  You can slice it back down to size with oobn2
later.

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:75: t.Fatalf("srv ReadMsgUnix failed: %#v",
err)
%v is fine

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:78: t.Fatalf("Unexpected flags=0x%x\n",
flags)
better failure message, like examples above.

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:81: t.Fatal("n should be 1 (the dummy
byte)")
better failure message, like examples above.

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:86: t.Fatalf("Should have received %d oob
bytes, got %d\n", oobn, oobn2)
better failure message, like examples above.

[albert.strasheim, 2012-03-18 16:20:53]

PTAL

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
File src/pkg/syscall/creds_linux_test.go (right):

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:5: // +build linux
On 2012/03/18 16:02:23, bradfitz wrote:
> You can keep or drop this.  I don't really mind either way.

Done.

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:16: func TestScmCredentials(t *testing.T) {
On 2012/03/18 16:02:23, bradfitz wrote:
> minor style nit: acronyms in Go should be all the same case.  So
> TestSCMCredentials.

Done.

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:66: t.Fatal("n should be zero")
On 2012/03/18 16:02:23, bradfitz wrote:
> Fatalf("WriteMsgUnix n = %d, want 0", n)

Done.

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:69: t.Fatalf("oobn should be len(oob)=%d,
not %d", len(oob), oobn)
On 2012/03/18 16:02:23, bradfitz wrote:
> "WriteMsgUnix oobn = %d, want %d"

Done.

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:72: oob2 := make([]byte, len(oob))
On 2012/03/18 16:02:23, bradfitz wrote:
> I might make this bigger than needed, so ReadMsgUnix has the ability to fail
in
> other ways (writing too much).  You can slice it back down to size with oobn2
> later.

Done.

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:78: t.Fatalf("Unexpected flags=0x%x\n",
flags)
On 2012/03/18 16:02:23, bradfitz wrote:
> better failure message, like examples above.

Done.

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:81: t.Fatal("n should be 1 (the dummy
byte)")
On 2012/03/18 16:02:23, bradfitz wrote:
> better failure message, like examples above.

Done.

http://codereview.appspot.com/5846059/diff/6001/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:86: t.Fatalf("Should have received %d oob
bytes, got %d\n", oobn, oobn2)
On 2012/03/18 16:02:23, bradfitz wrote:
> better failure message, like examples above.

Done.

[bradfitz, 2012-03-18 16:27:58]

LGTM

http://codereview.appspot.com/5846059/diff/6002/src/pkg/syscall/creds_linux_t...
File src/pkg/syscall/creds_linux_test.go (right):

http://codereview.appspot.com/5846059/diff/6002/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:15: func TestSCMCredentials(t *testing.T) {
This is weird enough that I would add a little comment above this, explaining
what the test does.

http://codereview.appspot.com/5846059/diff/6002/src/pkg/syscall/creds_linux_t...
src/pkg/syscall/creds_linux_test.go:101: t.Fatalf("ucreds not equal: %+v !=
%+v", ucred, newUcred)
make this of the form:

"ParseUnixCredentials = %+v, want %+v", newUncred, ucred

[albert.strasheim, 2012-03-18 16:35:07]

PTAL

Thanks for the review.

On 2012/03/18 16:27:58, bradfitz wrote:
> LGTM
> 
>
http://codereview.appspot.com/5846059/diff/6002/src/pkg/syscall/creds_linux_t...
> File src/pkg/syscall/creds_linux_test.go (right):
> 
>
http://codereview.appspot.com/5846059/diff/6002/src/pkg/syscall/creds_linux_t...
> src/pkg/syscall/creds_linux_test.go:15: func TestSCMCredentials(t *testing.T)
{
> This is weird enough that I would add a little comment above this, explaining
> what the test does.
 
Done.

>
http://codereview.appspot.com/5846059/diff/6002/src/pkg/syscall/creds_linux_t...
> src/pkg/syscall/creds_linux_test.go:101: t.Fatalf("ucreds not equal: %+v !=
> %+v", ucred, newUcred)
> make this of the form:
> 
> "ParseUnixCredentials = %+v, want %+v", newUncred, ucred

Done.

[bradfitz, 2012-03-18 17:12:45]

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

syscall: Test SCM_CREDENTIALS, SO_PASSCRED on Linux.

R=bradfitz, iant
CC=golang-dev
http://codereview.appspot.com/5846059

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[r2, 2012-03-18 21:05:38]

Before this there were no tests in syscall. It's the raw kernel interface and
not really Go's business to test. You've just broken a symmetry right before Go
1 is released. I will revert this change; you can send it again after Go 1 when
we have time to discuss it.

-rob

[bradfitz, 2012-03-18 21:13:47]

On Sun, Mar 18, 2012 at 2:05 PM, Rob 'Commander' Pike <r@google.com> wrote:

> Before this there were no tests in syscall. It's the raw kernel interface
> and not really Go's business to test.


You don't test that your syscall wrappers behave as you expect?


> You've just broken a symmetry right before Go 1 is released.
>

Symmetry with what?

[r2, 2012-03-18 21:19:25]

On 19/03/2012, at 8:13 AM, Brad Fitzpatrick wrote:

> On Sun, Mar 18, 2012 at 2:05 PM, Rob 'Commander' Pike <r@google.com> wrote:
> Before this there were no tests in syscall. It's the raw kernel interface and
not really Go's business to test.
> 
> You don't test that your syscall wrappers behave as you expect?

The whole tree does that, but of course for some obscure stuff there's reason to
test. My objection is not that syscall should not have tests, it's that we don't
know how it should test and now is not the time to work that out.
 
> 
> You've just broken a symmetry right before Go 1 is released.
>  
> Symmetry with what? 

Obscure physics term, sorry.  Think "changed a property of the package".

Leaving all that aside, the code base is supposed to be frozen now except for
documentation, critical bug fixes, and the tail of the windows story. Adding a
complex, hundred-line system-specific test right now, without sufficient time to
soak it, is unnecessary. The fact that the test in a package that doesn't really
have tests makes this even less supportable.

After Go 1.

-rob