Support `EtherAddr` and impl `Debug` for `InterfaceAddress/SockAddr` #813

LuoZijun · 2017-12-06T03:20:13Z

Add EtherAddr .
impl Debug for InterfaceAddress and SockAddr.

Closes #809

Susurrus · 2017-12-06T03:31:44Z

This is a lot of changes, and the commits don't really tell me what has happened here. Each discrete change should really be a separate commit with a description that provides context as to why it's a desired change. I can't really follow what all is being changed here easily, so I'd ask you split this up into separate commits that can evaluated individually.

LuoZijun · 2017-12-06T04:20:49Z

@Susurrus could you review again ? thx.

Susurrus · 2017-12-06T04:39:50Z

You have 2 commits with identical names and one commit with no name. Neither of which have a description.

Please break off the impl Debug changes from your changes that add EtherAddr at a minimum. The goal is to make code changes as small as possible will still being understandable. I have no context for what these changes are about and there seems to be a bunch of changes that aren't necessary, so this is quite hard for me to wrap my head around.

Susurrus · 2017-12-06T05:22:26Z

src/sys/socket/addr.rs

@@ -886,6 +916,20 @@ impl fmt::Display for SockAddr {
    }
 }

+impl fmt::Debug for SockAddr {


Why can't you just #[derive(Debug)] instead on line 692?

Susurrus · 2017-12-06T05:22:42Z

src/sys/socket/addr.rs

+
+impl fmt::Debug for EtherAddr {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "EtherAddr({})", self)


Why is this implemented differently than the others?

Susurrus · 2017-12-06T05:24:10Z

src/sys/socket/ffi.rs

+// Will be unsafe
+// when sdl_nlen > 40.
+#[cfg(any(target_os = "freebsd", target_os = "macos"))]
+pub struct sockaddr_dl {


Please read the CONTRIBUTING.md document. Structs should not be declared here, but instead in libc directly.

this struct from apple/xnu , and not defined in libc .

so, what can i do?

any suggestion ?

It's likely fine to add to libc. libc has been including types that aren't strictly in libc for a while now as long as they're included in system headers. You can make an issue about it in that repo first if you'd like to make sure, but it's likely fine to include. Also note that this will need to work on more than just these 2 platforms for this to get merged.

okay, see rust-lang/libc#866

Susurrus · 2017-12-06T05:24:54Z

So it looks like there are a lot less changes now, so not sure what happened there.

Susurrus · 2017-12-06T16:22:53Z

src/sys/socket/addr.rs

+    }
+}
+
+#[derive(PartialEq, Eq, Clone, Copy, Hash)]


These should be sorted alphabetically.

Susurrus · 2017-12-06T16:23:13Z

src/sys/socket/addr.rs

Please remove this extra newline

Susurrus · 2017-12-06T16:23:25Z

src/sys/socket/addr.rs

@@ -782,6 +871,9 @@ impl PartialEq for SockAddr {
            (SockAddr::Netlink(ref a), SockAddr::Netlink(ref b)) => {
                a == b
            }
+            (SockAddr::Ether(ref a), SockAddr::Ether(ref b)) => {
+              a == b


This isn't properly indented, see the block above.

Susurrus · 2017-12-06T16:24:02Z

src/sys/socket/addr.rs

+            #[cfg(any(target_os = "android", target_os = "linux"))]
+            SockAddr::Ether(..) => AddressFamily::Packet,
+            #[cfg(any(target_os = "dragonfly",
+              target_os = "freebsd",


These aren't aligned properly, the target_os part should all be vertically aligned.

Susurrus · 2017-12-06T16:24:19Z

src/sys/socket/addr.rs

@@ -712,6 +754,10 @@ impl SockAddr {
        SysControlAddr::from_name(sockfd, name, unit).map(|a| SockAddr::SysControl(a))
    }

+    pub fn new_ether(addr: EtherAddr) -> SockAddr {


I don't think we need this helper function.

Susurrus · 2017-12-06T16:24:46Z

src/sys/socket/addr.rs

 pub enum SockAddr {
    Inet(InetAddr),
    Unix(UnixAddr),
    #[cfg(any(target_os = "android", target_os = "linux"))]
    Netlink(NetlinkAddr),
    #[cfg(any(target_os = "ios", target_os = "macos"))]
    SysControl(SysControlAddr),
+    Ether(EtherAddr)


Please put a comma at the end of this line and this should also have doccomments. It'd be very much appreciated if you could provide doc comments for the other variants here as well.

Susurrus · 2017-12-06T16:25:30Z

src/sys/socket/addr.rs

+            #[cfg(any(target_os = "android", target_os = "linux"))]
+            libc::AF_PACKET => Some(AddressFamily::Packet),
+            #[cfg(any(target_os = "dragonfly",
+              target_os = "freebsd",


Also improperly indended.

LuoZijun · 2017-12-13T09:59:26Z

@Susurrus this PR rust-lang/libc#866 has merged on libc .

so i will remove struct sockaddr_dl in sys/socket/ffi.rs .

and cloud i rebase all change into one commit ?

Susurrus · 2017-12-13T16:08:17Z

The goal with the commits is to have changes that are as small and self-contained as possible while still being readable. So in this case I'd like to see two commits: 1) implement Debug for all the types and 2) add the EtherAddr implementation.

I'd also appreciate any tests or example code that could be easily written. This will give us a sense of how this API ends up being used and make sure we don't break anything during major refactorings.

LuoZijun · 2017-12-15T13:37:25Z

@Susurrus ping ...

Susurrus

And please squash your commits down from the 13 that they currently are into 2: the first that implements debug and the 2nd that does everything else. There should also not be any Merge commits, please rebase when you sync with HEAD.

Susurrus · 2017-12-15T16:23:30Z

src/sys/socket/addr.rs

+}
+
+#[derive(Clone, Copy, Eq, Hash, PartialEq)]
+pub struct EtherAddr(pub u8, pub u8, pub u8, pub u8, pub u8, pub u8);


This should be a newtype around either sockaddr_dl or sockaddr_ll depending on the platform.

Susurrus · 2017-12-15T16:27:09Z

src/sys/socket/addr.rs

+#[derive(Clone, Copy, Eq, Hash, PartialEq)]
+pub struct EtherAddr(pub u8, pub u8, pub u8, pub u8, pub u8, pub u8);
+
+impl EtherAddr {


This should have some getters probably to extract the desired info across platforms. Like for your specific use case extracting the address.

extracting the address from EtherAddr , that's easy:

let mac_addr = EtherAddr::new(20,20,20, 20,20,20); let a = mac_addr.0; let b = mac_addr.1; let c = mac_addr.2; // ...

This relates to my point above how the data should not be stored as a tuple, but the backing data store should be a sockaddr_ll or sockaddr_dl struct. Since this backing store will vary across platforms, having a method on EtherAddr that will return just the mac address would be useful.

Susurrus · 2017-12-15T16:28:30Z

src/sys/socket/addr.rs

+                // #[cfg(any(target_os = "android", target_os = "linux"))]
+                #[cfg(target_os = "linux")]
+                Some(AddressFamily::Packet) => {
+                    let sll: *const libc::sockaddr_ll = mem::transmute(addr);


This should work the same way as the other variants and not use mem::transmute().

Susurrus · 2017-12-15T16:28:43Z

src/sys/socket/addr.rs

@@ -748,6 +798,53 @@ impl SockAddr {
                #[cfg(any(target_os = "ios", target_os = "macos"))]
                Some(AddressFamily::System) => Some(SockAddr::SysControl(
                    SysControlAddr(*(addr as *const sys_control::sockaddr_ctl)))),
+                // FIXME: android is same ?


Please resolve this FIXME.

Susurrus · 2017-12-15T16:29:15Z

src/sys/socket/addr.rs

+                    Some(SockAddr::Ether(mac))
+                },
+                #[cfg(any(target_os = "dragonfly",
+                  target_os = "freebsd",


Please indent so target_os is always vertically aligned. There are other places in this document you can reference for the specific style.

LuoZijun · 2017-12-16T00:05:09Z

easy merge:

git merge 4e7a6ecd2b6fbbb7e0e6de40b93e9da7ccb8a034  # impl `Debug` for all sockaddr types
git merge 640e2fe5fadd4c28a497248f98fd0c7aece6c700  # Impl EtherAddr(MAC Addr)
git merge c3d3ea6062a415da75901d70709dc6d7d43ee618  # Add example for list interfaces

Susurrus · 2017-12-16T00:09:33Z

You should be rebasing on top of master, squashing your commits to just 3 as you do, and then force-push into this branch. So it'd be something like:

$ git fetch upstream
$ git rebase -i upstream/master

And on the commits you want to squash, change the pick to a s (for squash). It'll then ask you to modify the commit message if there wasn't a merge conflict. This link describes some of these actions.

Given where you're at, you'll likely want to git fetch upstream && git reset upstream/master to get back to a sane state. Then just make your two commits using git add --patch to choose just the right parts, and the git push origin -f to overwrite the branch this PR is tracking.

LuoZijun · 2017-12-16T00:28:08Z

@Susurrus wow, that's magic 👍

thank you!

Susurrus · 2017-12-16T00:57:05Z

Awesome, nicely done! Git can be tricky to use, and even harder to explain, I'm surprised what I said helped!

Anyways, let's remove that last commit as that example isn't very useful. And this will also need a CHANGELOG entry.

Susurrus · 2017-12-16T00:57:40Z

src/sys/socket/addr.rs

@@ -206,7 +206,7 @@ impl AddressFamily {
    /// Create a new `AddressFamily` from an integer value retrieved from `libc`, usually from
    /// the `sa_family` field of a `sockaddr`.
    ///
-    /// Currently only supports these address families: Unix, Inet (v4 & v6), Netlink
+    /// Currently only supports these address families: Unix, Inet (v4 & v6), Netlink, Link 


Doesn't it support Packet as well?

Susurrus · 2017-12-16T00:59:22Z

src/sys/socket/addr.rs

+}
+
+#[derive(Clone, Copy, Eq, Hash, PartialEq)]
+pub struct EtherAddr {


This should be a new type around the underlying libc types, not a custom strict. See how the other types are declared for what time talking about. You'll just have to make them different definitions for the two different platforms types since the libc types are different.

Susurrus · 2017-12-19T15:30:50Z

src/sys/socket/addr.rs

+
+
+#[derive(Clone, Copy, Eq, Hash, PartialEq)]
+pub struct EtherAddr {


Why didn't you make this a newtype around the underlying libc type as I asked? You add a lot of types that I don't think are necessary.

cfg_if! { if #[cfg(any(target_os = "android", target_os = "linux"))] { pub struct EtherAddr(libc::sockaddr_ll); else if #[cfg(any(target_os = "dragonfly", target_os = "freebsd", target_os = "ios", target_os = "macos", target_os = "netbsd", target_os = "openbsd"))] { pub struct EtherAddr(libc::sockaddr_dl); }

Then to access the MAC address, you can implement a EtherAddr.mac() or something like that. You can defer on all the other struct contents and just implement the MAC address part for now.

Code:

#[cfg(any(target_os = "android", target_os = "linux"))] pub struct EtherAddr(libc::sockaddr_ll); #[cfg(any(target_os = "dragonfly", target_os = "freebsd", target_os = "ios", target_os = "macos", target_os = "netbsd", target_os = "openbsd"))] pub struct EtherAddr(libc::sockaddr_dl); impl EtherAddr { pub fn addr(&self) -> [u8; 6]; }

is that ok ?

Yes, that's more what I was thinking. Is there any other data in there that's common across platforms that we could expose besides addr()?

just hwaddr ([u8; 6]) . no more other data.

i hope u know i havn't english skill.

sry for that.

@LuoZijun No worries about your language skills, we seem to be communicating just fine! And then that's fine if that's the only data there, I think we're on the right track for a nice API here!

LuoZijun · 2017-12-26T01:29:01Z

@Susurrus hi, need any changes ?

Susurrus

There's a ton of #[cfg...] statements around. There's a common idiom of creating a new module for the different platforms that has a single #[cfg..] for it and then you can just pub use ... depending on the platform. This results in a lot less #[cfg...] throughout the code. I'd suggest refactoring to use this approach (see the const module in sys/ioctl/platform/linux.rs)

Susurrus · 2017-12-26T16:47:49Z

src/sys/socket/addr.rs

+                      target_os = "netbsd",
+                      target_os = "openbsd",
+                      target_os = "linux",
+                      target_os = "android"))]


These all need to be alphabetized.

Susurrus · 2017-12-26T16:48:19Z

src/sys/socket/addr.rs

+                      target_os = "openbsd",
+                      target_os = "linux",
+                      target_os = "android"))]
+            SockAddr::Ether(_) => unimplemented!()


I'm not positive what as_ffi_pair() is used for, but why can't this be implemented for Ether? It needs a comment here explaining why it's unimplemented.

@Susurrus libc::sockaddr and libc::sockaddr_dl/libc::sockaddr_ll has diffences size (mem::size_of) . i think i can't implement.

Susurrus · 2017-12-26T16:48:45Z

src/sys/socket/addr.rs

+}
+
+


Remove the extra blank line.

Susurrus · 2017-12-26T16:49:43Z

src/sys/socket/addr.rs

+#[cfg(any(target_os = "android", target_os = "linux"))]
+impl EtherAddr {
+    pub fn new(sll: libc::sockaddr_ll) -> Option<EtherAddr> {


Why do you even implement new()? None of the other types implement it, so I suggest you drop it. You have this implemented to return an Option and then just unwrap the option ever time anyways, so I think this function is useless.

Susurrus · 2017-12-26T16:50:19Z

src/sys/socket/addr.rs

+    }
+
+    pub fn is_empty(&self) -> bool {


Let's remove this as I don't know why it's implemented.

Susurrus · 2017-12-26T16:52:55Z

src/sys/socket/addr.rs

+        let nlen = self.nlen();
+
+        [self.0.sdl_data[nlen    ] as u8,


Please run rustfmt on this as this is an odd way to type this out.

Susurrus · 2017-12-26T16:53:10Z

src/sys/socket/addr.rs

+    pub fn max_nlen(&self) -> usize { 46 }
+
+    pub fn addr(&self) -> [u8; 6] {


Again, all pub fns and types need doc comments.

Susurrus · 2017-12-26T16:53:16Z

src/sys/socket/addr.rs

+          self.0.sdl_data[nlen + 5] as u8 ]
+    }
+


Remove this blank line.

Susurrus · 2017-12-26T16:53:22Z

src/sys/socket/addr.rs

+impl Eq for EtherAddr {}
+
+


Remove this blank line

Susurrus · 2017-12-26T16:54:25Z

src/sys/socket/mod.rs

+          target_os = "netbsd",
+          target_os = "openbsd",
+          target_os = "linux",
+          target_os = "android"))]


Again, please alphabetize this list.

Susurrus

This is looking pretty good! A couple minor style issues and I still want to figure out what to do about as_ffi_pair as I think it should be implemented.

And if you have any test code or examples that could be added that could at least provide compile-tests for this additional API, it'd be most appreciated!

Susurrus · 2017-12-29T03:33:02Z

src/sys/socket/addr.rs

                // Other address families are currently not supported and simply yield a None
                // entry instead of a proper conversion to a `SockAddr`.
-                Some(_) | None => None,


Please leave this as-is.

Susurrus · 2017-12-29T04:15:51Z

src/sys/socket/addr.rs

+                      target_os = "macos",
+                      target_os = "netbsd",
+                      target_os = "openbsd"))]
+            // `libc::sockaddr` and `libc::sockaddr_dl`/`libc::sockaddr_ll`  has diffences size.


I think it's still correct to cast the datatype to sockaddr, though you'll need to do this differently for the sockaddr_dl and sockaddr_ll platforms. It looks like there are other libc APIs that do this kind of thing, which is why the length is being thrown around. I think a better way to look at this is that it's really returning a void pointer rather than one for an actual datatype.

And since we're having this conversation, can you add doccomments to this function?

I'm also wondering if we should alter the return type here to be (*const libc::sockaddr, libc::socklen_t). I think it's more clear how unsafe this is if we leave it a pointer. @asomers any comment on this as you might understand this stuff better than me.

Susurrus · 2017-12-29T04:18:30Z

src/sys/socket/mod.rs

@@ -27,6 +27,7 @@ pub use self::addr::{
    IpAddr,
    Ipv4Addr,
    Ipv6Addr,
+    EtherAddr


Please add a comma at the end of this line.

Susurrus · 2017-12-29T04:19:13Z

src/sys/socket/addr.rs

+            assert!(!self.is_empty());
+
+            let a = data[nlen] as u8;
+            let b = data[nlen+1] as u8;


Pleas put a space around the + for all of these.

LuoZijun · 2017-12-29T05:53:47Z

@Susurrus updated.

Susurrus · 2017-12-31T18:21:13Z

@LuoZijun This isn't passing CI.

LuoZijun · 2018-01-03T12:28:32Z

@Susurrus sorry for delay , i will fix that at tomorrow.

LuoZijun · 2018-01-04T23:23:06Z

@Susurrus updated.

Susurrus

This also needs a rebase now that another PR has landed.

Susurrus · 2018-01-07T16:58:25Z

src/sys/socket/addr.rs

                // Other address families are currently not supported and simply yield a None
                // entry instead of a proper conversion to a `SockAddr`.
                Some(_) | None => None,
            }
        }
    }

+    /// Conversion SockAddr to a libc's sockaddr.


"Conversion from nix's SockAddr type to the underlying libc sockaddr type.

This is useful for interfacing with other libc functions that don't yet have nix wrappers.

Returns a reference to the underlying data type (as a sockaddr reference) along with the size of the actual data type. sockaddr is commonly used as a proxy for a superclass as C doesn't support inheritance, so many functions that take a sockaddr * need to take the size of the underlying type as well and then internally cast it back."

Please use my above comment instead, using backticks (`) around type and library names as I did here.

okay, and how do u think rename EtherAddr to LinkAddr or HwAddr ?

I think LinkAddr would work nicely.

LuoZijun · 2018-01-10T23:44:05Z

@Susurrus do u interesting one RawSocket API to nix?

look here: https://github.com/LuoZijun/exodus/blob/master/netif/src/raw_socket.rs

Susurrus · 2018-01-11T01:44:22Z

I don't see a reason why that couldn't be added as well, but let's defer that work to another PR.

Susurrus · 2018-01-11T01:48:04Z

LGTM.

bors r+

813: Support `EtherAddr` and impl `Debug` for `InterfaceAddress/SockAddr` r=Susurrus a=LuoZijun 1. Add `EtherAddr` . 2. impl `Debug` for `InterfaceAddress` and `SockAddr`. Closes #809

bors · 2018-01-11T04:03:16Z

Build succeeded

Susurrus suggested changes Dec 6, 2017

View reviewed changes

Susurrus suggested changes Dec 15, 2017

View reviewed changes

LuoZijun force-pushed the patch-4 branch from 51a2642 to c3d3ea6 Compare December 16, 2017 00:22

LuoZijun mentioned this pull request Dec 16, 2017

support address family AF_PACKET #809

Closed

Susurrus suggested changes Dec 16, 2017

View reviewed changes

LuoZijun force-pushed the patch-4 branch 6 times, most recently from d8cbaae to 925cd87 Compare December 17, 2017 14:25

Susurrus suggested changes Dec 19, 2017

View reviewed changes

LuoZijun force-pushed the patch-4 branch 2 times, most recently from b9fda8b to cfacb5b Compare December 21, 2017 03:08

Susurrus suggested changes Dec 26, 2017

View reviewed changes

LuoZijun force-pushed the patch-4 branch from cfacb5b to 9fec2ba Compare December 27, 2017 19:23

Susurrus suggested changes Dec 29, 2017

View reviewed changes

LuoZijun force-pushed the patch-4 branch 2 times, most recently from 03bce40 to 3636de9 Compare December 29, 2017 05:50

LuoZijun force-pushed the patch-4 branch from 3636de9 to 46f9a37 Compare December 29, 2017 06:04

LuoZijun force-pushed the patch-4 branch from 46f9a37 to 1e5c39b Compare January 4, 2018 17:26

nix-rust deleted a comment from LuoZijun Jan 7, 2018

Susurrus suggested changes Jan 7, 2018

View reviewed changes

impl Debug for all sockaddr types

7f7266d

LuoZijun force-pushed the patch-4 branch from 1e5c39b to ecbbd46 Compare January 8, 2018 03:50

Impl LinkAddr(MAC Addr)

cf7c2dc

LuoZijun force-pushed the patch-4 branch from ecbbd46 to cf7c2dc Compare January 8, 2018 03:51

Susurrus approved these changes Jan 11, 2018

View reviewed changes

bors bot added a commit that referenced this pull request Jan 11, 2018

Merge #813

a00bd10

813: Support `EtherAddr` and impl `Debug` for `InterfaceAddress/SockAddr` r=Susurrus a=LuoZijun 1. Add `EtherAddr` . 2. impl `Debug` for `InterfaceAddress` and `SockAddr`. Closes #809

bors bot merged commit cf7c2dc into nix-rust:master Jan 11, 2018

LuoZijun deleted the patch-4 branch January 13, 2018 11:59



		#[derive(Clone, Copy, Eq, Hash, PartialEq)]
		pub struct EtherAddr {

		pub fn max_nlen(&self) -> usize { 46 }

		pub fn addr(&self) -> [u8; 6] {

Support EtherAddr and impl Debug for InterfaceAddress/SockAddr #813

Support EtherAddr and impl Debug for InterfaceAddress/SockAddr #813

Conversation

LuoZijun commented Dec 6, 2017 • edited by Susurrus Loading

Susurrus commented Dec 6, 2017

LuoZijun commented Dec 6, 2017

Susurrus commented Dec 6, 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

Susurrus commented Dec 6, 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

LuoZijun commented Dec 13, 2017

Susurrus commented Dec 13, 2017

LuoZijun commented Dec 15, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuoZijun commented Dec 16, 2017

Susurrus commented Dec 16, 2017

LuoZijun commented Dec 16, 2017 • edited Loading

Susurrus commented Dec 16, 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

LuoZijun commented Dec 26, 2017

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

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

LuoZijun commented Dec 29, 2017

Susurrus commented Dec 31, 2017 • edited Loading

LuoZijun commented Jan 3, 2018 • edited Loading

LuoZijun commented Jan 4, 2018

Susurrus left a comment

Choose a reason for hiding this comment

Susurrus Jan 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuoZijun commented Jan 10, 2018

Support `EtherAddr` and impl `Debug` for `InterfaceAddress/SockAddr` #813

Support `EtherAddr` and impl `Debug` for `InterfaceAddress/SockAddr` #813

LuoZijun commented Dec 6, 2017 •

edited by Susurrus

Loading

LuoZijun commented Dec 16, 2017 •

edited

Loading

Susurrus commented Dec 31, 2017 •

edited

Loading

LuoZijun commented Jan 3, 2018 •

edited

Loading

Susurrus Jan 7, 2018 •

edited

Loading