code review 7039044: syscall: simplify netlink sockets

syscall: simplify netlink sockets

[mikio, 2012-12-31 14:30:26]

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

[dave_cheney.net, 2013-01-02 03:47:55]

A few minor comments. This is a nice improvement, thank you. How is the test
coverage in this area, are all the changes you've made covered by unit tests ?

https://codereview.appspot.com/7039044/diff/4001/src/pkg/syscall/netlink_linu...
File src/pkg/syscall/netlink_linux.go (right):

https://codereview.appspot.com/7039044/diff/4001/src/pkg/syscall/netlink_linu...
src/pkg/syscall/netlink_linux.go:52: func NetlinkRIB(proto, family int) ([]byte,
error) {
would you please add some comments to this method, I don't think many people
understand the brainhurt that is the netlink api.

https://codereview.appspot.com/7039044/diff/4001/src/pkg/syscall/netlink_linu...
src/pkg/syscall/netlink_linux.go:63: seq := 1
var seq uint32 = 1

then you don't need the conversion below.

https://codereview.appspot.com/7039044/diff/4001/src/pkg/syscall/netlink_linu...
src/pkg/syscall/netlink_linux.go:71: nr, _, err := Recvfrom(s, rb, 0)
0 is a magic number, is it the number of bytes or the incoming port ?

https://codereview.appspot.com/7039044/diff/4001/src/pkg/syscall/netlink_linu...
src/pkg/syscall/netlink_linux.go:80: msgs, _ := ParseNetlinkMessage(rb)
can the error be safely ignored here ?

https://codereview.appspot.com/7039044/diff/4001/src/pkg/syscall/netlink_linu...
src/pkg/syscall/netlink_linux.go:95: goto done
could this be done with a labeled break ?

[rsc, 2013-01-07 04:56:43]

LGTM once Dave is happy.

[mikio, 2013-01-09 15:51:27]

> How is the test coverage in this area, are all the changes you've made covered
by unit tests?

sure, network interface tests in the net package covers all.

https://codereview.appspot.com/7039044/diff/4001/src/pkg/syscall/netlink_linu...
File src/pkg/syscall/netlink_linux.go (right):

https://codereview.appspot.com/7039044/diff/4001/src/pkg/syscall/netlink_linu...
src/pkg/syscall/netlink_linux.go:52: func NetlinkRIB(proto, family int) ([]byte,
error) {
once i tried but failed. it's still hard to me.
will try someday not only netlink but routing sockets.

https://codereview.appspot.com/7039044/diff/4001/src/pkg/syscall/netlink_linu...
src/pkg/syscall/netlink_linux.go:63: seq := 1
looks seq isn't necessary, will drop.

https://codereview.appspot.com/7039044/diff/4001/src/pkg/syscall/netlink_linu...
src/pkg/syscall/netlink_linux.go:71: nr, _, err := Recvfrom(s, rb, 0)
nope, it's flags.

https://codereview.appspot.com/7039044/diff/4001/src/pkg/syscall/netlink_linu...
src/pkg/syscall/netlink_linux.go:80: msgs, _ := ParseNetlinkMessage(rb)
good catch, thx.

https://codereview.appspot.com/7039044/diff/4001/src/pkg/syscall/netlink_linu...
src/pkg/syscall/netlink_linux.go:95: goto done
if you prefer break instead of goto, hm, will do.

[mikio, 2013-01-09 15:52:15]

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

Please take another look.

[dave_cheney.net, 2013-01-12 10:20:48]

LGTM. Some very minor comments.

https://codereview.appspot.com/7039044/diff/10001/src/pkg/syscall/netlink_lin...
File src/pkg/syscall/netlink_linux.go (right):

https://codereview.appspot.com/7039044/diff/10001/src/pkg/syscall/netlink_lin...
src/pkg/syscall/netlink_linux.go:90: if m.Header.Seq != uint32(1) ||
m.Header.Pid != v.Pid {
you don't need the conversion here

https://codereview.appspot.com/7039044/diff/10001/src/pkg/syscall/netlink_lin...
src/pkg/syscall/netlink_linux.go:107: // A NetlinkMessage represents a netlink
message.
I think this reads better as

// NetlinkMessage represents a netlink message.

https://codereview.appspot.com/7039044/diff/10001/src/pkg/syscall/netlink_lin...
src/pkg/syscall/netlink_linux.go:122: m := NetlinkMessage{}
m := NetlinkMessage{
    Header: *h,
    Data: ... // and so forth
}

https://codereview.appspot.com/7039044/diff/10001/src/pkg/syscall/netlink_lin...
src/pkg/syscall/netlink_linux.go:166: ra := NetlinkRouteAttr{}
ra := NetlinkRouteAddr{
    Attr: *a
    // and so forth
}

[mikio, 2013-01-14 10:29:14]

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

syscall: simplify netlink sockets

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

https://codereview.appspot.com/7039044/diff/10001/src/pkg/syscall/netlink_lin...
File src/pkg/syscall/netlink_linux.go (right):

https://codereview.appspot.com/7039044/diff/10001/src/pkg/syscall/netlink_lin...
src/pkg/syscall/netlink_linux.go:90: if m.Header.Seq != uint32(1) ||
m.Header.Pid != v.Pid {
On 2013/01/12 10:20:48, dfc wrote:
> you don't need the conversion here

Done.

https://codereview.appspot.com/7039044/diff/10001/src/pkg/syscall/netlink_lin...
src/pkg/syscall/netlink_linux.go:107: // A NetlinkMessage represents a netlink
message.
On 2013/01/12 10:20:48, dfc wrote:
> I think this reads better as
> 
> // NetlinkMessage represents a netlink message.

Done.

https://codereview.appspot.com/7039044/diff/10001/src/pkg/syscall/netlink_lin...
src/pkg/syscall/netlink_linux.go:122: m := NetlinkMessage{}
On 2013/01/12 10:20:48, dfc wrote:
> m := NetlinkMessage{
>     Header: *h,
>     Data: ... // and so forth
> }

Done.

https://codereview.appspot.com/7039044/diff/10001/src/pkg/syscall/netlink_lin...
src/pkg/syscall/netlink_linux.go:166: ra := NetlinkRouteAttr{}
On 2013/01/12 10:20:48, dfc wrote:
> ra := NetlinkRouteAddr{
>     Attr: *a
>     // and so forth
> }

Done.