-
Notifications
You must be signed in to change notification settings - Fork 72
tc: add TC filter for BPF/TC_BPF_*
#230
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @ver-nyan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request extends the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds support for BPF TC filters, which is a great addition. The implementation mostly follows the existing patterns in the crate. I've found one critical issue with the handling of TCA_BPF_TAG which causes incorrect serialization. I've left a detailed comment with a suggested fix. Once that is addressed, this should be good to go.
Regarding your question about renaming TcU32OptionFlags: renaming it to something more generic like TcaFlags makes sense given its use in other filters like flower and now bpf. However, as you noted, it would be a breaking API change. It might be a good idea to do this in a future major version bump. For this PR, reusing TcU32OptionFlags is a reasonable approach.
|
Could you explains why we introduce new dependency on |
I haven't updated it in here yet, but I ended up changing it The program |
3709dce to
2503116
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request does a great job of adding support for BPF TC filters, following the existing patterns in the crate. The new test case is also very helpful for verifying the implementation.
I found a small issue in the test data for TCA_BPF_NAME where the NLA length is incorrect, which would cause the test to fail. I've left a specific comment with a suggested fix.
Regarding your question about renaming TcU32OptionFlags: I agree that the name is a bit misleading now that it's used by multiple filter types. Renaming it to something more generic like TcClassifierFlags would improve clarity and maintainability. Since it's a breaking change, it's good to consider it carefully, but this PR seems like a good opportunity to do it, as you're touching related code.
Overall, great work!
2503116 to
9617e37
Compare
cathay4t
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just a small lint.
Add TC attribute for BPF filter.
9617e37 to
3325f23
Compare
Add TC option for BPF filter.
Also adds the
hexcrate for deserializing bytes into hex formatted string for parsing the BPF program tag.WIP, missing tests which im working on, but verified manually that it's at least deserializing correctly which is my usecase for Netflix/bpftop#190.
This is my first time actually working with netlink internals, so apologies my understanding is off. :)
Some question i have:
TCA_BPF_FLAGS_GENattribute uses the sameTCA_CLS_FLAGS_*flags defined inu32_flags.rsfile, so it's currently a wrapper ofTcU32OptionFlags. ref linkIn the kernel code, it looks like
flower(TCA_FLOWER_FLAGSref link) &matchall(TCA_MATCHALL_FLAGSref link) also use the same flags (so far i've only found them in conjunction withtc_flags_valid()).I was wondering if the bitflag
TcU32OptionFlagsshould be renamed to something more generic, likeTcaFlags. This would end up breaking API tho :/