Issue 6426047: code review 6426047: net: add read, write message methods to IPConn, UDPConn (Closed)
|
Created:
11 years, 9 months ago by mikio Modified:
11 years, 6 months ago Reviewers:
CC:
rsc, r, dfc, golang-dev Visibility:
Public. |
Descriptionnet: add read, write message methods to IPConn, UDPConn
Both methods allow to access the IP ancillary data through
socket control messages.
This CL is required for CL 6482044; go.net/ipv4: new package.
Patch Set 1 : diff -r b42e90a3d42f https://code.google.com/p/go #Patch Set 2 : diff -r 4e3a1967af09 https://code.google.com/p/go #Patch Set 3 : diff -r 62f087306b18 https://code.google.com/p/go #Patch Set 4 : diff -r 62f087306b18 https://code.google.com/p/go #Patch Set 5 : diff -r 1e84edee3397 https://code.google.com/p/go #
Total comments: 3
Patch Set 6 : diff -r 3181682928c9 https://code.google.com/p/go #
Total comments: 8
Patch Set 7 : diff -r 979c5e5b1308 https://code.google.com/p/go #Patch Set 8 : diff -r c09350860e11 https://code.google.com/p/go #MessagesTotal messages: 20
Hello rsc@golang.org (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.
I am sorry for being so uninformed, but how can an IP packet have out-of-band data? Russ
Sign in to reply to this message.
On Sat, Aug 4, 2012 at 4:49 AM, Russ Cox <rsc@golang.org> wrote: > I am sorry for being so uninformed, but how can an IP packet have > out-of-band data? CL description isn't accurate, my bad. IP control plane uses in-band, I mean no extra channels, communication style but IP stack in each node can take ancillary informations such as incoming interface index, raw-IP header, nested numbers of tunnels, etc for each packet. And IP stack can tell us such per-packet ancillaries when we toggle IP-level options on through out-of-band channel of socket.
Sign in to reply to this message.
There's a typo in the CL description. That's the most intelligent thing I have to say about Unix's network APIs. -rob
Sign in to reply to this message.
Hello rsc@golang.org, r@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Hello rsc@golang.org, r@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Thank you for this proposal. I'm not wild about the use of named return arguments, I think the intent would be clearer if you defined the addr variable before it was used. Is there a way you can write a test for these new methods? I guess IPConn requires root, but UDPConn might be possible. Also, please fix the typo in the description. http://codereview.appspot.com/6426047/diff/9007/src/pkg/net/iprawsock_posix.go File src/pkg/net/iprawsock_posix.go (right): http://codereview.appspot.com/6426047/diff/9007/src/pkg/net/iprawsock_posix.g... src/pkg/net/iprawsock_posix.go:85: return n, addr, err return$ http://codereview.appspot.com/6426047/diff/9007/src/pkg/net/iprawsock_posix.g... src/pkg/net/iprawsock_posix.go:146: func (c *IPConn) WriteMsgIP(b, oob []byte, addr *IPAddr) (n, oobn int, err error) { I think this function would be clearer without named return values. if !c.ok() { return 0, 0, syscall.EINVAL } sa, err := addr.sockaddr(c.fd.family) if err != nil { return 0, 0, &OpError{"write", c.fd.net, addr, err} } return c.fd.WriteMsg(b, oob, sa) http://codereview.appspot.com/6426047/diff/9007/src/pkg/net/udpsock_posix.go File src/pkg/net/udpsock_posix.go (right): http://codereview.appspot.com/6426047/diff/9007/src/pkg/net/udpsock_posix.go#... src/pkg/net/udpsock_posix.go:152: return c.fd.WriteMsg(b, oob, sa) ditto
Sign in to reply to this message.
all done.
Sign in to reply to this message.
Hello rsc@golang.org, r@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2012/08/28 03:24:36, mikio wrote: > Hello mailto:rsc@golang.org, mailto:r@golang.org, mailto:dave@cheney.net (cc: > mailto:golang-dev@googlegroups.com), > > Please take another look. Two minor comments. Please also include a link to the CL which uses these new methods.
Sign in to reply to this message.
http://codereview.appspot.com/6426047/diff/37001/src/pkg/net/iprawsock_posix.go File src/pkg/net/iprawsock_posix.go (right): http://codereview.appspot.com/6426047/diff/37001/src/pkg/net/iprawsock_posix.... src/pkg/net/iprawsock_posix.go:146: func (c *IPConn) WriteMsgIP(b, oob []byte, addr *IPAddr) (n, oobn int, err error) { These return arguments are never set within the method, I don't think they are helpful. http://codereview.appspot.com/6426047/diff/37001/src/pkg/net/udpsock_posix.go File src/pkg/net/udpsock_posix.go (right): http://codereview.appspot.com/6426047/diff/37001/src/pkg/net/udpsock_posix.go... src/pkg/net/udpsock_posix.go:140: func (c *UDPConn) WriteMsgUDP(b, oob []byte, addr *UDPAddr) (n, oobn int, err error) { Same as WriteMsgIP
Sign in to reply to this message.
On 2012/09/03 11:33:49, dfc wrote: > http://codereview.appspot.com/6426047/diff/37001/src/pkg/net/iprawsock_posix.... > src/pkg/net/iprawsock_posix.go:146: func (c *IPConn) WriteMsgIP(b, oob []byte, > addr *IPAddr) (n, oobn int, err error) { > These return arguments are never set within the method, I don't think they are > helpful. just for godoc, readability, doesn't make sense?
Sign in to reply to this message.
LGTM. As this is an addition to the public API, I suggest waiting for one of the authors. http://codereview.appspot.com/6426047/diff/37001/src/pkg/net/iprawsock_posix.go File src/pkg/net/iprawsock_posix.go (right): http://codereview.appspot.com/6426047/diff/37001/src/pkg/net/iprawsock_posix.... src/pkg/net/iprawsock_posix.go:146: func (c *IPConn) WriteMsgIP(b, oob []byte, addr *IPAddr) (n, oobn int, err error) { On 2012/09/03 11:33:49, dfc wrote: > These return arguments are never set within the method, I don't think they are > helpful. Fair enough. My concern was the return values are never assigned, new values are always returned.
Sign in to reply to this message.
anyone? On Fri, Sep 7, 2012 at 6:22 PM, <dave@cheney.net> wrote: > LGTM. As this is an addition to the public API, I suggest waiting for > one of the authors.
Sign in to reply to this message.
ping. Are there any Authors who can approve the additions to the net pkg API.
Sign in to reply to this message.
The new functions are fine. The changes to the existing functions I don't quite understand yet. http://codereview.appspot.com/6426047/diff/37001/src/pkg/net/iprawsock_posix.go File src/pkg/net/iprawsock_posix.go (right): http://codereview.appspot.com/6426047/diff/37001/src/pkg/net/iprawsock_posix.... src/pkg/net/iprawsock_posix.go:77: if a := sockaddrToIP(sa); a != nil { I am very sorry for being so out of it. That said, I am confused about the relation between this CL and CL 6445105, which seem to be making contradictory changes this code. http://codereview.appspot.com/6426047/diff/37001/src/pkg/net/iprawsock_posix.... 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? http://codereview.appspot.com/6426047/diff/37001/src/pkg/net/iprawsock_posix.... src/pkg/net/iprawsock_posix.go:146: func (c *IPConn) WriteMsgIP(b, oob []byte, addr *IPAddr) (n, oobn int, err error) { I think having the named returns helps documentation. It's also consistent with the other WriteMsgXX functions. http://codereview.appspot.com/6426047/diff/37001/src/pkg/net/udpsock_posix.go File src/pkg/net/udpsock_posix.go (right): http://codereview.appspot.com/6426047/diff/37001/src/pkg/net/udpsock_posix.go... src/pkg/net/udpsock_posix.go:73: if a := sockaddrToUDP(sa); a != nil { Same question about IPv4-mapped v6 addresses. http://codereview.appspot.com/6426047/diff/37001/src/pkg/net/udpsock_posix.go... src/pkg/net/udpsock_posix.go:140: func (c *UDPConn) WriteMsgUDP(b, oob []byte, addr *UDPAddr) (n, oobn int, err error) { On 2012/09/03 11:33:49, dfc wrote: > Same as WriteMsgIP Same reply.
Sign in to reply to this message.
On 2012/09/11 01:02:51, rsc wrote: > http://codereview.appspot.com/6426047/diff/37001/src/pkg/net/iprawsock_posix.... > 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? Nope, hmm... this should be addressed in another CL.
Sign in to reply to this message.
Hello rsc@golang.org, r@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=96fde1b15506 *** net: add read, write message methods to IPConn, UDPConn Both methods allow to access the IP ancillary data through socket control messages. This CL is required for CL 6482044; go.net/ipv4: new package. R=rsc, r, dave CC=golang-dev http://codereview.appspot.com/6426047
Sign in to reply to this message.
|
