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

Issue 7058062: code review 7058062: syscall: handle empty address in ReadFrom better (Closed)

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

Description

syscall: handle empty address in ReadFrom better Handle return values from recvfrom correctly when the kernel decides to not return an address. Fixes issue 4636. Fixes issue 4352.

Patch Set 1 #

Patch Set 2 : diff -r 1399878c6731 https://code.google.com/p/go #

Patch Set 3 : diff -r 1399878c6731 https://code.google.com/p/go #

Patch Set 4 : diff -r afa30db15b70 https://code.google.com/p/go #

Total comments: 2

Patch Set 5 : diff -r e55f615005ba http://code.google.com/p/go #

Patch Set 6 : diff -r 7bc28d72d49c http://code.google.com/p/go #

Total comments: 12

Patch Set 7 : diff -r 7f25ce26f38f http://code.google.com/p/go #

Total comments: 4

Patch Set 8 : diff -r f6172d444cc0 http://code.google.com/p/go #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+138 lines, -7 lines) Patch
A src/pkg/net/unix_test.go View 1 2 3 4 5 6 1 chunk +123 lines, -0 lines 1 comment Download
M src/pkg/net/unixsock_posix.go View 1 2 3 4 5 6 7 2 chunks +9 lines, -5 lines 0 comments Download
M src/pkg/syscall/syscall_bsd.go View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M src/pkg/syscall/syscall_linux.go View 1 2 3 4 5 6 7 1 chunk +3 lines, -1 line 0 comments Download

Messages

Total messages: 24
jeff.allen
Hello rsc@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
9 years, 11 months ago (2013-01-09 12:46:10 UTC) #1
dfc
We have to be careful to respect the existing go 1.0 behaviour. I don't know ...
9 years, 11 months ago (2013-01-09 12:48:53 UTC) #2
mikio
Thank you for your investigations. I guess we need to have a look at not ...
9 years, 11 months ago (2013-01-09 13:50:36 UTC) #3
mikio
I'm not sure whether we should fix DialUnix for accepting unnamed sockets. But if you ...
9 years, 11 months ago (2013-01-09 14:14:33 UTC) #4
jeff.allen
With this change, a call to net.DialUnix that would have failed before with an error ...
9 years, 11 months ago (2013-01-09 15:43:00 UTC) #5
jeff.allen
On 2013/01/09 13:50:36, mikio wrote: > Also please add Read/Write method tests to protoconn_test.go. These ...
9 years, 11 months ago (2013-01-09 16:11:17 UTC) #6
jeff.allen
Hello rsc@golang.org, mikioh.mikioh@gmail.com, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 11 months ago (2013-01-09 16:11:32 UTC) #7
mikio
On Thu, Jan 10, 2013 at 1:11 AM, <jeff.allen@gmail.com> wrote: > These changes to syscall ...
9 years, 11 months ago (2013-01-09 17:16:43 UTC) #8
rsc
As far as API: "unix" should be like "tcp". "unixgram" should be like "udp". Russ
9 years, 11 months ago (2013-01-09 19:54:10 UTC) #9
mikio
Hi Jeff, Could you please split this into two CLs: - syscall: tweak recvfrom stuff ...
9 years, 10 months ago (2013-01-14 14:04:19 UTC) #10
jeff.allen
Please take another look. I realized I didn't need to muck with the behavior of ...
9 years, 10 months ago (2013-01-15 15:45:39 UTC) #11
mikio
Seems tests don't work well on BSDs, break Plan9/Windows build. Please feel free to take: ...
9 years, 10 months ago (2013-01-16 05:05:53 UTC) #12
mikio
syscall: ReadFrom on unixgram sockets gave incorrect EAFNOSUPPORT Also please change the CL description to ...
9 years, 10 months ago (2013-01-16 05:08:56 UTC) #13
jeff.allen
Hello rsc@golang.org, mikioh.mikioh@gmail.com, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 10 months ago (2013-01-18 14:12:38 UTC) #14
rsc
Please check that the returned address is nil.
9 years, 10 months ago (2013-01-18 22:39:03 UTC) #15
mikio
https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go File src/pkg/net/unix_test.go (right): https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go#newcode18 src/pkg/net/unix_test.go:18: // use ioutil.TempFile to get a name that is ...
9 years, 10 months ago (2013-01-19 14:10:21 UTC) #16
jeff.allen
https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go File src/pkg/net/unix_test.go (right): https://codereview.appspot.com/7058062/diff/21001/src/pkg/net/unix_test.go#newcode18 src/pkg/net/unix_test.go:18: // use ioutil.TempFile to get a name that is ...
9 years, 10 months ago (2013-01-23 15:10:14 UTC) #17
mikio
https://codereview.appspot.com/7058062/diff/27001/src/pkg/net/unix_test.go File src/pkg/net/unix_test.go (right): https://codereview.appspot.com/7058062/diff/27001/src/pkg/net/unix_test.go#newcode72 src/pkg/net/unix_test.go:72: t.Errorf("neighbor address is %v", from) hm, on freebsd it ...
9 years, 10 months ago (2013-01-23 16:18:22 UTC) #18
mikio
On Thu, Jan 24, 2013 at 1:18 AM, <mikioh.mikioh@gmail.com> wrote: > hm, on freebsd it ...
9 years, 10 months ago (2013-01-23 23:57:36 UTC) #19
mikio
FYI https://codereview.appspot.com/7058062/diff/27001/src/pkg/net/unix_test.go File src/pkg/net/unix_test.go (right): https://codereview.appspot.com/7058062/diff/27001/src/pkg/net/unix_test.go#newcode72 src/pkg/net/unix_test.go:72: t.Errorf("neighbor address is %v", from) okay, just confirmed ...
9 years, 10 months ago (2013-01-24 02:16:42 UTC) #20
jeff.allen
OK, I have incorporated 7204050 into this CL, and now I think it is ready ...
9 years, 10 months ago (2013-01-30 12:42:27 UTC) #21
rsc
LGTM Actually it looks awful to me, but that's not your fault. I can't believe ...
9 years, 10 months ago (2013-01-30 18:00:31 UTC) #22
rsc
*** Submitted as https://code.google.com/p/go/source/detail?r=9379ab966cb2 *** syscall: handle empty address in ReadFrom better Handle return values ...
9 years, 10 months ago (2013-01-30 18:02:05 UTC) #23
mikio
9 years, 10 months ago (2013-01-31 00:01:39 UTC) #24
LGTM

Thank you for fixing this.
Hope Go 1.1 will have no bash on unix domain socket stuff.

Will revisit net tests later, until Go 1.1 cutoff.

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

https://codereview.appspot.com/7058062/diff/32001/src/pkg/net/unix_test.go#ne...
src/pkg/net/unix_test.go:115: var peer Addr
maybe,

_, from, err := c.ReadFrom(...)
  :
if from != nil{
  t.Errorf("peer adddress is %v", from)
}

might be consistent.
Sign in to reply to this message.

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