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

Hide unnecessary error checking from the user #22739

Merged
merged 2 commits into from
Feb 25, 2015
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Hide unnecessary error checking from the user
This affects the `set_non_blocking` function which cannot fail for Unix or
Windows, given correct parameters. Additionally, the short UDP write error case
has been removed as there is no such thing as "short UDP writes", instead, the
operating system will error out if the application tries to send a packet
larger than the MTU of the network path.
  • Loading branch information
tbu- committed Feb 23, 2015
commit d0c589d5ced5006f72d766af2ccecf699ff76176
21 changes: 8 additions & 13 deletions src/libstd/sys/common/net.rs
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,7 @@ pub fn connect_timeout(fd: sock_t,
#[cfg(windows)] use libc::WSAEWOULDBLOCK as WOULDBLOCK;

// Make sure the call to connect() doesn't block
try!(set_nonblocking(fd, true));
set_nonblocking(fd, true);

let ret = match unsafe { libc::connect(fd, addrp, len) } {
// If the connection is in progress, then we need to wait for it to
Expand Down Expand Up @@ -533,7 +533,7 @@ pub fn connect_timeout(fd: sock_t,
};

// be sure to turn blocking I/O back on
try!(set_nonblocking(fd, false));
set_nonblocking(fd, false);
return ret;

#[cfg(unix)]
Expand Down Expand Up @@ -626,7 +626,7 @@ pub struct Guard<'a> {
#[unsafe_destructor]
impl<'a> Drop for Guard<'a> {
fn drop(&mut self) {
assert!(set_nonblocking(self.fd, false).is_ok());
set_nonblocking(self.fd, false);
}
}

Expand Down Expand Up @@ -723,7 +723,7 @@ impl TcpStream {
fd: self.fd(),
guard: self.inner.lock.lock().unwrap(),
};
assert!(set_nonblocking(self.fd(), true).is_ok());
set_nonblocking(self.fd(), true);
ret
}

Expand Down Expand Up @@ -862,7 +862,7 @@ impl UdpSocket {
fd: self.fd(),
guard: self.inner.lock.lock().unwrap(),
};
assert!(set_nonblocking(self.fd(), true).is_ok());
set_nonblocking(self.fd(), true);
ret
}

Expand All @@ -887,9 +887,7 @@ impl UdpSocket {
storagep,
&mut addrlen) as libc::c_int
}));
sockaddr_to_addr(&storage, addrlen as uint).and_then(|addr| {
Ok((n as uint, addr))
})
Ok((n as uint, sockaddr_to_addr(&storage, addrlen as uint).unwrap()))
}

pub fn send_to(&mut self, buf: &[u8], dst: SocketAddr) -> IoResult<()> {
Expand All @@ -910,11 +908,8 @@ impl UdpSocket {
};

let n = try!(write(fd, self.write_deadline, buf, false, dolock, dowrite));
if n != buf.len() {
Err(short_write(n, "couldn't send entire packet at once"))
} else {
Ok(())
}
assert!(n == buf.len(), "UDP packet not completely written.");
Ok(())
}

pub fn join_multicast(&mut self, multi: IpAddr) -> IoResult<()> {
Expand Down
4 changes: 2 additions & 2 deletions src/libstd/sys/unix/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,9 @@ pub fn wouldblock() -> bool {
err == libc::EWOULDBLOCK as i32 || err == libc::EAGAIN as i32
}

pub fn set_nonblocking(fd: sock_t, nb: bool) -> IoResult<()> {
pub fn set_nonblocking(fd: sock_t, nb: bool) {
let set = nb as libc::c_int;
mkerr_libc(retry(|| unsafe { c::ioctl(fd, c::FIONBIO, &set) }))
mkerr_libc(retry(|| unsafe { c::ioctl(fd, c::FIONBIO, &set) })).unwrap();
}

// nothing needed on unix platforms
Expand Down
6 changes: 3 additions & 3 deletions src/libstd/sys/unix/pipe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -235,9 +235,9 @@ impl UnixListener {

_ => {
let (reader, writer) = try!(unsafe { sys::os::pipe() });
try!(set_nonblocking(reader.fd(), true));
try!(set_nonblocking(writer.fd(), true));
try!(set_nonblocking(self.fd(), true));
set_nonblocking(reader.fd(), true);
set_nonblocking(writer.fd(), true);
set_nonblocking(self.fd(), true);
Ok(UnixAcceptor {
inner: Arc::new(AcceptorInner {
listener: self,
Expand Down
6 changes: 3 additions & 3 deletions src/libstd/sys/unix/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,8 @@ impl Process {
unsafe {
let mut pipes = [0; 2];
assert_eq!(libc::pipe(pipes.as_mut_ptr()), 0);
set_nonblocking(pipes[0], true).ok().unwrap();
set_nonblocking(pipes[1], true).ok().unwrap();
set_nonblocking(pipes[0], true);
set_nonblocking(pipes[1], true);
WRITE_FD = pipes[1];

let mut old: c::sigaction = mem::zeroed();
Expand All @@ -362,7 +362,7 @@ impl Process {
fn waitpid_helper(input: libc::c_int,
messages: Receiver<Req>,
(read_fd, old): (libc::c_int, c::sigaction)) {
set_nonblocking(input, true).ok().unwrap();
set_nonblocking(input, true);
let mut set: c::fd_set = unsafe { mem::zeroed() };
let mut tv: libc::timeval;
let mut active = Vec::<(libc::pid_t, Sender<ProcessExit>, u64)>::new();
Expand Down
6 changes: 3 additions & 3 deletions src/libstd/sys/unix/tcp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ impl TcpListener {
-1 => Err(last_net_error()),
_ => {
let (reader, writer) = try!(unsafe { sys::os::pipe() });
try!(set_nonblocking(reader.fd(), true));
try!(set_nonblocking(writer.fd(), true));
try!(set_nonblocking(self.fd(), true));
set_nonblocking(reader.fd(), true);
set_nonblocking(writer.fd(), true);
set_nonblocking(self.fd(), true);
Ok(TcpAcceptor {
inner: Arc::new(AcceptorInner {
listener: self,
Expand Down
6 changes: 3 additions & 3 deletions src/libstd/sys/windows/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,13 +192,13 @@ pub fn wouldblock() -> bool {
err == libc::WSAEWOULDBLOCK as i32
}

pub fn set_nonblocking(fd: sock_t, nb: bool) -> IoResult<()> {
pub fn set_nonblocking(fd: sock_t, nb: bool) {
let mut set = nb as libc::c_ulong;
if unsafe { c::ioctlsocket(fd, c::FIONBIO, &mut set) != 0 } {
(if unsafe { c::ioctlsocket(fd, c::FIONBIO, &mut set) } != 0 {
Err(last_error())
} else {
Ok(())
}
}).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this panic in the true-path and leave out the else path instead of unwrapping a fresh Result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be equivalent and will provide the contents of a potential error.

Copy link
Contributor

Choose a reason for hiding this comment

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

dropping the else-path and calling unwrap() on Err(last_error()) would increase readability imho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}

pub fn init_net() {
Expand Down
13 changes: 12 additions & 1 deletion src/libstd/sys/windows/net.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ pub type wrlen_t = i32;

pub struct Socket(libc::SOCKET);

/// Checks whether the Windows socket interface has been started already, and
/// if not, starts it.
pub fn init() {
static START: Once = ONCE_INIT;

Expand All @@ -38,10 +40,16 @@ pub fn init() {
});
}

/// Returns the last error from the Windows socket interface.
fn last_error() -> io::Error {
io::Error::from_os_error(unsafe { c::WSAGetLastError() })
}

/// Checks if the signed integer is the Windows constant `SOCKET_ERROR` (-1)
/// and if so, returns the last error from the Windows socket interface. . This
/// function must be called before another call to the socket API is made.
///
/// FIXME: generics needed?
pub fn cvt<T: SignedInt>(t: T) -> io::Result<T> {
let one: T = Int::one();
if t == -one {
Expand All @@ -51,11 +59,14 @@ pub fn cvt<T: SignedInt>(t: T) -> io::Result<T> {
}
}

/// Provides the functionality of `cvt` for the return values of `getaddrinfo`
/// and similar, meaning that they return an error if the return value is 0.
pub fn cvt_gai(err: c_int) -> io::Result<()> {
if err == 0 { return Ok(()) }
cvt(err).map(|_| ())
}

/// Provides the functionality of `cvt` for a closure.
pub fn cvt_r<T: SignedInt, F>(mut f: F) -> io::Result<T> where F: FnMut() -> T {
cvt(f())
}
Expand Down Expand Up @@ -112,7 +123,7 @@ impl Socket {

impl Drop for Socket {
fn drop(&mut self) {
unsafe { let _ = libc::closesocket(self.0); }
unsafe { cvt(libc::closesocket(self.0)).unwrap(); }
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/libstd/sys/windows/tcp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ impl TcpAcceptor {
c::WSAEventSelect(socket, events[1], 0)
};
if ret != 0 { return Err(last_net_error()) }
try!(set_nonblocking(socket, false));
set_nonblocking(socket, false);
return Ok(stream)
}
}
Expand Down