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

Issue 6445105: code review 6445105: net: allow ReadFrom on IPConn to receive IPv4 header an... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 7 months ago by mikio
Modified:
10 years, 9 months ago
Reviewers:
dfc, cw, bradfitz
CC:
golang-dev, r, adg, dfc, gobot
Visibility:
Public.

Description

net: allow ReadFrom on IPConn to receive IPv4 header and options IPConn is used for controlloing raw IP protocols that stay on top of the IPv4 and IPv6 transports. In Go 1.0, not sure the reason, ReadFrom on IPConn with "ip4" drops the received IPv4 header and options. It's not helpful for routing, signalling protocols because they will require IPv4 header and options in some cases. This CL allows ReadFrom on IPConn with "ip4" to receive IPv4 header and opions, also makes raw IPv4 access API consistent across methods. API change: ReadFrom method on IPConn with "ip4" returns header and options the same as Read, ReadMsgIP methods on IPConn with "ip4" do. Fixes issue 3944. Note that IPv4 and IPv6 are completely different protocols. It's not easy to provide protocol agnostic methods on IPConn. Instead, we could provide external packages that focus on controlling each protocol such as go.net/ipv4, go.net/ipv6.

Patch Set 1 : diff -r 62f087306b18 https://code.google.com/p/go #

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

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

Total comments: 4

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

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

Total comments: 3

Patch Set 6 : diff -r 42b888d568d9 https://code.google.com/p/go #

Total comments: 3

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

Total comments: 4

Patch Set 8 : diff -r e77430da3316 https://code.google.com/p/go #

Patch Set 9 : diff -r 58f8a30f5b78 https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+60 lines, -11 lines) Patch
A doc/go1.2.html View 1 2 3 4 5 6 7 8 1 chunk +47 lines, -0 lines 0 comments Download
M src/pkg/net/ipraw_test.go View 1 2 3 4 5 1 chunk +3 lines, -4 lines 0 comments Download
M src/pkg/net/iprawsock_plan9.go View 1 2 3 4 5 1 chunk +5 lines, -1 line 0 comments Download
M src/pkg/net/iprawsock_posix.go View 1 2 3 4 5 2 chunks +5 lines, -6 lines 0 comments Download

Messages

Total messages: 38
mikio
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
11 years, 7 months ago (2012-08-11 16:09:15 UTC) #1
mikio
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 7 months ago (2012-08-12 01:16:19 UTC) #2
dfc
Does this affect the Go1 contract ?
11 years, 7 months ago (2012-08-16 11:54:28 UTC) #3
mikio
On Thu, Aug 16, 2012 at 8:54 PM, <dave@cheney.net> wrote: > Does this affect the ...
11 years, 7 months ago (2012-08-16 12:56:34 UTC) #4
dfc
I hope so too. On Thu, Aug 16, 2012 at 10:56 PM, Mikio Hara <mikioh.mikioh@gmail.com> ...
11 years, 7 months ago (2012-08-16 12:59:23 UTC) #5
mikio
On Thu, Aug 16, 2012 at 9:59 PM, Dave Cheney <dave@cheney.net> wrote: > I hope ...
11 years, 7 months ago (2012-08-16 13:12:08 UTC) #6
mikio
Hello golang-dev@googlegroups.com, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 7 months ago (2012-08-23 12:30:18 UTC) #7
gobot
R=rsc (assigned by rsc)
11 years, 7 months ago (2012-09-01 14:41:05 UTC) #8
dfc
Assuming this doesn't run afoul of the Go1 contract, LGTM with these small comments. http://codereview.appspot.com/6445105/diff/13001/src/pkg/net/ipraw_test.go ...
11 years, 6 months ago (2012-09-03 10:51:34 UTC) #9
rsc
http://codereview.appspot.com/6445105/diff/13001/src/pkg/net/iprawsock_posix.go File src/pkg/net/iprawsock_posix.go (left): http://codereview.appspot.com/6445105/diff/13001/src/pkg/net/iprawsock_posix.go#oldcode81 src/pkg/net/iprawsock_posix.go:81: if len(b) >= IPv4len { // discard ipv4 header ...
11 years, 6 months ago (2012-09-10 20:27:17 UTC) #10
cw
> I would like to understand what the API is supposed to be. The comments ...
11 years, 6 months ago (2012-09-10 21:10:16 UTC) #11
rsc
Right, I am trying to understand if (A) the CL is trying to preserve the ...
11 years, 6 months ago (2012-09-10 21:11:25 UTC) #12
cw
> (A) the CL is trying to preserve the headers iirc, would break the test, ...
11 years, 6 months ago (2012-09-10 21:35:27 UTC) #13
mikio
On 2012/09/10 21:35:27, cw wrote: > is this for make things work on some kernel/platform ...
11 years, 6 months ago (2012-09-18 17:50:46 UTC) #14
cw
I'm wondering if we shouldn't push part of this down into the platform specific code?
11 years, 6 months ago (2012-09-18 19:18:28 UTC) #15
mikio
oops, sorry, pls ignore.
11 years, 4 months ago (2012-11-20 10:57:37 UTC) #16
mikio
Hello rsc@golang.org, dave@cheney.net, cw@f00f.org (cc: gobot@golang.org, golang-dev@googlegroups.com), Please take another look.
11 years ago (2013-03-19 13:32:06 UTC) #17
bradfitz
Can you make the CL description be a sentence? On Tue, Mar 19, 2013 at ...
11 years ago (2013-03-19 17:18:05 UTC) #18
mikio
Hello rsc@golang.org, dave@cheney.net, cw@f00f.org, bradfitz@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com), Please take another look.
11 years ago (2013-03-19 17:35:00 UTC) #19
rsc
I suggested some revised text, but I don't actually think this should go in. This ...
11 years ago (2013-03-19 19:24:12 UTC) #20
mikio
On 2013/03/19 19:24:12, rsc wrote: > The exception would be if you can make the ...
11 years ago (2013-03-26 02:00:46 UTC) #21
mikio
PTAL if you think this change makes sense to us. https://codereview.appspot.com/6445105/diff/47001/doc/go1.1.html File doc/go1.1.html (right): https://codereview.appspot.com/6445105/diff/47001/doc/go1.1.html#newcode225 ...
11 years ago (2013-03-26 02:03:49 UTC) #22
mikio
ping
11 years ago (2013-03-31 08:09:31 UTC) #23
dfc
Hi Mikio, Issue 3944 was removed from Go1.1Maybe a while back. I think this change ...
10 years, 12 months ago (2013-04-01 12:00:06 UTC) #24
mikio
anyone else?
10 years, 12 months ago (2013-04-03 05:48:39 UTC) #25
bradfitz
I've never understood this Issue / CL well enough to feel comfortable LGTMing it. I ...
10 years, 12 months ago (2013-04-03 07:33:28 UTC) #26
mikio
let me restate the issue. well, simply this is a bug fix (really? yup). what ...
10 years, 12 months ago (2013-04-04 01:35:54 UTC) #27
mikio
Hello bradfitz@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com), Please take another look.
10 years, 12 months ago (2013-04-04 01:36:16 UTC) #28
dfc
I agree with the argument that this is a bug in Go 1.0 and is ...
10 years, 12 months ago (2013-04-04 01:41:16 UTC) #29
mikio
https://codereview.appspot.com/6445105/diff/73002/doc/go1.1.html File doc/go1.1.html (right): https://codereview.appspot.com/6445105/diff/73002/doc/go1.1.html#newcode522 doc/go1.1.html:522: In Go 1.0, reading methods on On 2013/04/04 01:41:16, ...
10 years, 12 months ago (2013-04-04 01:55:51 UTC) #30
mikio
ping
10 years, 11 months ago (2013-04-06 08:12:13 UTC) #31
dfc
Hello r and adg, Could I ask for your time to review this CL with ...
10 years, 11 months ago (2013-04-10 02:50:05 UTC) #32
adg
Russ was against it and Brad doesn't understand it. I can see some merit in ...
10 years, 11 months ago (2013-04-10 03:39:00 UTC) #33
dfc
Thanks for making the call. On 10/04/2013, at 13:39, adg@golang.org wrote: > Russ was against ...
10 years, 11 months ago (2013-04-10 04:15:33 UTC) #34
gobot
R=bradfitz (assigned by dsymonds)
10 years, 11 months ago (2013-04-18 02:51:43 UTC) #35
cw
Is this almost ready as it stands?
10 years, 10 months ago (2013-05-21 22:21:38 UTC) #36
bradfitz
NOT LGTM ... to make the thing unhappy. We're not going to do this. (discussed ...
10 years, 10 months ago (2013-05-21 22:22:54 UTC) #37
mikio
10 years, 9 months ago (2013-06-06 08:17:45 UTC) #38
*** Abandoned ***
Sign in to reply to this message.

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