Skip to content
This repository has been archived by the owner. It is now read-only.

/unix-socket

Implement safe sendmsg/recvmsg abstractions #16

Open
wants to merge 5 commits into
base: master
from

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@teisenbe @elmarco @sfackler @geofft @maghoff
@teisenbe
Copy link

teisenbe commented Sep 8, 2015

Supports SCM_CREDENTIALS and SCM_RIGHTS control messages

Resolves #4 and resolves #5

teisenbe added some commits Sep 7, 2015

@teisenbe
Implement safe sendmsg/recvmsg abstractions 
Supports SCM_CREDENTIALS and SCM_RIGHTS control messages
Loading status checks…
2ab2cc4
@teisenbe
Add tests and expose MSG_* flags
Loading status checks…
54eb9fa
@elmarco

This comment has been minimized.

Sign in to view
Copy link

elmarco commented Sep 16, 2015

thank you!
Cargo.toml Outdated
@@ -8,11 +8,16 @@ repository = "https://github.com/sfackler/rust-unix-socket"
documentation = "https://sfackler.github.io/rust-unix-socket/doc/v0.4.5/unix_socket"
readme = "README.md"
keywords = ["posix", "unix", "socket", "domain"]
links = "cmsg_manip"

This comment has been minimized.

Sign in to view
Copy link
@sfackler

sfackler Sep 17, 2015

Member

This isn't necessary - it's only needed when linking to third party stuff.
src/cmsg_manip/cmsg.c
@@ -0,0 +1,41 @@
#define _GNU_SOURCE

This comment has been minimized.

Sign in to view
Copy link
@sfackler

sfackler Sep 17, 2015

Member

Is this specifically necessary?

This comment has been minimized.

Sign in to view
Copy link
@sfackler

sfackler Sep 17, 2015

Member

Ah, nevermind, looks like it's required to get ucred.
src/lib.rs Outdated

fn set_passcred(&self, receive_creds: bool) -> io::Result<()> {
unsafe {
let v: libc::c_int = if receive_creds { 1 } else { 0 };

This comment has been minimized.

Sign in to view
Copy link
@sfackler

sfackler Sep 17, 2015

Member

You can also just do receive_creds as c_int FYI
src/sendmsg_impl.rs Outdated
}

#[allow(dead_code)]
#[link(name = "cmsg_manip")]

This comment has been minimized.

Sign in to view
Copy link
@sfackler

sfackler Sep 17, 2015

Member

This isn't required - the build.rs will make sure this happens.
src/lib.rs Outdated
/// flags is a pass-through of the flags specified in the sendmsg(2) man page
///
/// On success, returns the number of bytes written.
pub fn recvmsg(&self, buffers: &[&mut[u8]], cmsg_buffer: &mut [u8], flags: libc::c_int) -> io::Result<RecvMsgResult> {

This comment has been minimized.

Sign in to view
Copy link
@sfackler

sfackler Sep 17, 2015

Member

The APIs here seem slightly too low-level, IMO, specifically the flags field here.

This comment has been minimized.

Sign in to view
Copy link
@teisenbe

teisenbe Sep 21, 2015

Author

Yeah. I'm a little up in the air about the cmsg_buffer option, too. I'll probably replace it with a stack allocated array inside of the function
src/lib.rs Outdated
/// flags is a pass-through of the flags specified in the sendmsg(2) man page
///
/// On success, returns the number of bytes written.
pub fn sendmsg<P: AsRef<Path>>(&self, path: Option<P>, buffers: &[&[u8]], ctrl_msgs: &[ControlMsg], flags: libc::c_int) -> io::Result<usize> {

This comment has been minimized.

Sign in to view
Copy link
@sfackler

sfackler Sep 17, 2015

Member

Same deal here with flags
src/lib.rs Outdated
/// List of all control messages received during this call
pub control_msgs: Vec<ControlMsg>,
/// Flags returned by recvmsg, see the recv(2) man page for a list
pub flags: libc::c_int,

This comment has been minimized.

Sign in to view
Copy link
@sfackler

sfackler Sep 17, 2015

Member

And here
@sfackler

This comment has been minimized.

Sign in to view
Copy link
Member

sfackler commented Sep 17, 2015

Thanks for doing this! I left some comments here and there. Also, this'll need to be put behind a feature since these are Linux-only APIs I believe.

I'm not totally sure what the API for sendmsg and recvmsg should look like, but it seems like we can do better than a raw c_int of flags.
@teisenbe

This comment has been minimized.

Sign in to view
Copy link
Author

teisenbe commented Sep 21, 2015

I think all of the flags are potentially applicable and usable with this API. If you want, I can replace the c_int with a struct of bools to make it a bit more friendly? Can also take it out completely, but I think the options flags provides and the data it returns are genuinely useful. Will probably work to address comments today
@sfackler

This comment has been minimized.

Sign in to view
Copy link
Member

sfackler commented Sep 25, 2015

Oh yeah, the options flags are definitely useful, I'd just prefer something more structured than a raw c_int. A struct with bool setters/getters seems reasonable to me.
@teisenbe
Address comments
Loading status checks…
eaebccd
@teisenbe

This comment has been minimized.

Sign in to view
Copy link
Author

teisenbe commented Sep 25, 2015

Take a look at the latest version of the pull request. Replaced it with the struct as requested
@geofft

This comment has been minimized.

Sign in to view
Copy link

geofft commented Oct 8, 2015

I just saw this -- I have a PR for a pure-Rust implementation of this (just SCM_RIGHTS, but SCM_CREDENTIALS would be easy to add) in nix-rust/nix#179 (now nix-rust/nix#197). We should potentially make sure our APIs are in sync, and maybe it makes sense to share implementation, too.
src/lib.rs Outdated
@@ -771,6 +802,46 @@ impl UnixDatagram {
}
}

/// Receives data on the socket.
///
/// If path is None, the peer address set by the `connect` method will be used. If it has not

This comment has been minimized.

Sign in to view
Copy link
@sfackler

sfackler Oct 11, 2015

Member

path doesn't appear to be a parameter here anymore.
src/lib.rs Outdated
/// This interface allows receiving data into multiple buffers. This acts as if the buffers had been
/// concatenated in the order they were given.
///
/// cmsg_buffer is space to use for storing control messages.

This comment has been minimized.

Sign in to view
Copy link
@sfackler

sfackler Oct 11, 2015

Member

This field seems a bit weird - it's basically scratch space for the internal logic, right?

This comment has been minimized.

Sign in to view
Copy link
@teisenbe

teisenbe Oct 11, 2015

Author

Yeah, just to prevent having to do an allocation on every sendmsg call even if cmsgs aren't being used. Though I guess a stack allocation is basically free so could kill it
Cargo.toml
[dev-dependencies]
tempdir = "0.3"

[features]
default = ["from_raw_fd"]
default = ["from_raw_fd", "sendmsg"]

This comment has been minimized.

Sign in to view
Copy link
@sfackler

sfackler Oct 11, 2015

Member

Under what contexts would people want to disable this feature?

This comment has been minimized.

Sign in to view
Copy link
@teisenbe

teisenbe Oct 11, 2015

Author

Well, the SCM_RIGHTS stuff isn't portable past Linux (POSIX doesn't define what gets passed in the message, I believe)
@teisenbe

This comment has been minimized.

Sign in to view
Copy link
Author

teisenbe commented Oct 15, 2015

Finally got around to addressing the comments

@maghoff maghoff referenced this pull request Dec 14, 2015

Open

Support credential transfer #5

src/sendmsg_impl.rs
///
/// This is a Rust version of `struct ucred` from sys/socket.h
#[derive(Clone, Debug)]
pub struct UCred{

This comment has been minimized.

Sign in to view
Copy link
@maghoff

maghoff Dec 17, 2015

struct ucred now exists in libc, from version 0.2.3: rust-lang/libc@4920c7e :)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.