code review 10319043: go.net/ipv4: make use of net.PacketConn embedding

go.net/ipv4: make use of net.PacketConn embedding

[mikio, 2013-06-16 07:36:40]

Hello dave@cheney.net (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.net

[dave_cheney.net, 2013-06-16 07:40:24]

Very nice. Please add a test to ensure we don't break the contract

var _ net.PacketConn = &PacketConn{} // should do

On Sun, Jun 16, 2013 at 5:36 PM,  <mikioh.mikioh@gmail.com> wrote:
> Reviewers: dfc,
>
> Message:
> Hello dave@cheney.net (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go.net
>
>
> Description:
> go.net/ipv4: make use of net.PacketConn embedding
>
> Please review this at https://codereview.appspot.com/10319043/
>
> Affected files:
>   M ipv4/endpoint.go
>   M ipv4/helper_plan9.go
>   M ipv4/helper_unix.go
>   M ipv4/helper_windows.go
>   M ipv4/payload.go
>
>
> Index: ipv4/endpoint.go
> ===================================================================
> --- a/ipv4/endpoint.go
> +++ b/ipv4/endpoint.go
> @@ -18,15 +18,15 @@
>  }
>
>  type genericOpt struct {
> -       c net.Conn
> +       net.Conn
>  }
>
> -func (c *genericOpt) ok() bool { return c != nil && c.c != nil }
> +func (c *genericOpt) ok() bool { return c != nil && c.Conn != nil }
>
>  // NewConn returns a new Conn.
>  func NewConn(c net.Conn) *Conn {
>         return &Conn{
> -               genericOpt: genericOpt{c},
> +               genericOpt: genericOpt{Conn: c},
>         }
>  }
>
> @@ -42,10 +42,10 @@
>  }
>
>  type dgramOpt struct {
> -       c net.PacketConn
> +       net.PacketConn
>  }
>
> -func (c *dgramOpt) ok() bool { return c != nil && c.c != nil }
> +func (c *dgramOpt) ok() bool { return c != nil && c.PacketConn != nil }
>
>  // SetControlMessage sets the per packet IP-level socket options.
>  func (c *PacketConn) SetControlMessage(cf ControlFlags, on bool) error {
> @@ -65,7 +65,7 @@
>         if !c.payloadHandler.ok() {
>                 return syscall.EINVAL
>         }
> -       return c.payloadHandler.c.SetDeadline(t)
> +       return c.payloadHandler.PacketConn.SetDeadline(t)
>  }
>
>  // SetReadDeadline sets the read deadline associated with the
> @@ -74,7 +74,7 @@
>         if !c.payloadHandler.ok() {
>                 return syscall.EINVAL
>         }
> -       return c.payloadHandler.c.SetReadDeadline(t)
> +       return c.payloadHandler.PacketConn.SetReadDeadline(t)
>  }
>
>  // SetWriteDeadline sets the write deadline associated with the
> @@ -83,7 +83,7 @@
>         if !c.payloadHandler.ok() {
>                 return syscall.EINVAL
>         }
> -       return c.payloadHandler.c.SetWriteDeadline(t)
> +       return c.payloadHandler.PacketConn.SetWriteDeadline(t)
>  }
>
>  // Close closes the endpoint.
> @@ -91,16 +91,16 @@
>         if !c.payloadHandler.ok() {
>                 return syscall.EINVAL
>         }
> -       return c.payloadHandler.c.Close()
> +       return c.payloadHandler.PacketConn.Close()
>  }
>
>  // NewPacketConn returns a new PacketConn using c as its underlying
>  // transport.
>  func NewPacketConn(c net.PacketConn) *PacketConn {
>         return &PacketConn{
> -               genericOpt:     genericOpt{c.(net.Conn)},
> -               dgramOpt:       dgramOpt{c},
> -               payloadHandler: payloadHandler{c: c},
> +               genericOpt:     genericOpt{Conn: c.(net.Conn)},
> +               dgramOpt:       dgramOpt{PacketConn: c},
> +               payloadHandler: payloadHandler{PacketConn: c},
>         }
>  }
>
> @@ -166,8 +166,8 @@
>  // transport.
>  func NewRawConn(c net.PacketConn) (*RawConn, error) {
>         r := &RawConn{
> -               genericOpt:    genericOpt{c.(net.Conn)},
> -               dgramOpt:      dgramOpt{c},
> +               genericOpt:    genericOpt{Conn: c.(net.Conn)},
> +               dgramOpt:      dgramOpt{PacketConn: c},
>                 packetHandler: packetHandler{c: c.(*net.IPConn)},
>         }
>         fd, err := r.packetHandler.sysfd()
> Index: ipv4/helper_plan9.go
> ===================================================================
> --- a/ipv4/helper_plan9.go
> +++ b/ipv4/helper_plan9.go
> @@ -4,9 +4,7 @@
>
>  package ipv4
>
> -import (
> -       "syscall"
> -)
> +import "syscall"
>
>  func (c *genericOpt) sysfd() (int, error) {
>         // TODO(mikio): Implement this
> Index: ipv4/helper_unix.go
> ===================================================================
> --- a/ipv4/helper_unix.go
> +++ b/ipv4/helper_unix.go
> @@ -12,7 +12,7 @@
>  )
>
>  func (c *genericOpt) sysfd() (int, error) {
> -       switch p := c.c.(type) {
> +       switch p := c.Conn.(type) {
>         case *net.TCPConn, *net.UDPConn, *net.IPConn:
>                 return sysfd(p)
>         }
> @@ -20,7 +20,7 @@
>  }
>
>  func (c *dgramOpt) sysfd() (int, error) {
> -       switch p := c.c.(type) {
> +       switch p := c.PacketConn.(type) {
>         case *net.UDPConn, *net.IPConn:
>                 return sysfd(p.(net.Conn))
>         }
> @@ -28,7 +28,7 @@
>  }
>
>  func (c *payloadHandler) sysfd() (int, error) {
> -       return sysfd(c.c.(net.Conn))
> +       return sysfd(c.PacketConn.(net.Conn))
>  }
>
>  func (c *packetHandler) sysfd() (int, error) {
> Index: ipv4/helper_windows.go
> ===================================================================
> --- a/ipv4/helper_windows.go
> +++ b/ipv4/helper_windows.go
> @@ -11,7 +11,7 @@
>  )
>
>  func (c *genericOpt) sysfd() (syscall.Handle, error) {
> -       switch p := c.c.(type) {
> +       switch p := c.Conn.(type) {
>         case *net.TCPConn, *net.UDPConn, *net.IPConn:
>                 return sysfd(p)
>         }
> @@ -19,7 +19,7 @@
>  }
>
>  func (c *dgramOpt) sysfd() (syscall.Handle, error) {
> -       switch p := c.c.(type) {
> +       switch p := c.PacketConn.(type) {
>         case *net.UDPConn, *net.IPConn:
>                 return sysfd(p.(net.Conn))
>         }
> @@ -27,7 +27,7 @@
>  }
>
>  func (c *payloadHandler) sysfd() (syscall.Handle, error) {
> -       return sysfd(c.c.(net.Conn))
> +       return sysfd(c.PacketConn.(net.Conn))
>  }
>
>  func (c *packetHandler) sysfd() (syscall.Handle, error) {
> Index: ipv4/payload.go
> ===================================================================
> --- a/ipv4/payload.go
> +++ b/ipv4/payload.go
> @@ -11,11 +11,11 @@
>
>  // A payloadHandler represents the IPv4 datagram payload handler.
>  type payloadHandler struct {
> -       c net.PacketConn
> +       net.PacketConn
>         rawOpt
>  }
>
> -func (c *payloadHandler) ok() bool { return c != nil && c.c != nil }
> +func (c *payloadHandler) ok() bool { return c != nil && c.PacketConn != nil
> }
>
>  // ReadFrom reads a payload of the received IPv4 datagram, from the
>  // endpoint c, copying the payload into b.  It returns the number of
> @@ -27,14 +27,14 @@
>         }
>         oob := newControlMessage(&c.rawOpt)
>         var oobn int
> -       switch rd := c.c.(type) {
> +       switch c := c.PacketConn.(type) {
>         case *net.UDPConn:
> -               if n, oobn, _, src, err = rd.ReadMsgUDP(b, oob); err != nil
> {
> +               if n, oobn, _, src, err = c.ReadMsgUDP(b, oob); err != nil {
>                         return 0, nil, nil, err
>                 }
>         case *net.IPConn:
>                 nb := make([]byte, maxHeaderLen+len(b))
> -               if n, oobn, _, src, err = rd.ReadMsgIP(nb, oob); err != nil
> {
> +               if n, oobn, _, src, err = c.ReadMsgIP(nb, oob); err != nil {
>                         return 0, nil, nil, err
>                 }
>                 hdrlen := int(nb[0]&0x0f) << 2
> @@ -66,11 +66,11 @@
>         if dst == nil {
>                 return 0, errMissingAddress
>         }
> -       switch wr := c.c.(type) {
> +       switch c := c.PacketConn.(type) {
>         case *net.UDPConn:
> -               n, _, err = wr.WriteMsgUDP(b, oob, dst.(*net.UDPAddr))
> +               n, _, err = c.WriteMsgUDP(b, oob, dst.(*net.UDPAddr))
>         case *net.IPConn:
> -               n, _, err = wr.WriteMsgIP(b, oob, dst.(*net.IPAddr))
> +               n, _, err = c.WriteMsgIP(b, oob, dst.(*net.IPAddr))
>         default:
>                 return 0, errInvalidConnType
>         }
>
>

[mikio, 2013-06-16 07:49:04]

> var _ net.PacketConn = &PacketConn{} // should do

var _ net.PacketConn = &dgramOpt{} ?

> Very nice. Please add a test to ensure we don't break the contract

Ah, sorry not decided yet.
This CL just tweaks around endpoint types for that.

[dave_cheney.net, 2013-06-16 12:21:11]

LGTM. Very nice.

[mikio, 2013-06-16 15:37:27]

*** Submitted as
https://code.google.com/p/go/source/detail?r=5a65a7af55c6&repo=net ***

go.net/ipv4: make use of net.PacketConn embedding

R=dave
CC=golang-dev
https://codereview.appspot.com/10319043