Add getifaddrs/ifaddrs wrapper #764

NoraCodes · 2017-09-09T16:09:32Z

This code is from my getaddrs crate, which I think fits quite well into this crate. However, I am aware that there are a number of issues with integrating this code with nix, and would really appreciate some help dealing with those issues.

Error handling is basically nonexistent
I may not be doing memory management right? This is my first time writing any significant unsafe Rust.
This may be a more heavyweight wrapper than other nix-provided wrappers
Not properly feature gated, causing build failures (I think? OTOH all the other PRs are failing)

Many thanks for assistance.

asomers

This looks like a really nice addition to nix, especially since getifaddrs is so difficult to use on its own. The main thing that bothers me is that 'static lifetime where it doesn't belong. Why did you need to do that?

asomers · 2017-09-09T16:19:29Z

src/net/ifaddrs/if_flags.rs

+use libc;
+use libc::c_uint;
+
+bitflags! {


You can simplify this definition by using the libc_bitflags macro. See for example SFlag in src/sys/stat.rs.

asomers · 2017-09-09T16:20:10Z

src/net/ifaddrs/if_flags.rs

+        /// Driver signals L1 up (since Linux 2.6.17)
+        const IFF_LOWER_UP = 1<<16;
+        /// Driver signals dormant (since Linux 2.6.17)
+        const IFF_DORMANT = 1<<17;


Linux-specific flags like this one must be guarded with #[cfg()] directives.

asomers · 2017-09-09T16:26:39Z

src/net/ifaddrs/mod.rs

+/// interface configurations.
+pub struct InterfaceAddrs {
+    inner: *mut libc::ifaddrs,
+    current: Option<&'static libc::ifaddrs>,


Why 'static? That's normally for global variables, but libc::getifaddrs dynamically allocates its storage.

It works just as well using either *mut or &'static; I was told on users.rustlang that it's more "rusty" to do it this way, but I'm happy to revert it if that's better style for the nix crate.

I'm very skeptical. The only reason why your patch compiles now is because you cast that lifetime within an unsafe block. Could you please show me the post where somebody recommended using a static lifetime?

Here's the post: https://users.rust-lang.org/t/code-review-for-my-first-libc-wrapper/12743/9?u=silverwingedseraph

As I said, I'm happy to switch it back.

I think you should.

asomers · 2017-09-09T16:30:40Z

src/net/ifaddrs/mod.rs

+    pub fn query_system() -> Option<Self> {
+       let mut p = null_mut();
+
+       // UNSAFETY: Calling libc FFI function, which allocates memory and


These comments are unnecessary, at least in Nix. The whole crate is riddled with unsafe { libc::...} calls. Anybody who doesn't know why it's unsafe to call a libc function will soon figure out.

asomers · 2017-09-09T16:37:29Z

src/net/ifaddrs/mod.rs

+    pub name: String,
+
+    /// The address assigned to the interface for this protocol.
+    /// A value of `None` means the libc reported a type of IP address that 


s/a type of IP address/a type of address/
The address might not be IP. It might be a MAC, for example.

asomers · 2017-09-09T16:55:18Z

src/net/ifaddrs/sockaddr.rs

+                let s_addr_v4: [u8; 4];
+                unsafe { s_addr_v4 = transmute(data_v4.sin_addr.s_addr); }
+                Some(IpAddr::V4(
+                   Ipv4Addr::from(s_addr_v4) 


Actually, std::net::Ipv4Addr implements From for big endian u32, so you don't need to do the array conversion.

I have tried this and it results in having backwards addresses (lo with the address 1.0.0.127, et cetera).

This sounds like a bug in the standard library, then. If you can reproduce it in a standalone test program, please report it upstream.

asomers · 2017-09-09T16:56:22Z

src/net/ifaddrs/sockaddr.rs

+/// The caller is responsible for guaranteeing that the provided reference
+/// refers to valid memory.
+// Allowing unsused_unsafe because I like to annotate unsafety
+#[allow(unused_unsafe)]


You should not use allow(unused_unsafe). Either the entire function should be unsafe, or it should contain some unsafe blocks. Not both.

asomers · 2017-09-09T17:00:38Z

src/net/ifaddrs/mod.rs

+#[cfg(test)]
+mod tests {
+    #[test]
+    fn tests_get_if_addrs() {


s/get_if_addrs/getifaddrs/

asomers · 2017-09-09T17:03:09Z

src/net/ifaddrs/mod.rs

+    fn tests_get_if_addrs() {
+        let ifs = super::InterfaceAddrs::query_system().unwrap();
+        for i in ifs {
+            println!("{}:", i.name);


This isn't much of a test, because it doesn't verify anything. It's really an example, and should go in the examples directory. Making a proper test would be hard. You'd probably have to compare against the output of ifconfig.

asomers · 2017-09-09T17:04:23Z

src/net/ifaddrs/mod.rs

+impl InterfaceAddrs {
+    /// Produce an `InterfaceAddrs` from the system's information. Returns `None`
+    /// if there are no interfaces to be inspected.
+    pub fn query_system() -> Option<Self> {


May as well name this function getifaddrs. That's the nix convention. We usually name functions the same as the underlying C function, even though the API is rustier.

Susurrus · 2017-09-09T17:10:15Z

src/net/ifaddrs/iff_flags.rs

+        // These flags are available on modern Linuxes
+        #[cfg(any(target_os = "linux", target_os = "android"))]
+        /// Driver signals L1 up (since Linux 2.6.17)
+        const IFF_LOWER_UP = 1<<16;


All constants should be defined in libc if appropriate. Please upstream these constants and then switch to the libc_bitflags macro that's defined in nix (see src/macros).

Susurrus · 2017-09-09T17:12:45Z

First off, thanks for submitting this PR. Most of our functionality is contributed by others rather than us maintainers. Looking briefly through the code, I think it'd be appropriate to include in nix, and @asomers hasn't commented otherwise. We'll just need to do some minor cleanups to the code to bring it inline with nix's conventions.

One thing that this PR is missing is a changelog entry describing the addition of this API.

NoraCodes · 2017-09-09T20:15:13Z

Thanks to both of you for commenting so extensively! Many of @asomers concerns are addressed in a pending commit (which will be pushed after this comment). Remaining:

Error handling
Testing
Upstream IFF_LOWER_UP, IFF_DORMANT, and IFF_ECHO
Convert ifaddrs::iff_flags to use libc_bitflags! macro
Implement Other variant for non-IP addresses.
Switch away from 'static in InterfaceAddrs
Make a changelog entry

I'll do these over the weekend as much as possible, once my homework is done (darn college getting in the way of open source)

asomers · 2017-09-10T23:43:00Z

src/net/ifaddrs/sockaddr.rs

-/// It returns `None` if the libc reports a type of address that `std::net`
-/// doesn't understand, or if the given `sockaddr_input` was null.
+/// It returns `None` if the libc reports a type of address other than
+/// IPv4, IPv6, or EUI-84 MAC, or if the given `sockaddr_input` was null.


asomers · 2017-09-10T23:52:46Z

src/net/ifaddrs/sockaddr.rs

+                Some(IpAddr::V6(Ipv6Addr::from(data_v6.sin6_addr.s6_addr)).into())
+            }
+            libc::AF_PACKET => {
+                let data_mac: &libc::sockaddr_ll = transmute(sa);


On FreeBSD, you should use sockaddr_dl

asomers · 2017-09-10T23:54:08Z

src/net/ifaddrs/sockaddr.rs

+                let data_v6: &libc::sockaddr_in6 = transmute(sa);
+                Some(IpAddr::V6(Ipv6Addr::from(data_v6.sin6_addr.s6_addr)).into())
+            }
+            libc::AF_PACKET => {


Is AF_PACKET a Linux thing? In the BSD world, it's AF_LINK.

Yes, this is a Linux thing. Unfortunately I don't have a BSD box to test on, but I'll try to write conditionally compiled code for this.

asomers · 2017-09-10T23:56:17Z

src/net/ifaddrs/sockaddr.rs

+            }
+            libc::AF_PACKET => {
+                let data_mac: &libc::sockaddr_ll = transmute(sa);
+                if data_mac.sll_halen != 6 {


FYI, Infiniband addresses are longer than 6 bytes. I think it's fine for Nix to ignore them, at least for now.

Yes, this is intended to be implemented later.

asomers · 2017-09-10T23:57:20Z

src/net/ifaddrs/sockaddr.rs

+                        // 6, it's not EUI48 and we can't handle it.
+                } else {
+                    let a = data_mac.sll_addr;
+                    let eui: Eui48 = [a[0], a[1], a[2], a[3], a[4], a[5]];


Can this be expressed as an array operation?

Not as far as I can tell, unfortunately; that is, there's no way to go from data_mac.sll_addr[0..6] (&[u8]) to Eui48 (which is [u8; 6]) except explicitly, because there's no guarantee that &[u8] is 6 bytes long.

asomers · 2017-09-11T00:04:58Z

We still need some tests here. Fortunately, your code that calls libc::getifaddrs is well isolated from the code that interprets its output. Could you write a test that takes canned libc::getifaddrs output and converts it to a HashMap?

asomers · 2017-09-11T02:25:12Z

src/net/ifaddrs/sockaddr.rs

+
+/// Converts a `libc::sockaddr` into an `Option<IfAddrValue>`.
+///
+/// It returns `None` if the libc reports a type of address other than


I've been chewing on this, and I've concluded that we're doing it wrong. There are two problems:

Nix consumers can't extend this function to handle weird stuff like AF_IPX.

Handling link addresses is too platform specific. That functionality probably doesn't belong in Nix at all.

Both of these problems share a common solution. For unknown address families, sockaddr_to_ifaddrvalue should return Other(&libc::sockaddr) instead of None. That way Nix consumers can do whatever they need to handle AF_LINK or anything else.

I like that solution a lot, actually. I'll go ahead and implement that.

…lowing them.

NoraCodes · 2017-09-12T16:56:29Z

Note that a consequence of the change to returning Other(&sockaddr) is that the reference inside InterfaceAddrs now has a sane lifetime.

Add additional interface flags (IFF_) Adds the three missing IFF_ constants (IFF_LOWER_UP, IFF_DORMANT, and IFF_ECHO) per nix-rust/nix#764

NoraCodes · 2017-09-12T20:37:44Z

OK, my checklist is done. I'd love to have a code review of the current state of this at your leisure @asomers

asomers · 2017-09-12T23:53:33Z

src/net/ifaddrs/mod.rs

+    /// `Err(())` will be returned if `p` was void.
+    pub unsafe fn from_raw(p: *mut libc::ifaddrs) -> ::std::result::Result<Self, ()> {
+        match p.as_ref() {
+            Some(r) => Ok(Self {


This fails to compile. You should change Self to InterfaceAddrs

It compiles for me? Something must be screwy with my Rustup, I'll change it. Thanks.

You fixed the wrong line. Just check the Travis build log to see the error I'm talking about. If it works for you, that's probably because you're using a newer version of Rust than Travis. We're using 1.13.0 in Travis.

asomers · 2017-09-13T00:06:33Z

src/net/ifaddrs/tests.rs

+    assert_eq!(hm.len(), 2, "Expected 2 interfaces, found {}.", hm.len());
+
+    assert!(hm.contains_key("test_lo"));
+    assert!(hm.contains_key("test_ext"));


How about assertions for the addresses in hm? How about some IPv6 addresses and AF_LINK addresses in the mix?

asomers · 2017-09-13T00:21:58Z

src/net/ifaddrs/mod.rs

+    ///
+    /// # Errors
+    /// `Err(())` will be returned if `p` was void.
+    pub unsafe fn from_raw(p: *mut libc::ifaddrs) -> ::std::result::Result<Self, ()> {


Since this function is only used by test code, it should not be pub

asomers · 2017-09-13T00:24:00Z

src/net/ifaddrs/mod.rs

    inner: *mut libc::ifaddrs,
-    current: Option<&'static libc::ifaddrs>,
+    current: Option<&'a libc::ifaddrs>,
+    do_free: bool,


It looks like do_free only exists to support your test code. You can eliminate it by using mem::forget in the test code to prevent drop from being run.

asomers · 2017-09-13T00:37:20Z

src/net/ifaddrs/mod.rs

@@ -101,16 +127,17 @@ impl InterfaceAddrs {
            Some(r) => Ok(Self {


Once you eliminate do_free, this section can turn into a call to from_raw.

Susurrus

This all looks like relatively clean, but I'm not certain about the need for from_raw, but I don't fully understand the interface and it looks like @asomers has some suggestions for revising the interface that may overlap with my comments there.

Susurrus · 2017-10-18T04:37:39Z

src/net/ifaddrs/mod.rs

+//! # Examples
+//!
+//! You can access the basic information of the system in a few lines.
+//! The following program prints all the known addresses.


Let's get a colon at the end of this line.

Susurrus · 2017-10-18T04:37:54Z

src/net/ifaddrs/mod.rs

+//!         println!("\t{:?}", addr);
+//!    }
+//! }
+//!


Remove this blank line.

Susurrus · 2017-10-18T04:37:59Z

src/net/ifaddrs/mod.rs

+//! }
+//!
+//! ```
+//!


And this one.

Susurrus · 2017-10-18T04:38:12Z

src/net/ifaddrs/iff_flags.rs

+        IFF_AUTOMEDIA as c_uint;
+        /// The addresses are lost when the interface goes down.
+        IFF_DYNAMIC as c_uint;
+


Please remove this blank line.

Susurrus · 2017-10-18T04:38:22Z

src/net/ifaddrs/iff_flags.rs

+        IFF_DYNAMIC as c_uint;
+
+        // These flags are available on modern Linuxes
+        #[cfg(any(target_os = "linux", target_os = "android"))]


The OSes here should be listed alphabetically.

Susurrus · 2017-10-18T04:52:36Z

src/net/ifaddrs/mod.rs

+            // UNSAFETY: Calling libc FFI function which frees previously allocated
+            // memory.
+            unsafe {
+                // Ask the libc to drop free the memory it allocated when


"to free the memory" I think is what this should read.

Susurrus · 2017-10-18T04:53:11Z

src/net/ifaddrs/mod.rs

+pub struct InterfaceAddr<'a> {
+    /// The name of the interface
+    pub name: String,
+


Please remove these blank lines from between the elements in this struct.

Susurrus · 2017-10-18T04:53:32Z

src/net/ifaddrs/mod.rs

+
+
+/// Represents the configuration and state of a network interface.
+/// Interfaces are uniquely identified by name, and each interface is likely


Needs a blank line above for the generated docs to look pretty (run rustdoc to see what I'm talking about).

Susurrus · 2017-10-18T04:53:49Z

src/net/ifaddrs/mod.rs

+    }
+}
+
+


Remove this blank line. Should only be 1 between things in a file.

Susurrus · 2017-10-18T04:58:19Z

src/net/ifaddrs/sockaddr.rs

+        match sa.sa_family as i32 {
+            libc::AF_INET => {
+                let data_v4: &libc::sockaddr_in = transmute(sa);
+                // Transmuting a u32 into a [u8; 4] because


This should be wrapped to the same number of columns as the code. We should be wrapping everything to 99 characters (which is the Rust standard).

667: Add support for getifaddrs. r=asomers a=mwanner Here's a first attempt at a rustian interface for `getifaddrs`, please review. Changes for the changelog: - Added `nix::ifaddrs::{getifaddrs, InterfaceAddressIterator, InterfaceAddress and InterfaceFlags}` Closes #650. Closes #764.

NoraCodes added 2 commits September 9, 2017 11:05

Add preliminary getifaddrs code

986ebb0

Bring interface flags module into compliance with conventions

4021a36

asomers requested changes Sep 9, 2017

View reviewed changes

Susurrus suggested changes Sep 9, 2017

View reviewed changes

NoraCodes added 2 commits September 9, 2017 15:24

Clean up ifaddrs in accordance with review

71e508e

Move ifaddrs to error handling with nix::Result

5457cca

NoraCodes mentioned this pull request Sep 10, 2017

Add additional interface flags (IFF_) rust-lang/libc#761

Merged

Add support for AF_PACKET (link-level) addresses in net::ifaddrs

87b93a1

asomers reviewed Sep 10, 2017

View reviewed changes

asomers reviewed Sep 11, 2017

View reviewed changes

NoraCodes added 2 commits September 12, 2017 09:59

Return a reference to unrepresentable sockaddrs types instead of swal…

9dc2fb0

…lowing them.

Add test for ifaddrs

ee5d4b5

Add changelog entry for getifaddrs

0751bda

Convert iff_flags to using libc_bitflags macro

fc4a0f2

asomers reviewed Sep 12, 2017

View reviewed changes

Fix incompatible use of Self

c8cb083

asomers reviewed Sep 13, 2017

View reviewed changes

Susurrus suggested changes Oct 18, 2017

View reviewed changes

mwanner mentioned this pull request Nov 17, 2017

Add support for getifaddrs. #667

Merged

bors bot closed this in #667 Dec 4, 2017

		@@ -101,16 +127,17 @@ impl InterfaceAddrs {
		Some(r) => Ok(Self {



		/// Represents the configuration and state of a network interface.
		/// Interfaces are uniquely identified by name, and each interface is likely

Add getifaddrs/ifaddrs wrapper #764

Add getifaddrs/ifaddrs wrapper #764

Conversation

NoraCodes commented Sep 9, 2017 • edited Loading

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Sep 9, 2017

NoraCodes commented Sep 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers commented Sep 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoraCodes commented Sep 12, 2017

NoraCodes commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoraCodes commented Sep 9, 2017 •

edited

Loading

NoraCodes commented Sep 9, 2017 •

edited

Loading