HiveBrain v1.2.0
Get Started
← Back to all entries
patternrustMinor

Implementation of UNIX `tee` in Rust

Submitted by: @import:stackexchange-codereview··
0
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 tee program on UNIX systems. According to it's man page


The 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_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.