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

WIP: Reduce Explosion of Aya Error Types #1061

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dave-tucker
Copy link
Member

@dave-tucker dave-tucker commented Oct 18, 2024

I have been down a rabbit hole of cleaning up the aya error types 😅

Most of the important changes are in errors.rs.

TL;DR

Current exposed types are:

  • EbpfError
  • ProgramError
  • MapError
  • LinkError
  • PerfBufferError
  • SysError

Honestly I'm still thinking about how we could collapse those types.
Either into a single type, or at least fewer than we expose today.

Within each of those types, I've tried to remove any invariants
that don't have any business being public (e.g if a syscall fails
with -EINVAL, there is nothing at runtime you can do about it
other than bailing).

👆 (and the spaghetti of errors depending on other errors) are
replaced by an Other invariant that's a Box<dyn std::error::Error>.

There are still some pub(crate) XInternalError types, but these
are used only to make nice error messages. This could plausibly be
replaced with anyhow/context etc.. But I've left it as-is for now.


This change is Reviewable

Copy link

netlify bot commented Oct 18, 2024

Deploy Preview for aya-rs-docs failed.

Name Link
🔨 Latest commit e899c8e
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/67129caada3be000087d808b

Copy link

mergify bot commented Oct 18, 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 Oct 18, 2024
@mergify mergify bot requested a review from alessandrod October 18, 2024 16:31
@mergify mergify bot added aya This is about aya (userspace) aya-log Relating to aya-log aya-obj Relating to the aya-obj crate test A PR that improves test cases or CI labels Oct 18, 2024
I have been down a rabbit hole of cleaning up the aya error types 😅

Most of the important changes are in `errors.rs`.

TL;DR

Current exposed types are:

- `EbpfError`
- `ProgramError`
- `MapError`
- `LinkError`
- `PerfBufferError`
- `SysError`

Honestly I'm still thinking about how we could collapse those types.
Either into a single type, or at least fewer than we expose today.

Within each of those types, I've tried to remove any invariants
that don't have any business being public (e.g if a syscall fails
with -EINVAL, there is nothing at runtime you can do about it
other than bailing).

👆 (and the spaghetti of errors depending on other errors) are
replaced by an `Other` invariant that's a Box<dyn std::error::Error>.

There are still some `pub(crate) XInternalError` types, but these
are used only to make nice error messages. This could plausibly be
replaced with anyhow/context etc.. But I've left it as-is for now.

Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
@dave-tucker dave-tucker changed the title wip errors WIP: Reduce Explosion of Aya Error Types Oct 18, 2024
@tyrone-wu
Copy link
Contributor

I was wondering if the errors from aya/src/utils.rs should also be apart of this. They're technically not ebpf specific errors, but I think some parts would align with FileError.

@dave-tucker
Copy link
Member Author

I was wondering if the errors from aya/src/utils.rs should also be apart of this. They're technically not ebpf specific errors, but I think some parts would align with FileError.

Good call! I missed those on the first pass through.
Once I've got agreement on the general direction this PR is taking I'll go ahead and clean those up too

Copy link

mergify bot commented Nov 1, 2024

@dave-tucker, this pull request is now in conflict and requires a rebase.

@mergify mergify bot added the needs-rebase label Nov 1, 2024
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) aya-log Relating to aya-log aya-obj Relating to the aya-obj crate needs-rebase test A PR that improves test cases or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants