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

example: Add TCP header modification example #770

Merged
merged 2 commits into from
Apr 27, 2024

Conversation

ThisSeanZhang
Copy link
Contributor

@ThisSeanZhang ThisSeanZhang commented Apr 26, 2024

Old PR is: #767

  • Replaced tabs with spaces for consistency
  • Removed unstable features
  • Modified handling of attachment result
  • Replaced verbose options and included trigger command in README.md

Thank you for considering a contribution!

In order to streamline review experience for contributors and reviewers, please
be sure to read and follow the Contributor's Guide. It
lays out basic best practices, which, if followed will reduce unnecessary back
and forth and, ultimately, minimize the time it takes to get your change into
the library.

- Replaced tabs with spaces for consistency
- Removed unstable features
- Modified handling of attachment result
- Replaced verbose options and included trigger command in README.md
@ThisSeanZhang
Copy link
Contributor Author

How can I install a plugin in VSCode or check the same rules in CI in some other way? :P

@danielocfb
Copy link
Collaborator

How can I install a plugin in VSCode or check the same rules in CI in some other way? :P

You don't need VSCode, you can just do what the workflow does. So in your case check the clippy and/or rustfmt jobs:

clippy:
name: Lint with clippy
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Install deps
run: sudo apt-get install -y libelf-dev
- uses: dtolnay/rust-toolchain@stable
with:
components: rustfmt
- uses: Swatinem/rust-cache@v2
- run: cargo clippy --locked --no-deps --all-targets --tests -- -D warnings -D clippy::absolute_paths
rustfmt:
name: Check code formatting
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@nightly
with:
components: rustfmt
- run: cargo fmt -- --check

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.

Looks good to me now, thanks! Why did you remove the example from the workspace, though? I think it should stay there.

println!("BPF Attached Successfully!");
}
_ => {
println!("Failed to Attach BPF, Reason: {:?}", Error::last_os_error());
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 exit here or is there a good reason for continuing as if nothing happened?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I use panic or return Err or do you have a better suggestion?

panic!("Failed to Attach BPF, Reason: {:?}", Error::last_os_error());
// or
println!("Failed to Attach BPF, Reason: ");
return Err(Error::last_os_error().into());

Copy link
Collaborator

Choose a reason for hiding this comment

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

Return is probably best. Thanks!

@ThisSeanZhang
Copy link
Contributor Author

cargo clippy --locked --no-deps --all-targets --tests -- -D warnings -D clippy::absolute_paths

I executed cmd and then got this

$ cargo clippy --locked --no-deps --all-targets --tests -- -D warnings -D clippy::absolute_paths 
   Compiling libbpf-rs v0.23.0 (/home/sean/libbpf-rs/libbpf-rs)
error: transmute used without annotations
   --> libbpf-rs/src/print.rs:127:28
    |
127 |         unsafe { Some(mem::transmute(outer_print_cb as *const ())) };
    |                            ^^^^^^^^^ help: consider adding missing annotations: `transmute::<*const (), unsafe extern "C" fn(u32, *const i8, *mut libbpf_sys::__va_list_tag) -> i32>`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_transmute_annotations
    = note: `-D clippy::missing-transmute-annotations` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::missing_transmute_annotations)]`

error: could not compile `libbpf-rs` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `libbpf-rs` (lib) due to 1 previous error
error: could not compile `libbpf-rs` (lib test) due to 1 previous error

did I miss something...

@d-e-s-o
Copy link
Collaborator

d-e-s-o commented Apr 27, 2024

cargo clippy --locked --no-deps --all-targets --tests -- -D warnings -D clippy::absolute_paths

I executed cmd and then got this

$ cargo clippy --locked --no-deps --all-targets --tests -- -D warnings -D clippy::absolute_paths 
   Compiling libbpf-rs v0.23.0 (/home/sean/libbpf-rs/libbpf-rs)
error: transmute used without annotations
   --> libbpf-rs/src/print.rs:127:28
    |
127 |         unsafe { Some(mem::transmute(outer_print_cb as *const ())) };
    |                            ^^^^^^^^^ help: consider adding missing annotations: `transmute::<*const (), unsafe extern "C" fn(u32, *const i8, *mut libbpf_sys::__va_list_tag) -> i32>`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_transmute_annotations
    = note: `-D clippy::missing-transmute-annotations` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::missing_transmute_annotations)]`

error: could not compile `libbpf-rs` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `libbpf-rs` (lib) due to 1 previous error
error: could not compile `libbpf-rs` (lib test) due to 1 previous error

did I miss something...

You may be using nightly clippy or a newer version that has added this lint by default. Just allow it for now and move on?

cargo clippy --locked --no-deps --all-targets --tests -- -D warnings -D clippy::absolute_paths -A clippy::missing-transmute-annotations

- add  example to workspace
- exit program when attach fails
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.

Looks good to me, thanks again!

@danielocfb danielocfb merged commit 25a2b3b into libbpf:master Apr 27, 2024
12 checks passed
@ThisSeanZhang
Copy link
Contributor Author

Thank you very much for your help in this. 🥳

@ThisSeanZhang ThisSeanZhang deleted the example_tcp-option branch July 26, 2024 05:21
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.

3 participants