-
Notifications
You must be signed in to change notification settings - Fork 312
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
feat: Allow conversions to Program from ProgramInfo #1209
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for aya-rs-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
7509f86
to
dc12a10
Compare
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 6 of 8 files at r1, all commit messages.
Reviewable status: 6 of 8 files reviewed, 17 unresolved discussions (waiting on @alessandrod)
aya/src/programs/uprobe.rs
line 144 at r1 (raw file):
} /// Constructs an instance of [UProbe] from a [ProgramInfo].
missing ticks on types
aya/src/programs/uprobe.rs
line 155 at r1 (raw file):
/// check since the kernel doesn't expose this information. /// The caller is responsible for ensuring that the program /// type is correct (i.e they previously loaded it) otherwise
same "is correct"
aya/src/programs/uprobe.rs
line 160 at r1 (raw file):
/// # Errors /// /// - If the program type is not [`ProgramType::KProbe`]
kprobe? we're in uprobe. too many spaces
aya/src/programs/uprobe.rs
line 177 at r1 (raw file):
/// # Ok::<(), aya::programs::ProgramError>(()) /// ``` pub unsafe fn from_program_info(
how come these are one-offs for kprobe and uprobe and not defined using the macro?
aya/src/programs/probe.rs
line 27 at r1 (raw file):
/// Kind of probe program #[derive(Debug, Copy, Clone, PartialEq)]
🤔
test/integration-test/src/tests/info.rs
line 59 at r1 (raw file):
); for program in programs {
maybe a comment to explain why?
test/integration-test/src/tests/info.rs
line 61 at r1 (raw file):
for program in programs { let mut p: SocketFilter = SocketFilter::from_program_info(Some("simple_prog".to_string()), program).unwrap();
sniped me into #1211
aya/src/programs/mod.rs
line 997 at r1 (raw file):
$( impl $struct_name { /// Consructs a program from a [ProgramInfo].
missing ticks
aya/src/programs/mod.rs
line 1004 at r1 (raw file):
/// # Errors /// /// - If the program type is not a match
"does not match"
also, doesn't match what? please be specific
also missing periods on both bullets
aya/src/programs/mod.rs
line 1009 at r1 (raw file):
name: Option<String>, info: ProgramInfo, ) -> Result<$struct_name, ProgramError> {
use Self
aya/src/programs/mod.rs
line 1013 at r1 (raw file):
return Err(ProgramError::UnexpectedProgramType {}); } Ok($struct_name {
this can be Self
aya/src/programs/mod.rs
line 1016 at r1 (raw file):
data: ProgramData::from_bpf_prog_info( name, crate::MockableFd::from_fd(info.fd()?.as_fd().try_clone_to_owned()?),
can we avoid all these inline ?
s?
aya/src/programs/mod.rs
line 1018 at r1 (raw file):
crate::MockableFd::from_fd(info.fd()?.as_fd().try_clone_to_owned()?), Path::new(""), info.0,
would you mind destructuring instead?
aya/src/programs/mod.rs
line 1033 at r1 (raw file):
impl $struct_name { /// Consructs a program from a [ProgramInfo].
same comments as above. comments and code both
aya/src/programs/mod.rs
line 1086 at r1 (raw file):
/// ambiguous due to missing information in the kernel. /// The caller is responsible for ensuring that the program /// type is correct otherwise the behavior is undefined.
same comments as above plus explaining what "is correct" means
aya/src/programs/mod.rs
line 1109 at r1 (raw file):
} impl_from_prog_info_unsafe!(
why are these out of declaration order?
aya/src/programs/mod.rs
line 1135 at r1 (raw file):
impl_from_prog_info_attach_type!( CgroupSkb, Option<CgroupSkbAttachType>,
these two items that go together should be on the same line (and maybe drop the trailing comma on each line for clarity)
otherwise you could make the macro take just one pair and repeat its invocation. as written this is confusing.
Code quote:
CgroupSkb,
Option<CgroupSkbAttachType>,
dc12a10
to
7e54054
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: 1 of 8 files reviewed, 17 unresolved discussions (waiting on @alessandrod and @tamird)
aya/src/programs/mod.rs
line 997 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
missing ticks
Done
aya/src/programs/mod.rs
line 1004 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
"does not match"
also, doesn't match what? please be specific
also missing periods on both bullets
Done
aya/src/programs/mod.rs
line 1009 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
use Self
done
aya/src/programs/mod.rs
line 1013 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this can be Self
done
aya/src/programs/mod.rs
line 1016 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can we avoid all these inline
?
s?
done
aya/src/programs/mod.rs
line 1018 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
would you mind destructuring instead?
done
aya/src/programs/mod.rs
line 1033 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
same comments as above. comments and code both
done
aya/src/programs/mod.rs
line 1086 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
same comments as above plus explaining what "is correct" means
done
aya/src/programs/mod.rs
line 1109 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
why are these out of declaration order?
I wasn't aware that was something we were enforcing. done.
aya/src/programs/mod.rs
line 1135 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
these two items that go together should be on the same line (and maybe drop the trailing comma on each line for clarity)
otherwise you could make the macro take just one pair and repeat its invocation. as written this is confusing.
agreed. i've taken another pass at the macro to address this comment and the previous one.
I've made it more generic so we can keep programs in declaration order... and the arguments are passed in tuples so it's clear they belong together.
test/integration-test/src/tests/info.rs
line 59 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
maybe a comment to explain why?
done
test/integration-test/src/tests/info.rs
line 61 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
sniped me into #1211
thank you!
aya/src/programs/probe.rs
line 27 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
🤔
removed. was only previously required due to test of equality on this type
aya/src/programs/uprobe.rs
line 144 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
missing ticks on types
ticks added.
aya/src/programs/uprobe.rs
line 155 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
same "is correct"
done
aya/src/programs/uprobe.rs
line 160 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
kprobe? we're in uprobe. too many spaces
done
aya/src/programs/uprobe.rs
line 177 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
how come these are one-offs for kprobe and uprobe and not defined using the macro?
that was because of the additional check for ProbeKind. I've relaxed that check as the function is unsafe anyway and the safety docs tell you what you should be doing.
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 6 of 7 files at r2, all commit messages.
Reviewable status: 7 of 8 files reviewed, 12 unresolved discussions (waiting on @alessandrod)
aya/src/programs/mod.rs
line 1011 at r2 (raw file):
/// /// - If the program type reported by the kernel does not match /// that of the type you're converting to.
can we do something code-like here?
"...does not match [`Self::SomeAssociatedConstant`]"
```?
aya/src/programs/mod.rs
line 1020 at r2 (raw file):
return Err(ProgramError::UnexpectedProgramType {}); } let ProgramInfo { 0: bpf_progam_info} = info;
whoa i have never seen this syntax. why not write let ProgramInfo(bpf_program_info) = info
?
aya/src/programs/mod.rs
line 1026 at r2 (raw file):
Ok(Self { data: ProgramData::from_bpf_prog_info( name.map(|n| Cow::Borrowed(n)),
this can just be name.map(Into::into)
if you want
aya/src/programs/mod.rs
line 1133 at r2 (raw file):
/// concrete type of the program. It's up to the caller to ensure /// that the program type matches the type you're converting to. /// Otherwise, the behavior is undefined.
it took me a minute to understand this comment. maybe you could be more specific and say that the runtime type of KProbe and UProbe is the same, and so it can't be entirely checked. This is doubly confusing when you realize that the implementation does check the type.
Code quote:
/// We can't safely cast to `ProgramInfo` since we don't know the
/// concrete type of the program. It's up to the caller to ensure
/// that the program type matches the type you're converting to.
/// Otherwise, the behavior is undefined.
aya/src/programs/mod.rs
line 1163 at r2 (raw file):
// Order of arguments is as follows: // - Program, bpf_prog_type, unsafe // - Program, bpf_prog_type, unsafe, additional variable, variable type
What if the syntax was:
unsafe KProbe => ProgramType::KProbe, kind: ProbeKind
TracePoint => ProgramType::TracePoint
Xdp => ProgramType::Xdp, attach_type: XdpAttachType
(doesn't look like unsafe with extra param ever appears)
it would be awesome if we could do this without all this repetition of the doc comments. maybe we could do that with an optional fragment for the variable and its type plus a common macro shared by safe and unsafe?
test/integration-test/src/tests/info.rs
line 59 at r2 (raw file):
); // Iterate through loaded programs to exercise `from_program_info()`.
👍
test/integration-test/src/tests/info.rs
line 64 at r2 (raw file):
let mut p: UProbe = unsafe { UProbe::from_program_info( Some("simple_prog"),
do want this to also take a Cow<'static, str> so that dynamic strings are supported, or do we think this is always going to be static?
test/integration-test/src/tests/info.rs
line 71 at r2 (raw file):
}; // Ensure we can perform basic operations on the re-created program.
👍
35423b4
to
e382a86
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: 3 of 30 files reviewed, 12 unresolved discussions (waiting on @alessandrod and @tamird)
aya/src/programs/mod.rs
line 1011 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can we do something code-like here?
"...does not match [`Self::SomeAssociatedConstant`]" ```?
done
aya/src/programs/mod.rs
line 1020 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
whoa i have never seen this syntax. why not write
let ProgramInfo(bpf_program_info) = info
?
lol. yeah it felt weird. thanks for the correction.
aya/src/programs/mod.rs
line 1026 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this can just be
name.map(Into::into)
if you want
done
aya/src/programs/mod.rs
line 1133 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
it took me a minute to understand this comment. maybe you could be more specific and say that the runtime type of KProbe and UProbe is the same, and so it can't be entirely checked. This is doubly confusing when you realize that the implementation does check the type.
done
aya/src/programs/mod.rs
line 1163 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
What if the syntax was:
unsafe KProbe => ProgramType::KProbe, kind: ProbeKind TracePoint => ProgramType::TracePoint Xdp => ProgramType::Xdp, attach_type: XdpAttachType
(doesn't look like unsafe with extra param ever appears)
it would be awesome if we could do this without all this repetition of the doc comments. maybe we could do that with an optional fragment for the variable and its type plus a common macro shared by safe and unsafe?
Adding a constant removed the need to use ProgramType
at all.
It took me a long time, but the macro syntax is now:
unsafe KProbe kind : ProbeKind,
TracePoint.
Xdp attach_type : XdpAttachType,
unsafe FEntry
No repetition, and safety docs are included for unsafe impls.
test/integration-test/src/tests/info.rs
line 59 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
👍
Done.
test/integration-test/src/tests/info.rs
line 64 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
do want this to also take a Cow<'static, str> so that dynamic strings are supported, or do we think this is always going to be static?
I realised that the name field is never really used. Therefore I can remove it from the public API and just pass in None without there being any weird side-effects.
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 27 of 27 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @alessandrod)
aya/src/programs/perf_event.rs
line 131 at r3 (raw file):
impl PerfEvent { /// The type of the program according to the kernel. pub const PROGRAM_TYPE: ProgramType = ProgramType::PerfEvent;
hmm we don't want to put this associated constant into some trait they all implement?
aya/src/programs/mod.rs
line 329 at r3 (raw file):
impl Program { /// Returns the program type. pub fn prog_type(&self) -> ProgramType {
it would be great to deduplicate this against the new PROGRAM_TYPE constants
aya/src/programs/mod.rs
line 1029 at r3 (raw file):
Ok(Self { data: ProgramData::from_bpf_prog_info( None,
I think we need a comment here to explain why this is OK
aya/src/programs/mod.rs
line 1091 at r3 (raw file):
SkLookup, CgroupDevice, Iter
no comma here?
e382a86
to
8051b83
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: all files reviewed, 4 unresolved discussions (waiting on @alessandrod and @tamird)
aya/src/programs/mod.rs
line 329 at r3 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
it would be great to deduplicate this against the new PROGRAM_TYPE constants
done
aya/src/programs/mod.rs
line 1029 at r3 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
I think we need a comment here to explain why this is OK
I was too hasty to assume this wasn't important. We use it when calculating a pin path, so a call to program::pin()
would fail if this name is not set. I've adjusted the public API to make this mandatory to avoid this happening.
aya/src/programs/mod.rs
line 1091 at r3 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
no comma here?
ugh. I had to add a branch to the macro to handle the trailing comma 🤣 fixed.
aya/src/programs/perf_event.rs
line 131 at r3 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
hmm we don't want to put this associated constant into some trait they all implement?
we should, but that's a larger refactor that I don't want to take on right now.
a lot of what we do with Program
, Map
etc.. can be simplified with traits, but I don't have the time to design it.
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 r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alessandrod and @dave-tucker)
aya/src/programs/mod.rs
line 329 at r3 (raw file):
Previously, dave-tucker (Dave Tucker) wrote…
done
OK yeah that's something, I was hoping yet less duplication though I don't have ideas :(
1009158
to
8393d74
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.
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alessandrod and @dave-tucker)
test/integration-test/src/tests/info.rs
line 75 at r5 (raw file):
.unwrap(); // Ensure the program can be detached
period
Allow for a ProgramInfo to be converted into one of the program types that we support. This allows for a user of Aya access to reattach, pin or unload a program that was either, previously loaded, or was loaded by another process. Signed-off-by: Dave Tucker <[email protected]>
8393d74
to
1a1275d
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.
FYI you didn't reply to comments (though you did address one) so I didn't know this was ready for additional review.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alessandrod and @dave-tucker)
Allow for a ProgramInfo to be converted into one of the program types that we support. This allows for a user of Aya access to reattach, pin or unload a program that was either, previously loaded, or was loaded by another process.
This change is