aya icon indicating copy to clipboard operation
aya copied to clipboard

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

Open tyrone-wu opened this issue 1 year ago • 2 comments

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:

  • Have program_type() to return bpf_prog_type instead of the integer representation.
  • For fields that can't be 0, return Option<NonZero*> type to indicate None as field being unavailable.
  • Have name_as_str() use the bpf_name() feature probe as an additional check for whether field is available. Besides invalid unicode, None is returned if probe does not detect feature.
  • For mad_ids():
    • Now returns a result of Option<Vec<NonZeroU32>>, with None indicating field is unavailable.
    • On kernels below v4.15, the init closure that fills in the map info causes an E2BIG error since check_uarg_tail_zero() expects the entire struct to be zero bytes. I initially worked around this by having bpf_prog_get_info_by_fd() do an additional syscall if the first failed with E2BIG.
    • Now, map_ids() and bpf_prog_get_info_by_fd() both uses a new feature probe prog_info_map_ids() to detect if field available.
  • For gpl_compatible(), it now returns Option<bool>, with None indicating field is unavailable. It uses a new feature probe prog_info_gpl_compatible() to detect for field availability.

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:

  • Have map_type() to return bpf_map_type instead of the integer representation.
  • For fields that can't be 0, return Option<NonZero*> type to indicate None as field being unavailable.
  • For name_as_str(), it uses the bpf_name() feature probe to detect for field availability. Although the probe performs the check on program name, map name was introduced in the same commit as program name so it should suffice.

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):

  • 4.13.0
  • 4.15.0
  • 5.15.0
  • 6.1.0

tyrone-wu avatar Aug 04 '24 23:08 tyrone-wu

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.

netlify[bot] avatar Aug 04 '24 23:08 netlify[bot]

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

mergify[bot] avatar Aug 04 '24 23:08 mergify[bot]

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

mergify[bot] avatar Sep 01 '24 13:09 mergify[bot]

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.

tamird avatar Sep 06 '24 10:09 tamird

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> {
}

alessandrod avatar Sep 06 '24 10:09 alessandrod

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.

tamird avatar Sep 06 '24 11:09 tamird

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.

alessandrod avatar Sep 06 '24 11:09 alessandrod