-
Notifications
You must be signed in to change notification settings - Fork 286
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
maps: MapFd
and SockMapFd
are owned
#770
Conversation
✅ Deploy Preview for aya-rs-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hey @alessandrod, this pull request changes the Aya Public API and requires your review. |
cc @nrxus |
aya/src/bpf.rs
Outdated
@@ -510,7 +510,7 @@ impl<'a> BpfLoader<'a> { | |||
|
|||
obj.relocate_maps( | |||
maps.iter() | |||
.map(|(s, data)| (s.as_str(), data.fd, &data.obj)), | |||
.map(|(s, data)| (s.as_str(), data.fd().as_fd().as_raw_fd(), &data.obj)), |
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.
why not pass a BorrowedFd<'_>
here instead? The less references we have to RawFd
s the easier it'll be gauge that our file descriptors are all valid.
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.
IIRC the function we're calling here is available in no_std, so BorrowedFd
can't appear in its signature.
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.
Made that function available only in std
. Unclear to me how anyone would use such a thing without access to real file descriptors.
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.
I think we should undo the aya-obj bit. Relocation puts file descriptors in the instruction stream where they must be cast as integers anyway, so lifetime tracking doesn't buy us much and it makes aya-obj essentially useless on no_std.
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.
I can do that. But can you help me understand how someone would use this in no_std given you can't get your hands on an FD? Or is the idea that the user has an FD through some other means?
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.
Yes, it's unfortunate that *Fd are defined in std::os
, in an ideal world some of the os
definitions should be moved to core
like it happened for net
. But in a no_std
program it's easy to imagine that one could still create fds using lower level linux APIs, and it seems silly that we would preclude them from using aya-obj just for a type alias.
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.
I don't think this is equivalent to the core::net
types since those are just scalars like IP addresses. Anyway, done -- I just defined a local alias for RawFd in this crate in no_std.
} | ||
} | ||
|
||
impl Clone for MapData { |
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.
should we implement a try_clone()
for MapData
so maps can still be cloned?
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.
We can. In light of #703 I'm not sure how much it matters.
e281600
to
ad79c32
Compare
aya-obj/src/relocation.rs
Outdated
@@ -105,7 +110,11 @@ pub(crate) struct Symbol { | |||
|
|||
impl Object { | |||
/// Relocates the map references | |||
pub fn relocate_maps<'a, I: Iterator<Item = (&'a str, i32, &'a Map)>>( | |||
#[cfg(feature = "std")] | |||
pub fn relocate_maps< |
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.
This makes aya-obj a lot less useful without std. Do you think it would be too
painful to make this work with both RawFd and BorrowedFd? (Doesn't seem like but
I haven't tried)
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 previous discussion on this topic is here. How do you get your hands on an FD in no_std?
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.
Other than the aya-obj std thing, lgtm
`MapData::fd` is now a `MapFd`. This means that `MapData` now closes the file descriptor on drop. In the future we might consider making `MapFd` hold a `BorrowedFd` but this requires API design work due to overlapping borrows. Since `SockMapFd` is no longer `Copy`, attach methods to take it by reference to allow callers to use it multiple times as they are accustomed to doing. `SockMapFd` implements `try_clone`. `MapFd` and `SockMapFd` are now returned by reference to allow callers to avoid file descriptor cloning when desired. This is an API breaking change. Updates #612.
ad79c32
to
0dacb34
Compare
MapData::fd
is now aMapFd
. This means thatMapData
now closes thefile descriptor on drop. In the future we might consider making
MapFd
hold a
BorrowedFd
but this requires API design work due to overlappingborrows.
Since
SockMapFd
is no longerCopy
, attach methods to take it byreference to allow callers to use it multiple times as they are
accustomed to doing.
SockMapFd
implementstry_clone
.MapFd
andSockMapFd
are nowreturned by reference to allow callers to avoid file descriptor cloning
when desired.
This is an API breaking change.
Updates #612.