code review 13368044: net: add netaddr interface

net: add netaddr interface

This CL adds the netaddr interface that will carry a single network
endpoint address or a short list of IP addresses to dial helper
functions in the upcoming CLs.

This is in preparation for TCP connection setup with fast failover on
dual IP stack node as described in RFC 6555.

Update issue 3610
Update issue 5267

[mikio, 2013-08-29 01:10:40]

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

[bradfitz, 2013-08-29 04:22:58]

https://codereview.appspot.com/13368044/diff/6001/src/pkg/net/ipsock.go
File src/pkg/net/ipsock.go (right):

https://codereview.appspot.com/13368044/diff/6001/src/pkg/net/ipsock.go#newco...
src/pkg/net/ipsock.go:42: func firstFavoriteAddr(filter func(IP) IP, addrs
[]string, inetaddr func(IP) netaddr) netaddr {
To be clear, we have now have three things that can be nil:

- a nil *TCPAddr (or *UDPAddr, etc)
- a nil netaddr (from firstFavoriteAddr)
- a nil Addr (from netAddr().toAddr())

That's a lot of nil checking.

It would be nice if firstFavoriteAddr (and firstSupportedAddr) always returned a
non-nil interface, so you could always call ".toAddr()" on it.  I left a comment
about this earlier, but it was probably not clear.  (I suggested using a
multiAddr(nil) or something)

If you actually thought callers might care early that toAddr() would return nil
before calling it, you could have a sentinel value for a netaddr that will
fail.:

var noAddr netaddr = toAddrNiller{}
type toAddrNiller struct{}
func (toAddrNiller) toAddr() Addr { return nil }

Or something.

https://codereview.appspot.com/13368044/diff/6001/src/pkg/net/ipsock.go#newco...
src/pkg/net/ipsock.go:191: // resolveInternetAddr resolves addr that is either a
literal IP
this doesn't say whether this can return (nil, nil) or not.

the two cases where you return nil it says "should not happen".  Let's make
those panic instead for now (we can catch during the stabilization period) with
a comment, and then document that this never returns a non-nil netaddr or an
error.

[mikio, 2013-08-29 05:27:55]

On Thu, Aug 29, 2013 at 1:22 PM,  <bradfitz@golang.org> wrote:

> To be clear, we have now have three things that can be nil:
>
> - a nil *TCPAddr (or *UDPAddr, etc)
> - a nil netaddr (from firstFavoriteAddr)
> - a nil Addr (from netAddr().toAddr())
>
> That's a lot of nil checking.
>
> It would be nice if firstFavoriteAddr (and firstSupportedAddr) always
> returned a non-nil interface, so you could always call ".toAddr()" on
> it.  I left a comment about this earlier, but it was probably not clear.
>  (I suggested using a multiAddr(nil) or something)

How about returning an error instead? We need some signal that indicates
"no suitable address found" for the customers of resolveInternetAddr and
it might be explicit rather that returning a non-nil interface.

PS:

- a nil *TCPAddr (or *UDPAddr, etc) // a protocol address
- a nil Addr (from netaddr.toAddr()) // an address bearer for the API users
- a nil netaddr (from firstFavoriteAddr) // an internal address bearer
- a nil sockaddr // a bridge btw Addr and syscall.Sockaddr
- a nil syscall.Sockaddr (from sockaddr.sockaddr()) // a syscall address bearer

> That's a lot of nil checking.

agreed.

[bradfitz, 2013-08-29 05:46:27]

An error is okay too. Just not (nil, nil)
On Aug 28, 2013 10:27 PM, "Mikio Hara" <mikioh.mikioh@gmail.com> wrote:

> On Thu, Aug 29, 2013 at 1:22 PM,  <bradfitz@golang.org> wrote:
>
> > To be clear, we have now have three things that can be nil:
> >
> > - a nil *TCPAddr (or *UDPAddr, etc)
> > - a nil netaddr (from firstFavoriteAddr)
> > - a nil Addr (from netAddr().toAddr())
> >
> > That's a lot of nil checking.
> >
> > It would be nice if firstFavoriteAddr (and firstSupportedAddr) always
> > returned a non-nil interface, so you could always call ".toAddr()" on
> > it.  I left a comment about this earlier, but it was probably not clear.
> >  (I suggested using a multiAddr(nil) or something)
>
> How about returning an error instead? We need some signal that indicates
> "no suitable address found" for the customers of resolveInternetAddr and
> it might be explicit rather that returning a non-nil interface.
>
> PS:
>
> - a nil *TCPAddr (or *UDPAddr, etc) // a protocol address
> - a nil Addr (from netaddr.toAddr()) // an address bearer for the API users
> - a nil netaddr (from firstFavoriteAddr) // an internal address bearer
> - a nil sockaddr // a bridge btw Addr and syscall.Sockaddr
> - a nil syscall.Sockaddr (from sockaddr.sockaddr()) // a syscall address
> bearer
>
> > That's a lot of nil checking.
>
> agreed.
>

[mikio, 2013-08-29 05:55:02]

gotcha+ptal

https://codereview.appspot.com/13368044/diff/6001/src/pkg/net/ipsock.go
File src/pkg/net/ipsock.go (right):

https://codereview.appspot.com/13368044/diff/6001/src/pkg/net/ipsock.go#newco...
src/pkg/net/ipsock.go:42: func firstFavoriteAddr(filter func(IP) IP, addrs
[]string, inetaddr func(IP) netaddr) netaddr {
On 2013/08/29 04:22:58, bradfitz wrote:
> To be clear, we have now have three things that can be nil:
> 
> - a nil *TCPAddr (or *UDPAddr, etc)
> - a nil netaddr (from firstFavoriteAddr)
> - a nil Addr (from netAddr().toAddr())
> 
> That's a lot of nil checking.
> 
> It would be nice if firstFavoriteAddr (and firstSupportedAddr) always returned
a
> non-nil interface, so you could always call ".toAddr()" on it.  I left a
comment
> about this earlier, but it was probably not clear.  (I suggested using a
> multiAddr(nil) or something)
> 
> If you actually thought callers might care early that toAddr() would return
nil
> before calling it, you could have a sentinel value for a netaddr that will
> fail.:
> 
> var noAddr netaddr = toAddrNiller{}
> type toAddrNiller struct{}
> func (toAddrNiller) toAddr() Addr { return nil }
> 
> Or something.
> 

Done.

https://codereview.appspot.com/13368044/diff/6001/src/pkg/net/ipsock.go#newco...
src/pkg/net/ipsock.go:191: // resolveInternetAddr resolves addr that is either a
literal IP
On 2013/08/29 04:22:58, bradfitz wrote:
> this doesn't say whether this can return (nil, nil) or not.
> 
> the two cases where you return nil it says "should not happen".  Let's make
> those panic instead for now (we can catch during the stabilization period)
with
> a comment, and then document that this never returns a non-nil netaddr or an
> error.

Done.

[bradfitz, 2013-08-29 19:02:49]

LGTM



On Wed, Aug 28, 2013 at 10:55 PM, <mikioh.mikioh@gmail.com> wrote:

> gotcha+ptal
>
>
>
> https://codereview.appspot.**com/13368044/diff/6001/src/**
>
pkg/net/ipsock.go<https://codereview.appspot.com/13368044/diff/6001/src/pkg/net/ipsock.go>
> File src/pkg/net/ipsock.go (right):
>
> https://codereview.appspot.**com/13368044/diff/6001/src/**
>
pkg/net/ipsock.go#newcode42<https://codereview.appspot.com/13368044/diff/6001/src/pkg/net/ipsock.go#newcode42>
> src/pkg/net/ipsock.go:42: func firstFavoriteAddr(filter func(IP) IP,
> addrs []string, inetaddr func(IP) netaddr) netaddr {
> On 2013/08/29 04:22:58, bradfitz wrote:
>
>> To be clear, we have now have three things that can be nil:
>>
>
>  - a nil *TCPAddr (or *UDPAddr, etc)
>> - a nil netaddr (from firstFavoriteAddr)
>> - a nil Addr (from netAddr().toAddr())
>>
>
>  That's a lot of nil checking.
>>
>
>  It would be nice if firstFavoriteAddr (and firstSupportedAddr) always
>>
> returned a
>
>> non-nil interface, so you could always call ".toAddr()" on it.  I left
>>
> a comment
>
>> about this earlier, but it was probably not clear.  (I suggested using
>>
> a
>
>> multiAddr(nil) or something)
>>
>
>  If you actually thought callers might care early that toAddr() would
>>
> return nil
>
>> before calling it, you could have a sentinel value for a netaddr that
>>
> will
>
>> fail.:
>>
>
>  var noAddr netaddr = toAddrNiller{}
>> type toAddrNiller struct{}
>> func (toAddrNiller) toAddr() Addr { return nil }
>>
>
>  Or something.
>>
>
>
> Done.
>
>
> https://codereview.appspot.**com/13368044/diff/6001/src/**
>
pkg/net/ipsock.go#newcode191<https://codereview.appspot.com/13368044/diff/6001/src/pkg/net/ipsock.go#newcode191>
> src/pkg/net/ipsock.go:191: // resolveInternetAddr resolves addr that is
> either a literal IP
> On 2013/08/29 04:22:58, bradfitz wrote:
>
>> this doesn't say whether this can return (nil, nil) or not.
>>
>
>  the two cases where you return nil it says "should not happen".  Let's
>>
> make
>
>> those panic instead for now (we can catch during the stabilization
>>
> period) with
>
>> a comment, and then document that this never returns a non-nil netaddr
>>
> or an
>
>> error.
>>
>
> Done.
>
>
https://codereview.appspot.**com/13368044/<https://codereview.appspot.com/133...
>

[mikio, 2013-08-30 00:09:51]

*** Submitted as https://code.google.com/p/go/source/detail?r=87e254f60143 ***

net: add netaddr interface

This CL adds the netaddr interface that will carry a single network
endpoint address or a short list of IP addresses to dial helper
functions in the upcoming CLs.

This is in preparation for TCP connection setup with fast failover on
dual IP stack node as described in RFC 6555.

Update issue 3610
Update issue 5267

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/13368044