code review 4539084: syscall: add routing messages support for BSD variants

syscall: add routing messages support for BSD variants

[mikio, 2011-05-25 10:24:00]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-05-25 11:47:31]

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go
File src/pkg/syscall/route_bsd.go (right):

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:13: var darwinAMD64 = OS == "darwin" && ARCH ==
"amd64"
this could even be a const

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:21: salign = salign / 2
salign = 4

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:23: if salen == 0 {
Seems like the standard code below will do this anyway

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:31: anyMessageLen = unsafe.Sizeof(_anymsg)
unsafe.Sizeof(anyMessage{})
should work
?

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:34: const rtaMask = (RTA_IFA | RTA_NETMASK |
RTA_BRD)
drop ()

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:36: var rtaxMask = map[int]int{
weird to replace a shift with a map lookup
why not just use 1<<x below?

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:76: type RoutingMessage interface{}
Looks like things get slightly simpler if you make this

type RoutingMessage interface {
    sockaddr() []Sockaddr
}

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:159: if rtaMask&int(m.Header.Addrs)&rtaxMask[i] ==
0 {
If you make rtaxMask[i] 1<<i then you don't need the int().

for i := uint(0); i < RTAX_MAX; i++ {
    if m.Header.Addrs&rtaMask&(1<<i) == 0 {

I'm a little confused though.  Is it safe to "continue" here?
Does buf need to be adjusted first?

[mikio, 2011-05-26 09:05:19]

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go
File src/pkg/syscall/route_bsd.go (right):

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:13: var darwinAMD64 = OS == "darwin" && ARCH ==
"amd64"
On 2011/05/25 11:47:31, rsc wrote:
> this could even be a const

Done.

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:21: salign = salign / 2
On 2011/05/25 11:47:31, rsc wrote:
> salign = 4

Done.

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:23: if salen == 0 {
On 2011/05/25 11:47:31, rsc wrote:
> Seems like the standard code below will do this anyway

That's the way of the world...

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:31: anyMessageLen = unsafe.Sizeof(_anymsg)
On 2011/05/25 11:47:31, rsc wrote:
> unsafe.Sizeof(anyMessage{})
> should work
> ?

Done.

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:34: const rtaMask = (RTA_IFA | RTA_NETMASK |
RTA_BRD)
On 2011/05/25 11:47:31, rsc wrote:
> drop () 

Done.

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:36: var rtaxMask = map[int]int{
On 2011/05/25 11:47:31, rsc wrote:
> weird to replace a shift with a map lookup
> why not just use 1<<x below?

Thanks, removed.

Done.

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:76: type RoutingMessage interface{}
On 2011/05/25 11:47:31, rsc wrote:
> Looks like things get slightly simpler if you make this
> 
> type RoutingMessage interface {
>     sockaddr() []Sockaddr
> }

Thank you for the good suggestion.
 
Done.

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:159: if rtaMask&int(m.Header.Addrs)&rtaxMask[i] ==
0 {
On 2011/05/25 11:47:31, rsc wrote:

> for i := uint(0); i < RTAX_MAX; i++ {
>     if m.Header.Addrs&rtaMask&(1<<i) == 0 {

Captured.

> I'm a little confused though.  Is it safe to "continue" here?

I think so, yes.

> Does buf need to be adjusted first?

I guess no.

This CL already passed the test with CL4437087 and I took 
a look around BSD kernel code and concluded that kernel 
just packs a set of sockaddrs up as is; if a bit is on 
a value exists, if a bit is off there's no value, and we 
must skip that to avoid spurious access crash.

Done.

[mikio, 2011-05-26 09:06:21]

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

Please take another look.

[mikio, 2011-05-26 16:28:56]

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

Please take another look.

[rsc, 2011-05-26 21:04:07]

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go
File src/pkg/syscall/route_bsd.go (right):

http://codereview.appspot.com/4539084/diff/11003/src/pkg/syscall/route_bsd.go...
src/pkg/syscall/route_bsd.go:23: if salen == 0 {
On 2011/05/26 09:05:19, mikioh wrote:
> On 2011/05/25 11:47:31, rsc wrote:
> > Seems like the standard code below will do this anyway
> 
> That's the way of the world...

I don't understand.  I am saying "delete this statement".

[mikio, 2011-05-26 22:20:21]

On Fri, May 27, 2011 at 6:04 AM,  <rsc@golang.org> wrote:

>> > Seems like the standard code below will do this anyway
>
>> That's the way of the world...
>
> I don't understand.  I am saying "delete this statement".

Looks like I am confused.

You are saying that even if the kernel makes some strange padding
like RawSockaddr{Len: 0, Family 0}, the standard code can skip this
because go runtime or complier provides well-aligned memory access,
right?

[rsc, 2011-05-26 22:54:04]

Never mind, LGTM.

I thought the code said

if salen == 0 {
    return salen
}

[mikio, 2011-05-26 23:59:29]

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

Please take another look.

[rsc, 2011-05-27 00:02:07]

*** Submitted as http://code.google.com/p/go/source/detail?r=6f3b1a30a344 ***

syscall: add routing messages support for BSD variants

R=rsc
CC=golang-dev
http://codereview.appspot.com/4539084

Committer: Russ Cox <rsc@golang.org>

[mikio, 2011-05-27 00:58:05]

On Fri, May 27, 2011 at 7:53 AM, Russ Cox <rsc@golang.org> wrote:

> I thought the code said
>
> if salen == 0 {
>    return salen
> }

Ah, my bad. It's a bit hard to find out a slightly difference btw
salen and salign.

<http://www.mrc-cbu.cam.ac.uk/people/matt.davis/Cmabrigde/>

If someone gives me a nice and appropriate name generator,
I will treat a pint of ale, of course with fish and chips.

-- Mikio