-
Notifications
You must be signed in to change notification settings - Fork 271
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: support non-UTF8 probing #765
Conversation
✅ Deploy Preview for aya-rs-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hey @alessandrod, this pull request changes the Aya Public API and requires your review. |
aya/src/programs/kprobe.rs
Outdated
@@ -71,8 +71,12 @@ impl KProbe { | |||
/// target function. | |||
/// | |||
/// The returned value can be used to detach from the given function, see [KProbe::detach]. | |||
pub fn attach(&mut self, fn_name: &str, offset: u64) -> Result<KProbeLinkId, ProgramError> { | |||
attach(&mut self.data, self.kind, Path::new(fn_name), offset, None) | |||
pub fn attach<T: AsRef<Path>>( |
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.
As per discord conversation, this is not a Path
.
I think the most accurate and intuitive (for users) type would be OsStr
and AsRef<OsStr>
works for the common case attach("foo")
. To work with bytes though you have to type attach(OsStr::from_bytes(b"foo"))
since OsStr
can only be created from bytes via std::os::unix::ffi::OsStrExt
.
Therefore I'm leaning towards just making it AsRef<[u8]>
for convenience.
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.
PTAL; this is now OsStr
. You'll note that there were two conversions to OsStr
downstream of this, which have been removed.
Note also something strange in UProbe::attach
; the path to a library ends up being used as a function name passed to the common attach
function. I don't quite understand what's going on there -- I just added the conversions necessary to appease the type checker.
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.
lgtm. The argument to the common attach()
is misnamed, it should be target
and not fn_name
: it's a function name for kprobes, and a path for uprobes. That change doesn't belong to this PR tho so feel free to merge.
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, I dug around a bit and came to the same conclusion. It looks like the meaning of this argument differs based on the type of probe. We could probably do better with the type safety of ProbeKind
to encode this in the type system, but it seems correct. I've added a comment.
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.
Yes I think someone even mentioned this ProbeKind thing in a recent-ish PR?
5199bd0
to
cf905f5
Compare
cf905f5
to
1ccfdbc
Compare
Fixes #751.