code review 36030043: runtime, net: add support for GOOS=solaris

runtime, net: add support for GOOS=solaris

[aram, 2013-12-10 20:40:45]

Hello golang-dev@googlegroups.com,

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

[minux1, 2013-12-10 20:45:26]

link to the bitbucket repository is wrong.
(s/sunos/solaris/g error?)

[aram, 2013-12-10 20:54:44]

Done (though there's no info there).

Once I replace netpoll I plan to take a thorough look at net failures,
if they still exist.

-- 
Aram Hăvărneanu

[mikio, 2013-12-10 21:22:41]

Please don't include net/http pkg files in this CL.
Why not include net files of CL 36040043?

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/port_unix.go
File src/pkg/net/port_unix.go (right):

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/port_unix.go#...
src/pkg/net/port_unix.go:64: // TODO(dfc) SOLARIS! Y U NO KNOW http ?
i don't understand

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/port_unix.go#...
src/pkg/net/port_unix.go:65: if service == "http" {
no thanks
you can do the same thing as protocol database does

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (right):

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/sock_posix.go...
src/pkg/net/sock_posix.go:156: lsa, err := syscall.Getsockname(fd.sysfd)
no thanks
see https://codereview.appspot.com/26770045/

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/sock_solaris.go
File src/pkg/net/sock_solaris.go (right):

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/sock_solaris....
src/pkg/net/sock_solaris.go:8: // TODO(dfc)
i can't read out anything btw lines

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/tcp_test.go
File src/pkg/net/tcp_test.go (right):

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/tcp_test.go#n...
src/pkg/net/tcp_test.go:444: t.Skip("skipping on SUNOS")
SUNOS? Solaris?

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/tcpsockopt_so...
File src/pkg/net/tcpsockopt_solaris.go (right):

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/tcpsockopt_so...
src/pkg/net/tcpsockopt_solaris.go:5: // TCP socket options for openbsd
please drop the unnecessary (and wrong) comment line

[aram, 2013-12-10 23:40:39]

> Please don't include net/http pkg files in this CL.

Okay, but it's only one net/http files, where do you want to put
it, separate CL?

> Why not include net files of CL 36040043?

I created CL 36040043 to keep trivial cruft from real changes; to
make it easier to review. I can put net files here here if you want,
but then it's inconsistent with the other CLs.

Hopefully Dave can address the other comments.

[aram, 2013-12-10 23:41:27]

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (right):

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/sock_posix.go...
src/pkg/net/sock_posix.go:156: lsa, err := syscall.Getsockname(fd.sysfd)
On 2013/12/10 21:22:41, mikio wrote:
> no thanks
> see https://codereview.appspot.com/26770045/

But the fd is not usable... That's how I found this. I'll investigate.

[dfc, 2013-12-10 23:51:48]

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/port_unix.go
File src/pkg/net/port_unix.go (right):

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/port_unix.go#...
src/pkg/net/port_unix.go:65: if service == "http" {
On 2013/12/10 21:22:41, mikio wrote:
> no thanks
> you can do the same thing as protocol database does

Hi Mikio,

This was my comment, it probably wasn't phrased very well.

The problem is that /etc/services on some (all?) solaris machines does not have
an entry for http.

[mikio, 2013-12-11 00:01:58]

On 2013/12/10 23:40:39, aram wrote:

> Okay, but it's only one net/http files, where do you want to put
> it, separate CL?

anywhere appropriate.

> I created CL 36040043 to keep trivial cruft from real changes; to
> make it easier to review. I can put net files here here if you want,
> but then it's inconsistent with the other CLs.

as you know, the runtime, syscall, os, and net packages are kinda intersections.
almost all other packages across over those four and it's easy to make time
bomb, submarine bugs like "net: feature X doesn't work on platform Y with Go Z".
so i'd rather see an unified CL. thanks.

[mikio, 2013-12-11 00:05:56]

On 2013/12/10 23:51:48, dfc wrote:

> The problem is that /etc/services on some (all?) solaris machines does not
have
> an entry for http.

you can add such entry to package local database.

[mikio, 2013-12-11 00:44:35]

On Wed, Dec 11, 2013 at 8:41 AM,  <aram@mgk.ro> wrote:

> But the fd is not usable... That's how I found this. I'll investigate.

Yup, please make sure what's "not usable" on solaris.
Is that related to the state of underlying socket? IO gate stuff?
(syscalls?) kernel features? blah blah

[aram, 2013-12-11 20:57:51]

On Wed, Dec 11, 2013 at 12:51 AM,  <dave@cheney.net> wrote:
> The problem is that /etc/services on some (all?) solaris machines does
> not have an entry for http.

Some do. I think most do.

-- 
Aram Hăvărneanu

[aram, 2013-12-11 20:57:54]

On Wed, Dec 11, 2013 at 1:05 AM,  <mikioh.mikioh@gmail.com> wrote:
> you can add such entry to package local database.

I am missing something here. What is a package-local database?

Thanks,

-- 
Aram Hăvărneanu

[dfc, 2013-12-11 20:59:09]

What about just skipping this test on Solaris ? That sounds cleaner that the
hack I proposed. 

> On 12 Dec 2013, at 7:57, Aram Hăvărneanu <aram@mgk.ro> wrote:
> 
>> On Wed, Dec 11, 2013 at 12:51 AM,  <dave@cheney.net> wrote:
>> The problem is that /etc/services on some (all?) solaris machines does
>> not have an entry for http.
> 
> Some do. I think most do.
> 
> -- 
> Aram Hăvărneanu

[aram, 2013-12-11 21:03:23]

On Wed, Dec 11, 2013 at 9:59 PM, Dave Cheney <dave@cheney.net> wrote:
> What about just skipping this test on Solaris ? That sounds cleaner that the
hack I proposed.

Yes, I'll definitely disable the hack, I just want to understand the
proposed fix, however.

-- 
Aram Hăvărneanu

[aram, 2013-12-11 21:27:40]

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/tcp_test.go
File src/pkg/net/tcp_test.go (right):

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/tcp_test.go#n...
src/pkg/net/tcp_test.go:444: t.Skip("skipping on SUNOS")
On 2013/12/10 21:22:41, mikio wrote:
> SUNOS? Solaris?

Done.

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/tcpsockopt_so...
File src/pkg/net/tcpsockopt_solaris.go (right):

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/tcpsockopt_so...
src/pkg/net/tcpsockopt_solaris.go:5: // TCP socket options for openbsd
On 2013/12/10 21:22:41, mikio wrote:
> please drop the unnecessary (and wrong) comment line

I corrected it. Haven't dropped it to keep it consistent with the other
tcpsockopt_$GOOS.go files.

[mikio, 2013-12-12 00:43:54]

On 2013/12/11 20:57:54, aram wrote:

> I am missing something here. What is a package-local database?

such map variables services and protocols. in go 1.2 we added some pairs of
protocol name and number as preloaded stuff to protocols.

[dfc, 2013-12-12 01:24:17]

Hi Mikio,

Aram and I are both confused about what you are referring to. Could
you please point us to the place in the code.

Cheers

Dave

On Thu, Dec 12, 2013 at 11:43 AM,  <mikioh.mikioh@gmail.com> wrote:
> On 2013/12/11 20:57:54, aram wrote:
>
>> I am missing something here. What is a package-local database?
>
>
> such map variables services and protocols. in go 1.2 we added some pairs
> of protocol name and number as preloaded stuff to protocols.
>
> https://codereview.appspot.com/36030043/

[mikio, 2013-12-12 02:11:14]

On Thu, Dec 12, 2013 at 10:24 AM, Dave Cheney <dave@cheney.net> wrote:

> Aram and I are both confused about what you are referring to. Could
> you please point us to the place in the code.

http://golang.org/src/pkg/net/lookup.go#L14
http://golang.org/src/pkg/net/lookup_unix.go#L18

You can do the same thing.
http://golang.org/src/pkg/net/port_unix.go#L13
http://golang.org/src/pkg/net/port_unix.go#L17

[aram, 2013-12-12 11:03:25]

Thank you.

-- 
Aram Hăvărneanu

[aram, 2013-12-12 15:08:46]

PTAL

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/port_unix.go
File src/pkg/net/port_unix.go (right):

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/port_unix.go#...
src/pkg/net/port_unix.go:64: // TODO(dfc) SOLARIS! Y U NO KNOW http ?
On 2013/12/10 21:22:41, mikio wrote:
> i don't understand

Done.

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/port_unix.go#...
src/pkg/net/port_unix.go:65: if service == "http" {
On 2013/12/10 21:22:41, mikio wrote:
> no thanks
> you can do the same thing as protocol database does

Done.

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (right):

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/sock_posix.go...
src/pkg/net/sock_posix.go:156: lsa, err := syscall.Getsockname(fd.sysfd)
On 2013/12/10 21:22:41, mikio wrote:
> no thanks
> see https://codereview.appspot.com/26770045/

Done.

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/sock_posix.go...
src/pkg/net/sock_posix.go:156: lsa, err := syscall.Getsockname(fd.sysfd)
> But the fd is not usable... That's how I found this. I'll investigate.

Problem went away when we converted from raw system calls to using libc.

[mikio, 2013-12-13 00:31:46]

will have a look after toolchain, syscall and runtime land.
just out of curiosity (and not related to net directly but net uses
runtime-integrated poller), why select in runtime-netpoller instaed of
/dev/poll?

https://codereview.appspot.com/36030043/diff/140002/src/pkg/net/sock_solaris.go
File src/pkg/net/sock_solaris.go (right):

https://codereview.appspot.com/36030043/diff/140002/src/pkg/net/sock_solaris....
src/pkg/net/sock_solaris.go:1: // Copyright 2009 The Go Authors.  All rights
reserved.
s/2009/2013/
and a single space after punctuation for new files? probably.

https://codereview.appspot.com/36030043/diff/140002/src/pkg/net/sockopt_solar...
File src/pkg/net/sockopt_solaris.go (right):

https://codereview.appspot.com/36030043/diff/140002/src/pkg/net/sockopt_solar...
src/pkg/net/sockopt_solaris.go:1: // Copyright 2011 The Go Authors.  All rights
reserved.
s/2011/2013/
a single space following punctuation

[aram, 2013-12-13 11:49:20]

On Fri, Dec 13, 2013 at 1:31 AM,  <mikioh.mikioh@gmail.com> wrote:
> why select in runtime-netpoller instaed of
> /dev/poll?

netpoll_select.c is a port of gccgo's netpoll_select.c. In the
meantime we have a better event-ports based network poller. I haven't
tested it enough yet to be confident in it, that's why I haven't
submitted it in these CLs, I wanted to keep churn low. Since we will
have to wait some more on the linker changes, I'll probably test the
event-ports version and eventually update these CLs with it.

> and a single space after punctuation for new files? probably.

Two spaces come from fmt(1). I can't find it right now, but I remember
a thread on golang-dev that said two spaces are fine, because it's
fmt(1) output.

Thanks,

-- 
Aram Hăvărneanu

[mikio, 2013-12-13 23:53:56]

On Fri, Dec 13, 2013 at 8:49 PM, Aram Hăvărneanu <aram@mgk.ro> wrote:

> netpoll_select.c is a port of gccgo's netpoll_select.c. In the
> meantime we have a better event-ports based network poller. I haven't
> tested it enough yet to be confident in it, that's why I haven't
> submitted it in these CLs, I wanted to keep churn low. Since we will
> have to wait some more on the linker changes, I'll probably test the
> event-ports version and eventually update these CLs with it.

In that case you may pay a cost of test twice; for netpoll_select and
netpoll_devpoll, but that's fine. leave to you.

> Two spaces come from fmt(1). I can't find it right now, but I remember
> a thread on golang-dev that said two spaces are fine, because it's
> fmt(1) output.

FWIW, http://golang.org/doc/contribute.html#copyright. also I receive
sometimes a comment like: "... i'm trying to suppress these doubled
spaces but for local consistency it should probably be here".

[gobot, 2013-12-20 16:26:13]

Replacing golang-dev with golang-codereviews.

[aram, 2013-12-21 21:38:32]

https://codereview.appspot.com/36030043/diff/140002/src/pkg/net/sockopt_solar...
File src/pkg/net/sockopt_solaris.go (right):

https://codereview.appspot.com/36030043/diff/140002/src/pkg/net/sockopt_solar...
src/pkg/net/sockopt_solaris.go:1: // Copyright 2011 The Go Authors.  All rights
reserved.
On 2013/12/13 00:31:47, mikio wrote:
> s/2011/2013/
> a single space following punctuation

Done.

[aram, 2013-12-21 21:49:18]

Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com,
mikioh.mikioh@gmail.com, dave@cheney.net (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[aram, 2013-12-21 21:49:43]

> In that case you may pay a cost of test twice; for netpoll_select and
> netpoll_devpoll, but that's fine. leave to you.

Because this is progressing so slow (due to linker changes), I have
updated this CL with the event ports version. I also made it work without
busy-spinning.

[dfc, 2013-12-24 04:10:35]

LGTM with minor comments

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (right):

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/sock_posix.go...
src/pkg/net/sock_posix.go:156: lsa, err := syscall.Getsockname(fd.sysfd)
On 2013/12/10 23:41:27, aram wrote:
> On 2013/12/10 21:22:41, mikio wrote:
> > no thanks
> > see https://codereview.appspot.com/26770045/
> 
> But the fd is not usable... That's how I found this. I'll investigate.

aram: i already tried to merge this change a while back and it was rejected. 

https://codereview.appspot.com/26770045/

Let's drop this part of the CL.

https://codereview.appspot.com/36030043/diff/210001/src/pkg/net/sock_solaris.go
File src/pkg/net/sock_solaris.go (right):

https://codereview.appspot.com/36030043/diff/210001/src/pkg/net/sock_solaris....
src/pkg/net/sock_solaris.go:1: // Copyright 2009 The Go Authors.  All rights
reserved.
Fix date

https://codereview.appspot.com/36030043/diff/210001/src/pkg/net/sock_solaris....
src/pkg/net/sock_solaris.go:10: // TODO: Implement this
// TODO(aramh)

https://codereview.appspot.com/36030043/diff/210001/src/pkg/net/sockopt_solar...
File src/pkg/net/sockopt_solaris.go (right):

https://codereview.appspot.com/36030043/diff/210001/src/pkg/net/sockopt_solar...
src/pkg/net/sockopt_solaris.go:1: // Copyright 2011 The Go Authors.  All rights
reserved.
Fix date

[aram, 2013-12-24 11:17:31]

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (right):

https://codereview.appspot.com/36030043/diff/140001/src/pkg/net/sock_posix.go...
src/pkg/net/sock_posix.go:156: lsa, err := syscall.Getsockname(fd.sysfd)
On 2013/12/24 04:10:36, dfc wrote:
> On 2013/12/10 23:41:27, aram wrote:
> > On 2013/12/10 21:22:41, mikio wrote:
> > > no thanks
> > > see https://codereview.appspot.com/26770045/
> > 
> > But the fd is not usable... That's how I found this. I'll investigate.
> 
> aram: i already tried to merge this change a while back and it was rejected. 
> 
> https://codereview.appspot.com/26770045/
> 
> Let's drop this part of the CL.

This is not present in subsequent patch sets.

[iant, 2013-12-27 21:55:03]

https://codereview.appspot.com/36030043/diff/210001/src/pkg/net/dial_test.go
File src/pkg/net/dial_test.go (right):

https://codereview.appspot.com/36030043/diff/210001/src/pkg/net/dial_test.go#...
src/pkg/net/dial_test.go:118: if runtime.GOOS == "solaris" {
I hope that this bit will no longer be necessary since I committed
https://codereview.appspot.com/39200044 .

[dfc, 2013-12-28 04:57:50]

Thanks Ian, I think this has resolved the issue. We'll merge stuff and
remove this skip

On Sat, Dec 28, 2013 at 8:55 AM,  <iant@golang.org> wrote:
>
> https://codereview.appspot.com/36030043/diff/210001/src/pkg/net/dial_test.go
> File src/pkg/net/dial_test.go (right):
>
>
https://codereview.appspot.com/36030043/diff/210001/src/pkg/net/dial_test.go#...
> src/pkg/net/dial_test.go:118: if runtime.GOOS == "solaris" {
> I hope that this bit will no longer be necessary since I committed
> https://codereview.appspot.com/39200044 .
>
> https://codereview.appspot.com/36030043/

[iant, 2013-12-28 18:02:42]

https://codereview.appspot.com/36030043/diff/210001/src/pkg/net/dial_test.go
File src/pkg/net/dial_test.go (right):

https://codereview.appspot.com/36030043/diff/210001/src/pkg/net/dial_test.go#...
src/pkg/net/dial_test.go:486: if runtime.GOOS == "solaris" {
I think this test should now work on Solaris.

https://codereview.appspot.com/36030043/diff/210001/src/pkg/net/dial_test.go#...
src/pkg/net/dial_test.go:524: t.Skip("skipping test on solaris; see issue 6828")
I think this test should now work on Solaris.

[aram, 2013-12-29 17:42:54]

https://codereview.appspot.com/36030043/diff/210001/src/pkg/net/dial_test.go
File src/pkg/net/dial_test.go (right):

https://codereview.appspot.com/36030043/diff/210001/src/pkg/net/dial_test.go#...
src/pkg/net/dial_test.go:118: if runtime.GOOS == "solaris" {
On 2013/12/27 21:55:04, iant wrote:
> I hope that this bit will no longer be necessary since I committed
> https://codereview.appspot.com/39200044 .

Done.

https://codereview.appspot.com/36030043/diff/210001/src/pkg/net/dial_test.go#...
src/pkg/net/dial_test.go:486: if runtime.GOOS == "solaris" {
On 2013/12/28 18:02:45, iant wrote:
> I think this test should now work on Solaris.

Done.

https://codereview.appspot.com/36030043/diff/210001/src/pkg/net/dial_test.go#...
src/pkg/net/dial_test.go:524: t.Skip("skipping test on solaris; see issue 6828")
On 2013/12/28 18:02:45, iant wrote:
> I think this test should now work on Solaris.

Done.

[aram, 2013-12-29 17:45:37]

Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com,
mikioh.mikioh@gmail.com, dave@cheney.net, iant@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[mikio, 2014-01-08 05:06:21]

https://codereview.appspot.com/36030043/diff/250001/src/pkg/net/tcp_test.go
File src/pkg/net/tcp_test.go (right):

https://codereview.appspot.com/36030043/diff/250001/src/pkg/net/tcp_test.go#n...
src/pkg/net/tcp_test.go:443: if runtime.GOOS == "solaris" {
is there any specific reason to skip here? (just curious)

[aram, 2014-01-09 15:09:10]

https://codereview.appspot.com/36030043/diff/250001/src/pkg/net/tcp_test.go
File src/pkg/net/tcp_test.go (right):

https://codereview.appspot.com/36030043/diff/250001/src/pkg/net/tcp_test.go#n...
src/pkg/net/tcp_test.go:443: if runtime.GOOS == "solaris" {
On 2014/01/08 05:06:22, mikio wrote:
> is there any specific reason to skip here? (just curious)

It didn't work at some point, but now it does. Removed. Thanks.

Later edit: I take that back, still hangs on machine with a large number of
CPUs.

[aram, 2014-01-09 15:11:25]

Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com,
mikioh.mikioh@gmail.com, dave@cheney.net, iant@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[mikio, 2014-01-10 01:33:11]

On 2014/01/09 15:09:10, aram wrote:

> It didn't work at some point, but now it does. Removed. Thanks.

Glad to hear that.

> Later edit: I take that back, still hangs on machine with a large number of
> CPUs.

Poller stuff? Anyway let's land toolchain/runtime/os CLs first. Then Dmitry or
other people who can do smart parking lot maneuvers will give us some hint.

[mikio, 2014-01-17 08:35:44]

can you please apply the patch below; looks like solaris has a pretty strict
route lookup.

diff -r 322f646feecb src/pkg/net/ipraw_test.go
@@ -277,7 +277,7 @@
 		}
 	}
 
-	raddr := &IPAddr{IP: IPv4(127, 0, 0, 10).To4()}
+	raddr := &IPAddr{IP: IPv4(127, 0, 0, 1).To4()}
 	c, err := DialIP("ip:tcp", &IPAddr{IP: IPv4(127, 0, 0, 1)}, raddr)

https://codereview.appspot.com/36030043/diff/330001/src/pkg/net/interface_bsd...
File src/pkg/net/interface_bsd_test.go (right):

https://codereview.appspot.com/36030043/diff/330001/src/pkg/net/interface_bsd...
src/pkg/net/interface_bsd_test.go:5: // +build darwin dragonfly freebsd netbsd
openbsd solaris
please revert this line unless you have a plan to implement routing socket stuff
within the term of go1.3

https://codereview.appspot.com/36030043/diff/330001/src/pkg/net/interface_uni...
File src/pkg/net/interface_unix_test.go (right):

https://codereview.appspot.com/36030043/diff/330001/src/pkg/net/interface_uni...
src/pkg/net/interface_unix_test.go:5: // +build darwin dragonfly freebsd linux
netbsd openbsd solaris
ditto

[aram, 2014-01-17 16:01:10]

> can you please apply the patch below; looks like solaris has a pretty
> strict route lookup.
> 
> diff -r 322f646feecb src/pkg/net/ipraw_test.go
> @@ -277,7 +277,7 @@
>  		}
>  	}
>  
> -	raddr := &IPAddr{IP: IPv4(127, 0, 0, 10).To4()}
> +	raddr := &IPAddr{IP: IPv4(127, 0, 0, 1).To4()}
>  	c, err := DialIP("ip:tcp", &IPAddr{IP: IPv4(127, 0, 0, 1)}, raddr)
> 

Done.

https://codereview.appspot.com/36030043/diff/330001/src/pkg/net/interface_bsd...
File src/pkg/net/interface_bsd_test.go (right):

https://codereview.appspot.com/36030043/diff/330001/src/pkg/net/interface_bsd...
src/pkg/net/interface_bsd_test.go:5: // +build darwin dragonfly freebsd netbsd
openbsd solaris
On 2014/01/17 08:35:45, mikio wrote:
> please revert this line unless you have a plan to implement routing socket
stuff
> within the term of go1.3

Done.

https://codereview.appspot.com/36030043/diff/330001/src/pkg/net/interface_uni...
File src/pkg/net/interface_unix_test.go (right):

https://codereview.appspot.com/36030043/diff/330001/src/pkg/net/interface_uni...
src/pkg/net/interface_unix_test.go:5: // +build darwin dragonfly freebsd linux
netbsd openbsd solaris
On 2014/01/17 08:35:45, mikio wrote:
> ditto

Done.

[aram, 2014-01-17 16:02:07]

Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com,
mikioh.mikioh@gmail.com, dave@cheney.net, iant@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[aram, 2014-02-12 11:23:40]

Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com,
mikioh.mikioh@gmail.com, dave@cheney.net, iant@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[rsc, 2014-02-21 17:34:48]

LGTM

can we get this in soon please?

https://codereview.appspot.com/36030043/diff/540001/src/pkg/net/port_unix.go
File src/pkg/net/port_unix.go (right):

https://codereview.appspot.com/36030043/diff/540001/src/pkg/net/port_unix.go#...
src/pkg/net/port_unix.go:13: var services = map[string]map[string]int{
This seems unlikely to do anything useful.
services is overwritten by readServices.
Please revert.

https://codereview.appspot.com/36030043/diff/540001/src/pkg/runtime/netpoll_p...
File src/pkg/runtime/netpoll_port.c (right):

https://codereview.appspot.com/36030043/diff/540001/src/pkg/runtime/netpoll_p...
src/pkg/runtime/netpoll_port.c:5: // +build solaris
_port is a strange name. it suggests portable which this code is not. 
how about netpoll_solaris.c, and then this line can go away.

[aram, 2014-02-21 17:36:22]

https://codereview.appspot.com/36030043/diff/540001/src/pkg/net/port_unix.go
File src/pkg/net/port_unix.go (right):

https://codereview.appspot.com/36030043/diff/540001/src/pkg/net/port_unix.go#...
src/pkg/net/port_unix.go:13: var services = map[string]map[string]int{
Solaris lacks this in /etc/services.

[rsc, 2014-02-21 17:40:26]

https://codereview.appspot.com/36030043/diff/540001/src/pkg/net/port_unix.go
File src/pkg/net/port_unix.go (right):

https://codereview.appspot.com/36030043/diff/540001/src/pkg/net/port_unix.go#...
src/pkg/net/port_unix.go:13: var services = map[string]map[string]int{
On 2014/02/21 17:36:23, aram wrote:
> Solaris lacks this in /etc/services.

Are your tests passing? Because I see no way that this can have any effect.
Everything that looks at services calls readServices first, and readServices
blows this away.

[aram, 2014-02-22 17:16:56]

> Are your tests passing? Because I see no way that this can have any
> effect.

I see what's going on. The machine I mostly use for this has http in
/etc/services, and as such, test pass. Other machines don't. Dave wrote
a hack to fix the other machines, I re-wrote it like this to match the
code for protocols in lookup.go, but I fixed it wrong and didn't realise
because my /etc/services was functional.

I fixed it in a different way, however. I can't just revert because
on most machines it won't work. I just changed readServices to use the
already existing map.

[aram, 2014-02-22 17:17:24]

https://codereview.appspot.com/36030043/diff/540001/src/pkg/runtime/netpoll_p...
File src/pkg/runtime/netpoll_port.c (right):

https://codereview.appspot.com/36030043/diff/540001/src/pkg/runtime/netpoll_p...
src/pkg/runtime/netpoll_port.c:5: // +build solaris
On 2014/02/21 17:34:49, rsc wrote:
> _port is a strange name. it suggests portable which this code is not. 
> how about netpoll_solaris.c, and then this line can go away.

Done.

[aram, 2014-02-22 17:18:17]

Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com,
mikioh.mikioh@gmail.com, dave@cheney.net, iant@golang.org, rsc@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[minux1, 2014-02-22 22:24:00]

I'm not familiar with the event ports, so i didn't review that part
of the code.

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/interface_stu...
File src/pkg/net/interface_stub.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/interface_stu...
src/pkg/net/interface_stub.go:12: func interfaceTable(ifindex int) ([]Interface,
error) {
do you have further plans to implement these?

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/ipraw_test.go
File src/pkg/net/ipraw_test.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/ipraw_test.go...
src/pkg/net/ipraw_test.go:280: raddr := &IPAddr{IP: IPv4(127, 0, 0, 1).To4()}
why this change?

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/multicast_tes...
File src/pkg/net/multicast_test.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/multicast_tes...
src/pkg/net/multicast_test.go:28: case "plan9", "solaris":
is IPv4 Multicast unsupported in general on solaris, or we just lack a suitable
implementation here?

if it's the later case, open an issue, and link it here.
case "plan9":
   t.Skipf("skipping test on %q", runtime.GOOS)
case "solairs":
   t.Skipf("skipping test on solaris, see http://golang.org/issue/XXXX")

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/port_unix.go
File src/pkg/net/port_unix.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/port_unix.go#...
src/pkg/net/port_unix.go:14: "tcp": {"http": 80},
this affects every unix. could we move the initialization to a solaris specific
file? (and also add some comments why it's necessary)

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/sendfile_stub.go
File src/pkg/net/sendfile_stub.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/sendfile_stub...
src/pkg/net/sendfile_stub.go:5: // +build darwin netbsd openbsd solaris
the same question on the syscall package.
if it's just unimplemented in syscall, add some comment and a note to issue
5847.

(not implementing it won't affect inclusion of both CLs, we just need to keep
track of the issue so that we don't forget it.)

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/sock_solaris.go
File src/pkg/net/sock_solaris.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/sock_solaris....
src/pkg/net/sock_solaris.go:11: // NOTE: Never return a number bigger than 1<<16
- 1. See issue 5030.
should we use `/usr/sbin/ndd /dev/tcp tcp_conn_req_max_q` to query
the real maximum?

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/sockopt_solar...
File src/pkg/net/sockopt_solaris.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/sockopt_solar...
src/pkg/net/sockopt_solaris.go:1: // Copyright 2011 The Go Authors.  All rights
reserved.
s/2011/2014/

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/tcp_test.go
File src/pkg/net/tcp_test.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/tcp_test.go#n...
src/pkg/net/tcp_test.go:444: t.Skip("skipping on Solaris")
why skip on solaris? if it's an unresolved issue, file an issue and link here.

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/tcpsockopt_so...
File src/pkg/net/tcpsockopt_solaris.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/tcpsockopt_so...
src/pkg/net/tcpsockopt_solaris.go:1: // Copyright 2013 The Go Authors. All
rights reserved.
s/2013/2014/

https://codereview.appspot.com/36030043/diff/580001/src/pkg/runtime/netpoll_s...
File src/pkg/runtime/netpoll_solaris.c (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/runtime/netpoll_s...
src/pkg/runtime/netpoll_solaris.c:1: // Copyright 2013 The Go Authors. All
rights reserved.
s/2013/2014/

[mikio, 2014-02-24 00:48:08]

fyi

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/ipraw_test.go
File src/pkg/net/ipraw_test.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/ipraw_test.go...
src/pkg/net/ipraw_test.go:280: raddr := &IPAddr{IP: IPv4(127, 0, 0, 1).To4()}
On 2014/02/22 22:24:00, minux wrote:
> why this change?

why do you think a route to 127/8 is valid on solaris by default?

# go test -short
--- FAIL: TestIPConnRemoteName (0.00 seconds)
        ipraw_test.go:283: DialIP failed: dial ip:tcp 127.0.0.10: cannot assign
requested address

Routing Table: IPv4
  Destination           Gateway           Flags  Ref     Use     Interface 
-------------------- -------------------- ----- ----- ---------- --------- 
default              172.16.220.2         UG        2       2500 e1000g0
127.0.0.1            127.0.0.1            UH        2       7208 lo0
172.16.220.0         172.16.220.14        U         4      16524 e1000g0

[minux1, 2014-02-24 01:50:09]

On Sun, Feb 23, 2014 at 7:48 PM, <mikioh.mikioh@gmail.com> wrote:

> https://codereview.appspot.com/36030043/diff/580001/src/
> pkg/net/ipraw_test.go
> File src/pkg/net/ipraw_test.go (right):
>
> https://codereview.appspot.com/36030043/diff/580001/src/
> pkg/net/ipraw_test.go#newcode280
> src/pkg/net/ipraw_test.go:280: raddr := &IPAddr{IP: IPv4(127, 0, 0,
> 1).To4()}
> On 2014/02/22 22:24:00, minux wrote:
>
>> why this change?
>>
>
> why do you think a route to 127/8 is valid on solaris by default?
>
Perhaps I should ask "why we used 127.0.0.10 before"?

>
> # go test -short
> --- FAIL: TestIPConnRemoteName (0.00 seconds)
>         ipraw_test.go:283: DialIP failed: dial ip:tcp 127.0.0.10: cannot
> assign requested address
>
> Routing Table: IPv4
>   Destination           Gateway           Flags  Ref     Use
> Interface
> -------------------- -------------------- ----- ----- ----------
> ---------
> default              172.16.220.2         UG        2       2500 e1000g0
> 127.0.0.1            127.0.0.1            UH        2       7208 lo0
> 172.16.220.0         172.16.220.14        U         4      16524 e1000g0
>

[mikio, 2014-02-24 01:55:57]

On Mon, Feb 24, 2014 at 10:49 AM, minux <minux.ma@gmail.com> wrote:

> Perhaps I should ask "why we used 127.0.0.10 before"?

Right, because we didn't know what the Solaris IPv4 stack is, it has a
bit strict route lookup, unlike other implementation.

[aram, 2014-02-24 15:44:24]

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/interface_stu...
File src/pkg/net/interface_stub.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/interface_stu...
src/pkg/net/interface_stub.go:12: func interfaceTable(ifindex int) ([]Interface,
error) {
On 2014/02/22 22:24:00, minux wrote:
> do you have further plans to implement these?

Maybe, not in 1.3 though.

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/multicast_tes...
File src/pkg/net/multicast_test.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/multicast_tes...
src/pkg/net/multicast_test.go:28: case "plan9", "solaris":
Solaris has multicast, done.

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/port_unix.go
File src/pkg/net/port_unix.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/port_unix.go#...
src/pkg/net/port_unix.go:14: "tcp": {"http": 80},
On 2014/02/22 22:24:00, minux wrote:
> this affects every unix. could we move the initialization to a solaris
specific
> file? (and also add some comments why it's necessary)

This would make the code more complicated and confusing. I did this at mikio's
suggestion, it's analogous to the stuff we already have in lookup.go.

I added a comment just like in lookup.go.

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/sendfile_stub.go
File src/pkg/net/sendfile_stub.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/sendfile_stub...
src/pkg/net/sendfile_stub.go:5: // +build darwin netbsd openbsd solaris
On 2014/02/22 22:24:00, minux wrote:
> the same question on the syscall package.
> if it's just unimplemented in syscall, add some comment and a note to issue
> 5847.
> 
> (not implementing it won't affect inclusion of both CLs, we just need to keep
> track of the issue so that we don't forget it.)

Done.

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/sock_solaris.go
File src/pkg/net/sock_solaris.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/sock_solaris....
src/pkg/net/sock_solaris.go:11: // NOTE: Never return a number bigger than 1<<16
- 1. See issue 5030.
On 2014/02/22 22:24:00, minux wrote:
> should we use `/usr/sbin/ndd /dev/tcp tcp_conn_req_max_q` to query
> the real maximum?

No, that interface is not portable. There's no proper way to do this
dynamically.

Also, I believe it's always equal to syscall.SOMAXCONN anyway.

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/sockopt_solar...
File src/pkg/net/sockopt_solaris.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/sockopt_solar...
src/pkg/net/sockopt_solaris.go:1: // Copyright 2011 The Go Authors.  All rights
reserved.
On 2014/02/22 22:24:00, minux wrote:
> s/2011/2014/

No, this is just sockopt_bsd.go, I forgot to hg cp it. Fixed now, thanks.

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/tcp_test.go
File src/pkg/net/tcp_test.go (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/net/tcp_test.go#n...
src/pkg/net/tcp_test.go:444: t.Skip("skipping on Solaris")
On 2014/02/22 22:24:00, minux wrote:
> why skip on solaris? if it's an unresolved issue, file an issue and link here.

Done.

https://codereview.appspot.com/36030043/diff/580001/src/pkg/runtime/netpoll_s...
File src/pkg/runtime/netpoll_solaris.c (right):

https://codereview.appspot.com/36030043/diff/580001/src/pkg/runtime/netpoll_s...
src/pkg/runtime/netpoll_solaris.c:1: // Copyright 2013 The Go Authors. All
rights reserved.
On 2014/02/22 22:24:00, minux wrote:
> s/2013/2014/

Done.

[aram, 2014-02-24 15:45:42]

Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com,
mikioh.mikioh@gmail.com, dave@cheney.net, iant@golang.org, rsc@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[rsc, 2014-02-24 17:07:52]

LGTM

Assuming this doesn't break the build for other systems, I propose submitting
this as is and doing followup work in smaller CLs. It is getting too hard to
read the version-by-version diffs.

I tried to clpatch+test+submit this myself but the patch does not apply cleanly.

[aram, 2014-02-24 21:18:52]

> Assuming this doesn't break the build for other systems, I propose submitting
> this as is and doing followup work in smaller CLs. It is getting too hard to
> read the version-by-version diffs.
> 
> I tried to clpatch+test+submit this myself but the patch does not apply
cleanly.

Should work now.

[aram, 2014-02-24 21:20:33]

Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com,
mikioh.mikioh@gmail.com, dave@cheney.net, iant@golang.org, rsc@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[rsc, 2014-02-25 03:31:06]

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

runtime, net: add support for GOOS=solaris

LGTM=dave, rsc
R=golang-codereviews, minux.ma, mikioh.mikioh, dave, iant, rsc
CC=golang-codereviews
https://codereview.appspot.com/36030043

Committer: Russ Cox <rsc@golang.org>