code review 179510043: x/net/ipv4: add support for icmp filter on linux

x/net/ipv4: add support for icmp filter on linux

[mikio, 2014-12-02 03:26:36]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go.net

[mikio, 2014-12-02 03:29:46]

Hello iant@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[iant, 2014-12-02 03:44:42]

https://codereview.appspot.com/179510043/diff/410001/ipv4/icmp.go
File ipv4/icmp.go (right):

https://codereview.appspot.com/179510043/diff/410001/ipv4/icmp.go#newcode29
ipv4/icmp.go:29: // An ICMPFilter represents an ICMP message filter for incoming
Some doc comment, either here or for the Set method, needs to explain more. 
What does a filter do?

https://codereview.appspot.com/179510043/diff/410001/ipv4/icmp.go#newcode32
ipv4/icmp.go:32: mu sync.RWMutex
I suggest just using a sync.Mutex.  I don't see any need for the more complex
RWMutex.

https://codereview.appspot.com/179510043/diff/410001/ipv4/icmp_linux.go
File ipv4/icmp_linux.go (right):

https://codereview.appspot.com/179510043/diff/410001/ipv4/icmp_linux.go#newcode9
ipv4/icmp_linux.go:9: f.Data |= 1 << (uint32(typ) & 31)
Rather than passing a block parameter, I think it might be clearer to have two
methods, perhaps block(ICMPType) and accept(ICMPType).  What do you think?

[mikio, 2014-12-02 04:09:26]

On Tue, Dec 2, 2014 at 12:44 PM,  <iant@golang.org> wrote:

> Rather than passing a block parameter, I think it might be clearer to
> have two methods, perhaps block(ICMPType) and accept(ICMPType).  What do
> you think?

filt.Block and filt.Accept--bilateral ops look nice, thanks. do you
prefer to change existing methods on ipv6.ICMPFilter too?

[iant, 2014-12-02 16:43:52]

On Mon, Dec 1, 2014 at 8:09 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> On Tue, Dec 2, 2014 at 12:44 PM,  <iant@golang.org> wrote:
>
>> Rather than passing a block parameter, I think it might be clearer to
>> have two methods, perhaps block(ICMPType) and accept(ICMPType).  What do
>> you think?
>
> filt.Block and filt.Accept--bilateral ops look nice, thanks. do you
> prefer to change existing methods on ipv6.ICMPFilter too?

I suppose they should be more or less the same, yes.

Ian

[mikio, 2014-12-04 02:57:35]

ptal

https://codereview.appspot.com/179510043/diff/410001/ipv4/icmp.go
File ipv4/icmp.go (right):

https://codereview.appspot.com/179510043/diff/410001/ipv4/icmp.go#newcode29
ipv4/icmp.go:29: // An ICMPFilter represents an ICMP message filter for incoming
On 2014/12/02 03:44:41, iant wrote:
> Some doc comment, either here or for the Set method, needs to explain more. 
> What does a filter do?

Done.

https://codereview.appspot.com/179510043/diff/410001/ipv4/icmp.go#newcode32
ipv4/icmp.go:32: mu sync.RWMutex
thx, removed

https://codereview.appspot.com/179510043/diff/410001/ipv4/icmp_linux.go
File ipv4/icmp_linux.go (right):

https://codereview.appspot.com/179510043/diff/410001/ipv4/icmp_linux.go#newcode9
ipv4/icmp_linux.go:9: f.Data |= 1 << (uint32(typ) & 31)
thx, revised

[iant, 2014-12-04 04:42:08]

LGTM

[mikio, 2014-12-04 08:31:44]

*** Submitted as
https://code.google.com/p/go/source/detail?r=2cc654565017&repo=net ***

x/net/ipv4: add support for icmp filter on linux

LGTM=iant
R=iant
CC=golang-codereviews
https://codereview.appspot.com/179510043