code review 12414043: net: pick a pair of different address family IP address...

net: pick a pair of different address family IP addresses for DNS registered name

*** This CL includes CL 12956050 for convenience ***

This CL makes resolveInternetAddr return a list of addresses that
contain a pair of different address family IP addresses if possible.

This is in preparation for simple Happy Eyeballs connection setup for
TCP on dual stack IP host as described in RFC 6555.

Update issue 3610
Update issue 5267

[mikio, 2013-08-03 15:48:12]

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

[mikio, 2013-08-07 03:38:49]

ping

[bradfitz, 2013-08-07 16:53:17]

I'm concerned that this introduces allocations for the common case (not
having multiple Adds) in resolveInternetAddr.  Instead of returning a
[]Addr from resolveInternetAddr, can't we just introduce a new private type
(multiAddr []Addr) implementing Addr, then check whether Addr is a
multiAddr in places where it matters?

Also, try to minimize the diff size.  Look at lookup.go for instance:  why
is stuff moving around?



On Sat, Aug 3, 2013 at 8:48 AM, <mikioh.mikioh@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> 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
>
>
> Description:
> net: pick a pair of different address family IP addresses for DNS
> registered name
>
> This CL makes resolveInternetAddr return a list of addresses that
> contain a pair of different address family IP addresses if possible.
>
> This is in preparation for happy eyeballs-like connection setup on dual
> stack IP host.
>
> Update issue 3610
> Update issue 5267
>
> Please review this at
https://codereview.appspot.**com/12414043/<https://codereview.appspot.com/124...
>
> Affected files:
>   M src/pkg/net/dial.go
>   M src/pkg/net/dial_gen.go
>   M src/pkg/net/fd_unix.go
>   M src/pkg/net/fd_windows.go
>   M src/pkg/net/iprawsock.go
>   M src/pkg/net/ipsock.go
>   M src/pkg/net/lookup.go
>   M src/pkg/net/tcpsock.go
>   M src/pkg/net/udpsock.go
>
>
> --
>
> ---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...
> .
>
>
>

[mikio, 2013-08-08 21:13:48]

PTAL

PTAL

On Thu, Aug 8, 2013 at 1:53 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> I'm concerned that this introduces allocations for the common case (not
> having multiple Adds) in resolveInternetAddr.  Instead of returning a []Addr
> from resolveInternetAddr, can't we just introduce a new private type
> (multiAddr []Addr) implementing Addr, then check whether Addr is a multiAddr
> in places where it matters?

I hesitated about introducing []Addr in Addr, such thing that makes
readers confuse, but less garbage allocation sounds good. Will go with
proposal.

> Also, try to minimize the diff size.  Look at lookup.go for instance:  why
> is stuff moving around?

Looks like lookup.go and other platform-dep files have some styles;
e.g., Host, IP, ... Addr and I just don't want to break that.

[mikio, 2013-08-12 21:54:30]

ping

[bradfitz, 2013-08-12 22:50:25]

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

https://codereview.appspot.com/12414043/diff/61001/src/pkg/net/ipsock.go#newc...
src/pkg/net/ipsock.go:43: switch ma[0].(type) {
how about just:

return "multi"

https://codereview.appspot.com/12414043/diff/61001/src/pkg/net/ipsock.go#newc...
src/pkg/net/ipsock.go:56: s += " "
var buf []byte

append to a []byte instead and return string(buf)

https://codereview.appspot.com/12414043/diff/61001/src/pkg/net/lookup.go
File src/pkg/net/lookup.go (right):

https://codereview.appspot.com/12414043/diff/61001/src/pkg/net/lookup.go#newc...
src/pkg/net/lookup.go:7: import "time"
please stop rearranging files.  your diffs are already too big.  this just makes
it even bigger.

rearrange this back to minimize the diff size.

If you want to rearrange things, send those as separate no-op CLs.

[mikio, 2013-08-13 00:11:47]

PTAL

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

https://codereview.appspot.com/12414043/diff/61001/src/pkg/net/ipsock.go#newc...
src/pkg/net/ipsock.go:43: switch ma[0].(type) {
On 2013/08/12 22:50:25, bradfitz wrote:
> how about just:
> 
> return "multi"

Done.

https://codereview.appspot.com/12414043/diff/61001/src/pkg/net/ipsock.go#newc...
src/pkg/net/ipsock.go:56: s += " "
On 2013/08/12 22:50:25, bradfitz wrote:
> var buf []byte
> 
> append to a []byte instead and return string(buf) 

Done.

https://codereview.appspot.com/12414043/diff/61001/src/pkg/net/lookup.go
File src/pkg/net/lookup.go (right):

https://codereview.appspot.com/12414043/diff/61001/src/pkg/net/lookup.go#newc...
src/pkg/net/lookup.go:7: import "time"
On 2013/08/12 22:50:25, bradfitz wrote:
> please stop rearranging files.  your diffs are already too big.  this just
makes
> it even bigger.
> 
> rearrange this back to minimize the diff size.
> 
> If you want to rearrange things, send those as separate no-op CLs.

Done.

[mikio, 2013-08-13 07:43:27]

Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-08-13 16:11:03]

Now the pilotAddr everywhere scares me.  I think it's too easy to forget one and
have a multiAddr escape outside the net package.

Maybe the type system could help us here.

// iaddr is an internal Addr. Its Addr is either a public type, or a multiAddr.
type iaddr struct {
    a Addr
}

func (ia iaddr) Addr() Addr { return pilotAddr(ia.a) }

Then it's the same size as an interface value, but won't let us accidentally
leak a multiAddr out of the package.

Sorry for the design feedback, but to be fair: you threw out a CL without having
a design discussion first, IIRC.

(Is "pilot addr" a term I should know?)

[mikio, 2013-08-13 23:48:25]

thanks for the feedback, will do so.

> Sorry for the design feedback, but to be fair: you threw out a CL without
having
> a design discussion first, IIRC.

yup, my bad. honestly, I've never imagined how far this CL will go.

> (Is "pilot addr" a term I should know?)

nope, alternatives welcome.
I google'd plain-old telco tech terms and found it, because looks like net API
uses such thing; e.g., Dial.

[mikio, 2013-08-14 20:24:15]

Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-08-14 20:39:14]

Looking nicer.

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

https://codereview.appspot.com/12414043/diff/87001/src/pkg/net/ipsock.go#newc...
src/pkg/net/ipsock.go:32: var nilIntAddr intAddr
I'd just delete this and replace it with intAddr{} everywhere.  It's zero, not
nil.

https://codereview.appspot.com/12414043/diff/87001/src/pkg/net/ipsock.go#newc...
src/pkg/net/ipsock.go:34: // An intAddr reprensets an internal address bearer. 
It contains
represents.

But let's change the comment:

// An intAddr protects its wrapped Addr type from escaping the net package
// without validation.
// The wrapped Addr is either of a public type (TCPAddr, etc) or is of type
"multiAddr",
// representing a list of multiple addresses.

https://codereview.appspot.com/12414043/diff/87001/src/pkg/net/ipsock.go#newc...
src/pkg/net/ipsock.go:40: // Addr returns the pilot address when it consists of
a list of
s/pilot/first/

https://codereview.appspot.com/12414043/diff/87001/src/pkg/net/ipsock.go#newc...
src/pkg/net/ipsock.go:41: // addresses.  Otherwise it returns an address.
s/an/the/

[mikio, 2013-08-14 21:17:17]

PTAL

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

https://codereview.appspot.com/12414043/diff/87001/src/pkg/net/ipsock.go#newc...
src/pkg/net/ipsock.go:32: var nilIntAddr intAddr
On 2013/08/14 20:39:14, bradfitz wrote:
> I'd just delete this and replace it with intAddr{} everywhere.  It's zero, not
> nil.

Done.

https://codereview.appspot.com/12414043/diff/87001/src/pkg/net/ipsock.go#newc...
src/pkg/net/ipsock.go:34: // An intAddr reprensets an internal address bearer. 
It contains
thanks.

https://codereview.appspot.com/12414043/diff/87001/src/pkg/net/ipsock.go#newc...
src/pkg/net/ipsock.go:40: // Addr returns the pilot address when it consists of
a list of
On 2013/08/14 20:39:14, bradfitz wrote:
> s/pilot/first/

Done.

https://codereview.appspot.com/12414043/diff/87001/src/pkg/net/ipsock.go#newc...
src/pkg/net/ipsock.go:41: // addresses.  Otherwise it returns an address.
On 2013/08/14 20:39:14, bradfitz wrote:
> s/an/the/

Done.

[rsc, 2013-08-16 01:17:34]

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

https://codereview.appspot.com/12414043/diff/99001/src/pkg/net/ipsock.go#newc...
src/pkg/net/ipsock.go:55: // be used as the API that implements Addr interface;
don't return
An easy way to ensure this would be to make multiAddr not implement Addr.
Why does it implement Addr?

That is, you could do

type addrLIst []Addr

func (a addrList) Addr() Addr {
    if len(a) == 0 {
        return nil
    }
    return a[0]
}

and addrList would be used everywhere intAddr currently is. It would replace
both intAddr and multiAddr.

[mikio, 2013-08-16 04:19:43]

aha, thx!

On Friday, August 16, 2013 10:17:34 AM UTC+9, rsc wrote:
>
>
> https://codereview.appspot.com/12414043/diff/99001/src/pkg/net/ipsock.go 
> File src/pkg/net/ipsock.go (right): 
>
>
https://codereview.appspot.com/12414043/diff/99001/src/pkg/net/ipsock.go#newc...

>
>
src/pkg/net/ipsock.go:55<https://codereview.appspot.com/12414043/diff/99001/src/pkg/net/ipsock.go#newcode55src/pkg/net/ipsock.go:55>:

> // be used as the API that implements Addr 
> interface; don't return 
> An easy way to ensure this would be to make multiAddr not implement 
> Addr. 
> Why does it implement Addr? 
>
> That is, you could do 
>
> type addrLIst []Addr 
>
> func (a addrList) Addr() Addr { 
>      if len(a) == 0 { 
>          return nil 
>      } 
>      return a[0] 
> } 
>
> and addrList would be used everywhere intAddr currently is. It would 
> replace both intAddr and multiAddr. 
>
> https://codereview.appspot.com/12414043/ 
>

[mikio, 2013-08-18 12:47:16]

PTAL

> Why does it implement Addr? 

right, this patch set adds new interface netaddr instead.
does s/multiAddr/addrList/, and make addrList and other protocol address types
implement netaddr interface.
of course, no new allocation for the common; single resolved address case.

[mikio, 2013-08-22 00:42:57]

ping

[bradfitz, 2013-08-22 01:10:37]

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

https://codereview.appspot.com/12414043/diff/108001/src/pkg/net/ipsock.go#new...
src/pkg/net/ipsock.go:92: return nil
shouldn't this be addrList(nil) so .Addr() on it returns a nil Addr, and doesn't
crash?

firstFavoriteAddr doesn't document that it can return nil.

[mikio, 2013-08-23 12:29:51]

PTAL

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

https://codereview.appspot.com/12414043/diff/108001/src/pkg/net/ipsock.go#new...
src/pkg/net/ipsock.go:92: return nil
> shouldn't this be addrList(nil) so .Addr() on it returns a nil Addr, and
doesn't
> crash?

resolveInternetAddr will take care, so no.

> firstFavoriteAddr doesn't document that it can return nil.

added doc.

[mikio, 2013-08-23 21:52:03]

ping

[dave_cheney.net, 2013-08-26 00:35:34]

On 2013/08/23 21:52:03, mikio wrote:
> ping

Hi Mikio,

I've tried to review this CL a few times but it is just to large for me to fit
in my head at the moment. I know I always ask this, but is it possible to split
it into a series of smaller changes so the intention can be clearly expressed
and understood by the reviewers.

Cheers

Dave

[mikio, 2013-08-26 02:11:08]

as you wish.

On Mon, Aug 26, 2013 at 9:35 AM,  <dave@cheney.net> wrote:
> On 2013/08/23 21:52:03, mikio wrote:
>>
>> ping
>
>
> Hi Mikio,
>
> I've tried to review this CL a few times but it is just to large for me
> to fit in my head at the moment. I know I always ask this, but is it
> possible to split it into a series of smaller changes so the intention
> can be clearly expressed and understood by the reviewers.
>
> Cheers
>
> Dave
>
> https://codereview.appspot.com/12414043/

[mikio, 2013-08-28 08:54:53]

*** Abandoned ***