-
Notifications
You must be signed in to change notification settings - Fork 300
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
aya::programs::uprobe: add support for cookies #1133
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
b31736c
to
0c77783
Compare
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.
You'll need to run CARGO_CFG_BPF_TARGET_ARCH=x86_64 cargo +nightly xtask public-api --bless --target x86_64-unknown-linux-gnu
to regenerate the API fixtures I think.
Reviewed 33 of 33 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ajwerner)
aya/src/programs/mod.rs
line 223 at r1 (raw file):
IOError(#[from] io::Error), /// Providing a bpf cookie for perf event links is not supported.
pretty far away from "perf event links" here and it's not in the variant name
test/integration-test/src/tests/uprobe_cookie.rs
line 45 at r1 (raw file):
let read: [u8; 8] = (*read) .try_into() .with_context(|| format!("data: {:?}", read.len()))
data: {read:x}
? IOW do we want the data or just its length?
aya/src/sys/bpf.rs
line 408 at r1 (raw file):
attach_type: bpf_attach_type, flags: u32, args: Option<BpfLinkCreateArgs<'_>>,
nice.
aya/src/sys/bpf.rs
line 903 at r1 (raw file):
link, Err((_, e)) if e.raw_os_error() == Some(libc::EBADF), )
Might fit on one line
Code quote:
matches!(
link,
Err((_, e)) if e.raw_os_error() == Some(libc::EBADF),
)
aya/src/programs/uprobe.rs
line 51 at r1 (raw file):
} /// The location of in the target object file to which the uprobe is attached.
"of in"
aya/src/programs/uprobe.rs
line 62 at r1 (raw file):
} impl<'a> From<&'a str> for UProbeAttachLocation<'a> {
should there be a From<u64>
impl?
0c77783
to
b4a96d0
Compare
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.
Thanks for the review!
You'll need to run
CARGO_CFG_BPF_TARGET_ARCH=x86_64 cargo +nightly xtask public-api --bless --target x86_64-unknown-linux-gnu
to regenerate the API fixtures I think.
Needed a rustup update
to get this working, should be good now.
Reviewable status: 27 of 34 files reviewed, 5 unresolved discussions (waiting on @tamird)
aya/src/programs/mod.rs
line 223 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
pretty far away from "perf event links" here and it's not in the variant name
Made the error more specific to the cause rather than the source.
aya/src/programs/uprobe.rs
line 51 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
"of in"
Done.
aya/src/programs/uprobe.rs
line 62 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
should there be a
From<u64>
impl?
Sure, done.
aya/src/sys/bpf.rs
line 903 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Might fit on one line
Done.
test/integration-test/src/tests/uprobe_cookie.rs
line 45 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
data: {read:x}
? IOW do we want the data or just its length?
Done.
Hey @alessandrod, this pull request changes the Aya Public API and requires your 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.
Reviewed 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ajwerner and @alessandrod)
aya/src/programs/uprobe.rs
line 51 at r1 (raw file):
Previously, ajwerner wrote…
Done.
"to which the uprobe is to be attached"?
test/integration-test/src/tests/uprobe_cookie.rs
line 45 at r2 (raw file):
match read.try_into() { Ok(read) => seen.push(u64::from_le_bytes(read)), Err(_) => panic!("invalid ring buffer data: {read:x?}"),
Err(std::array::TryFromSliceError { .. })
for the OCD
Fixes aya-rs#1132. Note that this change does not add support in the public API for kprobes or tracepoints, but it's a trivial matter of plumbing. Along the way, the Uprobe::attach API is cleaned up to make the attachment location more coherent. The logic being: if we're going to be breaking the API anyway, may as well clean it up a bit. Furthermore, the aya::sys::bpf_link_attach function is cleaned up by properly modeling the the union in the final field with a rust enum.
b4a96d0
to
628b7fb
Compare
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.
Reviewable status: 32 of 34 files reviewed, 2 unresolved discussions (waiting on @alessandrod and @tamird)
aya/src/programs/uprobe.rs
line 51 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
"to which the uprobe is to be attached"?
Done.
test/integration-test/src/tests/uprobe_cookie.rs
line 45 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Err(std::array::TryFromSliceError { .. })
for the OCD
Done.
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.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alessandrod)
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.
10/10 no notes thanks both!
Fixes #1132.
Note that this change does not add support in the public API for kprobes or tracepoints, but it's a trivial matter of plumbing.
Along the way, the Uprobe::attach API is cleaned up to make the attachment location more coherent. The logic being: if we're going to be breaking the API anyway, may as well clean it up a bit.
This change is