Skip to content

Commit

Permalink
Replace use of RawFd with File for RefinedJob (#483)
Browse files Browse the repository at this point in the history
  • Loading branch information
@hgoldstein @mmstick
hgoldstein authored and mmstick committed Jul 28, 2017
1 parent 70a77e9 commit b4a8ae4
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 83 deletions.
62 changes: 12 additions & 50 deletions src/shell/job.rs
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use std::fs::File;
use std::process::{Command, Stdio};
use std::os::unix::io::{RawFd, FromRawFd};
use std::os::unix::io::{FromRawFd, IntoRawFd};

//use glob::glob;
use parser::{expand_string, ExpanderFunctions};
use parser::peg::RedirectFrom;
use smallstring::SmallString;
use sys;
use types::*;

#[derive(Debug, PartialEq, Clone, Copy)]
Expand Down Expand Up @@ -54,11 +54,11 @@ pub enum RefinedJob {
/// Arguments to pass in to the procedure
args: Array,
/// A file corresponding to the standard input for this builtin
stdin: Option<RawFd>,
stdin: Option<File>,
/// A file corresponding to the standard output for this builtin
stdout: Option<RawFd>,
stdout: Option<File>,
/// A file corresponding to the standard error for this builtin
stderr: Option<RawFd>,
stderr: Option<File>,
}
}

Expand All @@ -67,7 +67,7 @@ macro_rules! set_field {
match *$self {
RefinedJob::External(ref mut command) => {
unsafe {
command.$field(Stdio::from_raw_fd($arg));
command.$field(Stdio::from_raw_fd($arg.into_raw_fd()));
}
}
RefinedJob::Builtin { ref mut $field, .. } => {
Expand All @@ -89,16 +89,16 @@ impl RefinedJob {
}
}

pub fn stdin(&mut self, fd: RawFd) {
set_field!(self, stdin, fd);
pub fn stdin(&mut self, file: File) {
set_field!(self, stdin, file);
}

pub fn stdout(&mut self, fd: RawFd) {
set_field!(self, stdout, fd);
pub fn stdout(&mut self, file: File) {
set_field!(self, stdout, file);
}

pub fn stderr(&mut self, fd: RawFd) {
set_field!(self, stderr, fd);
pub fn stderr(&mut self, file: File) {
set_field!(self, stderr, file);
}

/// Returns a short description of this job: often just the command
Expand Down Expand Up @@ -140,44 +140,6 @@ impl RefinedJob {

}

impl Drop for RefinedJob {

// This is needed in order to ensure that the parent instance of RefinedJob
// cleans up after its own `RawFd`s; otherwise these would never be properly
// closed, never sending EOF, causing any process reading from these
// `RawFd`s to halt indefinitely.
fn drop(&mut self) {
match *self {
RefinedJob::External(ref mut cmd) => {
drop(cmd);
},
RefinedJob::Builtin {
ref mut name,
ref mut args,
ref mut stdin,
ref mut stdout,
ref mut stderr,
} => {
fn close(fd: Option<RawFd>) {
if let Some(fd) = fd {
if let Err(e) = sys::close(fd) {
eprintln!("ion: failed to close file descriptor '{}': {}",
fd,
e);
}
}
}
drop(name);
drop(args);
close(*stdin);
close(*stdout);
close(*stderr);
}
}
}

}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
75 changes: 42 additions & 33 deletions src/shell/pipe.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::io::{self, Error, Write};
use std::process::{Command, exit};
use std::os::unix::io::{FromRawFd, IntoRawFd, RawFd};
use std::os::unix::io::{FromRawFd, AsRawFd, RawFd};
use std::os::unix::process::CommandExt;
use std::fs::{File, OpenOptions};
use super::flags::*;
Expand All @@ -13,8 +13,9 @@ use super::signals::{self, SignalHandler};
use parser::peg::{Pipeline, Input, RedirectFrom};
use sys;


/// Use dup2 to replace `old` with `new` using `old`s file descriptor ID
pub fn redir(old: RawFd, new: RawFd) {
fn redir(old: RawFd, new: RawFd) {
if let Err(e) = sys::dup2(old, new) {
eprintln!("ion: could not duplicate {} to {}: {}", old, new, e);
}
Expand Down Expand Up @@ -89,7 +90,7 @@ impl<'a> PipelineExecution for Shell<'a> {
Some(Input::File(ref filename)) => {
if let Some(command) = piped_commands.first_mut() {
match File::open(filename) {
Ok(file) => command.0.stdin(file.into_raw_fd()),
Ok(file) => command.0.stdin(file),
Err(e) => {
eprintln!("ion: failed to redirect '{}' into stdin: {}",
filename, e)
Expand All @@ -102,7 +103,9 @@ impl<'a> PipelineExecution for Shell<'a> {
if !string.ends_with('\n') { string.push('\n'); }
match unsafe { stdin_of(&string) } {
Ok(stdio) => {
command.0.stdin(stdio);
command.0.stdin(unsafe {
File::from_raw_fd(stdio)
});
},
Err(e) => {
eprintln!(
Expand All @@ -128,16 +131,16 @@ impl<'a> PipelineExecution for Shell<'a> {
RedirectFrom::Both => {
match f.try_clone() {
Ok(f_copy) => {
command.0.stdout(f.into_raw_fd());
command.0.stderr(f_copy.into_raw_fd());
command.0.stdout(f);
command.0.stderr(f_copy);
},
Err(e) => {
eprintln!("ion: failed to redirect both stderr and stdout into file '{:?}': {}", f, e);
}
}
},
RedirectFrom::Stderr => command.0.stderr(f.into_raw_fd()),
RedirectFrom::Stdout => command.0.stdout(f.into_raw_fd()),
RedirectFrom::Stderr => command.0.stderr(f),
RedirectFrom::Stdout => command.0.stdout(f),
},
Err(err) => {
let stderr = io::stderr();
Expand Down Expand Up @@ -171,10 +174,10 @@ pub fn pipe (
foreground: bool
) -> i32 {

fn close(fd: Option<RawFd>) {
if let Some(fd) = fd {
if let Err(e) = sys::close(fd) {
eprintln!("ion: failed to close file descriptor '{}': {}", fd, e);
fn close(file: &Option<File>) {
if let &Some(ref file) = file {
if let Err(e) = sys::close(file.as_raw_fd()) {
eprintln!("ion: failed to close file '{:?}': {}", file, e);
}
}
}
Expand Down Expand Up @@ -255,12 +258,12 @@ pub fn pipe (
let ret = builtin(shell,
name,
&args,
*stdout,
*stderr,
*stdin);
close(*stdout);
close(*stderr);
close(*stdin);
stdout,
stderr,
stdin);
close(stdout);
close(stderr);
close(stdin);
exit(ret)
},
Ok(pid) => {
Expand Down Expand Up @@ -291,13 +294,19 @@ pub fn pipe (
eprintln!("ion: failed to create pipe: {:?}", e);
},
Ok((reader, writer)) => {
child.stdin(reader);
child.stdin(unsafe {
File::from_raw_fd(reader)
});
match mode {
RedirectFrom::Stderr => {
parent.stderr(writer);
parent.stderr(unsafe {
File::from_raw_fd(writer)
});
},
RedirectFrom::Stdout => {
parent.stdout(writer);
parent.stdout(unsafe {
File::from_raw_fd(writer)
});
},
RedirectFrom::Both => {
let temp = unsafe {
Expand All @@ -308,8 +317,8 @@ pub fn pipe (
eprintln!("ion: failed to redirect stdout and stderr: {}", e);
}
Ok(duped) => {
parent.stderr(temp.into_raw_fd());
parent.stdout(duped.into_raw_fd());
parent.stderr(temp);
parent.stdout(duped);
}
}
}
Expand Down Expand Up @@ -387,7 +396,7 @@ fn execute(shell: &mut Shell, job: &mut RefinedJob, foreground: bool) -> i32 {
let args: Vec<&str> = args
.iter()
.map(|x| x as &str).collect();
let code = builtin(shell, name, &args, *stdout, *stderr, *stdin);
let code = builtin(shell, name, &args, stdout, stderr, stdin);
redir(stdout_bk, sys::STDOUT_FILENO);
redir(stderr_bk, sys::STDERR_FILENO);
redir(stdin_bk, sys::STDIN_FILENO);
Expand Down Expand Up @@ -445,18 +454,18 @@ fn builtin(
shell: &mut Shell,
name: &str,
args: &[&str],
stdout: Option<RawFd>,
stderr: Option<RawFd>,
stdin: Option<RawFd>,
stdout: &Option<File>,
stderr: &Option<File>,
stdin: &Option<File>,
) -> i32 {
if let Some(fd) = stdin {
redir(fd, sys::STDIN_FILENO);
if let Some(ref file) = *stdin {
redir(file.as_raw_fd(), sys::STDIN_FILENO);
}
if let Some(fd) = stdout {
redir(fd, sys::STDOUT_FILENO);
if let Some(ref file) = *stdout {
redir(file.as_raw_fd(), sys::STDOUT_FILENO);
}
if let Some(fd) = stderr {
redir(fd, sys::STDERR_FILENO);
if let Some(ref file) = *stderr {
redir(file.as_raw_fd(), sys::STDERR_FILENO);
}
// The precondition for this function asserts that there exists some `builtin`
// in `shell` named `name`, so we unwrap here
Expand Down

0 comments on commit b4a8ae4

Please sign in to comment.