Skip to content

bpf: remove unused GetProgNextID, GetProgFDByID and GetProgInfoByFD #10187

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 19, 2020
Merged

bpf: remove unused GetProgNextID, GetProgFDByID and GetProgInfoByFD #10187

merged 1 commit into from
Feb 19, 2020

Conversation

tklauser
Copy link
Member

@tklauser tklauser commented Feb 14, 2020 •
edited
Loading

These are not used by Cilium. If they'd be needed in the future they
should be added to github.com/cilium/ebpf

Signed-off-by: Tobias Klauser tklauser@distanz.ch

This change is Reviewable

@tklauser tklauser added pending-review kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. labels Feb 14, 2020
@tklauser tklauser requested a review from a team February 14, 2020 10:49
@tklauser
Copy link
Member Author

test-me-please

@pchaigno
Copy link
Member

I'm not sure we can/want to remove those. These functions were added in #3899 to be used by a separate project (https://github.com/genuinetools/bpfps).

@tklauser
Copy link
Member Author

I'm not sure we can/want to remove those. These functions were added in #3899 to be used by a separate project (https://github.com/genuinetools/bpfps).

Do we guarantee API stability for non-Cilium projects? It seems github.com/genuinetools/bpfps is vendoring a version github.com/cilium/cilium which still has these funcs. When building it against latest cilium master, it's broken already anyhow:

# github.com/genuinetools/bpfps
./main.go:63:21: not enough arguments in call to bpf.CheckOrMountFS
	have (string)
	want (string, bool)
make: *** [basic.mk:48: bpfps] Error 2

@tklauser
Copy link
Member Author

Ideally these functions should probably be added to github.com/cilium/ebpf so they can be safely consumed by other packages.

@coveralls
Copy link

coveralls commented Feb 14, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 44.467% when pulling 45e5c46 on pr/tklauser/bpf-prog-remove-getprogfdbyid into 9f492a1 on master.

@brb
Copy link
Member

brb commented Feb 14, 2020

Do we guarantee API stability for non-Cilium projects?

@aanm ^^?

@aanm
Copy link
Member

aanm commented Feb 14, 2020

test-me-please

aanm
aanm requested changes Feb 14, 2020
View reviewed changes
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tklauser The whole file pkg/bpf/prog_linux.go can be removed since its not used anywhere.

@aanm
Copy link
Member

aanm commented Feb 14, 2020

Do we guarantee API stability for non-Cilium projects?

@aanm ^^?

I don't see how we could enforce this for all the code base. We have api/v1 that should guarantee the API stability.

@tklauser tklauser force-pushed the pr/tklauser/bpf-prog-remove-getprogfdbyid branch from 97bdbfb to 73ad53f Compare February 14, 2020 20:53
@tklauser tklauser changed the title bpf: remove unused GetProgNextID and GetProgFDByID bpf: remove unused GetProgNextID, GetProgFDByID and GetProgInfoByFD Feb 14, 2020
@tklauser
Copy link
Member Author

test-me-please

@aanm aanm added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Feb 17, 2020
@aanm
Copy link
Member

aanm commented Feb 17, 2020

@tklauser needs rebase

@tklauser
bpf: remove unused GetProgNextID and GetProgFDByID
45e5c46 
These are not used by Cilium.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser tklauser force-pushed the pr/tklauser/bpf-prog-remove-getprogfdbyid branch from 73ad53f to 45e5c46 Compare February 17, 2020 13:12
@tklauser tklauser removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Feb 17, 2020
@tklauser
Copy link
Member Author

tklauser commented Feb 17, 2020 •
edited by aanm
Loading

test-me-please

failed with known tls flake

@aanm aanm added this to the 1.8 milestone Feb 18, 2020
@aanm aanm merged commit 48327b9 into master Feb 19, 2020
@aanm aanm deleted the pr/tklauser/bpf-prog-remove-getprogfdbyid branch February 19, 2020 00:30
@brb brb mentioned this pull request Feb 20, 2020
Merged
@brb brb mentioned this pull request Feb 21, 2020
Merged
@nebril nebril mentioned this pull request Feb 21, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanm aanm aanm requested changes

Assignees
No one assigned
Labels
kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
1.8
Development

Successfully merging this pull request may close these issues.
8 participants
@tklauser @pchaigno @coveralls @brb @aanm @borkmann @joestringer @nebril