Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/net/bpf: implement interface fmt.Stringer on BPF instructions #18538

Closed
breml opened this issue Jan 6, 2017 · 8 comments
Closed

x/net/bpf: implement interface fmt.Stringer on BPF instructions #18538

breml opened this issue Jan 6, 2017 · 8 comments

Comments

@breml
Copy link
Contributor

@breml breml commented Jan 6, 2017

What version of Go are you using (go version)?

go version go1.7.4 linux/amd64

What operating system and processor architecture are you using (go env)?

linux, amd64

What did you do?

I would like to print bpf.Instructions in the assembler notation defined in https://www.kernel.org/doc/Documentation/networking/filter.txt.

What did you expect to see?

I suggest to implement fmt.Stringer an all types which implement bpf.Instruction.

For example the instructions

[]bpf.Instruction{
	bpf.LoadExtension{Num: bpf.ExtRand},
	bpf.JumpIf{Cond: bpf.JumpGreaterThan, Val: 4294967, SkipTrue: 1},
	bpf.RetConstant{Val: 1024},
	bpf.RetConstant{Val:0},
}

could be printed as

ld #rand
jgt #4294967, 1
ret #1024
ret #0

I have an implementation for the output in assembler notation (without fmt.Stringer): https://github.com/breml/bpfutils/blob/master/string.go#L26-L218 and I offer to provide a CL for the above suggestion, if this change is accepted.

What did you see instead?

Is not implemented.

@titanous titanous added this to the Unreleased milestone Jan 7, 2017
@titanous
Copy link
Member

@titanous titanous commented Jan 7, 2017

/cc @danderson @mikioh

@danderson
Copy link

@danderson danderson commented Jan 7, 2017

This seems fine, and a CL would be welcome as far as I'm concerned (though I don't have direct commit access, @mikioh has the final say there).

The only thing I'm worried about is what to do about RawInstruction. By design, RawInstruction is also an Instruction, so that programmers can specify custom opcodes for forks of the BPF machine. Those instructions might not match a known instruction of the Linux BPF implementation, so... What does String() return for that?

I suppose the simplest option is simply to not implement fmt.Stringer on RawInstruction. Does that seem reasonable to you?

@breml
Copy link
Contributor Author

@breml breml commented Jan 7, 2017

It is not my intend to extend the existing bpf.Instruction interface but simply implement the fmt.Stringer interface on the types (like bpf.RetConstant) where it makes sense (respectively where the assembler instruction is defined).
So I fully agree with the suggestion by @danderson to not implement fmt.Stringer on RawInstruction.

@mikioh
Copy link
Contributor

@mikioh mikioh commented Jan 12, 2017

I'll push the submit button once @danderson is happy.

@danderson
Copy link

@danderson danderson commented Jan 12, 2017

@breml's plan sounds great to me. Is there a CL already? I'll happily review one.

@breml
Copy link
Contributor Author

@breml breml commented Jan 12, 2017

@danderson there is not yet a CL. The implementation is straight forward. Currently I am looking for the best way to implement the tests.

One possibility would be to add a go style table driven test which includes for every bpf.Instruction the expected string.
An other possibility would be to use the already existing testdata and validate, that for all instructions in testdata/all_instructions.bpf the respective line in testdata/all_instructions.txt is generated. The downside of this approach is, the instructions in testdata/all_instructions.txt do need some preprocessing (remove all lines with comments, remove empty lines a and most importantly resolve the jump labels to the correct relativ jump distance).

@danderson what do you suggest?

@breml
Copy link
Contributor Author

@breml breml commented Jan 12, 2017

@danderson Just uploaded a CL, which uses option 1 for the tests (see my previous comment).

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 12, 2017

CL https://golang.org/cl/35171 mentions this issue.

@golang golang locked and limited conversation to collaborators Jan 13, 2018
NET12115 added a commit to NET12115/Golang-C-NET that referenced this issue Feb 28, 2022
Fixes golang/go#18538

Change-Id: Ic0627352f96ad5fa138633d1e1ccfaf76294d621
Reviewed-on: https://go-review.googlesource.com/35171
Run-TryBot: Matt Layher <mdlayher@gmail.com>
Reviewed-by: Matt Layher <mdlayher@gmail.com>
Reviewed-by: David Anderson <dave@natulte.net>
Reviewed-by: Mikio Hara <mikioh.mikioh@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants