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

bpf2go: add flag for alternative filename stem #770

Merged
merged 1 commit into from Sep 28, 2022

Conversation

MarcusWichelmann
Copy link
Contributor

In our project, the eBPF source files are named with underscores, like ext_vx_decap.c. We want to have the .go and .o files generated by bpf2go to follow the same naming scheme (e.g. ext_vx_decap_bpfel.go). But currently, when setting the <ident> parameter to ext_vx_decap, the result will be some broken Go type names like type ext_vx_decapPrograms

So instead, this PR adds a new -filename-stem flag, which allows specifying an alternative filename prefix while one can keep the ident as it is supposed to.
The -filename-stem flag is optional and if it's not set, the ident will be used instead. So this is not a breaking change.

Sample usage:

bpf2go [...] -filename-stem ext_vx_decap ExtVxDecap src/ext_vx_decap.c

@lmb lmb self-requested a review August 29, 2022 17:19
@lmb
Copy link
Collaborator

lmb commented Aug 29, 2022

To clarify, you want your Go types to be ExtVxDecap... but the output to be called ext_vx_decap_bpfel.go?

@MarcusWichelmann
Copy link
Contributor Author

Exactly.

Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

I think the flag is probably fine. I had another idea though: massage ident so that uppercase letters are turned into underscore followed by the lowercase letter. That would transform ExtFooBar into ext_foo_bar. What do you think?

if filenameStem == "" {
filenameStem = b2g.ident
}
stem := fmt.Sprintf("%s_%s", strings.ToLower(filenameStem), tgt.clang)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we really lowercase an explicitly given stem?

@@ -81,6 +81,7 @@ func run(stdout io.Writer, pkg, outputDir string, args []string) (err error) {
fs.StringVar(&b2g.makeBase, "makebase", "", "write make compatible depinfo files relative to `directory`")
fs.Var(&b2g.cTypes, "type", "`Name` of a type to generate a Go declaration for, may be repeated")
fs.BoolVar(&b2g.skipGlobalTypes, "no-global-types", false, "Skip generating types for map keys and values, etc.")
fs.StringVar(&b2g.filenameStem, "filename-stem", "", "alternative stem for names of generated files (defaults to ident)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Please call this -output-stem and call the new field outputStem, so it matches outputDir.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be a check to make sure b2g.outputStem doesn't contain directory separators. Said another way -output-stem ../foo should be rejected. A test for this would also be nice.

Something like filepath.Clean(outputStem) != outputStem or similar.

@MarcusWichelmann
Copy link
Contributor Author

Thanks for the review!
An automatic transformation of the ident would also be fine, but will be hard to get right in cases like uppercase abbreviations as often seen (per convention) in Go type names. e.g. ExtIPV4Firewall -> ext_ipv4_firwall.
Of course, this is also algorithmically solvable, but there might still be cases where the algorithm will get it wrong and then the developer has no way to fix it manually. e.g. SFlowSomething -> sflow_something (not s_flow_something)
So a flag does probably provide better control.

But I'll work on your suggestions and update this PR.

@lmb
Copy link
Collaborator

lmb commented Sep 1, 2022

I had a look around, and there are at least a couple open source cases that would break: https://sourcegraph.com/search?q=context:global+go:generate+.%2B%3F+bpf2go&patternType=regexp Let's stick to the flag.

@MarcusWichelmann
Copy link
Contributor Author

Have done the requested changes now. I have squashed my changes, because the new commit changes everything from the previous commit.

I'm not sure, how you intended the usage of filepath.Clean, because that won't prevent one from using -output-stem "dir/some_stem". Instead I rely on the absence of path separation chars (/).

@MarcusWichelmann
Copy link
Contributor Author

I rebased this branch onto current master.

How do you think about this PR, can this be merged?

Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

Sorry for this, the PR fell through the cracks. I missed your last message somehow. Thank you for your contribution!

@lmb lmb merged commit 1f78277 into cilium:master Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants