tee: copy standard input to both stdout and a file

Question

I implemented the tee command from linux. The program also handles the -a option for adding text to the end of a file if it already exists.

I would like you to find some weaknesses, bugs or anything else in the implementation.

#define _POSIX_C_SOURCE 2

#include <errno.h>
#include <err.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>

enum { BUF_SIZE = 1024 };

int
main(int argc, char *argv[])
{
    char buf[BUF_SIZE];
    bool append = false;

    int opt;
    while ((opt = getopt(argc, argv, "+a")) != -1) {
        switch (opt) {
            case 'a':
                append = true;
                break;
            default:
                errx(1, "Usage: %s [-a] [FILE]...", argv[0]);
        }
    }
    argc -= optind;
    argv += optind;

    // Open output files
    int nfiles = argc;
    int *files;
    if (NULL == (files = calloc(nfiles + 1, sizeof(int)))) {
        err(1, "Failed to allocate memory for file handles.");
    }

    int flags = O_WRONLY | O_CREAT | (append ? O_APPEND : O_TRUNC);

    files[0] = 1; // stdout
    for (int i = 0; i < nfiles; ++i) {
        if (-1 == (files[i+1] = open(argv[i], flags, 0644))) {
            err(1, "Failed to open file '%s' for writing", argv[i]);
        }
    }

    ssize_t len;
    while (0 < (len = read(0, buf, sizeof(buf)))) {
        for (int i = 0; i < nfiles+1; ++i) {
            size_t remaining = len;
            while (remaining) {
                ssize_t bytes_written;
                if (-1 == (bytes_written = write(files[i], buf, (size_t)len))) {
                    err(1, "Failed to write to file number '%d'", i);
                }
                remaining -= (size_t)bytes_written;
            }
        }
    }
    if (len == -1) {
        err(1, "Failed to write to file");
    }

    for (int i = 1; i < nfiles; ++i) {
        if (-1 == close(files[i])) {
            err(1, "Failed to close file '%s'", argv[i]);
        }
    }

    free(files);

    return 0;
}

Answer 1

Disclaimer: My C is a little rusty, as I don't write it regularly.

Your code is clear and straightforward and I'm happy with your variable names (which is not always the case), but there are some inconsistencies and one glaring bug.

In your error messages you usually report the name of the file encountering the error but in the write() loop you report the number, which isn't terribly helpful.

The error reported when (len == -1) says you failed to write, but actually you failed to read.

Your write loop is intended to deal with the possibility that a write() call doesn't actually process the whole buffer, but as it's always called with the same buffer address it will always write the start of the buffer, which isn't what you want.

I hope this is some help!

Answer 2

sizeof (what)

read(0, buf, sizeof(buf)) is a nice way to determine the size of buf. Code used the size of the object.

Better to do likewise with files = calloc(nfiles + 1, sizeof(int)). Use the size of the referenced object, not the type. Easy to code right, review and maintain.

files = calloc(nfiles + 1, sizeof files[0])

Avoid naked magic numbers

What is 0644? Better as:

#define SOME_USEFUL_NAME 0644
open(argv[i], flags, SOME_USEFUL_NAME)

Less complex lines. Initialize at declaration

       // ssize_t bytes_written;
       // if (-1 == (bytes_written = write(files[i], buf, (size_t)len)))  

       ssize_t bytes_written = write(files[i], buf, (size_t)len);
       if (-1 == bytes_written) {

Bug?

When i == nfiles, below code is attempting write(null_pointer, buf, (size_t)len). Why iterate i < nfiles+1 and not i < nfiles. Why allocate for nfiles+1?

    for (int i = 0; i < nfiles+1; ++i) {
        size_t remaining = len;
        while (remaining) {
            ssize_t bytes_written;
            if (-1 == (bytes_written = write(files[i], buf, (size_t)len))) {

Answer 3

I'm going to mostly take one part of the code to make some points, but it applies throughout.

I have to say that I dislike side effects on the right of logical expressions. Whilst I appreciate that putting a const on the left is designed to prevent unintentional assignment, I find it adds enough obfuscation that it just increases the risk of other unintentional errors. This also goes for assignment within a logical expression, or for function calls that have side effects (modifying their inputs or wider scope).

I also dislike the idea of taking a loop and then perverting the index. It sets the reader up to understand one thing about the sequence and then does something else with it. The only exception here should be for look-ahead/behind references. By making a conditional on the 'exception to the rule' it makes it clearer (IMHO) what the intention is.

Whilst pre-increment in a for loop has no effect on the sequence, it 'looks' wrong because its a 'post' iteration expression, yet ++i looks like an intention to do it 'pre' iteration. Again, it leads the reader to understand one thing and then does another.

When an API provides defines for flags/modes then they should be used, and often its useful to define what the combination of flags is intended to mean.

Original:

files[0] = 1; // stdout
for (int i = 0; i < nfiles; ++i) {
    if (-1 == (files[i+1] = open(argv[i], flags, 0644))) {
        err(1, "Failed to open file '%s' for writing", argv[i]);
    }
}

Alternative:

#define STDIN  0
#define STDOUT 1
#define CREATION_MODE (S_IRUSR | S_IRUSR) | S_IRGRP | S_IROTH // 0644

for (int i = 0; i < nfiles; ++i) {
    if (i == 0) {
        files[i] = STDOUT;
    } else {
        files[i] = open(argv[i], flags, CREATION_MODE);
        if (files[i]) == -1) {
            err(1, "Failed to open file '%s' for writing", argv[i]);
        }
    }
}

argc/argv - I dislike the idea of modifying input arguments, because it requires a full scan/search of the code to recognise that this approach has been used. For well known arguments (like argc/argv) we should be able to look at the code and trust that they are as passed. In this case, it would be better to establish new variables that contain the baseline, e.g.

char **file_names = &argv[optind];
int nfiles = argc - optind;

In the main copying loop, in the event that write gets interrupted and returns a value less than the requested size, the loop will corrupt the output data, as you are referencing the beginning of buf and len without accounting for the fact that some might already have been written.

Finally, this condition produces the wrong message:

if (len == -1) {
    err(1, "Failed to write to file");  // should be 'read from'
}