Issue 7511043: code review 7511043: net: use original raddr if getpeername fails (Closed)
|
Created:
12 years, 2 months ago by TylerB Modified:
12 years, 2 months ago Reviewers:
CC:
dave_cheney.net, mikio, rsc, golang-dev Visibility:
Public. |
Description net: use original raddr if getpeername fails
In sock_posix.go:socket, check if the Getpeername returned nil. If it did, use the raddr that was passed to socket().
Fixes issue 3838.
Patch Set 1 #Patch Set 2 : diff -r d4e1ec84876c https://code.google.com/p/go #Patch Set 3 : diff -r d4e1ec84876c https://code.google.com/p/go #Patch Set 4 : diff -r a69d9442029e https://code.google.com/p/go #Patch Set 5 : diff -r a69d9442029e https://code.google.com/p/go #Patch Set 6 : diff -r fa13899da667 https://code.google.com/p/go #
Total comments: 1
Patch Set 7 : diff -r fa13899da667 https://code.google.com/p/go #
Total comments: 9
Patch Set 8 : diff -r fa13899da667 https://code.google.com/p/go #Patch Set 9 : diff -r fa13899da667 https://code.google.com/p/go #MessagesTotal messages: 21
Hello 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.
On 2013/03/06 16:02:56, TylerB wrote: > Hello mailto:golang-dev@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go Please include a test
Sign in to reply to this message.
I'm not certain how to write a test for this issue. I'll take a look and see what I can find. On 2013/03/06 18:14:33, dfc wrote: > On 2013/03/06 16:02:56, TylerB wrote: > > Hello mailto:golang-dev@googlegroups.com, > > > > I'd like you to review this change to > > https://code.google.com/p/go > > Please include a test
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
I think we need not only for IPConn but UnixConn, considerations for both sides initiator and responder because the real issue is 3721.
Sign in to reply to this message.
On 2013/03/07 02:08:32, mikio wrote: > I think we need not only for IPConn but UnixConn, considerations for both sides > initiator and responder because the real issue is 3721. I assume that would be in unixsock_posix.go? Is this a change that should be done under 3838 in this CL, or should a new bug be created for it?
Sign in to reply to this message.
> I assume that would be in unixsock_posix.go? I'd like to prefer more generic solution at sock_posix.go if possible. Or leave as it is if it's hard to address how LocalAddr, RemoteAddr methods on ProtocolConn should behave across over platforms. > Is this a change that should be done under 3838 in this CL, or should a new bug > be created for it? I think 3838 doesn't say that's a specific issue to IPConn.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dave@cheney.net, mikioh.mikioh@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
leaving for mikio. thanks. https://codereview.appspot.com/7511043/diff/17001/src/pkg/net/ipraw_test.go File src/pkg/net/ipraw_test.go (right): https://codereview.appspot.com/7511043/diff/17001/src/pkg/net/ipraw_test.go#n... src/pkg/net/ipraw_test.go:350: if result.RemoteAddr() == nil { You know what the remote addr should be. Test that it really is. (Use reflect.DeepEqual.)
Sign in to reply to this message.
On 2013/03/11 20:59:28, rsc wrote:
> src/pkg/net/ipraw_test.go:350: if result.RemoteAddr() == nil {
> You know what the remote addr should be. Test that it really is.
> (Use reflect.DeepEqual.)
Done.
Sign in to reply to this message.
Do you plan to add test cases for UnixConn and UDPConn? https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go File src/pkg/net/ipraw_test.go (right): https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go#n... src/pkg/net/ipraw_test.go:341: s/DialIP/IPConnRemoteName/ https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go#n... src/pkg/net/ipraw_test.go:346: raddr := &IPAddr{IP: IPv4(127, 0, 0, 10).To4()} if you want to use reflect.DeepEqual instead of IP.Equal. https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go#n... src/pkg/net/ipraw_test.go:347: drop blank https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go#n... src/pkg/net/ipraw_test.go:348: s/result/c/ https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go#n... src/pkg/net/ipraw_test.go:352: s/result/c/ https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go#n... src/pkg/net/ipraw_test.go:353: t.Fatalf("got %#v, expected %#v", c.RemoteAddr(), raddr) https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/sock_posix.go File src/pkg/net/sock_posix.go (right): https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/sock_posix.go#n... src/pkg/net/sock_posix.go:66: pls drop blank line https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/sock_posix.go#n... src/pkg/net/sock_posix.go:70: ditto
Sign in to reply to this message.
I don't seem to be able to view your comments. The site says "error: old chunk mismatch". Is there anything I can do about that? As for the tests, I totally spaced it. I'll look in to it. On 2013/03/12 00:04:19, mikio wrote: > Do you plan to add test cases for UnixConn and UDPConn? > > https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go > File src/pkg/net/ipraw_test.go (right): > > https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go#n... > src/pkg/net/ipraw_test.go:341: > s/DialIP/IPConnRemoteName/ > > https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go#n... > src/pkg/net/ipraw_test.go:346: > raddr := &IPAddr{IP: IPv4(127, 0, 0, 10).To4()} > if you want to use reflect.DeepEqual instead of IP.Equal. > > https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go#n... > src/pkg/net/ipraw_test.go:347: > drop blank > > https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go#n... > src/pkg/net/ipraw_test.go:348: > s/result/c/ > > https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go#n... > src/pkg/net/ipraw_test.go:352: > s/result/c/ > > https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go#n... > src/pkg/net/ipraw_test.go:353: > t.Fatalf("got %#v, expected %#v", c.RemoteAddr(), raddr) > > https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/sock_posix.go > File src/pkg/net/sock_posix.go (right): > > https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/sock_posix.go#n... > src/pkg/net/sock_posix.go:66: > pls drop blank line > > https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/sock_posix.go#n... > src/pkg/net/sock_posix.go:70: > ditto
Sign in to reply to this message.
On Tue, Mar 12, 2013 at 8:17 AM, <tylerbunnell@gmail.com> wrote: > I don't seem to be able to view your comments. The site says "error: old > chunk mismatch". Is there anything I can do about that? Actually me, not sure what's going on at backyard. But sometimes "unified diff" works, sometimes. > As for the tests, I totally spaced it. I'll look in to it. Thanks.
Sign in to reply to this message.
On 2013/03/12 00:21:57, mikio wrote: > Actually me, not sure what's going on at backyard. > But sometimes "unified diff" works, sometimes. That did it! Thanks! :)
Sign in to reply to this message.
and, https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go File src/pkg/net/ipraw_test.go (right): https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go#n... src/pkg/net/ipraw_test.go:351: defer c.Close()
Sign in to reply to this message.
I'm not entirely certain how to write a good test for DialUnix. With DialIP, even if I give it a bogus address to dial, it still returns without error and gives me an fd to test. With DialUnix, an error is returned immediately and I have nothing to test. Perhaps I'm going about both tests the wrong way? On 2013/03/12 00:47:42, mikio wrote: > and, > > https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go > File src/pkg/net/ipraw_test.go (right): > > https://codereview.appspot.com/7511043/diff/23001/src/pkg/net/ipraw_test.go#n... > src/pkg/net/ipraw_test.go:351: > defer c.Close()
Sign in to reply to this message.
Given that the fix is so general, a test of just the easiest specific protocol seems okay to me.
Sign in to reply to this message.
On Tue, Mar 12, 2013 at 9:15 AM, <tylerbunnell@gmail.com> wrote: > I'm not entirely certain how to write a good test for DialUnix. With > DialIP, even if I give it a bogus address to dial, it still returns > without error and gives me an fd to test. With DialUnix, an error is > returned immediately and I have nothing to test. Hm, perhaps you need to run test w/ "unixgram" only. I'll take over other test cases if you don't mind and will submit this CL. PS: can you please drop unnecessary blank lines from sock_posix.go before I submit.
Sign in to reply to this message.
On 2013/03/12 23:23:15, mikio wrote: > Hm, perhaps you need to run test w/ "unixgram" only. > I'll take over other test cases if you don't mind and will > submit this CL. Sounds good! > PS: can you please drop unnecessary blank lines from > sock_posix.go before I submit. Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=a55e2bc546c4 *** net: use original raddr if getpeername fails Fixes issue 3838. R=dave, mikioh.mikioh, rsc CC=golang-dev https://codereview.appspot.com/7511043 Committer: Russ Cox <rsc@golang.org>
Sign in to reply to this message.
|
