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: add Setter interface #19912

Closed
mdlayher opened this issue Apr 10, 2017 · 9 comments
Closed

x/net/bpf: add Setter interface #19912

mdlayher opened this issue Apr 10, 2017 · 9 comments
Labels
FrozenDueToAge NeedsFix Proposal Proposal-Accepted
Milestone

Comments

@mdlayher
Copy link
Member

@mdlayher mdlayher commented Apr 10, 2017

In order to standardize BPF filter attachment in Go, I propose an interface which mimics the most popular signature used in the wild:

package bpf

// A Setter is a type which can attach a compiled BPF filter to itself.
type Setter interface {
    SetBPF(filter []bpf.RawInstruction) error
}

I believe this pattern was first used in x/net/ipv{4,6}:

https://godoc.org/golang.org/x/net/ipv4#PacketConn.SetBPF
https://godoc.org/golang.org/x/net/ipv6#PacketConn.SetBPF

I've used it in a couple of my own repositories as well:

https://godoc.org/github.com/mdlayher/raw#Conn.SetBPF
https://godoc.org/github.com/mdlayher/netlink#Conn.SetBPF

Finally, on at least two occasions, I have defined an unexported interface that contains a method using this proposed interface:

https://github.com/coreos/torus/blob/master/block/aoe/aoe.go#L106-L111
https://github.com/mdlayher/netlink/blob/master/conn.go#L230-L234

For simplicity and standardization, I'd like to add the proposed interface to x/net/bpf's API, in order to ensure consistency between types that can attach a BPF filter to themselves.

/cc @danderson, @mikioh, @breml

@mdlayher mdlayher added this to the Proposal milestone Apr 10, 2017
@rsc
Copy link
Contributor

@rsc rsc commented Apr 17, 2017

Given that this interface is already implemented by x/net/ipv4 and x/net/ipv6, it seems fine to add the interface definition too. Any thoughts, @danderson, @mikioh?

@mikioh
Copy link
Contributor

@mikioh mikioh commented Apr 18, 2017

I'll leave it up to @danderson.

@mikioh
Copy link
Contributor

@mikioh mikioh commented Jun 2, 2017

ping @danderson

@danderson
Copy link

@danderson danderson commented Jun 2, 2017

The interface definition seems fine, however: what benefit does it provide? I don't understand what benefit this definition provides, given that the package itself doesn't make use of it. My first instinct is that this is an attempt to define a java-style interface that other packages can implements... Except that this isn't needed in Go. So, I'm just confused :)

What am I missing?

@danderson
Copy link

@danderson danderson commented Jun 2, 2017

To be clear: if the intent is just to try and steer implementors to standardize on using that method signature to attach BPF programs, sure, I'm happy to go along with it. It just makes me feel a bit strange to have a package define an interface exclusively as a "serving suggestion" for other packages. If I'm alone in feeling weird about that, then definitely let's do it :)

@mdlayher
Copy link
Member Author

@mdlayher mdlayher commented Jun 6, 2017

Hmmm, not sure how I missed this notification, sorry!

To be clear: if the intent is just to try and steer implementors to standardize on using that method signature to attach BPF programs, sure, I'm happy to go along with it.

That's exactly it. Think of it like the interfaces defined in: https://golang.org/pkg/encoding/.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 6, 2017

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

@rsc
Copy link
Contributor

@rsc rsc commented Jun 12, 2017

@danderson, OK to accept this proposal?

@danderson
Copy link

@danderson danderson commented Jun 12, 2017

LGTM, ship it.

@bradfitz bradfitz changed the title proposal: x/net/bpf: add Setter interface x/net/bpf: add Setter interface Jun 12, 2017
@bradfitz bradfitz added the NeedsFix label Jun 12, 2017
@bradfitz bradfitz added this to the Unreleased milestone Jun 12, 2017
@bradfitz bradfitz removed this from the Proposal milestone Jun 12, 2017
@golang golang locked and limited conversation to collaborators Jun 13, 2018
NET12115 added a commit to NET12115/Golang-C-NET that referenced this issue Feb 28, 2022
This interface can be implemented by types which are capable of
attaching compiled BPF filters to themselves, such as
x/net/ipv{4,6}.PacketConn.

Fixes golang/go#19912

Change-Id: I4674c3e312c173d5a7a3beb3cd53704f8c08e29b
Reviewed-on: https://go-review.googlesource.com/44972
Run-TryBot: Matt Layher <mdlayher@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Mikio Hara <mikioh.mikioh@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix Proposal Proposal-Accepted
Projects
None yet
Development

No branches or pull requests

6 participants