aya: Use OwnedFd in FdLink. #709
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. |
| call: "BPF_OBJ_GET", | ||
| io_error, | ||
| })?; | ||
| pub fn from_fd(fd: OwnedFd) -> Result<MapData, MapError> { |
There was a problem hiding this comment.
technically a breaking API change but the old API was unsafe as it relied on the user knowing not to close the RawFd after the fact. This makes it more explicit that this function will take ownership of the file descriptor.
There was a problem hiding this comment.
I think we should do this in a separate PR? You're only changing the signature
but otherwise everything else stays the same. Why not do the signature change
when we actually change the impl?
There was a problem hiding this comment.
That would mean the bpf_map_info_by_fd call on line 591 would require an unsafe call to BorrowedFd::borrow_raw.
I'm OK with either approach.
There was a problem hiding this comment.
Yeah the bpf_map_info_by_fd change is also not related to links (it's for maps), so I think it could have gone in a separate PR too (the PR does state this is about links).
MapData::from_fd is effectively the only public API change in this PR - without this change I wouldn't have had to look at the PR at all :P
That said no need to add more churn! Thanks a lot @nrxus, looks good I'm approving
|
Hey @alessandrod, this pull request changes the Aya Public API and requires your review. |
mergify
bot
added
api/needs-review
There was a problem hiding this comment.
Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alessandrod and @nrxus)
a discussion (no related file):
Previously, nrxus (Andres) wrote…
(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)
technically a breaking API change but the old API was unsafe as it relied on the user knowing not to close the
RawFdafter the fact. This makes it more explicit that this function will take ownership of the file descriptor.
Looks like a win to me.
aya/src/maps/mod.rs line 584 at r1 (raw file):
} /// Loads a map from an [`OwnedFd`].
I think it'd be better avoid repeating the type here.
Code quote:
OwnedFd
There was a problem hiding this comment.
Reviewable status: 19 of 20 files reviewed, 1 unresolved discussion (waiting on @alessandrod and @tamird)
aya/src/maps/mod.rs line 584 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
I think it'd be better avoid repeating the type here.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @alessandrod)
|
@nrxus, this pull request is now in conflict and requires a rebase. |
|
@nrxus apologies for the conflict. Let's rebase and get this landed. |
tamird
force-pushed
the
fd-link-owned-fd
branch
from
2eb2baa
to
8ebf0ac
Compare
|
I took care of the conflict. |
There was a problem hiding this comment.
Reviewed 20 of 20 files at r3, all commit messages.
Dismissed @dave-tucker from 4 discussions.
Reviewable status:
I had to add a few
into_raw_fd()to avoid ballooning up this PR with changes toMapDataor toProgramData(because the syscall toBPF_OBJ_GETnow returns anOwnedFd) Those are more involved so they should each be their own PR.As far as I could tell there weren't any leaked file descriptors being fixed by this, this is just future proofing.
This change is