-
Notifications
You must be signed in to change notification settings - Fork 381
aya: implement TryFrom<[Program Type]> for FdLink for various program types #1264
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
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Pull Request Overview
This PR implements TryFrom conversions for FdLink for various program link types to support link pinning as referenced in the FdLink documentation.
- Added TryFrom implementations for SockOps, CgroupSockAddr, CgroupSock, and CgroupSkb program link types.
- Each conversion verifies that the link contains an FD, returning an error for ProgAttach variants.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| aya/src/programs/sock_ops.rs | Added TryFrom implementation for SockOpsLink to FdLink. |
| aya/src/programs/cgroup_sock_addr.rs | Added TryFrom implementation for CgroupSockAddrLink to FdLink. |
| aya/src/programs/cgroup_sock.rs | Added TryFrom implementation for CgroupSockLink to FdLink. |
| aya/src/programs/cgroup_skb.rs | Added TryFrom implementation for CgroupSkbLink to FdLink. |
dave-tucker
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.
The Copilot review isn't completely wrong. Please could you add the following macro?
macro_rules! impl_try_into_fdlink {
($wrapper:ident, $inner:ident) => {
impl TryFrom<$wrapper> for FdLink {
type Error = LinkError;
fn try_from(value: $wrapper) -> Result<Self, Self::Error> {
if let $inner::FdLink(fd) = value.into_inner() {
Ok(fd)
} else {
Err(LinkError::InvalidLink)
}
}
}
};
}
pub(crate) use impl_try_into_fdlink;This can the be used as:
impl_try_into_fdlink!(CgroupSockLink, CgroupSockLinkInner);Same can be done for XDP, Tc, etc... i.e all other programs that implement this.
We may have to rename some of the enum invariants to make this macro work, but that's not public API afaict so shouldn't be an issue. We've been inconsistent - sometimes it's LinkInner::Fd other times it's LinkInner::FdLink. I prefer the former so lets standardize on that if we can.
Renaming to implement a macro for TryFrom<link> for FdLink for various program types Refs: aya-rs#1264
Renaming to implement a macro for TryFrom<link> for FdLink for various program types Refs: aya-rs#1264
Refs: aya-rs#1264 Co-authored-by: Benjamin Barzen <bbarzen@amazon.com>
|
Hey @alessandrod, this pull request changes the Aya Public API and requires your review. |
Renaming to implement a macro for TryFrom<link> for FdLink for various program types Refs: aya-rs#1264
Refs: aya-rs#1264 Co-authored-by: Benjamin Barzen <bbarzen@amazon.com>
Renaming to implement a macro for TryFrom<link> for FdLink for various program types Refs: aya-rs#1264 Co-authored-by: Benjamin Barzen <bbarzen@amazon.com>
Refs: aya-rs#1264 Co-authored-by: Benjamin Barzen <bbarzen@amazon.com>
|
Please squash your commits so there is only one and run |
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.
Reviewed 1 of 8 files at r4, 11 of 12 files at r5, 1 of 4 files at r6, 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @alessandrod, @dave-tucker, and @mehnazyunus)
aya/src/programs/links.rs line 561 at r7 (raw file):
macro_rules! impl_try_into_fdlink { ($wrapper:ident, $inner:ident) => { impl TryFrom<$wrapper> for FdLink {
please use $crate::programs::FdLink to avoid having to import it in all the invokers.
aya/src/programs/links.rs line 562 at r7 (raw file):
($wrapper:ident, $inner:ident) => { impl TryFrom<$wrapper> for FdLink { type Error = LinkError;
please use $crate::programs::LinkError to avoid having to import it in all the invokers.
aya/src/programs/links.rs line 568 at r7 (raw file):
Ok(fd) } else { Err(LinkError::InvalidLink)
ditto.
ef4bd52 to
be457c1
Compare
|
Hey @alessandrod, this pull request changes the Aya Public API and requires your review. |
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.
Code looks good but needs a rebase and another squash.
Reviewed 2 of 16 files at r8, 14 of 14 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alessandrod, @dave-tucker, and @mehnazyunus)
… types Implements TryFrom for FdLink for CgroupSkb, CgroupSock, CgroupSockAddr and SockOps program types. This allows support for link pinning for these program types, aligning with the documentation for FdLink. Fixes: aya-rs#739 Co-authored-by: Benjamin Barzen <bbarzen@amazon.com>
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.
Reviewed 3 of 3 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alessandrod, @dave-tucker, and @mehnazyunus)
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.
Pull Request Overview
This PR introduces TryFrom implementations for FdLink for several program types (CgroupSkb, CgroupSock, CgroupSockAddr, SockOps, etc.) to support link pinning as documented. It leverages a new macro (impl_try_into_fdlink) to reduce boilerplate and updates enum variant names (from FdLink to Fd) for consistency across modules.
- Uses a new macro to consolidate TryFrom implementations.
- Updates naming from FdLink to Fd in internal enums.
- Revises the public API documentation to expose these changes.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| xtask/public-api/aya.txt | Updated API doc to include the new TryFrom implementations (check for duplicate entries). |
| aya/src/programs/xdp.rs | Renamed internal enum variant from FdLink to Fd and updated conversion logic. |
| aya/src/programs/uprobe.rs | Replaced explicit TryFrom implementation with macro usage. |
| aya/src/programs/trace_point.rs | Similar macro conversion and variant name update. |
| aya/src/programs/tc.rs | Converted TryFrom implementations and renamed enum variants. |
| aya/src/programs/sock_ops.rs | Added macro conversion for SockOpsLink. |
| aya/src/programs/perf_event.rs | Applied macro usage and updated conversion fallbacks. |
| aya/src/programs/perf_attach.rs | Updated enum variant naming in PerfLinkInner. |
| aya/src/programs/kprobe.rs | Replaced explicit TryFrom with macro usage. |
| aya/src/programs/iter.rs | Consolidated TryFrom implementations using the macro. |
| aya/src/programs/cgroup_sock_addr.rs | Added macro implementation for TryFrom. |
| aya/src/programs/cgroup_sock.rs | Added macro implementation for TryFrom. |
| aya/src/programs/cgroup_skb.rs | Added macro implementation for TryFrom. |
Comments suppressed due to low confidence (1)
aya/src/programs/xdp.rs:161
- [nitpick] The renaming of the enum variant from 'FdLink' to 'Fd' improves consistency; please ensure that all related documentation and references are updated accordingly.
.insert(XdpLink::new(XdpLinkInner::Fd(FdLink::new(link_fd))))
Implements TryFrom for FdLink for CgroupSkb, CgroupSock, CgroupSockAddr and SockOps program types. This allows support for link pinning for these program types, aligning with the documentation for FdLink.
Fixes: #739
This change is