Remove FFI types from the public API #186
Conversation
There was a problem hiding this comment.
After discussing this in #184 (comment), I'm ok with the change.
src/sys/unix.rs
Outdated
| @@ -942,11 +937,13 @@ impl crate::Socket { | |||
| feature = "all", | |||
| any(target_os = "android", target_os = "fuchsia", target_os = "linux") | |||
| ))] | |||
| pub fn device(&self) -> io::Result<Option<CString>> { | |||
| pub fn device(&self) -> io::Result<Option<Vec<u8>>> { | |||
| use std::convert::TryInto as _; | |||
There was a problem hiding this comment.
P.S: You don't need to use as _.
There was a problem hiding this comment.
Ack. I usually do this with traits to avoid bringing the trait name into the local namespace.
src/sys/unix.rs
Outdated
| unsafe { MaybeUninit::<[MaybeUninit<u8>; libc::IFNAMSIZ]>::uninit().assume_init() }; | ||
| let mut len = buf.len() as libc::socklen_t; | ||
| unsafe { MaybeUninit::uninit().assume_init() }; | ||
| let mut len = buf.len().try_into().unwrap(); |
There was a problem hiding this comment.
Let's leave the original code. The buf initialisation is the same as MaybeUninit::uninit_array to which we should move in the future. And we know that IFNAMSIZ can't be larger than libc::socklen_t::MAX so the as usage is fine, I don't like unwrap too much.
There was a problem hiding this comment.
The buf initialization is the same as the original code (in this PR, not in #184); I've just removed some repeated type parameters.
I agree that IFNAMSIZ in practice can't fail this conversion, but why think about it? This code is safer and should compile to the same thing.
src/sys/unix.rs
Outdated
| // include any null bytes. | ||
| Ok(Some(unsafe { CString::from_vec_unchecked(name) })) | ||
| let buf = &buf[..len.try_into().unwrap()]; | ||
| Ok(Some(unsafe { mem::transmute::<_, &[u8]>(buf) }.into())) |
There was a problem hiding this comment.
Let's not use transmute, it's too easy to misuse. Also let's keep the same code as in MaybeUninit::slice_assume_init_ref so we can switch to that easily once stable.
It think the following would be shortest.
// Safety: `len` bytes are writen by the OS, this includes a null
// terminator. However we don't need the null terminator.
let buf = unsafe { slice::from_raw_parts(buf.as_ptr().cast(), (len - 1) as usize) };
Ok(Some(buf.into_vec()))There was a problem hiding this comment.
len does not include a null terminator AFAIK.
Other than that, this code is the same as it would be with slice_assume_init_ref. That code would be:
let buf = &buf[..len.try_into().unwrap()];
Ok(Some(unsafe { MaybeUninit::slice_assume_init_ref(buf) }.into()))There was a problem hiding this comment.
len does include a null terminate, I've just manually tried this patch. The test currently doesn't fail it it doesn't have permissions (mainly for the CI), but we might have to change that. I'll fiddle with this a bit more and merge this changing the signature to [u8] rather than CStr.
P.S. it requires sudo setcap cap_net_raw=ep $test_bin, something that we might have to do in the CI as well.
There was a problem hiding this comment.
I'm about 95% sure that len does not include the null terminator when the name is IFNAMSIZ characters long. That is probably an edge case worth testing.
There was a problem hiding this comment.
After a bit more manual testing. I couldn't create an interface with a name larger than 15 bytes, with IFNAMSIZ being 16.
sudo ip link add test_interface0 type bridge # Fine.
sudo ip link add test_interface01 type bridge # Error: Attribute failed policy validation.I also tested binding to test_interface01 with the implementation in #187 which worked.
|
@Thomasdezeeuw you should be able to push to this PR rather than working in a separate one. |
|
@tamird I didn't know I could push to some else's branch. |
|
@tamird I've pushed the changes to this branch. |
tests/socket.rs
Outdated
| panic!( | ||
| "failing to bind to any device. \ | ||
| Note that on Linux this requires the `CAP_NET_RAW` permission. \ | ||
| This can be given using `sudo setcap cap_net_raw=ep $test_bin`." |
There was a problem hiding this comment.
Should this mention whatever flag is necessary to run this test case given the ignore attribute?
There was a problem hiding this comment.
I've changed it to just say "failed to bind to any device", the other info is already in the ignore attribute.
With flag do you mean --ignore passed to cargo test, or do mean another flag? If the first than I don't it's needed.
Since the test is now not run by default it doesn't make sense to allow permission errors. Also reduces explanation in the `panic!` calls since that's already in the ignore attribute.
Extracted from #184 @Thomasdezeeuw.