-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Unix socket doc #38236
Unix socket doc #38236
Conversation
/// | ||
/// ``` | ||
/// | ||
/// ``` |
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 example doesn't seem particularly helpful :)
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 damn!
/// ``` | ||
/// use std::os::unix::net::UnixListener; | ||
/// | ||
/// let socket = match UnixListener::bind("/tmp/sock") { |
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.
IMHO every single example exhaustively matching on socket creation is sort of overzealous.
if let Ok(socket) = UnixListener::bind("/tmp/sock") {
// show off the feature
}
would be a much better alternative IMO.
EDIT: exhaustive matching is fine on the examples of functions that do the actual socket creation, though.
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 I'll just go for unwrap
for the other cases.
/// ```no_run | ||
/// use std::os::unix::net::UnixStream; | ||
/// | ||
/// let socket = match UnixStream::connect("/tmp/sock") { |
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.
unused variable :)
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 don't see it as an issue here, on the opposite. The point is to show people how it works, and creating a (unused) variable helps it.
/// let (sock1, sock2) = match UnixStream::pair() { | ||
/// Ok((sock1, sock2)) => (sock1, sock2), | ||
/// Err(e) => { | ||
/// println!("Couldn't connect: {:?}", e); |
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.
Couldn’t create a pair of sockets.
6c5046c
to
878341f
Compare
Updated. |
477ba42
to
fdbd760
Compare
And urls fixed as well. Build fails but doesn't have errors so I guess it's okay? |
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 good, just a few comments 🎊
/// use std::os::unix::net::UnixStream; | ||
/// | ||
/// let socket = UnixStream::connect("/tmp/sock").unwrap(); | ||
/// socket.set_nonblocking(true).expect("Couldn't set non blocking"); |
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.
Nit: 'nonblocking' is one word
/// use std::os::unix::net::UnixDatagram; | ||
/// | ||
/// let sock = UnixDatagram::unbound().unwrap(); | ||
/// sock.send_to(b"omelette AU fromage", "/some/sock").expect("send_to function failed"); |
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.
Nit: why is "AU" capitalized?
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.
To help english people improve their french. :p
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 me, a person who doesn't know any French, it doesn't help me learn French, it just makes me confused :)
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.
Haha. Well, I'll "fixed" it then.
/// | ||
/// let sock = UnixDatagram::unbound().unwrap(); | ||
/// sock.connect("/some/sock").expect("Couldn't connect"); | ||
/// sock.send(b"omelette AU fromage").expect("send_to function failed"); |
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.
Ditto "AU"
/// let socket = UnixListener::bind("/tmp/sock").unwrap(); | ||
/// let addr = socket.local_addr().expect("Couldn't get local address"); | ||
/// assert_eq!(addr.is_unnamed(), false); | ||
/// ``` |
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 add an example here where is_unnamed()
is true
?
/// let socket = UnixListener::bind("/tmp/sock").unwrap(); | ||
/// let addr = socket.local_addr().expect("Couldn't get local address"); | ||
/// assert_eq!(addr.as_pathname(), Some(Path::new("/tmp/sock"))); | ||
/// ``` |
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.
Same here. If we can create an unnamed socket, can we show here that as_pathname()
is None
?
fdbd760
to
6011527
Compare
Updated. |
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.
r=me unless you want to address the nits
/// let addr = socket.local_addr().expect("Couldn't get local address"); | ||
/// assert_eq!(addr.is_unnamed(), false); | ||
/// | ||
/// // with is_unnamed() as true 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.
Nit: Alternatively, you could also do something like this:
# Examples
An unnamed address:
```
...
```
A named 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.
Oh good point!
/// use std::os::unix::net::UnixStream; | ||
/// | ||
/// let socket = UnixStream::connect("/tmp/sock").unwrap(); | ||
/// let sock_copy = socket.try_clone().expect("Couldn't clone socket..."); |
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.
Nit: The ellipsis here doesn't seem necessary.
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 ellipsis? What do you mean? The expect
call?
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.
6011527
to
2979946
Compare
I addressed the nits. |
/// let addr = socket.local_addr().expect("Couldn't get local address"); | ||
/// assert_eq!(addr.as_pathname(), Some(Path::new("/tmp/sock"))); | ||
/// | ||
/// // with as_pathname() as true 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.
Did you want to do the same thing 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.
Oh indeed!
r=me unless you want to address that comment |
2979946
to
cc180fb
Compare
And updated too. |
@bors r+ rollup 🎉 |
📌 Commit cc180fb has been approved by |
…rewsxcv Unix socket doc r? @frewsxcv
@bors r- Errors: #38387 (comment) |
cc180fb
to
2938e6a
Compare
📌 Commit 2938e6a has been approved by |
…rewsxcv Unix socket doc r? @frewsxcv
⌛ Testing commit 2938e6a with merge 1ca362c... |
💔 Test failed - auto-mac-64-opt-rustbuild |
@bors: retry |
⌛ Testing commit 2938e6a with merge 92039a2... |
💔 Test failed - auto-mac-64-opt-rustbuild |
@bors: retry
…On Fri, Dec 16, 2016 at 7:59 AM, bors ***@***.***> wrote:
💔 Test failed - auto-mac-64-opt-rustbuild
<https://buildbot.rust-lang.org/builders/auto-mac-64-opt-rustbuild/builds/3277>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#38236 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAD95AkNg-hoR9IvK-BHpnHYUbjLkOxUks5rIrV4gaJpZM4LHd2K>
.
|
…rewsxcv Unix socket doc r? @frewsxcv
@bors retry |
@bors: retry |
…rewsxcv Unix socket doc r? @frewsxcv
Rollup of 29 pull requests - Successful merges: #37761, #38006, #38131, #38150, #38158, #38171, #38208, #38215, #38236, #38245, #38289, #38302, #38315, #38346, #38388, #38395, #38398, #38418, #38432, #38451, #38463, #38468, #38470, #38471, #38472, #38478, #38486, #38493, #38498 - Failed merges: #38271, #38483
r? @frewsxcv