code review 7300047: syscall, net: Fix unix socket autobind on Linux.

syscall, net: Fix unix socket autobind on Linux.

[albert.strasheim, 2013-02-05 18:45:32]

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

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

[iant, 2013-02-05 18:55:35]

https://codereview.appspot.com/7300047/diff/3003/src/pkg/net/linux_test.go
File src/pkg/net/linux_test.go (right):

https://codereview.appspot.com/7300047/diff/3003/src/pkg/net/linux_test.go#ne...
src/pkg/net/linux_test.go:1: // Copyright 2013 The Go Authors.  All rights
reserved.
I don't think "linux_test.go" is a good name for this test.  How about
autobind_test.go?

https://codereview.appspot.com/7300047/diff/3003/src/pkg/net/linux_test.go#ne...
src/pkg/net/linux_test.go:17: t.Errorf("ListenUnixgram failed: %v", err)
s/Errorf/Fatalf/

https://codereview.appspot.com/7300047/diff/3003/src/pkg/net/linux_test.go#ne...
src/pkg/net/linux_test.go:21: if len(s) != 6 {
I'm not sure I see the point of testing the name like this.  The code will work
fine if the name has a different length or a different format.  I think the only
important point is that the length > 1 and it starts with '@'.

[albert.strasheim, 2013-02-05 19:29:50]

PTAL

On 2013/02/05 18:55:35, iant wrote:
> https://codereview.appspot.com/7300047/diff/3003/src/pkg/net/linux_test.go
> File src/pkg/net/linux_test.go (right):
> 
>
https://codereview.appspot.com/7300047/diff/3003/src/pkg/net/linux_test.go#ne...
> src/pkg/net/linux_test.go:1: // Copyright 2013 The Go Authors.  All rights
> reserved.
> I don't think "linux_test.go" is a good name for this test.  How about
> autobind_test.go?

Done.
 
>
https://codereview.appspot.com/7300047/diff/3003/src/pkg/net/linux_test.go#ne...
> src/pkg/net/linux_test.go:17: t.Errorf("ListenUnixgram failed: %v", err)
> s/Errorf/Fatalf/

Done.

>
https://codereview.appspot.com/7300047/diff/3003/src/pkg/net/linux_test.go#ne...
> src/pkg/net/linux_test.go:21: if len(s) != 6 {
> I'm not sure I see the point of testing the name like this.  The code will
work
> fine if the name has a different length or a different format.  I think the
only
> important point is that the length > 1 and it starts with '@'.

The manual page explicitly says:

"The address consists of a null byte followed by 5 bytes in the character set
[0-9a-f]."

so I figured I'd test for that. I can drop it if you prefer. If you decide we
should keep it, should these be Fatalf or Errorf?

Cheers

Albert

[iant, 2013-02-05 19:49:00]

On 2013/02/05 19:29:50, albert.strasheim wrote:
>
https://codereview.appspot.com/7300047/diff/3003/src/pkg/net/linux_test.go#ne...
> > src/pkg/net/linux_test.go:21: if len(s) != 6 {
> > I'm not sure I see the point of testing the name like this.  The code will
> work
> > fine if the name has a different length or a different format.  I think the
> only
> > important point is that the length > 1 and it starts with '@'.
> 
> The manual page explicitly says:
> 
> "The address consists of a null byte followed by 5 bytes in the character set
> [0-9a-f]."
> 
> so I figured I'd test for that. I can drop it if you prefer. If you decide we
> should keep it, should these be Fatalf or Errorf?

We're not writing a kernel test here.  We're writing a Go test.  My opinion is
that we test what matters for Go programs.  And it seems to me that what matters
for Go programs is that they can autobind a unix domain socket, and get back a
string that they can use to then connect to that socket.  The details of the
string really don't matter for a typical Go program.  So testing the details
doesn't seem useful.

What does seem useful, on the other hand, would be trying to connect to that
socket using the name we get back.  Want to extend the test to do that?

As far as Errorf vs. Fatalf, the only question is whether the test can continue
to run.  If creating the socket fails, the rest of the test will simply crash,
so better to use Fatalf.

[mikio, 2013-02-05 20:28:23]

On Wed, Feb 6, 2013 at 4:49 AM,  <iant@golang.org> wrote:

> What does seem useful, on the other hand, would be trying to connect to
> that socket using the name we get back.  Want to extend the test to do
> that?

I agree with Ian.
Also we already have unix_test.go, please use it.

[albert.strasheim, 2013-02-05 23:17:39]

Howdy

On Tue, Feb 5, 2013 at 10:28 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> On Wed, Feb 6, 2013 at 4:49 AM,  <iant@golang.org> wrote:
>> What does seem useful, on the other hand, would be trying to connect to
>> that socket using the name we get back.  Want to extend the test to do
>> that?
> I agree with Ian.

Will do.

> Also we already have unix_test.go, please use it.

unix_test.go is currently:

// +build !plan9,!windows

I poked around, as far as I can tell this autobind business might be
Linux-only. Do you have other info?

Cheers

Albert

[albert.strasheim, 2013-02-05 23:43:58]

PTAL

On 2013/02/05 19:49:00, iant wrote:
> On 2013/02/05 19:29:50, albert.strasheim wrote:
> >
>
https://codereview.appspot.com/7300047/diff/3003/src/pkg/net/linux_test.go#ne...
> > > src/pkg/net/linux_test.go:21: if len(s) != 6 {
> > > I'm not sure I see the point of testing the name like this.  The code will
> > work
> > > fine if the name has a different length or a different format.  I think
the
> > only
> > > important point is that the length > 1 and it starts with '@'.
> > The manual page explicitly says:
> > "The address consists of a null byte followed by 5 bytes in the character
set
> > [0-9a-f]."
> > so I figured I'd test for that. I can drop it if you prefer. If you decide
we
> > should keep it, should these be Fatalf or Errorf?
> We're not writing a kernel test here.  We're writing a Go test.  My opinion is
> that we test what matters for Go programs.  And it seems to me that what
matters
> for Go programs is that they can autobind a unix domain socket, and get back a
> string that they can use to then connect to that socket.  The details of the
> string really don't matter for a typical Go program.  So testing the details
> doesn't seem useful.

Cool. Checking now for that length is at least 2 bytes.

> What does seem useful, on the other hand, would be trying to connect to that
> socket using the name we get back.  Want to extend the test to do that?

Added a connect test.
 
> As far as Errorf vs. Fatalf, the only question is whether the test can
continue
> to run.  If creating the socket fails, the rest of the test will simply crash,
> so better to use Fatalf.

Fatalf all the way.

The test is still in autobind_test.go because unix_test.go is for all the Unixen
and I'm pretty sure this stuff is Linux only.

Regards

Albert

[mikio, 2013-02-05 23:49:53]

Hi Albert,

> I poked around, as far as I can tell this autobind business might be
> Linux-only. Do you have other info?

switch runtime.GOOS {
case "linux"*
default:
   t.Skipf(*...")
}

[albert.strasheim, 2013-02-06 07:27:00]

PTAL

On 2013/02/05 23:49:53, mikio wrote:
> switch runtime.GOOS {
> case "linux"*
> default:
>    t.Skipf(*...")
> }

did it with an if

[mikio, 2013-02-06 11:17:33]

looks good, a few nits

https://codereview.appspot.com/7300047/diff/5004/src/pkg/net/unix_test.go
File src/pkg/net/unix_test.go (right):

https://codereview.appspot.com/7300047/diff/5004/src/pkg/net/unix_test.go#new...
src/pkg/net/unix_test.go:132: l, err := ListenUnixgram("unixgram", laddr)
I'd prefer c instead of l, because it returns a Conn variable.
Also ln might be better than l for Listener variables. (1 or l ?)

https://codereview.appspot.com/7300047/diff/5004/src/pkg/net/unix_test.go#new...
src/pkg/net/unix_test.go:150: t.Fatalf("Dial failed: %v", err)
s/Dial/DialUnix/

https://codereview.appspot.com/7300047/diff/5004/src/pkg/net/unix_test.go#new...
src/pkg/net/unix_test.go:155: if *autoAddr != *remoteAddr {
reflect.DeepEqual?

[albert.strasheim, 2013-02-06 14:18:07]

PTAL

On 2013/02/06 11:17:33, mikio wrote:
> looks good, a few nits
> 
> https://codereview.appspot.com/7300047/diff/5004/src/pkg/net/unix_test.go
> File src/pkg/net/unix_test.go (right):
> 
>
https://codereview.appspot.com/7300047/diff/5004/src/pkg/net/unix_test.go#new...
> src/pkg/net/unix_test.go:132: l, err := ListenUnixgram("unixgram", laddr)
> I'd prefer c instead of l, because it returns a Conn variable.
> Also ln might be better than l for Listener variables. (1 or l ?)

made it c1 and c2.

>
https://codereview.appspot.com/7300047/diff/5004/src/pkg/net/unix_test.go#new...
> src/pkg/net/unix_test.go:150: t.Fatalf("Dial failed: %v", err)
> s/Dial/DialUnix/

fixed.
 
>
https://codereview.appspot.com/7300047/diff/5004/src/pkg/net/unix_test.go#new...
> src/pkg/net/unix_test.go:155: if *autoAddr != *remoteAddr {
> reflect.DeepEqual?

fixed.

[mikio, 2013-02-06 14:44:25]

LGTM

[iant, 2013-02-06 14:45:47]

LGTM

[iant, 2013-02-06 14:46:05]

*** Submitted as https://code.google.com/p/go/source/detail?r=59fb68816436 ***

syscall, net: Fix unix socket autobind on Linux.

R=rsc, iant, mikioh.mikioh
CC=golang-dev
https://codereview.appspot.com/7300047

Committer: Ian Lance Taylor <iant@golang.org>