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

prog: Allow empty data for prog.Test and prog.Run #685

Merged
merged 2 commits into from
Jul 5, 2022

Conversation

danobi
Copy link
Contributor

@danobi danobi commented May 26, 2022

Certain prog types (SK_LOOKUP, RAW_TRACEPOINT and
RAW_TRACEPOINT_WRITABLE) require that data_in is NULL. The previous
check prevented 0 length data_in.

Fix by removing check.

Followup from #647.

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.

Could you add a test for this? AFAIR SK_LOOKUP programs will refuse to run if given DataIn, so you can use that to make sure this works.

@dxuuu
Copy link

dxuuu commented Jun 27, 2022

Ping

Copy link
Collaborator

@ti-mo ti-mo 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 the long wait, there has been some PTO in between. Thanks for your patience! Left some minor comments.

@@ -179,6 +179,13 @@ import (
{"FuncInfo", "bpf_func_info", nil},
{"LineInfo", "bpf_line_info", nil},
{"XdpMd", "xdp_md", nil},
{
"BpfSkLookup", "bpf_sk_lookup",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@lmb Shouldn't this be BPFSKLookup? Same for XdpMdand BtfInfo above. :/

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's easier to automate generating names like BtfInfo vs capitalising some things.

prog_test.go Outdated
Family: 2, /* AF_INET */
}
opts := RunOptions{
Data: []byte{},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could be removed, the call to sys.NewSlicePointer(opts.Data) in testRun() will return a Pointer{} whether Data is nil or length 0.

prog_test.go Outdated
defer prog.Close()

skLookup := sys.BpfSkLookup{
Family: 2, /* AF_INET */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to add an entry to internal/unix/types_*.go.

@ti-mo ti-mo requested a review from lmb June 28, 2022 21:07
Will be used for unit testing BPF_PROG_TYPE_SK_LOOKUP progs. IPs are
stored as arrays of bytes to make use from Go easier.
Certain prog types (SK_LOOKUP, RAW_TRACEPOINT and
RAW_TRACEPOINT_WRITABLE) require that data_in is NULL. The previous
check prevented 0 length data_in.

Fix by removing check.
@lmb
Copy link
Collaborator

lmb commented Jul 5, 2022

Sorry for the long delay, I wasn't aware that you had made changes and then I was busy with life when you pinged me. Thanks for following up :)

@lmb lmb merged commit 426053e into cilium:master Jul 5, 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.

4 participants