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

Suppress dead code warning, as dead code appears to be intentional. #29

Merged
merged 2 commits into from Sep 14, 2018

Conversation

Projects
None yet
2 participants
@bolinfest
Contributor

bolinfest commented Sep 13, 2018

When doing a fresh compile, rustc complains that table and
callbacks are unused in PerfMap. There is a comment in struct PerfMap that explains their presence, so it seems best to eliminate the
noise with a #[allow(dead_code)].

Suppress dead code warning, as dead code appears to be intentional.
When doing a fresh compile, `rustc` complains that `table` and
`callbacks` are unused in `PerfMap`. There is a comment in `struct
PerfMap` that explains their presence, so it seems best to eliminate the
noise with a `#[allow(dead_code)]`.
@bolinfest

This comment has been minimized.

Contributor

bolinfest commented Sep 13, 2018

Sorry, I meant to make these two commits separate pull requests. I'm not so good with the GitHub-PR flow...

@bolinfest

This comment has been minimized.

Contributor

bolinfest commented Sep 13, 2018

I'm happy to try to clean it up if you'd prefer. Also, I can try to move the autoformatting in my second commit into its own commit: I'm not sure if the file was previously not auto-formatted or if my settings for rustfmt are different than yours.

@brayniac

This comment has been minimized.

Collaborator

brayniac commented Sep 13, 2018

Hi @bolinfest - thanks for your submission. I'd accept the dead code warning fix and Drop trait as one PR. If we can leave-off the rustfmt changes for now, I think we probably want to add a rustfmt config to make sure we're more consistent with rustfmt style going forward. This should be handled in a separate PR imo.

@bolinfest

This comment has been minimized.

Contributor

bolinfest commented Sep 13, 2018

@brayniac OK, I believe I properly backed the formatting changes out of this stack.

@bolinfest

This comment has been minimized.

Contributor

bolinfest commented Sep 13, 2018

Also, is the opensnoop example supposed to work? I tried to run it to verify my changes didn't break anything, but I got:

$ sudo ./target/debug/examples/opensnoop
cannot attach kprobe, Invalid argument
write(-:kprobes/r_do_sys_open_bcc_26648): Device or resource busy
Error: todo: oh no
@brayniac

This comment has been minimized.

Collaborator

brayniac commented Sep 14, 2018

@bolinfest - I believe opensnoop example was working previously. Also noticing CI has stopped running automatically for PRs following repo migration to the rust-bpf org. Looking into both these aspects.

@brayniac

This comment has been minimized.

Collaborator

brayniac commented Sep 14, 2018

The opensnoop example works for me both on your branch as well as on master. Do the other examples fail as well?

Also, I've fixed the Travis CI integration. Can you try a force-push to your working branch? I'm hoping that will trigger a CI run for this PR.

Thanks!

@bolinfest

This comment has been minimized.

Contributor

bolinfest commented Sep 14, 2018

Given the nature of the error message I saw, I tried running https://github.com/iovisor/bcc/blob/master/introspection/bps.c to list all BPF programs to see if I had some other kprobe that was interfering. I got:

$ sudo ./build/introspection/bps 
      BID TYPE                 UID  #MAPS LoadTime     NAME           
      166 cgroup skb             0      2 Sep13/16:42                 
      167 cgroup skb             0      2 Sep13/16:42                 
      168 cgroup skb             0      2 Sep13/16:42                 
      169 cgroup skb             0      2 Sep13/16:42                 
      170 cgroup skb             0      2 Sep13/16:42                 
      171 cgroup skb             0      2 Sep13/16:42      

so no other kprobes in the list...I'll try rebooting just to be extra sure.

If it matters, I'm on Ubutnu 18.04. What have you been testing with?

@bolinfest

This comment has been minimized.

Contributor

bolinfest commented Sep 14, 2018

I had to change the code so I could force push, so I changed this:

fn drop(&mut self) {

to this:

fn drop(&mut self) -> () {
@bolinfest

This comment has been minimized.

Contributor

bolinfest commented Sep 14, 2018

Let's see if changing it back still triggers a new CI run...

Implement Drop trait for BPF.
Looking at the Python frontend for bcc, it appears that
the pointer for the "program" returned by any of:

* `bpf_module_create_c_from_string()`
* `bpf_module_create_b()`
* `bpf_module_create_c()`

Should be cleaned up when the `BPF` object is destroyed via
`bpf_module_destroy()` from libbpf:

https://github.com/iovisor/bcc/blob/0cae0dd3ad34738259a8843dca788f41e8dfa634/src/python/bcc/__init__.py#L1250

Admittedly, the current version of `bpf_common.h` leaves a bit to be
desired with respect to documentation today, so this is not obvious:

https://github.com/iovisor/bcc/blob/0cae0dd3ad34738259a8843dca788f41e8dfa634/src/cc/bpf_common.h
@bolinfest

This comment has been minimized.

Contributor

bolinfest commented Sep 14, 2018

I haven't rebooted yet. I still can't get opensnoop to work, but sudo target/debug/examples/strlen seems to work.

@bolinfest

This comment has been minimized.

Contributor

bolinfest commented Sep 14, 2018

My problem with opensnoop may be due to the version of libbpf being targeted.

I believe the latest libbpf.h defines bpf_attach_kprobe() to take 5 arguments:

https://github.com/iovisor/bcc/blob/0cae0dd3ad34738259a8843dca788f41e8dfa634/src/cc/libbpf.h#L71-L72

But it appears that src/core/kprobe.rs passes 9 arguments:

bpf_attach_kprobe(
code.as_raw_fd(),
attach_type,
cname.as_ptr(),
cfunction.as_ptr(),
pid,
cpu,
group_fd,
None,
ptr::null_mut(),
)

To be fair, https://github.com/rust-bpf/bcc-sys mentions that different versions of the bcc-sys crate target different versions of bcc. rust-bcc currently targets version 0.4.0 of bcc-sys whereas the latest is 0.7.0.

What is the policy of this crate with respect to updating its bcc-sys dependency?

@brayniac

This comment has been minimized.

Collaborator

brayniac commented Sep 14, 2018

@bolinfest - please take a look at #27 - basically, we need the ability to target multiple bcc versions with bcc-sys. That work is not completed at this time.

The opensnoop example works fine with your changes. I'm going to proceed to merge this PR.

Thanks again for your submission.

@brayniac brayniac merged commit 7862205 into rust-bpf:master Sep 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment