-
Notifications
You must be signed in to change notification settings - Fork 369
Fix TODOs of unwrap calls by implementing Error Variants #1389
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
tamird
left a comment
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.
@tamird reviewed 1 of 1 files at r2, 8 of 8 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @belohnung)
-- commits line 8 at r3:
can you squash these? i don't see the need for these commits
Code quote:
New commits in r1 on 6/14/2024 at 1:41 PM:
- 212eb53: fix(aya): fix panic when creating map on custom ubuntu kernel
New commits in r2 on 11/10/2025 at 4:07 PM:
- 1dfea36: Merge branch 'aya-rs:main' into main
New commits in r3 on 11/10/2025 at 4:31 PM:
- a471a9a: fix: replace panics with proper error handling
aya/src/programs/mod.rs line 188 at r3 (raw file):
/// source error #[source] source: std::ffi::NulError,
add NulError to the imports at the top plz
aya/src/programs/mod.rs line 231 at r3 (raw file):
Btf(#[from] BtfError), /// The program is not attached.
please update
aya/src/programs/mod.rs line 238 at r3 (raw file):
/// source error #[source] source: Option<std::ffi::NulError>,
should not be optional
aya/src/programs/mod.rs line 603 at r3 (raw file):
use std::os::unix::ffi::OsStrExt as _; let path_string = CString::new(path.as_ref().as_os_str().as_bytes()).map_err(|e| {
please call this source instead of e
aya/src/programs/mod.rs line 604 at r3 (raw file):
let path_string = CString::new(path.as_ref().as_os_str().as_bytes()).map_err(|e| { ProgramError::InvalidName {
this is misleading, this is an invalid path, not an invalid name.
aya/src/programs/mod.rs line 706 at r3 (raw file):
let prog_name = if let Some(name) = name.as_deref() { let prog_name = CString::new(name).map_err(|err @ std::ffi::NulError { .. }| {
the source error is right here
this code should be
let prog_name = name.as_deref().map(|name| {
CString::new(name).map_err(|source| {
let name = name.to_owned();
ProgramError::InvalidName { name, source }
})
}).transpose()?;aya/src/maps/info.rs line 121 at r3 (raw file):
use std::os::unix::ffi::OsStrExt as _; let path_string = CString::new(path.as_ref().as_os_str().as_bytes()).map_err(|_| {
can you keep the original error as #[source]?
aya/src/programs/info.rs line 224 at r3 (raw file):
let path_string = CString::new(path.as_ref().as_os_str().as_bytes()).map_err(|e| { ProgramError::InvalidName {
ditto, misleading
aya-obj/src/relocation.rs line 95 at r3 (raw file):
symbol_kind: SymbolKind, /// The relocation size size: u8,
why is the size useful?
aya-obj/src/relocation.rs line 404 at r3 (raw file):
// R_BPF_64_64 this is a ld_imm64 text relocation SymbolKind::Section if rel.size == 64 => sym.address + ins.imm as u64, _ => {
s/_/symbol_kind/ so it's clear this is not dropped on the floor
aya/src/programs/links.rs line 353 at r3 (raw file):
let path_string = CString::new(path.as_ref().as_os_str().as_bytes()).map_err(|e| LinkError::InvalidPath {
please call this source instead of e
aya/src/programs/links.rs line 607 at r3 (raw file):
/// source error #[source] source: std::ffi::NulError,
import please
aya/src/programs/xdp.rs line 107 at r3 (raw file):
/// transparently falls back to the legacy netlink-based attach path. pub fn attach(&mut self, interface: &str, flags: XdpFlags) -> Result<XdpLinkId, ProgramError> { let c_interface = CString::new(interface).map_err(|e| ProgramError::InvalidInterfaceName {
please call this source instead of e
aya/src/programs/raw_trace_point.rs line 54 at r3 (raw file):
/// The returned value can be used to detach, see [RawTracePoint::detach]. pub fn attach(&mut self, tp_name: &str) -> Result<RawTracePointLinkId, ProgramError> { let tp_name_c = CString::new(tp_name).map_err(|e| ProgramError::InvalidName {
please call this source instead of e
0f59fe8 to
964eb84
Compare
|
CI is failing. |
|
yes, can you help me figure out what the problem is? |
|
|
964eb84 to
454c3ca
Compare
tamird
left a comment
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.
@tamird reviewed 9 of 9 files at r6, 9 of 9 files at r7, 11 of 11 files at r8, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @belohnung)
aya-obj/src/relocation.rs line 95 at r3 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
why is the size useful?
maybe relocation number would be better, like above
aya/src/maps/mod.rs line 120 at r8 (raw file):
/// source error #[source] source: std::ffi::NulError,
add this to the use at the top plz
aya/src/programs/links.rs line 605 at r8 (raw file):
InvalidPath { /// path path: String,
make this a PathBuf so you don't have to use .display() which is lossy
aya/src/maps/info.rs line 122 at r6 (raw file):
let path_string = CString::new(path.as_ref().as_os_str().as_bytes()).map_err(|source| { MapError::InvalidName {
should be InvalidPath, no?
aya/src/programs/mod.rs line 245 at r8 (raw file):
InvalidPath { /// path path: String,
make this a PathBuf so you don't have to use .display() which is lossy
72a09c6 to
1fb4bb1
Compare
This commit addresses several potential panic scenarios by introducing proper error handling.
1fb4bb1 to
ebc2b62
Compare
tamird
left a comment
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.
@alessandrod could you PTAL?
@tamird reviewed 11 of 11 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @belohnung)
This PR addresses several potential panic scenarios by introducing
proper error handling:
New error variants added:
RelocationError::UnsupportedRelocationTargetfor unsupportedsymbol kinds in relocations
ProgramError::InvalidInterfaceNamefor invalid network interfacenames
ProgramError::InvalidNameupdated to include optional NulErrorsource
LinkError::InvalidPathfor invalid pathsThe extra commit which does not produce a diff and had been merged before IIRC.
This change is