code review 7352045: net: return correct point-to-point interface address on...

net: return correct point-to-point interface address on linux

On Linux point-to-point interface an IFA_ADDRESS attribute
represents a peer address. For a correct interface address
we should take an IFA_LOCAL attribute instead.

Fixes issue 4839.

[mikio, 2013-02-18 14:51:23]

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

[dfc, 2013-02-19 01:54:12]

Is there a way I can test this manually?

[mikio, 2013-02-19 03:13:48]

On Tue, Feb 19, 2013 at 10:54 AM,  <dave@cheney.net> wrote:

> Is there a way I can test this manually?

I used pptp as the same environment of the reporter,
but you can use any logical interface stuff.

Here you go, http://play.golang.org/p/eC8nAtCe26.

[rsc, 2013-02-19 16:00:47]

LGTM

[mikio, 2013-02-19 22:31:54]

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

net: return correct point-to-point interface address on linux

On Linux point-to-point interface an IFA_ADDRESS attribute
represents a peer address. For a correct interface address
we should take an IFA_LOCAL attribute instead.

Fixes issue 4839.

R=golang-dev, dave, rsc
CC=golang-dev
https://codereview.appspot.com/7352045

[gpaul, 2013-02-21 07:46:09]

This CL seems to have slowed iface.Addrs() down quite a bit:

From 9c3930413c1b
To 53a4da6a4f4a

benchmark         old ns/op    new ns/op    delta
BenchmarkAddrs        55663     92844026  +166696.66%

func BenchmarkAddrs(b *testing.B) {
	eth0, err := net.InterfaceByName("eth0")
	if err != nil {
		panic(err)
	}
	for i:=0; i<b.N; i++ {
		eth0.Addrs()
	}
}


On 2013/02/19 22:31:54, mikio wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=53a4da6a4f4a ***
> 
> net: return correct point-to-point interface address on linux
> 
> On Linux point-to-point interface an IFA_ADDRESS attribute
> represents a peer address. For a correct interface address
> we should take an IFA_LOCAL attribute instead.
> 
> Fixes issue 4839.
> 
> R=golang-dev, dave, rsc
> CC=golang-dev
> https://codereview.appspot.com/7352045

[dfc, 2013-02-21 08:57:09]

Bugger. Could you please propose a CL with your benchmark, and maybe open an
issue with the -cpuprofile output so we can find the culprit. 

Cheers

Dave

On 21/02/2013, at 18:46, gustav.paul@gmail.com wrote:

> This CL seems to have slowed iface.Addrs() down quite a bit:
> 
> From 9c3930413c1b
> To 53a4da6a4f4a
> 
> benchmark         old ns/op    new ns/op    delta
> BenchmarkAddrs        55663     92844026  +166696.66%
> 
> func BenchmarkAddrs(b *testing.B) {
>    eth0, err := net.InterfaceByName("eth0")
>    if err != nil {
>        panic(err)
>    }
>    for i:=0; i<b.N; i++ {
>        eth0.Addrs()
>    }
> }
> 
> 
> On 2013/02/19 22:31:54, mikio wrote:
>> *** Submitted as
> https://code.google.com/p/go/source/detail?r=53a4da6a4f4a ***
> 
>> net: return correct point-to-point interface address on linux
> 
>> On Linux point-to-point interface an IFA_ADDRESS attribute
>> represents a peer address. For a correct interface address
>> we should take an IFA_LOCAL attribute instead.
> 
>> Fixes issue 4839.
> 
>> R=golang-dev, dave, rsc
>> CC=golang-dev
>> https://codereview.appspot.com/7352045
> 
> 
> 
> https://codereview.appspot.com/7352045/

[mikio, 2013-02-21 10:37:13]

On Thu, Feb 21, 2013 at 4:46 PM,  <gustav.paul@gmail.com> wrote:

> This CL seems to have slowed iface.Addrs() down quite a bit:

yup, sorry for my laziness and thanks for your alert.
somehow i thought i already did memoize, will fix soon.