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
Work around missing libc constants on Fuchsia #184
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,11 +7,6 @@ | |
// except according to those terms. | ||
|
||
use std::cmp::min; | ||
#[cfg(all( | ||
feature = "all", | ||
any(target_os = "android", target_os = "fuchsia", target_os = "linux") | ||
))] | ||
use std::ffi::{CStr, CString}; | ||
#[cfg(not(target_os = "redox"))] | ||
use std::io::IoSlice; | ||
use std::marker::PhantomData; | ||
|
@@ -942,11 +937,19 @@ 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 _; | ||
|
||
// TODO(https://github.com/rust-lang/libc/pull/2010): Remove this. | ||
#[cfg(target_os = "fuchsia")] | ||
const IFNAMSIZ: libc::size_t = 16; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's update libc (after rust-lang/libc#2013 is merged) and use those constants. Socket2 used to be filled with defined constants from libc, I don't want to go back to that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pr to update libc: rust-lang/libc#2014. |
||
|
||
#[cfg(not(target_os = "fuchsia"))] | ||
const IFNAMSIZ: libc::size_t = libc::IFNAMSIZ; | ||
|
||
// TODO: replace with `MaybeUninit::uninit_array` once stable. | ||
let mut buf: [MaybeUninit<u8>; libc::IFNAMSIZ] = | ||
unsafe { MaybeUninit::<[MaybeUninit<u8>; libc::IFNAMSIZ]>::uninit().assume_init() }; | ||
let mut len = buf.len() as libc::socklen_t; | ||
let mut buf: MaybeUninit<[u8; IFNAMSIZ]> = MaybeUninit::uninit(); | ||
let mut len = IFNAMSIZ.try_into().unwrap(); | ||
unsafe { | ||
syscall!(getsockopt( | ||
self.inner, | ||
|
@@ -959,21 +962,13 @@ impl crate::Socket { | |
if len == 0 { | ||
Ok(None) | ||
} else { | ||
// Allocate a buffer for `CString` with the length including the | ||
// null terminator. | ||
let len = len as usize; | ||
let mut name = Vec::with_capacity(len); | ||
|
||
// TODO: use `MaybeUninit::slice_assume_init_ref` once stable. | ||
// Safety: `len` bytes are writen by the OS, this includes a null | ||
// terminator. However we don't copy the null terminator because | ||
// `CString::from_vec_unchecked` adds its own null terminator. | ||
let buf = unsafe { slice::from_raw_parts(buf.as_ptr().cast(), len - 1) }; | ||
name.extend_from_slice(buf); | ||
|
||
// Safety: the OS initialised the string for us, which shouldn't | ||
// include any null bytes. | ||
Ok(Some(unsafe { CString::from_vec_unchecked(name) })) | ||
Ok(Some( | ||
unsafe { buf.assume_init() }[..len.try_into().unwrap()].into(), | ||
)) | ||
} | ||
} | ||
|
||
|
@@ -990,9 +985,9 @@ impl crate::Socket { | |
feature = "all", | ||
any(target_os = "android", target_os = "fuchsia", target_os = "linux") | ||
))] | ||
pub fn bind_device(&self, interface: Option<&CStr>) -> io::Result<()> { | ||
pub fn bind_device(&self, interface: Option<&[u8]>) -> io::Result<()> { | ||
let (value, len) = if let Some(interface) = interface { | ||
(interface.as_ptr(), interface.to_bytes_with_nul().len()) | ||
(interface.as_ptr(), interface.len()) | ||
} else { | ||
(ptr::null(), 0) | ||
}; | ||
|
@@ -1058,9 +1053,15 @@ impl crate::Socket { | |
any(target_os = "android", target_os = "fuchsia", target_os = "linux") | ||
))] | ||
pub fn freebind(&self) -> io::Result<bool> { | ||
// TODO(https://github.com/rust-lang/libc/pull/2011): Remove this. | ||
#[cfg(target_os = "fuchsia")] | ||
const IP_FREEBIND: c_int = 15; | ||
|
||
#[cfg(not(target_os = "fuchsia"))] | ||
const IP_FREEBIND: c_int = libc::IP_FREEBIND; | ||
|
||
unsafe { | ||
getsockopt::<c_int>(self.inner, libc::SOL_SOCKET, libc::IP_FREEBIND) | ||
.map(|reuse| reuse != 0) | ||
getsockopt::<c_int>(self.inner, libc::SOL_SOCKET, IP_FREEBIND).map(|reuse| reuse != 0) | ||
} | ||
} | ||
|
||
|
@@ -1078,14 +1079,14 @@ impl crate::Socket { | |
any(target_os = "android", target_os = "fuchsia", target_os = "linux") | ||
))] | ||
pub fn set_freebind(&self, reuse: bool) -> io::Result<()> { | ||
unsafe { | ||
setsockopt( | ||
self.inner, | ||
libc::SOL_SOCKET, | ||
libc::IP_FREEBIND, | ||
reuse as c_int, | ||
) | ||
} | ||
// TODO(https://github.com/rust-lang/libc/pull/2011): Remove this. | ||
#[cfg(target_os = "fuchsia")] | ||
const IP_FREEBIND: c_int = 15; | ||
|
||
#[cfg(not(target_os = "fuchsia"))] | ||
const IP_FREEBIND: c_int = libc::IP_FREEBIND; | ||
|
||
unsafe { setsockopt(self.inner, libc::SOL_SOCKET, IP_FREEBIND, reuse as c_int) } | ||
} | ||
} | ||
|
||
|
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 did you change this to
Vec<u8>
?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.
Because the interface name is not guaranteed to be nul terminated if its length is IF_NAMSIZ.
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.
That's a fair point, but couldn't we use
IFNAMSIZ + 1
then?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.
Maybe, but why would you want to? It's easiest to work with a vector than with a CString.
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.
You can call
CString::into_bytes
to get a vector. The main benefit ofCString
/CStr
is that they guarantee C-string compatibility, whereVec<u8>
doesn't.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.
Understood, but why is that a benefit? Why do we want ffi types in the public API? What will the caller do with this return value that requires a C string?
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.
b"hel\0lo"
is a perfectly valid vector of bytes, however as C string is likely invalid because it stops after "hel". TheCString
type ensure the caller knows the string can't contain null bytes. This is mainly useful inset_device
, so the caller doesn't pass half names of devices. For consistency it's nicer ifdevice
also returns C string.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.
From the perspective of this API (
setsockopt
SO_BINDTODEVICE
),b"hel\0lo"
is a perfectly valid name. There's nothing in the API documentation that indicates that the nul byte is interpreted at all, since the caller always supplies the length. I haven't been able to create a local device with a name containing a nul byte, but I don't see any reason that such a thing would be illegal.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.
After looking through the Linux source I think you're right. There is only a special case for "\0", which it considers an empty string, same as passing a zero length. I'll see to review #186.