|
|
Descriptionnet: reduce unnecessary syscall.Sockaddr conversions
This CL makes IPAddr, UDPAddr and TCPAddr implement sockaddr
interface, UnixAddr is already sockaddr interface compliant, and
reduces unnecessary conversions between net.Addr, net.sockaddr and
syscall.Sockaddr.
This is in preparation for runtime-integrated network pollster for BSD
variants.
Update issue 5199
Patch Set 1 : diff -r ac6aff96e8fc https://code.google.com/p/go #
Total comments: 20
Patch Set 2 : diff -r 42ec35f8b19c https://code.google.com/p/go #Patch Set 3 : diff -r 42ec35f8b19c https://code.google.com/p/go #
Total comments: 6
Patch Set 4 : diff -r 9df300790bba https://code.google.com/p/go #
Total comments: 4
Patch Set 5 : diff -r 7c06f390d1ad https://code.google.com/p/go #
MessagesTotal messages: 22
Thanks Mikio. I think this can be broken up a bit more. I'm also seeing a test failure on OS X darwin --- FAIL: TestUDPConnLocalName (0.00 seconds) panic: runtime error: index out of range [recovered] panic: runtime error: index out of range goroutine 422 [running]: testing.funcĀ·005() /Users/dfc/go/src/pkg/testing/testing.go:355 +0xda net.IP.IsMulticast(0x0, 0x0, 0x0, 0x3cd9c0) net/_test/ip.go:114 +0xc0 net.listenerSockaddr(0x8, 0x2, 0x47caa0, 0x3cd9c0, 0x0, ...) net/_test/sock_unix.go:18 +0x24f net.socket(0x1fb3a0, 0x4, 0x2, 0x2, 0x0, ...) net/_test/sock_posix.go:40 +0x1bc net.internetSocket(0x1fb3a0, 0x4, 0x47caa0, 0x3cd9c0, 0x0, ...) net/_test/ipsock_posix.go:88 +0x16c net.ListenUDP(0x1fb3a0, 0x4, 0x3cd9c0, 0xc2100b9570, 0x1, ...) net/_test/udpsock_posix.go:227 +0x1ad net.TestUDPConnLocalName(0xc2100ba1b0) /Users/dfc/go/src/pkg/net/udp_test.go:152 +0xe3 testing.tRunner(0xc2100ba1b0, 0x3cb508) /Users/dfc/go/src/pkg/testing/testing.go:360 +0x8e created by testing.RunTests /Users/dfc/go/src/pkg/testing/testing.go:440 +0x88e goroutine 1 [chan receive]: testing.RunTests(0x2503c8, 0x3cac80, 0x69, 0x69, 0x4bbf01) /Users/dfc/go/src/pkg/testing/testing.go:441 +0x8b1 testing.Main(0x2503c8, 0x3cac80, 0x69, 0x69, 0x3c2fc0, ...) /Users/dfc/go/src/pkg/testing/testing.go:372 +0x8c main.main() net/_test/_testmain.go:413 +0x11b goroutine 162 [chan send]: net.runDatagramPacketConnServer(0xc21010ac60, 0x1fb380, 0x3, 0x1ff3b0, 0xb, ...) /Users/dfc/go/src/pkg/net/server_test.go:392 +0x49c created by net.TestTimeoutUDP /Users/dfc/go/src/pkg/net/timeout_test.go:244 +0x1b3 goroutine 17 [syscall]: runtime.goexit() /Users/dfc/go/src/pkg/runtime/proc.c:1332 goroutine 160 [chan send]: net.funcĀ·055() /Users/dfc/go/src/pkg/net/timeout_test.go:167 +0x8c created by net.TestWriteTimeout /Users/dfc/go/src/pkg/net/timeout_test.go:170 +0x5f5 https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/iprawsock_posix.go File src/pkg/net/iprawsock_posix.go (right): https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/iprawsock_posix... src/pkg/net/iprawsock_posix.go:184: fd, err := internetSocket(net, laddr, raddr, deadline, syscall.SOCK_RAW, proto, "dial", sockaddrToIP) please move this to a separate CL. https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/iprawsock_posix... src/pkg/net/iprawsock_posix.go:205: fd, err := internetSocket(net, laddr, nil, noDeadline, syscall.SOCK_RAW, proto, "listen", sockaddrToIP) please move this to a separate CL. https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_posix.go File src/pkg/net/sock_posix.go (left): https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_posix.go#o... src/pkg/net/sock_posix.go:51: // This socket is used by a listener. This method is complex, please retain this comment. https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_posix.go#o... src/pkg/net/sock_posix.go:78: // This socket is used by a dialer. ditto https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_posix.go File src/pkg/net/sock_posix.go (right): https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_posix.go#n... src/pkg/net/sock_posix.go:64: } else if laddr != nil && raddr != nil { please add a comment to explain what is happening in this branch https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_unix.go File src/pkg/net/sock_unix.go (right): https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_unix.go#ne... src/pkg/net/sock_unix.go:12: switch laddr.(type) { switch laddr := laddr.(type) https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_unix.go#ne... src/pkg/net/sock_unix.go:19: if laddr.(*UDPAddr) != nil && laddr.(*UDPAddr).IP.IsMulticast() { drop the assertions https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_unix.go#ne... src/pkg/net/sock_unix.go:23: addr := *laddr.(*UDPAddr) same. https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/tcpsock_posix.go File src/pkg/net/tcpsock_posix.go (right): https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/tcpsock_posix.g... src/pkg/net/tcpsock_posix.go:192: fd, err = internetSocket(net, laddr, raddr, deadline, syscall.SOCK_STREAM, 0, "dial", sockaddrToTCP) please move this to a separate CL. https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/udpsock_posix.go File src/pkg/net/udpsock_posix.go (right): https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/udpsock_posix.g... src/pkg/net/udpsock_posix.go:202: fd, err := internetSocket(net, laddr, nil, noDeadline, syscall.SOCK_DGRAM, 0, "listen", sockaddrToUDP) please move this to a separate CL.
Sign in to reply to this message.
On Tue, Jul 30, 2013 at 1:06 PM, <dave@cheney.net> wrote: > I think this can be broken up a bit more. I'm also seeing a test failure > on OS X darwin Thanks. Funny, I didn't have that failures on darwin/amd64. Did you apply this CL clearly? Also this CL includes CL 11980043 temporarily. I mean, CL 11980043 first. > src/pkg/net/iprawsock_posix.go:184: fd, err := internetSocket(net, > laddr, raddr, deadline, syscall.SOCK_RAW, proto, "dial", sockaddrToIP) > please move this to a separate CL. Thanks but no thanks. Dropping toAddr is related to temporal scaffolds in socket().
Sign in to reply to this message.
On Tue, Jul 30, 2013 at 2:37 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote: > Thanks. Funny, I didn't have that failures on darwin/amd64. Never mind. You mean test w/o -short, thx.
Sign in to reply to this message.
On 2013/07/30 05:38:49, mikio wrote: > On Tue, Jul 30, 2013 at 2:37 PM, Mikio Hara <mailto:mikioh.mikioh@gmail.com> wrote: > > > Thanks. Funny, I didn't have that failures on darwin/amd64. > > Never mind. You mean test w/o -short, thx. Confirmed. I see the same problem with go test net on linux/amd64, after applying this CL.
Sign in to reply to this message.
On Tue, Jul 30, 2013 at 3:33 PM, <dave@cheney.net> wrote: > Confirmed. I see the same problem with Fixed, also removed temporal merged fragments come from CL 11980043. You can test w/ both CL 11980043 and 12010043. Otherwise, you will see why CL 11980043 is required, like the following: /Users/mikioh/go/src/pkg/testing/testing.go:355 +0xda net.(*TCPAddr).sockaddr(0x0, 0x2, 0x1, 0x0, 0x0, ...) /Users/mikioh/go/src/pkg/net/tcpsock_posix.go:49 +0x32 net.socket(0x1e8340, 0x3, 0x2, 0x1, 0x0, ...) /Users/mikioh/go/src/pkg/net/sock_posix.go:65 +0x683 net.internetSocket(0x1e8340, 0x3, 0x4545f8, 0x0, 0x4545f8, ...) /Users/mikioh/go/src/pkg/net/ipsock_posix.go:125 +0x161 net.dialTCP(0x1e8340, 0x3, 0x0, 0xc210049d50, 0x0, ...) /Users/mikioh/go/src/pkg/net/tcpsock_posix.go:159 +0xf5 net.dial(0x1e8340, 0x3, 0xc21005a3f0, 0xf, 0x0, ...) /Users/mikioh/go/src/pkg/net/dial.go:156 +0x3df net.resolveAndDial(0x1e8340, 0x3, 0xc21005a3f0, 0xf, 0x0, ...) /Users/mikioh/go/src/pkg/net/fd_unix.go:54 +0x15e net.(*Dialer).Dial(0x4a2d60, 0x1e8340, 0x3, 0xc21005a3f0, 0xf, ...) /Users/mikioh/go/src/pkg/net/dial.go:146 +0x94 net.Dial(0x1e8340, 0x3, 0xc21005a3f0, 0xf, 0x1eb890, ...) /Users/mikioh/go/src/pkg/net/dial.go:131 +0x72
Sign in to reply to this message.
Thanks for sticking with me. Re: 11980043 and 12010043 it is clear to me now that they are both required for this change. Can you please merge them back again, sorry for making you jump through hoops. On Tue, Jul 30, 2013 at 5:56 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote: > On Tue, Jul 30, 2013 at 3:33 PM, <dave@cheney.net> wrote: > >> Confirmed. I see the same problem with > > Fixed, also removed temporal merged fragments come from CL 11980043. > > You can test w/ both CL 11980043 and 12010043. > Otherwise, you will see why CL 11980043 is required, like the following: > > /Users/mikioh/go/src/pkg/testing/testing.go:355 +0xda > net.(*TCPAddr).sockaddr(0x0, 0x2, 0x1, 0x0, 0x0, ...) > /Users/mikioh/go/src/pkg/net/tcpsock_posix.go:49 +0x32 > net.socket(0x1e8340, 0x3, 0x2, 0x1, 0x0, ...) > /Users/mikioh/go/src/pkg/net/sock_posix.go:65 +0x683 > net.internetSocket(0x1e8340, 0x3, 0x4545f8, 0x0, 0x4545f8, ...) > /Users/mikioh/go/src/pkg/net/ipsock_posix.go:125 +0x161 > net.dialTCP(0x1e8340, 0x3, 0x0, 0xc210049d50, 0x0, ...) > /Users/mikioh/go/src/pkg/net/tcpsock_posix.go:159 +0xf5 > net.dial(0x1e8340, 0x3, 0xc21005a3f0, 0xf, 0x0, ...) > /Users/mikioh/go/src/pkg/net/dial.go:156 +0x3df > net.resolveAndDial(0x1e8340, 0x3, 0xc21005a3f0, 0xf, 0x0, ...) > /Users/mikioh/go/src/pkg/net/fd_unix.go:54 +0x15e > net.(*Dialer).Dial(0x4a2d60, 0x1e8340, 0x3, 0xc21005a3f0, 0xf, ...) > /Users/mikioh/go/src/pkg/net/dial.go:146 +0x94 > net.Dial(0x1e8340, 0x3, 0xc21005a3f0, 0xf, 0x1eb890, ...) > /Users/mikioh/go/src/pkg/net/dial.go:131 +0x72
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dave@cheney.net (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/iprawsock_posix.go File src/pkg/net/iprawsock_posix.go (right): https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/iprawsock_posix... src/pkg/net/iprawsock_posix.go:184: fd, err := internetSocket(net, laddr, raddr, deadline, syscall.SOCK_RAW, proto, "dial", sockaddrToIP) thx but no thx https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/iprawsock_posix... src/pkg/net/iprawsock_posix.go:205: fd, err := internetSocket(net, laddr, nil, noDeadline, syscall.SOCK_RAW, proto, "listen", sockaddrToIP) ditto https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_posix.go File src/pkg/net/sock_posix.go (left): https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_posix.go#o... src/pkg/net/sock_posix.go:51: // This socket is used by a listener. On 2013/07/30 04:06:15, dfc wrote: > This method is complex, please retain this comment. Done. https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_posix.go#o... src/pkg/net/sock_posix.go:78: // This socket is used by a dialer. On 2013/07/30 04:06:15, dfc wrote: > ditto Done. https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_posix.go File src/pkg/net/sock_posix.go (right): https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_posix.go#n... src/pkg/net/sock_posix.go:64: } else if laddr != nil && raddr != nil { On 2013/07/30 04:06:15, dfc wrote: > please add a comment to explain what is happening in this branch Done. https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_unix.go File src/pkg/net/sock_unix.go (right): https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_unix.go#ne... src/pkg/net/sock_unix.go:12: switch laddr.(type) { On 2013/07/30 04:06:15, dfc wrote: > switch laddr := laddr.(type) Done. https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_unix.go#ne... src/pkg/net/sock_unix.go:19: if laddr.(*UDPAddr) != nil && laddr.(*UDPAddr).IP.IsMulticast() { On 2013/07/30 04:06:15, dfc wrote: > drop the assertions Done. https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/sock_unix.go#ne... src/pkg/net/sock_unix.go:23: addr := *laddr.(*UDPAddr) On 2013/07/30 04:06:15, dfc wrote: > same. Done. https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/tcpsock_posix.go File src/pkg/net/tcpsock_posix.go (right): https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/tcpsock_posix.g... src/pkg/net/tcpsock_posix.go:192: fd, err = internetSocket(net, laddr, raddr, deadline, syscall.SOCK_STREAM, 0, "dial", sockaddrToTCP) thx but no thx https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/udpsock_posix.go File src/pkg/net/udpsock_posix.go (right): https://codereview.appspot.com/12010043/diff/8001/src/pkg/net/udpsock_posix.g... src/pkg/net/udpsock_posix.go:202: fd, err := internetSocket(net, laddr, nil, noDeadline, syscall.SOCK_DGRAM, 0, "listen", sockaddrToUDP) thx but no thx
Sign in to reply to this message.
LGTM with some minor comment nits. I would like to see at least one other reviewer on this code before submitting. I am happy that the code is correct, but proving it to myself has not been simple and the requirement to unbox the typed *net.TCPAddr (or similar) to an untyped nil for this code to work correctly makes me worry. Fortunately, the tests do catch this, although some cases like *net.IPAddr.toAddr()/sockaddr() are not covered. https://codereview.appspot.com/12010043/diff/27001/src/pkg/net/sock_posix.go File src/pkg/net/sock_posix.go (right): https://codereview.appspot.com/12010043/diff/27001/src/pkg/net/sock_posix.go#... src/pkg/net/sock_posix.go:52: if laddr != nil && raddr == nil { // converts sockaddr for a named listener I'd like to see a comment that said // laddr is not nil and raddr is nil, we are opening a socket to bind to a listening port. https://codereview.appspot.com/12010043/diff/27001/src/pkg/net/sock_posix.go#... src/pkg/net/sock_posix.go:64: } else if laddr != nil && raddr != nil { // converts sockaddr for a named dialer same, but this time we are dialing from a specific source ip https://codereview.appspot.com/12010043/diff/27001/src/pkg/net/sock_posix.go#... src/pkg/net/sock_posix.go:84: if raddr != nil { // converts sockaddr for a named or unnamed dialer again a better comment please, // laddr is nil and raddr is not nil, we are connecting to a remote host, the operating system will choose the source ip and port.
Sign in to reply to this message.
On Tue, Jul 30, 2013 at 10:18 PM, <dave@cheney.net> wrote: > Fortunately, the tests do catch this, although some cases like *net.IPAddr.toAddr()/sockaddr() are not covered. You need to run tests w/ administrator,supervisor privilege. > src/pkg/net/sock_posix.go:52: if laddr != nil && raddr == nil { // > converts sockaddr for a named listener > I'd like to see a comment that said > > // laddr is not nil and raddr is nil, we are opening a socket to bind to > a listening port. Well, I'm missing your point. Do you really want such comments on those scaffolds? They'll be removed in the next CL but two or three. Also looks like you want comments for the whole socket() implementation, how to deal with arguments without context, not for each branching.
Sign in to reply to this message.
On Tue, Jul 30, 2013 at 10:18 PM, <dave@cheney.net> wrote: > I am happy that the code is correct, but proving it to myself has not > been simple and the requirement to unbox the typed *net.TCPAddr (or > similar) to an untyped nil for this code to work correctly makes me > worry. Are you suggesting to not use interfaces? Or don't test both by if statement and put non-nil interface shims everywhere? Or something more straightforward stuff?
Sign in to reply to this message.
> > // laddr is not nil and raddr is nil, we are opening a socket to bind to > > a listening port. > > Well, I'm missing your point. Do you really want such comments > on those scaffolds? They'll be removed in the next CL but two or > three. Also looks like you want comments for the whole socket() > implementation, how to deal with arguments without context, not > for each branching. This method is very complicated and I would prefer it to have more comments explaining what each branch means.
Sign in to reply to this message.
> Are you suggesting to not use interfaces? > Or don't test both by if statement and put non-nil interface shims everywhere? > Or something more straightforward stuff? I think your approach is correct, sorry it took me so long to understand this. My concern is this behavior is not immediately obvious, ie, someone could refactor sockaddr() to the original form before this change. The tests _will_ catch this, but a small comment explaining why this piece of logic is there would solve the issue before it happened.
Sign in to reply to this message.
On Wednesday, July 31, 2013 9:57:19 AM UTC+9, Dave Cheney wrote: This method is very complicated and I would prefer it to have more > comments explaining what each branch means. I see, thanks.
Sign in to reply to this message.
PTAL, thanks. Will submit this afternoon and send the next one. https://codereview.appspot.com/12010043/diff/27001/src/pkg/net/sock_posix.go File src/pkg/net/sock_posix.go (right): https://codereview.appspot.com/12010043/diff/27001/src/pkg/net/sock_posix.go#... src/pkg/net/sock_posix.go:52: if laddr != nil && raddr == nil { // converts sockaddr for a named listener On 2013/07/30 13:18:29, dfc wrote: > I'd like to see a comment that said > > // laddr is not nil and raddr is nil, we are opening a socket to bind to a > listening port. Done. https://codereview.appspot.com/12010043/diff/27001/src/pkg/net/sock_posix.go#... src/pkg/net/sock_posix.go:64: } else if laddr != nil && raddr != nil { // converts sockaddr for a named dialer > same, but this time we are dialing from a specific source ip sorry but your proposal doesn't look correct. https://codereview.appspot.com/12010043/diff/27001/src/pkg/net/sock_posix.go#... src/pkg/net/sock_posix.go:84: if raddr != nil { // converts sockaddr for a named or unnamed dialer > // laddr is nil and raddr is not nil, we are connecting to a remote host, the > operating system will choose the source ip and port. ditto
Sign in to reply to this message.
> PTAL, thanks. > Will submit this afternoon and send the next one. I would like to see a second reviewer before submitting.
Sign in to reply to this message.
I would like to see a second reviewer before submitting. waiting...
Sign in to reply to this message.
What is a "preflight socket helper" and what is a "scaffold" and which of these "scaffolds" will be removed later? Looking at the CL now, but let's clarify the CL description at least. On Tue, Jul 30, 2013 at 9:05 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote: > I would like to see a second reviewer before submitting. > > waiting... > > -- > > --- > You received this message because you are subscribed to the Google Groups > "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. > > >
Sign in to reply to this message.
Thanks, CL description is updated. On Thu, Aug 1, 2013 at 12:45 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > What is a "preflight socket helper" Dropped. I mean, a internal function that makes sockets ready to be used read, write I/O operations; specifically it configures required socket options before syscall.Bind, Listen or Connect calls. > and what is a "scaffold" and which of these "scaffolds" will be removed later? Also dropped. This CL is a fragment of CL 8726051 and has some code that doesn't exist in CL 8726051, to avoid breaking build.
Sign in to reply to this message.
LGTM but see comments I still don't fully understand how these dozen CLs are cut up, but I do agree this is getting simpler, so I'm happy enough if Dave is also happy. https://codereview.appspot.com/12010043/diff/38001/src/pkg/net/sock_posix.go File src/pkg/net/sock_posix.go (right): https://codereview.appspot.com/12010043/diff/38001/src/pkg/net/sock_posix.go#... src/pkg/net/sock_posix.go:51: // This function makes a preflight network file descriptor for more "preflight". reword? https://codereview.appspot.com/12010043/diff/38001/src/pkg/net/unixsock_posix.go File src/pkg/net/unixsock_posix.go (right): https://codereview.appspot.com/12010043/diff/38001/src/pkg/net/unixsock_posix... src/pkg/net/unixsock_posix.go:16: func unixSocket(net string, laddr, raddr sockaddr, mode string, deadline time.Time) (*netFD, error) { What's the motivation here? If we're making a unix socket, taking a remote address of a concrete unix socket address seems fine. What does being generic buy us here?
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=0c05398daad8 *** net: reduce unnecessary syscall.Sockaddr conversions This CL makes IPAddr, UDPAddr and TCPAddr implement sockaddr interface, UnixAddr is already sockaddr interface compliant, and reduces unnecessary conversions between net.Addr, net.sockaddr and syscall.Sockaddr. This is in preparation for runtime-integrated network pollster for BSD variants. Update issue 5199 R=golang-dev, dave, bradfitz CC=golang-dev https://codereview.appspot.com/12010043 https://codereview.appspot.com/12010043/diff/38001/src/pkg/net/sock_posix.go File src/pkg/net/sock_posix.go (right): https://codereview.appspot.com/12010043/diff/38001/src/pkg/net/sock_posix.go#... src/pkg/net/sock_posix.go:51: // This function makes a preflight network file descriptor for On 2013/08/01 18:13:25, bradfitz wrote: > more "preflight". reword? Done. https://codereview.appspot.com/12010043/diff/38001/src/pkg/net/unixsock_posix.go File src/pkg/net/unixsock_posix.go (right): https://codereview.appspot.com/12010043/diff/38001/src/pkg/net/unixsock_posix... src/pkg/net/unixsock_posix.go:16: func unixSocket(net string, laddr, raddr sockaddr, mode string, deadline time.Time) (*netFD, error) { On 2013/08/01 18:13:25, bradfitz wrote: > What's the motivation here? If we're making a unix socket, taking a remote > address of a concrete unix socket address seems fine. What does being generic > buy us here? I'd like to collapse both internetSocket and unixSocket because they are already unnecessary. Handling protocol-specific addresses and preparing namespace stuff are already done by dial, listen functions. Also other task that working together with runtime-integrated network pollster is not good for this one. I think there's no reason to keep this stuff, kinda just taking some args and passing the args to lowers.
Sign in to reply to this message.
Thanks for persevering. I hope the remaining changes here won't be as painful. On Sat, Aug 3, 2013 at 2:32 PM, <mikioh.mikioh@gmail.com> wrote: > *** Submitted as > https://code.google.com/p/go/source/detail?r=0c05398daad8 *** > > net: reduce unnecessary syscall.Sockaddr conversions > > This CL makes IPAddr, UDPAddr and TCPAddr implement sockaddr > interface, UnixAddr is already sockaddr interface compliant, and > reduces unnecessary conversions between net.Addr, net.sockaddr and > syscall.Sockaddr. > > This is in preparation for runtime-integrated network pollster for BSD > variants. > > Update issue 5199 > > R=golang-dev, dave, bradfitz > CC=golang-dev > https://codereview.appspot.com/12010043 > > > > > https://codereview.appspot.com/12010043/diff/38001/src/pkg/net/sock_posix.go > File src/pkg/net/sock_posix.go (right): > > https://codereview.appspot.com/12010043/diff/38001/src/pkg/net/sock_posix.go#... > src/pkg/net/sock_posix.go:51: // This function makes a preflight network > file descriptor for > On 2013/08/01 18:13:25, bradfitz wrote: >> >> more "preflight". reword? > > > Done. > > > https://codereview.appspot.com/12010043/diff/38001/src/pkg/net/unixsock_posix.go > File src/pkg/net/unixsock_posix.go (right): > > https://codereview.appspot.com/12010043/diff/38001/src/pkg/net/unixsock_posix... > src/pkg/net/unixsock_posix.go:16: func unixSocket(net string, laddr, > raddr sockaddr, mode string, deadline time.Time) (*netFD, error) { > On 2013/08/01 18:13:25, bradfitz wrote: >> >> What's the motivation here? If we're making a unix socket, taking a > > remote >> >> address of a concrete unix socket address seems fine. What does being > > generic >> >> buy us here? > > > I'd like to collapse both internetSocket and unixSocket because they are > already unnecessary. Handling protocol-specific addresses and preparing > namespace stuff are already done by dial, listen functions. Also other > task that working together with runtime-integrated network pollster is > not good for this one. I think there's no reason to keep this stuff, > kinda just taking some args and passing the args to lowers. > > https://codereview.appspot.com/12010043/
Sign in to reply to this message.
|