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.

Sign up for GitHub

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

Jump to bottom

Fix clippy warnings #80

Merged
merged 1 commit into from May 10, 2021
Merged

Fix clippy warnings #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions libbpf-cargo/src/btf/btf.rs
View file
Open in desktop
Expand Up @@ -22,9 +22,12 @@ pub struct Btf<'a> {
impl<'a> Btf<'a> {
pub fn new(name: &str, object_file: &[u8]) -> Result<Option<Self>> {
let cname = CString::new(name)?;
let mut obj_opts = libbpf_sys::bpf_object_open_opts::default();
obj_opts.sz = std::mem::size_of::<libbpf_sys::bpf_object_open_opts>() as libbpf_sys::size_t;
obj_opts.object_name = cname.as_ptr();
let obj_opts = libbpf_sys::bpf_object_open_opts {
sz: std::mem::size_of::<libbpf_sys::bpf_object_open_opts>() as libbpf_sys::size_t,
object_name: cname.as_ptr(),
..Default::default()
Copy link
Member

Choose a reason for hiding this comment

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

does this ensure that all the bytes are zeroed out, even padding?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so.

Example: https://godbolt.org/z/r75T13Pcd

Rust generates code to call ::default() which is implemented by https://docs.rs/libbpf-sys/0.3.0-1/src/libbpf_sys/bindings.rs.html#2987-2991 and the implementation zeros out the struct using https://doc.rust-lang.org/stable/std/mem/fn.zeroed.html . From reading the docs there and looking at rust-lang/rust#70230 , I don't believe bindgen is generating the right code here. It should probably generate a memset.

Also note that this PR doesn't change the old behavior. Functionally it should be the same as before.

Copy link
Member

Choose a reason for hiding this comment

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

There's definitely padding: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=b7342c8fe84d6bb1e01333e4e07f7a79 (48 vs 38). I think this should be a bindgen bug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danobi I think I'm following along, though I'm still p new to systems programming.

It sounds like the padding is bad because we want libbpf-rs to be as conservative as possible when it comes to memory, yeah? Also, It seems like rust-lang/rust#70230 is going to take some time to resolve, and when it does it'll be in the nightly release at first.

I can revert these changes around calling default() and add a comment so that clippy ignores em. That work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed up the revert - let me know if that's not the right path, and I can undo em

Copy link
Member

@danobi danobi May 10, 2021 •
edited

Choose a reason for hiding this comment

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

I think your original changes were good. I think it's a bug with bindgen (a tool used to generate rust bindings to c/c++ libraries). I'm working on fixing the bindgen bug in rust-lang/rust-bindgen#2051

Even then, the non-zero padding is currently a theoretical issue. We haven't seen issues with it yet -- not to say it won't happen in the future (b/c bindgen is invoking undefined behavior)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted the revert!

};

let bpf_obj = unsafe {
libbpf_sys::bpf_object__open_mem(
object_file.as_ptr() as *const c_void,
Expand Down
9 changes: 6 additions & 3 deletions libbpf-cargo/src/gen.rs
View file
Open in desktop
Expand Up @@ -538,9 +538,12 @@ fn gen_skel_link_getter(

fn open_bpf_object(name: &str, data: &[u8]) -> Result<*mut libbpf_sys::bpf_object> {
let cname = CString::new(name)?;
let mut obj_opts = libbpf_sys::bpf_object_open_opts::default();
obj_opts.sz = std::mem::size_of::<libbpf_sys::bpf_object_open_opts>() as libbpf_sys::size_t;
obj_opts.object_name = cname.as_ptr();
let obj_opts = libbpf_sys::bpf_object_open_opts {
sz: std::mem::size_of::<libbpf_sys::bpf_object_open_opts>() as libbpf_sys::size_t,
object_name: cname.as_ptr(),
..Default::default()
};

let object = unsafe {
libbpf_sys::bpf_object__open_mem(
data.as_ptr() as *const c_void,
Expand Down
5 changes: 3 additions & 2 deletions libbpf-cargo/src/metadata.rs
View file
Open in desktop
@@ -1,4 +1,5 @@
use std::fs;
use std::path::Path;
use std::path::PathBuf;

use anyhow::{bail, Result};
Expand Down Expand Up @@ -33,7 +34,7 @@ pub struct UnprocessedObj {
fn get_package(
debug: bool,
package: &Package,
workspace_target_dir: &PathBuf,
workspace_target_dir: &Path,
) -> Result<Vec<UnprocessedObj>> {
if debug {
println!("Metadata for package={}", package.name);
Expand Down Expand Up @@ -65,7 +66,7 @@ fn get_package(
};

// Respect custom target directories specified by package
let mut target_dir = workspace_target_dir.clone();
let mut target_dir = workspace_target_dir.to_path_buf();
let out_dir = if let Some(d) = package_metadata.target_dir {
if debug {
println!("Custom target_dir={}", d.to_string_lossy());
Expand Down
8 changes: 4 additions & 4 deletions libbpf-rs/src/skeleton.rs
View file
Open in desktop
Expand Up @@ -165,10 +165,10 @@ impl<'a> ObjectSkeletonConfigBuilder<'a> {
// Holds `CString`s alive so pointers to them stay valid
let mut string_pool = Vec::new();

// NB: use default() to zero out struct
Copy link
Contributor Author

@shaqq shaqq May 10, 2021 •
edited

Choose a reason for hiding this comment

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

@danobi I also got rid of this, since it appears to be a comment about zeroing out the bytes, but it sounds like we'll fix it with the bind-gen bugfix

let mut s = bpf_object_skeleton::default();

s.sz = size_of::<bpf_object_skeleton>() as u64;
let mut s = libbpf_sys::bpf_object_skeleton {
sz: size_of::<bpf_object_skeleton>() as u64,
..Default::default()
};

if let Some(ref n) = self.name {
s.name = str_to_cstring_and_pool(&n, &mut string_pool)?;
Expand Down