Skip to content
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

[WIP] Netlink bits #202

Closed
wants to merge 3 commits into from
Closed

[WIP] Netlink bits #202

wants to merge 3 commits into from

Conversation

polachok
Copy link
Contributor

@polachok polachok commented Nov 8, 2015

Makes it (almost) possible to create a netlink socket.
See #200
Not really because socket() doesn't accept protocol value, and netlink requires it.

@carllerche
Copy link
Contributor

Cool! I'll keep my eye on this.

@polachok
Copy link
Contributor Author

Any ideas on how to solve build failures? Haven't seen this kind before.

@carllerche
Copy link
Contributor

Sorry for the delay! Feel free to ping me on IRC if I am slow :)

I believe that the error is due to using glob imports. By having use super::<something> in the netlinks mod and then in the super mod you have use netlinks::* there is a cycle and I don't think rustc is smart enough yet to figure it out.

I've definitely gotten bit by this in the past and the error messages are very confusing!

@polachok
Copy link
Contributor Author

It's actually smart enough in nightly (and 1.4 I'm using).
I'll try to fix that for 1.1.

@polachok polachok force-pushed the netlink branch 6 times, most recently from b366061 to b8d69ed Compare November 21, 2015 04:27
@polachok
Copy link
Contributor Author

Ok, I finally managed to please 1.1 compiler, please take a look.
I'd also appreciate advice on how to go about the socket() issue (see top message).

@carllerche
Copy link
Contributor

shoot... i let this slip!

Regarding the socket issue, the nix binding should expose the flag and we'll have to bump the version.

@@ -446,7 +460,9 @@ impl fmt::Display for UnixAddr {
#[derive(Copy)]
pub enum SockAddr {
Inet(InetAddr),
Unix(UnixAddr)
Unix(UnixAddr),
#[cfg(any(target_os = "linux", target_os = "android"))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent to match the affected item?

@kamalmarhubi
Copy link
Member

Hi @polachok! I'm going over outstanding PRs on nix. Since you got #211 in, I think this should be mergeable after a few little changes! Thanks!

#[cfg(any(target_os = "linux", target_os = "android"))]
#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)]
#[repr(C)]
pub struct sockaddr_nl {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(sorry for the derive suggestion, should have realised this was present)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also just realised this would mean none of those impls would be present. unclear if they're required or not?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libc already defines Clone and Copy for most or all of its structs (see s! macro: http://rust-lang-nursery.github.io/libc/x86_64-unknown-linux-gnu/src/libc/macros.rs.html#42). So unless PartialEq, Eq, Debug and Hash are necessary, it should be fine to use the libc version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they make sense for the raw struct in our code, they would probably make sense for the corresponding raw struct in libc. Would you create a PR for libc?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just created rust-lang/libc#159 for that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR was rejected, with rationale:

At this time we're not looking for higher level abstractions in libc (e.g. any other form of "Rust code" like Eq or Hash). That's in general left to higher level libraries.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it still makes sense to use the struct from libc and implement the required traits as in:

diff --git a/src/sys/socket/addr.rs b/src/sys/socket/addr.rs
index eadd990..1f38c4d 100644
--- a/src/sys/socket/addr.rs
+++ b/src/sys/socket/addr.rs
@@ -17,16 +17,6 @@ use ::sys::socket::addr::netlink::NetlinkAddr;
  *
  */

-#[cfg(any(target_os = "linux", target_os = "android"))]
-#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)]
-#[repr(C)]
-pub struct sockaddr_nl {
-    pub nl_family: sa_family_t,
-    nl_pad: libc::c_ushort,
-    pub nl_pid: u32,
-    pub nl_groups: u32
-}
-
 #[repr(i32)]
 #[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)]
 pub enum AddressFamily {
@@ -503,7 +493,7 @@ impl SockAddr {
             SockAddr::Inet(InetAddr::V6(ref addr)) => (mem::transmute(addr), mem::size_of::<libc::sockaddr_in6>() as libc::socklen_t),
             SockAddr::Unix(UnixAddr(ref addr, len)) => (mem::transmute(addr), (len + mem::size_of::<libc::sa_family_t>()) as libc::socklen_t),
             #[cfg(any(target_os = "linux", target_os = "android"))]
-            SockAddr::Netlink(NetlinkAddr(ref sa)) => (mem::transmute(sa), mem::size_of::<sockaddr_nl>() as libc::socklen_t),
+            SockAddr::Netlink(NetlinkAddr(ref sa)) => (mem::transmute(sa), mem::size_of::<libc::sockaddr_nl>() as libc::socklen_t),
         }
     }
 }
@@ -559,21 +549,41 @@ impl fmt::Display for SockAddr {

 #[cfg(any(target_os = "linux", target_os = "android"))]
 pub mod netlink {
-    use ::sys::socket::addr::{AddressFamily,sockaddr_nl};
-    use libc::sa_family_t;
-    use std::fmt;
+    use ::sys::socket::addr::{AddressFamily};
+    use libc::{sa_family_t, sockaddr_nl};
+    use std::{fmt, mem};
+    use std::hash::{Hash, Hasher};

-    #[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)]
+    #[derive(Copy, Clone)]
     pub struct NetlinkAddr(pub sockaddr_nl);

+    // , PartialEq, Eq, Debug, Hash
+    impl PartialEq for NetlinkAddr {
+        fn eq(&self, other: &Self) -> bool {
+            let (inner, other) = (self.0, other.0);
+            (inner.nl_family, inner.nl_pid, inner.nl_groups) ==
+            (other.nl_family, other.nl_pid, other.nl_groups)
+        }
+    }
+
+    impl Eq for NetlinkAddr {}
+
+    impl Hash for NetlinkAddr {
+        fn hash<H: Hasher>(&self, s: &mut H) {
+            let inner = self.0;
+            (inner.nl_family, inner.nl_pid, inner.nl_groups).hash(s);
+        }
+    }
+
+
     impl NetlinkAddr {
         pub fn new(pid: u32, groups: u32) -> NetlinkAddr {
-            NetlinkAddr(sockaddr_nl {
-                nl_family: AddressFamily::Netlink as sa_family_t,
-                nl_pad: 0,
-                nl_pid: pid,
-                nl_groups: groups,
-            })
+            let mut addr: sockaddr_nl = unsafe { mem::zeroed() };
+            addr.nl_family = AddressFamily::Netlink as sa_family_t;
+            addr.nl_pid = pid;
+            addr.nl_groups = groups;
+
+            NetlinkAddr(addr)
         }

         pub fn pid(&self) -> u32 {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(above patch should git apply cleanly on your PR)

@kamalmarhubi
Copy link
Member

One last thing re using a struct from libc. Let me know what you think, then we'll get this in! :-)

@kamalmarhubi
Copy link
Member

actually if I've written the patch, I might as well just merge this... rebased and merged as f167e8f...745c791
(ci run at 745c791: https://travis-ci.org/kamalmarhubi/nix-rust/builds/105444337)

@kamalmarhubi
Copy link
Member

Thanks @polachok!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants