Work around missing libc constants on Fuchsia #184
Conversation
tamird
changed the title
|
@Thomasdezeeuw this is ready for a look. Let me know if you'd like me to add something to the changelog. |
| @@ -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>>> { | |||
There was a problem hiding this comment.
Why did you change this to Vec<u8>?
There was a problem hiding this comment.
Because the interface name is not guaranteed to be nul terminated if its length is IF_NAMSIZ.
There was a problem hiding this comment.
That's a fair point, but couldn't we use IFNAMSIZ + 1 then?
There was a problem hiding this comment.
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.
You can call CString::into_bytes to get a vector. The main benefit of CString/CStr is that they guarantee C-string compatibility, where Vec<u8> doesn't.
There was a problem hiding this comment.
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.
b"hel\0lo" is a perfectly valid vector of bytes, however as C string is likely invalid because it stops after "hel". The CString type ensure the caller knows the string can't contain null bytes. This is mainly useful in set_device, so the caller doesn't pass half names of devices. For consistency it's nicer if device also returns C string.
There was a problem hiding this comment.
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.
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.
|
|
||
| // 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.
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.
Pr to update libc: rust-lang/libc#2014.
|
Closing in favour of #186, the Fuchsia constants depend on rust-lang/libc#2014. |
No description provided.