patternrustMinor
Implementation of UNIX `tee` in Rust
Viewed 0 times
rustimplementationteeunix
Problem
I am currently reading Michael Kerrisk's book, The Linux Programming Interface. This is one of the exercises from the end of chapter 4 (I'm working in Rust, not C).
A Rust implementation of the
The tee utility copies standard input to standard output, making a copy in zero or more files. The output is unbuffered.
So running
running
It's a small example but I'd love feedback on whether this is idiomatic Rust code and whether the efficiency could be improved.
A Rust implementation of the
tee program on UNIX systems. According to it's man pageThe tee utility copies standard input to standard output, making a copy in zero or more files. The output is unbuffered.
So running
echo "foo" | tee will print "foo" to the terminal.running
echo "foo" | tee myfile will print "foo" to the terminal and also create the file myfile with the contents "foo".It's a small example but I'd love feedback on whether this is idiomatic Rust code and whether the efficiency could be improved.
use std::env;
use std::io::{self, Read};
use std::io::prelude::Write;
use std::error::Error;
use std::fs::File;
fn main() {
let fname = match env::args().nth(1) {
Some(name) => name,
None => String::from("/dev/null"),
};
let mut outfile = match File::create(&fname) {
Ok(file) => file,
Err(why) => panic!("{}", why.description()),
};
loop {
let mut line = String::new();
match io::stdin().read_to_string(&mut line) {
Ok(_) => {}
Err(_) => std::process::exit(0),
}
if line.is_empty() {
std::process::exit(0);
}
println!("{}", &line);
let _ = outfile.write(line.as_bytes());
}
}Solution
High-level issues:
-
making a copy in zero or more files
It appears the code only handles zero or one file.
-
The output is unbuffered.
The code is very much buffering the input. Check out
Read all bytes until EOF in this source, placing them into
That means that the program has to wait for the entire input string to exist before it writes a single byte. That input could be gigabytes!
-
The code assumes that all input is a UTF-8 encoded string. This
More tactical issues:
-
Don't
-
Check out all the methods that exist on
-
Hardcoding
-
Printing out the errors
other os error
vs.
Is a directory (os error 21)
-
-
I don't believe the loop will ever loop, since the entire input is read into the string as discussed above.
-
If the loop did loop, reallocating the entire string for each iteration is inefficient. Better to just clear it and reuse the memory.
-
There's no need to pass a reference when using
-
Don't ignore errors!
-
The exit code isn't well defined in all cases. What is it when panicking?
-
Standard in and out would be repeatedly locked in each iteration. Not as efficient as a single lock.
Sorry for going a bit off the deep end, but the problem was interesting. I'd probably write it a little like this:
-
making a copy in zero or more files
It appears the code only handles zero or one file.
-
The output is unbuffered.
The code is very much buffering the input. Check out
read_to_string:Read all bytes until EOF in this source, placing them into
buf.That means that the program has to wait for the entire input string to exist before it writes a single byte. That input could be gigabytes!
-
The code assumes that all input is a UTF-8 encoded string. This
cat cannot be used with images or zipfiles any any kind of binary data.More tactical issues:
-
Don't
use single items from a prelude. The point of a prelude is to be a glob import of all the things you might want.-
Check out all the methods that exist on
Option or Result. For example, unwrap_or_else encapsulates the default value case.-
Hardcoding
/dev/null ties the implementation to a specific OS (maybe OK because you are making cat) but also to a filesystem layout. This cat can't be used when /dev/ isn't available, which would likely be during a nasty system emergency. Would be better for the program to not write anything instead of writing to /dev/null.-
Printing out the errors
description shows less information than printing out the error:other os error
vs.
Is a directory (os error 21)
-
expect may be more compact than a match or unwrap_or_else, depending on the desired error format.-
I don't believe the loop will ever loop, since the entire input is read into the string as discussed above.
-
If the loop did loop, reallocating the entire string for each iteration is inefficient. Better to just clear it and reuse the memory.
-
There's no need to pass a reference when using
println!; that macro automatically references all the arguments.-
Don't ignore errors!
let _ is a really big red flag. cat has few responsibilities, but writing output correctly is fairly critical! The program should exit when writing fails, such as when a pipe is closed.-
The exit code isn't well defined in all cases. What is it when panicking?
-
Standard in and out would be repeatedly locked in each iteration. Not as efficient as a single lock.
use std::env;
use std::io::{self, Read, Write};
use std::fs::File;
fn main() {
let fname = env::args().nth(1).unwrap_or_else(|| String::from("/dev/null"));
let mut outfile = File::create(&fname).expect("Unable to create file");
let mut line = String::new();
loop {
line.clear();
io::stdin().read_to_string(&mut line).unwrap_or_else(|_| {
std::process::exit(0)
});
if line.is_empty() {
std::process::exit(0);
}
println!("{}", line);
outfile.write(line.as_bytes()).expect("Unable to write!");
}
}Sorry for going a bit off the deep end, but the problem was interesting. I'd probably write it a little like this:
use std::env;
use std::io::{self, Read, Write};
use std::fs::File;
const BUFFER_SIZE: usize = 8 * 1024;
fn main() {
let mut buf = vec![0; BUFFER_SIZE];
let stdin = io::stdin();
let mut stdin = stdin.lock();
let stdout = io::stdout();
let stdout = stdout.lock();
let mut outputs: Vec = env::args().skip(1).map(|fname| {
let f = File::create(&fname).unwrap_or_else(|e| {
panic!("Unable to create file {}: {}", fname, e);
});
Box::new(f) as Box
}).collect();
outputs.push(Box::new(stdout));
loop {
let bytes = stdin.read(&mut buf).expect("Unable to read input");
if bytes == 0 { break }
// **Very important**, otherwise you can end up with
// Heartbleed-esque bugs! I'm chosing to shadow `buf` to
// deliberately prevent using it again in this loop.
let buf = &buf[..bytes];
for output in &mut outputs {
output.write_all(buf).expect("Unable to write output");
}
}
}Code Snippets
use std::env;
use std::io::{self, Read, Write};
use std::fs::File;
fn main() {
let fname = env::args().nth(1).unwrap_or_else(|| String::from("/dev/null"));
let mut outfile = File::create(&fname).expect("Unable to create file");
let mut line = String::new();
loop {
line.clear();
io::stdin().read_to_string(&mut line).unwrap_or_else(|_| {
std::process::exit(0)
});
if line.is_empty() {
std::process::exit(0);
}
println!("{}", line);
outfile.write(line.as_bytes()).expect("Unable to write!");
}
}use std::env;
use std::io::{self, Read, Write};
use std::fs::File;
const BUFFER_SIZE: usize = 8 * 1024;
fn main() {
let mut buf = vec![0; BUFFER_SIZE];
let stdin = io::stdin();
let mut stdin = stdin.lock();
let stdout = io::stdout();
let stdout = stdout.lock();
let mut outputs: Vec<_> = env::args().skip(1).map(|fname| {
let f = File::create(&fname).unwrap_or_else(|e| {
panic!("Unable to create file {}: {}", fname, e);
});
Box::new(f) as Box<Write>
}).collect();
outputs.push(Box::new(stdout));
loop {
let bytes = stdin.read(&mut buf).expect("Unable to read input");
if bytes == 0 { break }
// **Very important**, otherwise you can end up with
// Heartbleed-esque bugs! I'm chosing to shadow `buf` to
// deliberately prevent using it again in this loop.
let buf = &buf[..bytes];
for output in &mut outputs {
output.write_all(buf).expect("Unable to write output");
}
}
}Context
StackExchange Code Review Q#144628, answer score: 4
Revisions (0)
No revisions yet.