-
Notifications
You must be signed in to change notification settings - Fork 654
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
Add support for getifaddrs. #667
Conversation
One area I'm not quite sure about is the use of the nix specific This works for me, ATM. However, I think nix should better provide some way to represent those entries as well. |
src/ifaddrs.rs
Outdated
|
||
use sys::socket::InetAddr; | ||
|
||
#[cfg(any(target_os = "linux", target_os = "emscripten", target_os = "fuchsia"))] |
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.
There's no need to use a module here. Please use a single libc_bitflags
instance and annotate each entry with a cfg
attribute with their platform support.
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 also think Android supports all these as well (you can find this out easily by searching the libc repo).
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.
Android doesn't support getifaddrs
, that's probably why I excluded it. But you're right, the flags themselves are known to Android as well.
Maybe I should move InterfaceFlags
to net/if_.rs
, as those constants are defined in /usr/include/net/if.h
and not in ifaddrs.h
.
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.
Android does, but libc does need to have it added.
I think it's fine having them here for now.
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.
Oh, and: I used the module to reduce the amount of #[cfg()]
conditions. I only used the module as I didn't find any good way to write this without repeating the conditions many times. I'm fine if you prefer the latter and changed things accordingly.
src/ifaddrs.rs
Outdated
mod flags { | ||
use libc; | ||
libc_bitflags!( | ||
/// Flags for `InterfaceAddress` entries returned by `getifaddrs` |
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 comment doesn't really provide any detail. Can you summarize what these values actually refer to?
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 duplicated the comment from the C header, now.
src/ifaddrs.rs
Outdated
|
||
/// Describes a single address for an interface as returned by `getifaddrs`. | ||
pub struct InterfaceAddress { | ||
/// name of the interface |
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.
Please capitalize the first character of all of these comments
src/ifaddrs.rs
Outdated
|
||
/// Get interface addresses using libc's `getifaddrs` | ||
/// | ||
/// Note that the underlying implementation differs between implementations and |
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 underlying implementation differs between implementations", that's pretty redundant. Can you rephrase this to be a little more clear?
src/ifaddrs.rs
Outdated
/// } | ||
/// } | ||
/// ``` | ||
#[cfg(not(any(target_os = "android", target_os = "haiku")))] |
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.
Can you check that this isn't on Android? According to this it should.
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.
And the BSDs, too. They all have it.
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.
Indeed, it's already defined in libc
for all BSDs and Solaris as well.
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 checked against rust's libc bindings, where src/unix/notbsd/android/mod.rs
doesn't define getifaddrs
(at least not in 3f903a1d from master as of Jul 11).
And: Huh? not(any(target_os = "android", target_os = "haiku"))
already includes the BSDs.
src/sys/socket/addr.rs
Outdated
/// Creates an `IpAddr` struct from libc's sockaddr. | ||
/// | ||
/// Note that this supports only IPv4 and IPv6 address families. | ||
pub unsafe fn from_libc_sockaddr(addr: *mut libc::sockaddr) -> Option<InetAddr> { |
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 is this unsafe and the other from_libc
isn't? What's the reason for providing this API?
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 marked this one unsafe because it takes a raw pointer, which could point anywhere and yield a segfault. Again, this is not a public function, but only used internally (by that other InterfaceAddress::from_libc_ifaddrs
function)
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.
If it's not a public function, you should remove the pub
Thanks for the PR! There are a few things to address from this, notably a lack of tests besides the doctest that you added (which is great BTW). |
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 looks like the start of a pretty good API. I like that you used Iterator
. In addition to the inline comments I made, I think that this could really use some better tests. I think you'll need to find a way to mock libc::getifaddrs
and return some canned data. Does Rust have a mocking library yet?
src/sys/socket/addr.rs
Outdated
@@ -64,6 +64,24 @@ impl InetAddr { | |||
} | |||
} | |||
|
|||
/// Creates an `IpAddr` struct from libc's sockaddr. |
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.
s/IpAddr/InetAddr/
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.
Right, that's a last-minute typo. I didn't notice SockAddr
before and currently think the getifaddrs
API shoud rather use SockAddr
.
src/sys/socket/addr.rs
Outdated
None | ||
} else { | ||
let family = (*addr).sa_family as u32; | ||
match mem::transmute::<u32, AddressFamily>(family) { |
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.
Instead of using transmute here, it's better to provide an AddressFamily::from_u32
method.
src/ifaddrs.rs
Outdated
|
||
use sys::socket::InetAddr; | ||
|
||
#[cfg(any(target_os = "linux", target_os = "emscripten", target_os = "fuchsia"))] |
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.
nix doesn't support emscripten or fuschia, so you don't need to mention them here. It doesn't hurt, though, so you can leave it in if you want to. But why not enable it for the BSDs? They support all this stuff. If there are a few Linux specific flags, you can #[cfg()]
individual flags instead of the whole thing.
src/ifaddrs.rs
Outdated
target_os = "dragonfly", target_os = "openbsd", target_os = "netbsd", | ||
target_os = "bitrig", target_os = "solaris"))] | ||
{ | ||
addr.destination = unsafe { InetAddr::from_libc_sockaddr(info.ifa_dstaddr) }; |
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.
Looks like you forgot to set the broadcast for non-Linux platforms. You can do it the same way as on Linux. Depending on the flags, either ifa_dstaddr
or ifa_broadaddr
is set. The only difference is that on BSD, one of those is a macro for the other, whereas on Linux they're a union.
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.
Ah, it's the same field for BSDs as well? I did it that way due to rust's libc bindings, where there is not ifa_broadaddr
or ifa_broadcastaddr
defined. I'll check the C headers from FreeBSD and adjust.
Given the BSDs also lack IFF_BROADCAST
and IFF_POINTOPOINT
, how do they distinguish between broadcast vs destination address?
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 just checked, and libc does define both IFF_BROADCAST
and IFF_POINTOPOINT
for most, if not all, platforms.
src/ifaddrs.rs
Outdated
/// | ||
/// # Example | ||
/// ``` | ||
/// match nix::ifaddrs::getifaddrs() { |
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.
For the example code, it would probably be clearer to use unwrap
instead of a match
block
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.
An unwrap is very likely to fail due to the many address = None
cases you'll encounter. It certainly makes cargo test
fail on my box (Debian, with many AF_PACKET entries that cannot be represented ATM).
src/ifaddrs.rs
Outdated
/// } | ||
/// } | ||
/// ``` | ||
#[cfg(not(any(target_os = "android", target_os = "haiku")))] |
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.
And the BSDs, too. They all have it.
src/ifaddrs.rs
Outdated
#[cfg(not(any(target_os = "android", target_os = "haiku")))] | ||
pub fn getifaddrs() -> Result<InterfaceAddressIterator> { | ||
let mut addrs: *mut libc::ifaddrs = unsafe { mem::zeroed() }; | ||
match unsafe { libc::getifaddrs(&mut addrs) } { |
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 probably simplify this logic to something like:
Errno::result(unsafe { libc::getifaddrs(&mut addrs) }).map(|_| Ok(InterfaceAddressIterator{base: addrs, next:addrs}))
Proper support for BSDs depends on corrections in libc, see rust-lang/libc#682. I'm currently in favour of disabling |
src/net/if_.rs
Outdated
libc_bitflags!( | ||
/// Standard interface flags, used by `getifaddrs` | ||
pub flags InterfaceFlags: libc::c_int { | ||
#[cfg(any(target_os = "linux", target_os = "android"))] |
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 target_os
list should be alphabetized.
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.
libc defines most of these constants for most platforms, not just Linux. You should expand the #[cfg()]
directives appropriately.
I'd prefer to wait on this until rust-lang/libc#682 gets addressed assuming you're willing to take that on. Were you planning on doing that? |
Not really, sorry. The itch I was trying to scratch originally was |
@mwanner I'm fine with that as long as the code documents why we aren't exposing it everywhere and it references a bug filed with |
@posborne I think there are two options, one is deliver this just for Linux or the second is get someone working on resolving the |
Actually, the libc definition is only wrong for NetBSD. FreeBSD is fine. |
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 this is really close. You shouldn't need access to a BSD machine in order to fix the IFF_*
definitions. It's simply a matter of going through libc and checking which operating systems define which flags.
src/ifaddrs.rs
Outdated
target_os = "dragonfly", target_os = "openbsd", target_os = "netbsd", | ||
target_os = "bitrig", target_os = "solaris"))] | ||
{ | ||
addr.destination = unsafe { InetAddr::from_libc_sockaddr(info.ifa_dstaddr) }; |
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 just checked, and libc does define both IFF_BROADCAST
and IFF_POINTOPOINT
for most, if not all, platforms.
src/net/if_.rs
Outdated
libc_bitflags!( | ||
/// Standard interface flags, used by `getifaddrs` | ||
pub flags InterfaceFlags: libc::c_int { | ||
#[cfg(any(target_os = "linux", target_os = "android"))] |
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.
libc defines most of these constants for most platforms, not just Linux. You should expand the #[cfg()]
directives appropriately.
src/sys/socket/addr.rs
Outdated
/// Creates an `IpAddr` struct from libc's sockaddr. | ||
/// | ||
/// Note that this supports only IPv4 and IPv6 address families. | ||
pub unsafe fn from_libc_sockaddr(addr: *mut libc::sockaddr) -> Option<InetAddr> { |
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.
If it's not a public function, you should remove the pub
Thanks for the heads up, @asomers. Looks like I made an attempt to update this PR to match current libc sources. |
@asomers wrote:
Given |
I can take a look at Android support as well. I happen to need to target Android aarch64 myself and bionic has had getifaddrs for some time but the libc bindings appear to be missing that on the rust side (explicitly). No need to gate this on that work though, as it will definitely require libc changes. |
@posborne It looks like |
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.
Looking pretty good! I just noticed a few style/convention issues that would be nice to cleanup before merging. Good work!
src/ifaddrs.rs
Outdated
/// } | ||
/// ``` | ||
pub fn getifaddrs() -> Result<InterfaceAddressIterator> { | ||
let mut addrs: *mut libc::ifaddrs = unsafe { mem::zeroed() }; |
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.
Can this not be mem::uninitialized()
instead?
src/net/if_.rs
Outdated
libc_bitflags!( | ||
/// Standard interface flags, used by `getifaddrs` | ||
pub struct InterfaceFlags: libc::c_int { | ||
IFF_UP; |
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.
Please add doccomments for each variant.
src/net/if_.rs
Outdated
IFF_DEBUG; | ||
IFF_LOOPBACK; | ||
IFF_POINTOPOINT; | ||
#[cfg(not(any(target_os = "freebsd", target_os = "dragonfly", target_os = "haiku")))] |
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.
Please alphabetize all OSes listed in #[cfg
s. Additionally please use a whitelist instead of a blacklist for targets for consistency.
src/sys/socket/addr.rs
Outdated
@@ -201,6 +201,23 @@ pub enum AddressFamily { | |||
Natm = libc::AF_NATM, | |||
} | |||
|
|||
impl AddressFamily { | |||
pub fn from_i32(family: i32) -> Option<AddressFamily> { |
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.
Please add a doccomment for this function as it's public.
src/sys/socket/addr.rs
Outdated
libc::AF_UNIX => Some(AddressFamily::Unix), | ||
libc::AF_INET => Some(AddressFamily::Inet), | ||
libc::AF_INET6 => Some(AddressFamily::Inet6), | ||
#[cfg(any(target_os = "linux", target_os = "android"))] |
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.
Please alphabetize the OSes.
src/lib.rs
Outdated
@@ -43,6 +43,9 @@ pub mod poll; | |||
|
|||
pub mod net; | |||
|
|||
#[cfg(not(any(target_os = "android", target_os = "haiku")))] |
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 prefer to avoid blocklists with #cfg
s, can you instead whitelist all appropriate platforms?
src/ifaddrs.rs
Outdated
|
||
let ifu : *mut libc::sockaddr; | ||
|
||
#[cfg(any(target_os = "linux", target_os = "emscripten", target_os = "fuchsia"))] |
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.
Probably want to use cfg_if!
here instead.
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 didn't manage to use a cfg_if!
for just these two statements, but extracted a small private function, now. Seems cleaner to me that way, but YMMV.
src/ifaddrs.rs
Outdated
destination: None, | ||
}; | ||
|
||
let ifu : *mut libc::sockaddr; |
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.
Remove the space before the colon.
src/ifaddrs.rs
Outdated
@@ -0,0 +1,137 @@ | |||
//! Query network interface addresses | |||
//! | |||
//! Uses the Linux and/or BSD specific function getifaddrs to query the list |
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.
Please put backticks around getifaddrs
so it renders nicely with rustdoc
.
src/sys/socket/addr.rs
Outdated
#[cfg(any(target_os = "ios", target_os = "macos"))] | ||
Some(AddressFamily::System) => Some(SockAddr::SysControl( | ||
SysControlAddr(*(addr as *mut sys_control::sockaddr_ctl)))), | ||
// No other address families are supported, including netlink and syscontrol. |
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.
Is this a TODO
or FIXME
or just impossible? Please clarify this comment further.
@Susurrus thanks for your review. I think I covered all points. |
src/ifaddrs.rs
Outdated
/// Get interface addresses using libc's `getifaddrs` | ||
/// | ||
/// Note that the underlying implementation differs between OSes and the nix | ||
/// crate doesn't support `AF_PACKET` entries. Therefore, the returned list |
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.
Would be a nice change in the future to support AF_PACKET
/AF_LINK
in order to be able to build something out for getting MAC addresses (useful for, among other things, getting a node id for UUID V1 generation). Not gating for the first pass.
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 this comment could be made more clear as to why this isn't implemented. Was it because we were lazy or because the OSes differed enough it'd be quite difficult to expose a nice API here?
src/ifaddrs.rs
Outdated
use net::if_::*; | ||
|
||
/// Describes a single address for an interface as returned by `getifaddrs`. | ||
pub struct InterfaceAddress { |
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.
Can we implement or derive Debug
/Display
for this. Also, I think deriving Clone
, PartialEq
, and Hash
would be good.
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 agree with this as well, should definitely have Clone
, Copy
, Debug
, Eq
, Hash
, and PartialEq
. We shouldn't implement Display
here and I don't think we should implement Default
here either.
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.
In commit ef40c9c, I added derive
s for Clone
, Eq
, Hash
, and PartialEq
, but not Copy
. The struct uses String
s, so I don't this that marker is applicable.
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.
Oh, and Debug
. I wasn't quite sure how much or in what format to return from fn fmt
. Also note that SockAddr
doesn't implement Debug
, but Display
. Is this really consistent?
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.
Just a few more style changes and some derives to get sorted, but definitely almost there!
src/net/if_.rs
Outdated
#[cfg(any(target_os = "android", target_os = "fuchsia", target_os = "ios", | ||
target_os = "linux", target_os = "macos", target_os = "netbsd", | ||
target_os = "openbsd"))] | ||
/// Avoid use of trailers. (see |
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.
Please put the doccomments for items above any #[cfg
s for those items. Applies in several places here.
src/lib.rs
Outdated
@@ -43,6 +43,10 @@ pub mod poll; | |||
|
|||
pub mod net; | |||
|
|||
#[cfg(any(target_os = "dragonfly", target_os = "freebsd", target_os = "netbsd", |
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 linux
is out of alphabetical order, and could you list these 1-per-line? When cfg
's get longer than a line we like to wrap with each target on its own line.
src/ifaddrs.rs
Outdated
/// Get interface addresses using libc's `getifaddrs` | ||
/// | ||
/// Note that the underlying implementation differs between OSes and the nix | ||
/// crate doesn't support `AF_PACKET` entries. Therefore, the returned list |
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 this comment could be made more clear as to why this isn't implemented. Was it because we were lazy or because the OSes differed enough it'd be quite difficult to expose a nice API here?
src/ifaddrs.rs
Outdated
use net::if_::*; | ||
|
||
/// Describes a single address for an interface as returned by `getifaddrs`. | ||
pub struct InterfaceAddress { |
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 agree with this as well, should definitely have Clone
, Copy
, Debug
, Eq
, Hash
, and PartialEq
. We shouldn't implement Display
here and I don't think we should implement Default
here either.
src/ifaddrs.rs
Outdated
/// Use the function `getifaddrs` to create this Iterator. Note that the | ||
/// actual list of interfaces can be iterated once and will be freed as | ||
/// soon as the Iterator goes out of scope. | ||
pub struct InterfaceAddressIterator { |
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 should definitely derive Debug
here, but does it make sense to derive any of the other standard ones (Clone
, Copy
, Eq
, Hash
, and PartialEq
)?
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.
Clone
certainly doesnt make much sense, as the
InterfaceAddressIteratoris used to govern the memory allocated by
getifaddrs. I added
Debug,
Eq,
Hash, and
PartialEq`.
src/net/if_.rs
Outdated
/// Avoid use of trailers. (see | ||
/// [`netdevice(7)`](http://man7.org/linux/man-pages/man7/netdevice.7.html)) | ||
IFF_NOTRAILERS; | ||
#[cfg(any(target_os = "android", target_os = "dragonfly", target_os = "fuchsia", |
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.
Please list targets one-per-line when wrapping these cfg
s.
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.
Very minor things left, then we're GTM.
src/ifaddrs.rs
Outdated
} | ||
|
||
cfg_if! { | ||
if #[cfg(any(target_os = "emscripten", target_os = "fuchsia", target_os="linux"))] { |
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.
Needs spacing around the =
before linux
.
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.
Oops, corrected.
src/ifaddrs.rs
Outdated
/// | ||
/// Note that the underlying implementation differs between OSes. Only the | ||
/// most common address families are supported by the nix crate (due to | ||
/// lack of time and complexity of testing). For any entry not supported, |
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.
If I was looking at this as a user, my first thought would be oh, what are those specific address families? I think this information is embedded in InterfaceFlags
, yes? If so, that should be mentioned in this comment.
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 some of the InterfaceFlags
are specific per address family, yes. However, it's the returned SockAddr
type(s) that directly map to an AddressFamily
, see from_libc_sockaddr
. I tried to improve the comment.
Also, could you squash this down into 1 commit? Makes viewing the history of |
ad5488e
to
f1e5db7
Compare
src/net/if_.rs
Outdated
IFF_RUNNING; | ||
/// Resources allocated. | ||
#[cfg(any(target_os = "freebsd"))] | ||
IFF_DRV_RUNNING; |
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 should be IFF_RUNNING
instead of IFF_DRV_RUNNING
. The former is an alias for the latter. But you may need to add it to libc first. Ditto for IFF_OACTIVE
below.
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.
libc
defines only IFF_DRV_RUNNING
and IFF_DRV_OACTIVE
for FreeBSD, so that's what I used. Given I don't really use - let alone know - FreeBSD, I don't quite feel in the position to open a PR on libc
.
Especially since FreeBSD seems to prefer the DRV
variant since version 8.1:
https://www.freebsd.org/cgi/man.cgi?query=ifnet
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 distinction between the DRV
variants and the non-DRV
variants only matters for kernel code. You can read the gory details at https://svnweb.freebsd.org/base?view=revision&revision=148886 . It's perfectly fine to use the non-DRV
variants in userland code, especially since their values are identical. For a cross-platform library like nix, the non-DRV
variants are better, because they're portable. The current definitions are at https://svnweb.freebsd.org/base/head/sys/net/if.h?view=markup#l170 . You can go ahead and submit a PR to libc based on those definitions, even if you don't have a FreeBSD VM to test on. If you like, CC me as a reviewer on the libc PR.
src/net/if_.rs
Outdated
/// Avoid use of trailers. (see | ||
/// [`netdevice(7)`](http://man7.org/linux/man-pages/man7/netdevice.7.html)) | ||
#[cfg(any(target_os = "android", | ||
target_os = "bitrig", |
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.
There's no need to worry about bitrig anymore. A future nix PR will probably remove all references. For now, it doesn't really matter whether or not you leave it in.
https://www.phoronix.com/scan.php?page=news_item&px=Bitrig-Fork-Dead
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.
Okay, thanks for the hint.
src/net/if_.rs
Outdated
/// Is able to select media type via ifmap. (see | ||
/// [`netdevice(7)`](http://man7.org/linux/man-pages/man7/netdevice.7.html)) | ||
#[cfg(any(target_os = "android", target_os = "freebsd", target_os = "fuchsia", target_os = "linux"))] | ||
IFF_PORTSEL; |
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 is what caused your build failure. IFF_PORTSEL
is not defined on FreeBSD.
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.
Ah, I see. That's clearly wrong. Corrected.
src/net/if_.rs
Outdated
/// [`netdevice(7)`](http://man7.org/linux/man-pages/man7/netdevice.7.html)) | ||
IFF_PROMISC; | ||
|
||
// bitmask: 0x0200 |
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.
Please remove all these "bitmask: 0xXXX" constants and the blank line between all enum entries here.
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.
Those greatly simplified assembling the list of IFF_
constants and will help maintaining it in the future. Can you please suggest a viable alternative? Dropping that information is not a good option, IMO.
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.
These are all listed in /usr/include/linux/if.h
, so was this helpful for the other platforms? I don't know how this will help make things more maintainable except be incorrect or out of date possible since these values will never likely be updated. I think it's best we remove them.
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.
@Susurrus Yes, because I could map to the other OSes' header files by bitmask and ensure I got the complete list covered (these flags aren't alphabetically ordered, but by value). If you want to take over and keep the list in sync with libc, I'm fine with having the bitmasks removed. However, I'm certainly not ever gonna do it again w/o the information these hints provide.
That being said, maybe there's a better way to "encode" them that better conforms to the style guide applicable for nix.
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 understand how it helped you to compile the list, but it won't be helpful moving forward in maintaining it as I mentioned above. That being said, I'm not going to throw too much more of a fight over it as I don't think this is that important. Please do, however, remove the blank lines in between the variants.
Use IFF_OACTIVE and IFF_RUNNING even on FreeBSD. Deprecate the DRV ones. According to @asomers, libc should propagate the use of the portable constants `IFF_OACTIVE` and `IFF_RUNNING` for user-space applications, instead of `IFF_DRV_OACTIVE` and `IFF_DRV_RUNNING`. It least that's my understanding of [his comment](nix-rust/nix#667 (comment)) in nix-rust/nix#667.
@lucab It's basically whichever gets there first at this point. I would recommend we focus our efforts on this PR since #764 hasn't been touched in months and I don't expect it to ever resolve. @mwanner Please remove the blank lines from |
src/ifaddrs.rs
Outdated
/// | ||
/// Note that the underlying implementation differs between OSes. Only the | ||
/// most common address families are supported by the nix crate (due to | ||
/// lack of time and complexity of testing). The address familiy is encoded |
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.
s/familiy/family/
src/ifaddrs.rs
Outdated
/// } | ||
/// }, | ||
/// Err(err) => { | ||
/// println!("error getting interface addresses: {}", err) |
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 would simplify this example in order to show just the getifaddrs
, by doing:
- a
try!
/unwrap
/expect
on thegetifaddrs
- a
while let
on the interator
The resulting example would be less indented and more compact:
let addrs = getifaddrs()?;
while let Ok(a) = addrs.next() {
println!(...)
}
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.
Good point, I stripped the error handling.
Note however, that the iterator is not over a Result<..>
, but over multiple InterfaceAddress
structs, where always get an interface_name
, but for which we do not (yet) support the address type. Therefore, neither while let Ok(..)
nor while let Some(..)
work and the simple for
loop likely is the easiest. I tried to clarify the example in that regard.
I agree it would be nice to have Except for typos and very verbose example, this PR looks nice at a first glance. Thanks for this! |
e200bc4
to
8dc8137
Compare
@mwanner #801 changed the scope of bit-constants, see #801 (comment). Those are now |
8dc8137
to
985ea01
Compare
@Susurrus, @asomers, @lucab: I think I covered all concerns and the result now passes CI tests as well. As two of you voted against the bitmask comments and the corresponding grouping by empty lines, I removed those. Given the lack of that information, I wonder if alphabetical ordering of the To track progress on the requested support of Given @lucab's simplified example wasn't correct, I fear the Compared to #764, this variant is closer to the libc interface, not providing a |
The way maintenance of this will work is that someone will find a constant that's missing. So they'll just add them. There really isn't going to be someone doing a full audit of these per-platform, so I think we're fine here. I think you're overly concerned with the maintenance burden of that bitmask.
Perfect, thanks.
I don't think we need a complete solution on the first pass, I think this one is pretty good. And I agree with keeping a verbose example. My only question here would be how to make sure that incorrect usage of this API fails at compile time. I think @lucab's example would have failed at compile time, so I think concerns about confusion about this API might be unfounded.
That's a good thing. Overall, I think this looks pretty good. I suggest we merge it. I'll wait until @asomers responds (@lucab feel free to chime in here as well if you have anything to add). |
Yep, LGTM. bors r+ |
Thanks for doing this, and sorry my PR died. I over estimated my ability to do open source while in school, but I'm glad to see that the functionality has been added! |
This function never should've been public, since it's basically impossible to use directly. It's only public due to an oversight from PR nix-rust#667 .
1215: Remove sys::socket::addr::from_libc_sockaddr from the public API r=posborne a=asomers This function never should've been public, since it's basically impossible to use directly. It's only public due to an oversight from PR #667 . Co-authored-by: Alan Somers <asomers@gmail.com>
Here's a first attempt at a rustian interface for
getifaddrs
, please review.Changes for the changelog:
nix::ifaddrs::{getifaddrs, InterfaceAddressIterator, InterfaceAddress and InterfaceFlags}
Closes #650.
Closes #764.