Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(7)

Issue 12010043: code review 12010043: net: reduce unnecessary syscall.Sockaddr conversions (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 8 months ago by mikio
Modified:
10 years, 8 months ago
Reviewers:
dfc, bradfitz
CC:
golang-dev, dfc, bradfitz
Visibility:
Public.

Description

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

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+95 lines, -74 lines) Patch
M src/pkg/net/iprawsock_posix.go View 2 3 chunks +7 lines, -4 lines 0 comments Download
M src/pkg/net/ipsock_posix.go View 1 2 3 4 1 chunk +1 line, -12 lines 0 comments Download
M src/pkg/net/sock_posix.go View 1 2 3 4 4 chunks +41 lines, -17 lines 0 comments Download
M src/pkg/net/sock_unix.go View 1 1 chunk +11 lines, -11 lines 0 comments Download
M src/pkg/net/sock_windows.go View 1 1 chunk +11 lines, -11 lines 0 comments Download
M src/pkg/net/tcpsock_posix.go View 2 4 chunks +8 lines, -5 lines 0 comments Download
M src/pkg/net/udpsock_posix.go View 2 4 chunks +8 lines, -5 lines 0 comments Download
M src/pkg/net/unixsock_posix.go View 3 chunks +8 lines, -9 lines 0 comments Download

Messages

Total messages: 22
dfc
Thanks Mikio. I think this can be broken up a bit more. I'm also seeing ...
10 years, 8 months ago (2013-07-30 04:06:14 UTC) #1
mikio
On Tue, Jul 30, 2013 at 1:06 PM, <dave@cheney.net> wrote: > I think this can ...
10 years, 8 months ago (2013-07-30 05:37:21 UTC) #2
mikio
On Tue, Jul 30, 2013 at 2:37 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote: > Thanks. Funny, ...
10 years, 8 months ago (2013-07-30 05:38:49 UTC) #3
dfc
On 2013/07/30 05:38:49, mikio wrote: > On Tue, Jul 30, 2013 at 2:37 PM, Mikio ...
10 years, 8 months ago (2013-07-30 06:33:39 UTC) #4
mikio
On Tue, Jul 30, 2013 at 3:33 PM, <dave@cheney.net> wrote: > Confirmed. I see the ...
10 years, 8 months ago (2013-07-30 07:56:36 UTC) #5
dfc
Thanks for sticking with me. Re: 11980043 and 12010043 it is clear to me now ...
10 years, 8 months ago (2013-07-30 12:41:15 UTC) #6
mikio
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
10 years, 8 months ago (2013-07-30 13:00:01 UTC) #7
mikio
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.go#newcode184 src/pkg/net/iprawsock_posix.go:184: fd, err := internetSocket(net, laddr, raddr, deadline, syscall.SOCK_RAW, proto, ...
10 years, 8 months ago (2013-07-30 13:08:13 UTC) #8
dfc
LGTM with some minor comment nits. I would like to see at least one other ...
10 years, 8 months ago (2013-07-30 13:18:28 UTC) #9
mikio
On Tue, Jul 30, 2013 at 10:18 PM, <dave@cheney.net> wrote: > Fortunately, the tests do ...
10 years, 8 months ago (2013-07-31 00:07:56 UTC) #10
mikio
On Tue, Jul 30, 2013 at 10:18 PM, <dave@cheney.net> wrote: > I am happy that ...
10 years, 8 months ago (2013-07-31 00:13:26 UTC) #11
dfc
> > // laddr is not nil and raddr is nil, we are opening a ...
10 years, 8 months ago (2013-07-31 00:57:19 UTC) #12
dfc
> Are you suggesting to not use interfaces? > Or don't test both by if ...
10 years, 8 months ago (2013-07-31 00:59:18 UTC) #13
mikio
On Wednesday, July 31, 2013 9:57:19 AM UTC+9, Dave Cheney wrote: This method is very ...
10 years, 8 months ago (2013-07-31 01:29:46 UTC) #14
mikio
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): ...
10 years, 8 months ago (2013-07-31 03:28:27 UTC) #15
dfc
> PTAL, thanks. > Will submit this afternoon and send the next one. I would ...
10 years, 8 months ago (2013-07-31 03:30:39 UTC) #16
mikio
I would like to see a second reviewer before submitting. waiting...
10 years, 8 months ago (2013-07-31 04:05:19 UTC) #17
bradfitz
What is a "preflight socket helper" and what is a "scaffold" and which of these ...
10 years, 8 months ago (2013-07-31 15:45:51 UTC) #18
mikio
Thanks, CL description is updated. On Thu, Aug 1, 2013 at 12:45 AM, Brad Fitzpatrick ...
10 years, 8 months ago (2013-08-01 00:04:37 UTC) #19
bradfitz
LGTM but see comments I still don't fully understand how these dozen CLs are cut ...
10 years, 8 months ago (2013-08-01 18:13:25 UTC) #20
mikio
*** Submitted as https://code.google.com/p/go/source/detail?r=0c05398daad8 *** net: reduce unnecessary syscall.Sockaddr conversions This CL makes IPAddr, UDPAddr ...
10 years, 8 months ago (2013-08-03 04:32:35 UTC) #21
dfc
10 years, 8 months ago (2013-08-03 04:35:41 UTC) #22
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b