aya-rs / aya

Aya is an eBPF library for the Rust programming language, built with a focus on developer experience and operability.
https://aya-rs.dev/book/
Apache License 2.0
3.25k stars 291 forks source link

aya,aya-obj,integration-test: add better support in `ProgramInfo` & `MapInfo` for old kernels #1007

Closed tyrone-wu closed 2 months ago

tyrone-wu commented 3 months ago

This adds support for the field availability in ProgramInfo and MapInfo.

Srry this is long, let me know and I can try to TL;DR it.

For ProgramInfo:

There was a redundant export of loaded_programs() which gave it 2 method of access: aya::loaded_programs and aya::programs::loaded_programs. To avoid confusion, it should now only be aya::programs::loaded_programs.

For MapInfo:

Integration tests for ProgramInfo and MapInfo are updated to use kernel_assert/kernel_assert_eq macro. The macro first performs the assertion check. If assertion passes, continue with test. If failed, then check host's kernel version. If host is below the feature version, then continue with test and log it in stderr (can be observed with -- --nocapture). Otherwise, panic šŸ˜°.

I verified that this passes on versions (i use ubuntu mainline images for my integration tests):

netlify[bot] commented 3 months ago

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
Latest commit fbb09304a2de0d8baf7ea20c9727fcd2e4fb7f41
Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/66d5def807650600083b06fb
Deploy Preview https://deploy-preview-1007--aya-rs-docs.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mergify[bot] commented 3 months ago

Hey @alessandrod, this pull request changes the Aya Public API and requires your review.

mergify[bot] commented 2 months ago

@tyrone-wu, this pull request is now in conflict and requires a rebase.

tamird commented 2 months ago

Overall, I agree with @alessandrod, we shouldn't use Option<NonZero*> as return values but Option<IntegerType> instead (0 mapping to None)

Why is that better? If we know they can't be zero it's better to communicate that in the type.

alessandrod commented 2 months ago

Why is that better? If we know they can't be zero it's better to communicate that in the type.

Because API ergonomics matter.

NonZero<T> doesn't Deref to T, so I have to call .get(). Having to call get seems worse than having extra information there's no use for, especially since NonZero<T>::new(v) return Option<NonZero<T>>, so I have to match and get.

// This _does_ provide useful information - I can't call f(0)
fn f(x: NonZeroU32) {
}

// Option tells me whether the map_id is there or not. Then what use is it to me to know that the map id won't be 0? 
// This would be useful if I were to pass the return value to something else, but we have stronger wrappers for things like ProgramIds etc.
fn map_id(&self) -> Option<NonZeroU32> {
}
tamird commented 2 months ago

Why is that better? If we know they can't be zero it's better to communicate that in the type.

Because API ergonomics matter.

NonZero<T> doesn't Deref to T, so I have to call .get(). Having to call get seems worse than having extra information there's no use for, especially since NonZero<T>::new(v) return Option<NonZero<T>>, so I have to match and get.

// This _does_ provide useful information - I can't call f(0)
fn f(x: NonZeroU32) {
}

// Option tells me whether the map_id is there or not. Then what use is it to me to know that the map id won't be 0? 
// This would be useful if I were to pass the return value to something else, but we have stronger wrappers for things like ProgramIds etc.
fn map_id(&self) -> Option<NonZeroU32> {
}

Then why expose the scalar at all? If the value doesn't matter, make it opaque.

alessandrod commented 2 months ago

Then why expose the scalar at all? If the value doesn't matter, make it opaque.

We should definitely have types for all these IDs, but babysteps. Even without stronger ids this PR was an improvement over what was there before, and removing NonZeroU32 is an improvement over what's there, imo.