code review 13832043: go.net/ipv4: Add Flags field to Header

go.net/ipv4: Add Flags field to Header

Fixes issue 6453.

[marant, 2013-09-24 11:18:59]

Hello golang-dev@googlegroups.com,

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

[mikio, 2013-10-04 09:49:53]

sorry for being late.

https://codereview.appspot.com/13832043/diff/8001/ipv4/header.go
File ipv4/header.go (right):

https://codereview.appspot.com/13832043/diff/8001/ipv4/header.go#newcode54
ipv4/header.go:54: DontFragment  = 0x4000 // Don't Fragment flag
const (
        DontFragment HeaderFlags = 1<<iota // ...
        MoreFragments                      // ...
)

https://codereview.appspot.com/13832043/diff/8001/ipv4/header.go#newcode65
ipv4/header.go:65: Flags    int    // flags
can you add a new type, like HeaderFlags or HeaderControlFlags?

https://codereview.appspot.com/13832043/diff/8001/ipv4/header.go#newcode99
ipv4/header.go:99: flagsAndFragOff := (h.FragOff & 0x1FFF) | h.Flags
drop unnecessary blank lines

[marant, 2013-10-04 10:20:18]

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

Please take another look.

[mikio, 2013-10-04 13:20:49]

Have you signed a CLA?
http://golang.org/doc/contribute.html#copyright

[marant, 2013-10-04 13:52:29]

On 2013/10/04 13:20:49, mikio wrote:
> Have you signed a CLA?
> http://golang.org/doc/contribute.html#copyright

Ah, I somehow missed that on the contribution guide. I signed the CLA just now.

[mikio, 2013-10-05 00:45:44]

https://codereview.appspot.com/13832043/diff/19001/ipv4/header.go
File ipv4/header.go (right):

https://codereview.appspot.com/13832043/diff/19001/ipv4/header.go#newcode39
ipv4/header.go:39: type headerField int
ah, this is a dreg i overlooked.
can you please delete this while we are here, thanks.

https://codereview.appspot.com/13832043/diff/19001/ipv4/header.go#newcode57
ipv4/header.go:57: MoreFragments                                // More
Fragments flag
i mean:

const (
        MoreFragments HeaderFlags = 1 << iota // more fragments flag
        DontFragment                          // don't fragment flag
)

https://codereview.appspot.com/13832043/diff/19001/ipv4/header.go#newcode81
ipv4/header.go:81: return fmt.Sprintf("ver: %v, hdrlen: %v, tos: %#x, totallen:
%v, id: %#x, fragoff: %#x, ttl: %v, proto: %v, cksum: %#x, src: %v, dst: %v",
h.Version, h.Len, h.TOS, h.TotalLen, h.ID, h.FragOff, h.TTL, h.Protocol,
h.Checksum, h.Src, h.Dst)
no Flags?

https://codereview.appspot.com/13832043/diff/19001/ipv4/header.go#newcode100
ipv4/header.go:100: flagsAndFragOff := (h.FragOff & 0x1FFF) | int(h.Flags)
flagsAndFragOff := int(h.Flags<<13) | int(h.FragOff&0x1fff)

https://codereview.appspot.com/13832043/diff/19001/ipv4/header.go#newcode147
ipv4/header.go:147: h.Flags = HeaderFlags(h.FragOff & 0xE000)
 h.Flags = HeaderFlags(h.FragOff) >> 13

https://codereview.appspot.com/13832043/diff/19001/ipv4/header.go#newcode148
ipv4/header.go:148: h.FragOff = h.FragOff & 0x1FFF
h.FragOff = h.FragOff & 0x1fff
(lower alphanum like around here)

[marant, 2013-10-07 05:03:34]

https://codereview.appspot.com/13832043/diff/19001/ipv4/header.go
File ipv4/header.go (right):

https://codereview.appspot.com/13832043/diff/19001/ipv4/header.go#newcode39
ipv4/header.go:39: type headerField int
On 2013/10/05 00:45:44, mikio wrote:
> ah, this is a dreg i overlooked.
> can you please delete this while we are here, thanks.

Done.

https://codereview.appspot.com/13832043/diff/19001/ipv4/header.go#newcode81
ipv4/header.go:81: return fmt.Sprintf("ver: %v, hdrlen: %v, tos: %#x, totallen:
%v, id: %#x, fragoff: %#x, ttl: %v, proto: %v, cksum: %#x, src: %v, dst: %v",
h.Version, h.Len, h.TOS, h.TotalLen, h.ID, h.FragOff, h.TTL, h.Protocol,
h.Checksum, h.Src, h.Dst)
On 2013/10/05 00:45:44, mikio wrote:
> no Flags?

Done.

https://codereview.appspot.com/13832043/diff/19001/ipv4/header.go#newcode100
ipv4/header.go:100: flagsAndFragOff := (h.FragOff & 0x1FFF) | int(h.Flags)
On 2013/10/05 00:45:44, mikio wrote:
> flagsAndFragOff := int(h.Flags<<13) | int(h.FragOff&0x1fff)

Done.

https://codereview.appspot.com/13832043/diff/19001/ipv4/header.go#newcode148
ipv4/header.go:148: h.FragOff = h.FragOff & 0x1FFF
On 2013/10/05 00:45:44, mikio wrote:
> h.FragOff = h.FragOff & 0x1fff
> (lower alphanum like around here)

Done.

[mikio, 2013-10-07 16:03:52]

Thanks. It looks very close.

https://codereview.appspot.com/13832043/diff/29001/ipv4/header.go
File ipv4/header.go (right):

https://codereview.appspot.com/13832043/diff/29001/ipv4/header.go#newcode81
ipv4/header.go:81: return fmt.Sprintf("ver: %v, hdrlen: %v, tos: %#x, totallen:
%v, id: %#x, fragoff: %#x, ttl: %v, proto: %v, cksum: %#x, src: %v, dst: %v",
h.Version, h.Len, h.TOS, h.TotalLen, h.ID, h.Flags, h.FragOff, h.TTL,
h.Protocol, h.Checksum, h.Src, h.Dst)
go tool vet says:
header.go:81: wrong number of args for format in Sprintf call: 11 needed but 12
args

https://codereview.appspot.com/13832043/diff/29001/ipv4/header.go#newcode100
ipv4/header.go:100: flagsAndFragOff := (h.FragOff & 0x1fff) | int(h.Flags<<14)
h.Flags<<14 doesn't look correct to me.
Does your test work correctly when we set ipv4.MoreFragments?

const (
        MoreFragments HeaderFlags = 1 << iota // more Fragments flag
        DontFragment                          // don't Fragment flag
)

and

       flagsAndFragOff := (h.FragOff & 0x1fff) | int(h.Flags<<13)
       h.Flags = HeaderFlags(h.FragOff&0xe000) >> 13

?

[marant, 2013-10-07 17:54:35]

https://codereview.appspot.com/13832043/diff/29001/ipv4/header.go
File ipv4/header.go (right):

https://codereview.appspot.com/13832043/diff/29001/ipv4/header.go#newcode81
ipv4/header.go:81: return fmt.Sprintf("ver: %v, hdrlen: %v, tos: %#x, totallen:
%v, id: %#x, fragoff: %#x, ttl: %v, proto: %v, cksum: %#x, src: %v, dst: %v",
h.Version, h.Len, h.TOS, h.TotalLen, h.ID, h.Flags, h.FragOff, h.TTL,
h.Protocol, h.Checksum, h.Src, h.Dst)
On 2013/10/07 16:03:53, mikio wrote:
> go tool vet says:
> header.go:81: wrong number of args for format in Sprintf call: 11 needed but
12
> args

Done.

https://codereview.appspot.com/13832043/diff/29001/ipv4/header.go#newcode100
ipv4/header.go:100: flagsAndFragOff := (h.FragOff & 0x1fff) | int(h.Flags<<14)
On 2013/10/07 16:03:53, mikio wrote:
> h.Flags<<14 doesn't look correct to me.
> Does your test work correctly when we set ipv4.MoreFragments?
> 
> const (
>         MoreFragments HeaderFlags = 1 << iota // more Fragments flag
>         DontFragment                          // don't Fragment flag
> )
> 
> and
> 
>        flagsAndFragOff := (h.FragOff & 0x1fff) | int(h.Flags<<13)
>        h.Flags = HeaderFlags(h.FragOff&0xe000) >> 13
> 
> ?
Yes, you are correct. I had the flags defined in the wrong order which caused
the confusion.

[mikio, 2013-10-07 21:52:17]

LGTM

[mikio, 2013-10-07 21:53:10]

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

go.net/ipv4: add Flags field to Header

Fixes issue 6453.

R=golang-dev, mikioh.mikioh
CC=golang-dev
https://codereview.appspot.com/13832043

Committer: Mikio Hara <mikioh.mikioh@gmail.com>