|
|
Descriptionnet: 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 #
MessagesTotal messages: 38
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
Sign in to reply to this message.
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Does this affect the Go1 contract ?
Sign in to reply to this message.
On Thu, Aug 16, 2012 at 8:54 PM, <dave@cheney.net> wrote: > Does this affect the Go1 contract ? I guess, on the border. hope not crossing yet. PacketConn doc says nothing special on the detail of packet itself. we maybe could think that this is a simple minor bug we can fix for the go1 life duration because there's no API signature, args, other changes that can be detected by api conformance tools. on the other hand, this CL will make a small API behavioral change certainly even if the previous behavior is wrong.
Sign in to reply to this message.
I hope so too. On Thu, Aug 16, 2012 at 10:56 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote: > On Thu, Aug 16, 2012 at 8:54 PM, <dave@cheney.net> wrote: > >> Does this affect the Go1 contract ? > > I guess, on the border. hope not crossing yet. > > PacketConn doc says nothing special on the detail of packet itself. > > we maybe could think that this is a simple minor bug we can fix > for the go1 life duration because there's no API signature, args, > other changes that can be detected by api conformance tools. > > on the other hand, this CL will make a small API behavioral change > certainly even if the previous behavior is wrong.
Sign in to reply to this message.
On Thu, Aug 16, 2012 at 9:59 PM, Dave Cheney <dave@cheney.net> wrote: > I hope so too. CL 6426047 could be a workaround in the case we should mark ReadFrom on IPConn was broken but not repair it.
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.
R=rsc (assigned by rsc)
Sign in to reply to this message.
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 File src/pkg/net/ipraw_test.go (right): http://codereview.appspot.com/6445105/diff/13001/src/pkg/net/ipraw_test.go#ne... src/pkg/net/ipraw_test.go:151: if err != nil { if _, err := c.Write(echo); err != nil { http://codereview.appspot.com/6445105/diff/13001/src/pkg/net/ipraw_test.go#ne... src/pkg/net/ipraw_test.go:157: _, err := c.Read(reply) if _, err := c.Read(reply); err != nil { http://codereview.appspot.com/6445105/diff/13001/src/pkg/net/ipraw_test.go#ne... src/pkg/net/ipraw_test.go:163: hdrlen := (int(reply[0]) & 0xf) * 4 what happens if hdrlen > len(reply) ?
Sign in to reply to this message.
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.... src/pkg/net/iprawsock_posix.go:81: if len(b) >= IPv4len { // discard ipv4 header I am confused. Is the issue that you don't want to discard the IPv4 header anymore, or is it that the header is already gone and we were throwing away data bytes? I would like to understand what the API is supposed to be. The comments don't really say.
Sign in to reply to this message.
> I would like to understand what the API is supposed to be. The comments don't > really say. I think this was my code from a couple of years back, I could be wrong. IIRC, IPv4 raw sockets have headers prepended to them. IPv6 do not. So discarding them for IPv4 makes sense to keep the interface more uniform. @mikio, does that seem reasonable?
Sign in to reply to this message.
Right, I am trying to understand if (A) the CL is trying to preserve the headers or (B) we have somehow started using a system call that drops the headers and the code is trying to avoid dropping the beginning of the data. Russ
Sign in to reply to this message.
> (A) the CL is trying to preserve the headers iirc, would break the test, it does a ping to localhost > (B) we have somehow started using a system call that drops the headers > and the code is trying to avoid dropping the beginning of the data. which would also break the test is this for make things work on some kernel/platform that behaves differently? i think when i did this i tested it on linux, bsd and mcapple and they all behaved the same way i suspect someone wants the headers (which is arguably fair) but not what the API did on the past i would prefer APIs that don't involve syscall be fairly uniform and not protocol specific if you need protocol/platform specific thins, importing syscall or unsafe might be required
Sign in to reply to this message.
On 2012/09/10 21:35:27, cw wrote: > is this for make things work on some kernel/platform that behaves differently? unfortunately SOCK_RAW for Windows. http://msdn.microsoft.com/en-us/library/windows/desktop/ms740548(v=vs.85).aspx but fortunately not implemented yet. ;) Russ also pointed out interesting question: src/pkg/net/iprawsock_posix.go:79: if addr.IP.To4() != nil && len(b) > 20 { // discard ipv4 header If sa is an IPv6 sockaddr but it contains an IPv4-mapped address, then addr.IP.To4() will be non-nil here, so you get different behavior for "ip6" using IPv4-mapped addresses vs "ip6" using non-IPv4-mapped addresses. Is that intended? let me sleep on it.
Sign in to reply to this message.
I'm wondering if we shouldn't push part of this down into the platform specific code?
Sign in to reply to this message.
oops, sorry, pls ignore.
Sign in to reply to this message.
Hello rsc@golang.org, dave@cheney.net, cw@f00f.org (cc: gobot@golang.org, golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Can you make the CL description be a sentence? On Tue, Mar 19, 2013 at 6:32 AM, <mikioh.mikioh@gmail.com> wrote: > Hello rsc@golang.org, dave@cheney.net, cw@f00f.org (cc: > gobot@golang.org, golang-dev@googlegroups.com), > > Please take another look. > > > https://codereview.appspot.**com/6445105/<https://codereview.appspot.com/6445... > > -- > > ---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<golang-dev%2Bunsubscribe@googlegrou... > . > For more options, visit https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o... > . > > >
Sign in to reply to this message.
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.
Sign in to reply to this message.
I suggested some revised text, but I don't actually think this should go in. This breaks existing programs. It seems like we should instead document the distinction in the IPConn type doc comment. The exception would be if you can make the case that there are no possible useful programs with the current behavior, in which case it's just a bug. But it seems to me that if you are using ReadFrom then it's okay not to get headers back because you can find out who sent it using the separate address result. 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 doc/go1.1.html:225: The previous <code>ReadFrom</code> method on <code>IPConn</code> In Go 1.0, reading methods on <code>IPConn</code> were inconsistent about whether the data being read included the IPv4 header and options: <code>Read</code> included them, but <code>ReadFrom</code> did not. In Go 1.1, both include the IPv4 header and options.
Sign in to reply to this message.
On 2013/03/19 19:24:12, rsc wrote: > The exception would be if you can make the case that there are no possible > useful programs with the current behavior, in which case it's just a bug. But it > seems to me that if you are using ReadFrom then it's okay not to get headers > back because you can find out who sent it using the separate address result. Okay, let me try it. 1) Some protocols on IPv4 require to validate received packets for security or other reasons, not only a pair of addresses but TTL or other fields in the received header. Read/WriteMsgIP in Go 1.1 would be a solution for that but... 2) Even in Go 1.1 Windows doesn't support Read/WriteMsgIP because old Windows kernels don't implement recv/sendmsg API. For now, raw protocol implementations on Windows need ReadFrom/WriteTo for controlling its IPv4 protocol stack. Does this make sense?
Sign in to reply to this message.
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 doc/go1.1.html:225: The previous <code>ReadFrom</code> method on <code>IPConn</code> Thanks https://codereview.appspot.com/6445105/diff/47001/doc/go1.1.html#newcode225 doc/go1.1.html:225: The previous <code>ReadFrom</code> method on <code>IPConn</code> On 2013/03/19 19:24:13, rsc wrote: > In Go 1.0, reading methods on <code>IPConn</code> were inconsistent about > whether the data being read included the IPv4 header and options: > <code>Read</code> included them, but <code>ReadFrom</code> did not. > In Go 1.1, both include the IPv4 header and options. Done.
Sign in to reply to this message.
Hi Mikio, Issue 3944 was removed from Go1.1Maybe a while back. I think this change is good too go, but as it involves a break with the Go 1.0 contract, I don't think I can review it. I'm going to assign this to Brad to make the call on the Go 1.0 contract break. Sorry. Dave https://codereview.appspot.com/6445105/diff/63001/doc/go1.1.html File doc/go1.1.html (right): https://codereview.appspot.com/6445105/diff/63001/doc/go1.1.html#newcode464 doc/go1.1.html:464: Please include an Upgrading: paragraph similar to the other changes mentioned in this document. This could be as simple as saying 'Callers of IPCon.ReadFrom should adjust their code to handle the additional header data'
Sign in to reply to this message.
anyone else?
Sign in to reply to this message.
I've never understood this Issue / CL well enough to feel comfortable LGTMing it. I would rather just document the status quo and not break anybody. I'd rather people somehow opt-in to the new consistent behavior, rather than have it forced on them. I'm sure I'm still not understanding the issue. On Mon, Apr 1, 2013 at 5:00 AM, <dave@cheney.net> wrote: > Hi Mikio, > > Issue 3944 was removed from Go1.1Maybe a while back. I think this change > is good too go, but as it involves a break with the Go 1.0 contract, I > don't think I can review it. > > I'm going to assign this to Brad to make the call on the Go 1.0 contract > break. > > Sorry. > > Dave > > > https://codereview.appspot.**com/6445105/diff/63001/doc/**go1.1.html<https://... > File doc/go1.1.html (right): > > https://codereview.appspot.**com/6445105/diff/63001/doc/** > go1.1.html#newcode464<https://codereview.appspot.com/6445105/diff/63001/doc/go1.1.html#newcode464> > doc/go1.1.html:464: > Please include an Upgrading: paragraph similar to the other changes > mentioned in this document. This could be as simple as saying 'Callers > of IPCon.ReadFrom should adjust their code to handle the additional > header data' > > https://codereview.appspot.**com/6445105/<https://codereview.appspot.com/6445... >
Sign in to reply to this message.
let me restate the issue. well, simply this is a bug fix (really? yup). what trying to fix is allowing ipconn("ip4") to receive ipv4 header (hm). that's important for people who want to write network control plane stuff to the infrastructures (stumbled twice). https://codereview.appspot.com/6445105/diff/63001/doc/go1.1.html File doc/go1.1.html (right): https://codereview.appspot.com/6445105/diff/63001/doc/go1.1.html#newcode464 doc/go1.1.html:464: thanks. https://codereview.appspot.com/6445105/diff/63001/doc/go1.1.html#newcode464 doc/go1.1.html:464: On 2013/04/01 12:00:06, dfc wrote: > Please include an Upgrading: paragraph similar to the other changes mentioned in > this document. This could be as simple as saying 'Callers of IPCon.ReadFrom > should adjust their code to handle the additional header data' Done.
Sign in to reply to this message.
Hello bradfitz@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
I agree with the argument that this is a bug in Go 1.0 and is allowed by the Go 1 contract. Please wait for r or brad to sign off on the API breakage. 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 s/reading//g https://codereview.appspot.com/6445105/diff/73002/doc/go1.1.html#newcode524 doc/go1.1.html:524: were inconsistent about whether the data being read included the IPv4 header and options: I think semicolon is a better choice than colon here.
Sign in to reply to this message.
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, dfc wrote: > s/reading//g why? other methods have no flaws. https://codereview.appspot.com/6445105/diff/73002/doc/go1.1.html#newcode524 doc/go1.1.html:524: were inconsistent about whether the data being read included the IPv4 header and options: On 2013/04/04 01:41:16, dfc wrote: > I think semicolon is a better choice than colon here. Done.
Sign in to reply to this message.
ping
Sign in to reply to this message.
Hello r and adg, Could I ask for your time to review this CL with a hope to deciding if it will be included in 1.1 Cheers Dave
Sign in to reply to this message.
Russ was against it and Brad doesn't understand it. I can see some merit inĀ it, but I'm just not comfortable approving a backward-incompatible change this late in the process. I know it has been 8 months in the making and this must be frustrating. I'm sorry, but it'll have to wait for 1.2.
Sign in to reply to this message.
Thanks for making the call. On 10/04/2013, at 13:39, adg@golang.org wrote: > Russ was against it and Brad doesn't understand it. I can see some merit > in it, but I'm just not comfortable approving a backward-incompatible > change this late in the process. > > I know it has been 8 months in the making and this must be frustrating. > I'm sorry, but it'll have to wait for 1.2. > > https://codereview.appspot.com/6445105/ > > -- > > ---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.
R=bradfitz (assigned by dsymonds)
Sign in to reply to this message.
Is this almost ready as it stands?
Sign in to reply to this message.
NOT LGTM ... to make the thing unhappy. We're not going to do this. (discussed earlier) On Tue, May 21, 2013 at 3:21 PM, <cw@f00f.org> wrote: > Is this almost ready as it stands? > > https://codereview.appspot.**com/6445105/<https://codereview.appspot.com/6445... >
Sign in to reply to this message.
|