Add bitwise decoder logic

[sbezverk]

Signed-off-by: Serguei Bezverkhi sbezverk@cisco.com
This PR partially implements: #70

[sbezverk]

@stapelberg Please review when you have a chance, thank you.

[sbezverk]

I tested this change in my e2e testing.

Initial programmed tables/chains/rules: {"Name":"nftables_ipv4","Use":0,"Flags":0,"Family":2}{"Name":"chain-1","Table":{"Name":"nftables_ipv4","Use":0,"Flags":0,"Family":2},"Hooknum":1,"Priority":0,"Type":"filter","Policy":1}[{"DestRegister":1,"Base":"expr.PayloadBaseNetworkHeader","Len":1,"Offset":9},{"Op":"expr.CmpOpEq","Register":1,"Data":["0x1"]},{"DestRegister":1,"Base":"expr.PayloadBaseNetworkHeader","Len":4,"Offset":16},{"SourceRegister":1,"DestRegister":1,"Len":4,"Mask":["0xff","0xff","0x0","0x0"],"Xor":["0x0","0x0","0x0","0x0"]},{"Op":"expr.CmpOpEq","Register":1,"Data":["0x1","0x1","0x0","0x0"]},{"Kind":"0x0"}]
Learned rules for table: nftables_ipv4, chain: chain-1 rules: [{"DestRegister":1,"Base":"expr.PayloadBaseNetworkHeader","Len":1,"Offset":9},{"Op":"expr.CmpOpEq","Register":1,"Data":["0x1"]},{"DestRegister":1,"Base":"expr.PayloadBaseNetworkHeader","Len":4,"Offset":16},{"SourceRegister":1,"DestRegister":1,"Len":4,"Mask":["0xff","0xff","0x0","0x0"],"Xor":["0x0","0x0","0x0","0x0"]},{"Op":"expr.CmpOpEq","Register":1,"Data":["0x1","0x1","0x0","0x0"]},{"Kind":"0x0"}]

[stapelberg]

Looks good. Can you also add a unit test please?

[sbezverk]

@stapelberg Adding a unit test is not straight forward in this scenario, as to test it, a rule really needs to be programmed and then get it and then parsed. I will see what else can be done.

[mdlayher]

LGTM

[sbezverk]

@stapelberg any chance you could merge it please?

[sbezverk]

@stapelberg could you please merge it?

[stapelberg]

Instead of t.Errorf+continue and prefixing each error with the tests name, please use subtests with t.Fatalf: http://blog.golang.org/subtests

[sbezverk]

done

[stapelberg]

This does not seem to be addressed yet? I don’t see you using a subtest even in the latest changes?

[sbezverk]

Please check diff again. it has all been done. Not sure why you do not see it.

[stapelberg]

I don’t see a subtest (t.Run), even after clearing my browser cache. Looking into your branch, I don’t see it either: https://github.com/sbezverk/nftables/blob/add_bitwise_decoder/expr/bitwise_test.go

Did you forget to push a commit, or misunderstand what I meant?

[sbezverk]

https://github.com/sbezverk/nftables/blob/add_bitwise_decoder/expr/bitwise_test.go#L36

	for _, tt := range tests {
		nbw := Bitwise{}
		data, err := tt.bw.marshal()
		if err != nil {
			t.Fatalf("Test \"%s\" failed to marshal Bitwise struct with error: %+v", tt.name, err)

		}

I tried force push, but it says no changes. Not sure what else can be done..

[stapelberg]

In your code excerpt, I don’t see t.Run either?

[sbezverk]

Right, I thought your comment was just about Errorf/continue vs Fatalf.

[stapelberg]

Is this used? Can you run go mod tidy please? Alternatively, try reverting your changes to go.sum and rebuild.

[sbezverk]

run go mod tidy

go mod tidy
go: downloading github.com/jsimonetti/rtnetlink v0.0.0-20190606172950-9527aa82566a
go: extracting github.com/jsimonetti/rtnetlink v0.0.0-20190606172950-9527aa82566a

[stapelberg]

Can you mark this test as parallelizable by calling t.Parallel() as first statement please?

[sbezverk]

done

[stapelberg]

Is this how one matches a /16 IPv4 subnet mask? If so, add a comment please.

[sbezverk]

done

[sbezverk]

@stapelberg comments are addressed in the latest revision, PTAL.

[stapelberg]

Here and below: prefixing the test name manually is now superfluous with subtests, which automatically include the test name.

Please use something like:

t.Fatalf("marshal: %v", err)