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

add: separate mod for xdp #625

Merged
merged 1 commit into from
Dec 20, 2023
Merged

add: separate mod for xdp #625

merged 1 commit into from
Dec 20, 2023

Conversation

syogaraj
Copy link

@syogaraj syogaraj commented Dec 9, 2023

XDP-related operations are now added as a separate mod as discussed in #618

Closes:
#618
#205
#206
#263

@danielocfb
Copy link
Collaborator

Will take a look by Wednesday the latest.

Copy link
Collaborator

@danielocfb danielocfb left a comment

Choose a reason for hiding this comment

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

Thanks for your pull request! Your change seems mostly fine. I left a few comments. I think we should think about adding more tests. Specifically, for

  • query()
  • forced errors for attach(), detach(), query() and query_id()
  • double attach() & detach() (second one should fail, I would expect)?

What do you think?

libbpf-rs/src/xdp.rs Outdated Show resolved Hide resolved
libbpf-rs/src/xdp.rs Outdated Show resolved Hide resolved
libbpf-rs/src/xdp.rs Outdated Show resolved Hide resolved
libbpf-rs/src/xdp.rs Outdated Show resolved Hide resolved
libbpf-rs/src/xdp.rs Outdated Show resolved Hide resolved
libbpf-rs/src/xdp.rs Outdated Show resolved Hide resolved
libbpf-rs/src/xdp.rs Outdated Show resolved Hide resolved
@syogaraj syogaraj force-pushed the xdp branch 2 times, most recently from bf8fc85 to 3252fd5 Compare December 14, 2023 06:08
@syogaraj
Copy link
Author

@danielocfb,

  1. test cases for query and query_id will not return any error even if the prog is not loaded to the interface. If that's the case, the returned opts struct will have the prog_id as 0.
  2. Added a test case for forced error in the attach API. i.e., attach twice and expect an error.
  3. Double detach doesn't seem to raise an error.
  4. Some of the requested changes were made since I took references from tc.rs (ex: unnecessary casts and usage of errno). Have updated with the required changes..

@syogaraj syogaraj requested a review from danielocfb December 14, 2023 06:24
Copy link
Collaborator

@danielocfb danielocfb left a comment

Choose a reason for hiding this comment

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

Thanks for updating! I left a few more comments regarding the API. Looking closer at it I think there are some cases that we want to support but do not currently. Let me know what you think.

libbpf-rs/src/xdp.rs Show resolved Hide resolved
libbpf-rs/src/xdp.rs Outdated Show resolved Hide resolved
libbpf-rs/src/xdp.rs Outdated Show resolved Hide resolved
libbpf-rs/src/xdp.rs Outdated Show resolved Hide resolved
libbpf-rs/tests/test_xdp.rs Show resolved Hide resolved
@danielocfb
Copy link
Collaborator

  1. test cases for query and query_id will not return any error even if the prog is not loaded to the interface. If that's the case, the returned opts struct will have the prog_id as 0.

I think error handling for these functions is broken currently. Don't know if that will change much, though.

@syogaraj syogaraj force-pushed the xdp branch 2 times, most recently from 15b61ba to fc02f68 Compare December 16, 2023 14:58
Signed-off-by: yogaraj.s <[email protected]>
Copy link
Collaborator

@danielocfb danielocfb left a comment

Choose a reason for hiding this comment

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

Seems okay. Thanks.

@danielocfb danielocfb merged commit 3d93298 into libbpf:master Dec 20, 2023
10 checks passed
danielocfb pushed a commit that referenced this pull request Dec 20, 2023
Add a CHANGELOG entry for #625 and fix up various minor details.

Signed-off-by: Daniel Müller <[email protected]>
@syogaraj syogaraj deleted the xdp branch December 21, 2023 01:08
@syogaraj
Copy link
Author

syogaraj commented Jan 2, 2024

@danielocfb Can you please release a build for this?

@danielocfb
Copy link
Collaborator

Hi @syogaraj , we just released 0.22.0 and haven't accumulated much since then. Could it wait another week or so?

@syogaraj
Copy link
Author

syogaraj commented Jan 3, 2024

We really need this support in the libbpf-rs for our use case. Can we expect the release in a week? Asking to avoid manually building it from source and use them.

@danielocfb
Copy link
Collaborator

We can publish a new release this week.

@danielocfb
Copy link
Collaborator

FYI, I just pushed 0.22.1.

@syogaraj
Copy link
Author

syogaraj commented Jan 5, 2024

Thank you so much!

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.

2 participants