Skip to content
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

fix unsupported maps warning message #1092

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

banditopazzo
Copy link
Contributor

@banditopazzo banditopazzo commented Nov 18, 2024

fix #1081

the proposed solution is to remove the warning log and instead provide more meaningful information directly within the error itself.

without considering the issue, right now we have just a warning log (which contains the important information), but this is misleading since it actually leads to an error. I believe this can cause confusion because the critical details should be part of the error itself. For this reason I consolidated all the relevant information into the error message.

Additionally, I updated the map_type field type in Unsupported error fromu32 to bpf_map_type, as an unsupported map has a specific map type, which is distinct from a generic u32 used in cases like the InvalidMapType error.

PS: Regarding InvalidMapType and other map-related errors, I believe they should also include the names of the maps.


This change is Reviewable

Copy link

netlify bot commented Nov 18, 2024

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit a1cacec
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/673f55b741b5e60008050f1c
😎 Deploy Preview https://deploy-preview-1092--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 mergify bot added the aya This is about aya (userspace) label Nov 18, 2024
Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good. Please run CARGO_CFG_BPF_TARGET_ARCH=x86_64 cargo +nightly xtask public-api --bless --target x86_64-unknown-linux-gnu to regenerate the API file.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @banditopazzo)


aya/src/maps/mod.rs line 198 at r1 (raw file):

    /// Unsupported Map type
    #[error("the map {name} is of type {map_type:#?} which is currently unsupported in Aya, use `allow_unsupported_maps()` to load it anyways")]

can we make this a little shorter? seems way too long.

@banditopazzo banditopazzo force-pushed the 1081-fix-unsupported-map-warning branch from b87361f to 3f5ce23 Compare November 21, 2024 14:52
Copy link

mergify bot commented Nov 21, 2024

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

@mergify mergify bot added the api/needs-review Makes an API change that needs review label Nov 21, 2024
@mergify mergify bot requested a review from alessandrod November 21, 2024 14:52
@mergify mergify bot added the test A PR that improves test cases or CI label Nov 21, 2024
Copy link
Member

@tamird tamird left a 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 r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alessandrod and @banditopazzo)


aya/src/maps/mod.rs line 198 at r2 (raw file):

    /// Unsupported Map type
    #[error("map {name} is of type {map_type:#?}, unsupported in Aya; use `allow_unsupported_maps()` to load it")]

Why do we need {:#?} rather than {:?}?

#[error("type of {name} ({map_type:?}) is unsupported; see `EbpfLoader::allow_unsupported_maps`")]

Code quote:

 #[error("map {name} is of type {map_type:#?}, unsupported in Aya; use `allow_unsupported_maps()` to load it")]

Remove the warning log altogether; either it's an error or it isn't.
@tamird tamird force-pushed the 1081-fix-unsupported-map-warning branch from 3f5ce23 to a1cacec Compare November 21, 2024 15:45
Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @alessandrod)

@tamird
Copy link
Member

tamird commented Nov 21, 2024

Thanks!

@tamird tamird merged commit a167550 into aya-rs:main Nov 21, 2024
24 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api/needs-review Makes an API change that needs review aya This is about aya (userspace) test A PR that improves test cases or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unsupported map type warning message doesn't go away when unsupported type maps are allowed
2 participants