Implementation of the "tee" utility program

Question

How can I optimize the program? To make it more optimal? The program should be more optimal in terms of source code, compiled executable size, memory usage, speed, bugs etc. Please criticize the program from these points of view.

The -a option adds text to the end of a file (if it exists). The program clones the tee functionality that reads the standard input, and then writes its contents to the standard output.

#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>
#include <fcntl.h>
#include <sys/stat.h>
#include "apue.h"

int writeAll(int fd, char *buf, int buflen);

int main(int argc, char* argv[])
{
    struct stat status;
    int option;
    bool append = false;
    int errCode = 0;

    while((option=getopt(argc,argv,"a"))!=-1) {
        switch(option)
        {
            case 'a':
                append = true;
                break;
        }
    }

    // We need to write in all the files given as parameter AND stdout.
    int numFileDescriptors = argc - optind + 1;

    int *fileDescriptors = (int *)malloc((numFileDescriptors + 1) * sizeof(int));
    char **fileNames = (char **)malloc((numFileDescriptors + 1) * sizeof(char *));

    int lastFileDescriptor = 0;
    fileDescriptors[0] = STDOUT_FILENO;
    fileNames[0] = "stdout";

    int flags = O_CREAT | O_WRONLY;
    if (append) {
        flags = flags | O_APPEND;
    } else {
        flags = flags | O_TRUNC;
    }

    for(int i=optind; i < argc; i++) {
        if (access(argv[i], F_OK) == 0) {
            if (access(argv[i], W_OK) < 0) {
                err_msg("%s: Permission denied", argv[i]);
                errCode = 1;
                continue;
            }
        }

        if (lstat(argv[i],&status) < 0) {
            status.st_mode=0;
        }

        if(S_ISDIR(status.st_mode)) {
            err_msg("%s: Is a directory",argv[i]);
            errCode = 1;
            continue;
        }

        int fd = open(argv[i], flags, 0644);
        if (fd < 0) {
            err_msg("%s: Failed to open", argv[i]);
            errCode = 1;
            continue;
        }

        lastFileDescriptor = lastFileDescriptor + 1;
        fileDescriptors[lastFileDescriptor] = fd;
        fileNames[lastFileDescriptor] = argv[i];
    }

    while(true) {
        size_t len = 0;
        ssize_t read = 0;
        char *line = NULL;

        read = getline(&line, &len, stdin);
        if (read == -1) {
            break;
        }

        for(int i=0; i <= lastFileDescriptor; i++) {
            int written = writeAll(fileDescriptors[i], line, strlen(line));
            if (written < 0) {
                err_msg("%s: Failed to write", fileNames[i]);
                errCode = 1;
            }
        }
    }

    for(int i=0; i <= lastFileDescriptor; i++) {
        close(fileDescriptors[i]);
    }

    free(fileDescriptors);
    free(fileNames);

    return errCode;
}

int writeAll(int fd, char *buf, int buflen) {
    ssize_t written = 0;

    while(written < buflen) {
        int writtenThisTime = write(fd, buf + written, buflen - written);
        if (writtenThisTime < 0) {
            return writtenThisTime;
        }

        written = written + writtenThisTime;
    }

    return written;
} 

This is a follow up question to another attempt to implement the tee command.

Update: I also wrote a test for this program, I would like you to give me feedback about it as well. How can I improve it

#!/bin/bash

# get the testing utilities

. $(dirname "$0")/testutil.sh

CMD="tee"

# test for stdout only
run_test "tee stdout only" < file.txt

# test for stdout and a directory 
run_test "tee stdout and directory" .. < file1.txt

TEMP_FILE1=`mktemp`
TEMP_FILE2=`mktemp`

# test for stdout and multiple files
run_test "tee stdout and multiple files" $TEMP_FILE1 $TEMP_FILE2 < file1.txt

# test for stdout and inaccesible file

chmod 000 $TEMP_FILE1 
run_test "tee stdout and inaccesible file" $TEMP_FILE1 < file1.txt

# cleanup

rm -f $TEMP_FILE1 $TEMP_FILE2 

Answer 1

There are things that could be simplified:

• The variable append exists just to decide the value of flags. Why not just set flags earlier, and modify it when argument -a is seen?
• There's no need to cast the return value from malloc().
• We add 1 when calculating numFileDescriptors, then add another 1 when allocating. That second addition appears unnecessary.
• There's no point performing an access check before open(). The accessibility could change between the two calls anyway. Just call open(), and report any failure (ideally including strerror(errno) in the message). The same goes for testing whether a file is a directory.
• lastFileDescriptor = lastFileDescriptor + 1 is a cumbersome way to write ++lastFileDescriptor.

On the other hand, some things are too simple:

• We fail to handle malloc() returning a null pointer.
• We never free() the getline() buffer (line).
• We create a new buffer every time we call getline() - we should declare (and initialise) line and n outside the loop, and only deallocate after the loop has ended.

Answer 2

General Observations

This is definitely an improvement over the previous question. All calls to exit() have been removed from main. I will say that some of them could have been converted to return statements but overall the code is better. The code is readable and the variable and function name are understandable.

Optimization

You are asking to optimize for both size and speed, in a program this size it may be possible, but in most cases you are going to have to decide to optimize one in favor of the other. Unless you are programming in an embedded environment you probably want to optimize for speed over size.

It is also important to note that the first rule of optimization is don't optimize. Most C compilers today can do a better job of optimizing the code when compiled -O3 then programmers can do. Where programmers have to do optimization is where they can't use the optimizing features of the compiler, this will happen mostly in embedded systems where the compiler may optimize out the code that writes to the hardware addresses. In this case the programmer needs to look at the generated assembly code and may need to write some functions in assembly rather than C.

To optimize code properly one needs to test the code, find any bottlenecks (things that slow down the code) in the performance and then optimize those bottlenecks. Sometimes optimization includes removing or changing a single assembly code instruction, for instance some processors have a decrement and test instruction that combines subtraction and testing for zero in a single instruction, in this case for loops should count down rather than up, but an optimizing compiler should know about this.

To write optimal code it is important to be equally comfortable in C and the target assembly language.

If by optimizing source code you mean reducing the lines of code, the most important thing in programming is to make the code readable and maintainable. If you program professionally someone else may be maintaining your code for 20 or 30 years after you write it. Even you may not recognize it after a year or two.

Observations About the Code

Test for Possible Memory Allocation Errors

In modern high-level languages such as C++, memory allocation errors throw an exception that the programmer can catch. This is not the case in the C programming language. While it is rare in modern computers because there is so much memory, memory allocation can fail, especially if the code is working in a limited memory application such as embedded control systems. In the C programming language when memory allocation fails, the functions malloc(), calloc() and realloc() return NULL. Referencing any memory address through a NULL pointer results in undefined behavior (UB).

Possible unknown behavior in this case can be a memory page error (in Unix this would be call Segmentation Violation), corrupted data in the program and in very old computers it could even cause the computer to reboot (corruption of the stack pointer).

To prevent this undefined behavior a best practice is to always follow the memory allocation statement with a test that the pointer that was returned is not NULL.

Example of Current Code:

    int* fileDescriptors = (int*)malloc((numFileDescriptors + 1) * sizeof(int));
    char** fileNames = (char**)malloc((numFileDescriptors + 1) * sizeof(char*));

Example of Current Code with Test:

    int* fileDescriptors = (int*)malloc((numFileDescriptors + 1) * sizeof(*fileDescriptors));
    if (!fileDescriptors)
    {
        fprintf(stderr, "%s: In main() malloc of the array of file descriptors failed\n", argv[0]);
        return EXIT_FAILURE;
    }
    char** fileNames = (char**)malloc((numFileDescriptors + 1) * sizeof(*fileNames));
    if (!fileDescriptors)
    {
        fprintf(stderr, "%s: In main() malloc of the array of file descriptors failed\n", argv[0]);
        return EXIT_FAILURE;
    }

Convention When Using Memory Allocation in C

When using malloc(), calloc() or realloc() in C a common convention is to sizeof(*PTR) rather sizeof(PTR_TYPE), this make the code easier to maintain and less error prone, since less editing is required if the type of the pointer changes. This is shown above in Example of Current Code with Test:.

Possible Bug

You don't want to enter the following loop if the variable errCode is already set, and the code should break out of the loop if err_code ever gets set.

    while (true) {
        size_t len = 0;
        ssize_t read = 0;
        char* line = NULL;

        read = getline(&line, &len, stdin);
        if (read == -1) {
            break;
        }

        for (int i = 0; i <= lastFileDescriptor; i++) {
            int written = writeAll(fileDescriptors[i], line, strlen(line));
            if (written < 0) {
                err_msg("%s: Failed to write", fileNames[i]);
                errCode = 1;
            }
        }
    }

It is also important to note that this loop should be it's own function.

Possible Improvements to Readability and Understandably

EXIT_FAILURE and EXIT_SUCCESS

These 2 system defined macros are part of the C programming standard and they are also included in C++, however, they predate C++. The code might be slightly more readable and maintainable of they were used. I would modify the code in main(int argc, char* argv[]) by changing the name of the variable errCode to exitStatus and using the macros in the following manner:

    int exitStatus = EXIT_SUCCESS;

    ...

    for (int i = optind; i < argc; i++) {
        if (access(argv[i], F_OK) == 0) {
            if (access(argv[i], W_OK) < 0) {
                err_msg("%s: Permission denied", argv[i]);
                exitStatus = EXIT_FAILURE;
                continue;
            }
        }

        if (lstat(argv[i], &status) < 0) {
            status.st_mode = 0;
        }

        if (S_ISDIR(status.st_mode)) {
            err_msg("%s: Is a directory", argv[i]);
            exitStatus = EXIT_FAILURE;
            continue;
        }

        int fd = open(argv[i], flags, 0644);
        if (fd < 0) {
            err_msg("%s: Failed to open", argv[i]);
            exitStatus = EXIT_FAILURE;
            continue;
        }

        lastFileDescriptor = lastFileDescriptor + 1;
        fileDescriptors[lastFileDescriptor] = fd;
        fileNames[lastFileDescriptor] = argv[i];
    }

Opportunity for Code Optimization

You can reduce 7 lines of code to 2 lines of code here:

    int flags = O_CREAT | O_WRONLY;
    if (append) {
        flags = flags | O_APPEND;
    }
    else {
        flags = flags | O_TRUNC;
    }

can be written as

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

Code Organization

Function prototypes are very useful in large programs that contain multiple source files, and that in case they will be in header files. In a single file program like this it is better to put the main() function at the bottom of the file and all the functions that get used in the proper order above main(). Keep in mind that every line of code written is another line of code where a bug can crawl into the code.

Functions / Complexity

The function main(int argc, char* argv[]) is 103 line of code and comments, and this fills almost 2 screens of an IDE or editor. A generally accepted best practice is that a function should not be larger than a single screen. Anything larger than a single screen is very hard to understand, code, debug and maintain. A programmer may lose sight of what is going on in another portion of the function. Try to keep functions under 50 lines of code (I've had one supervisor that told me any function larger than 10 lines of code is too large). The only time you might want to break this rule is where performance is seriously degraded by function calls and that only happens in very limited circumstances.

The function main(int argc, char* argv[]) is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

The main() function has been reduced from 103 lines of code to 62 lines of code below.

#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>
#include <fcntl.h>
#include <sys/stat.h>
#include "apue.h"

int writeAll(int fd, char* buf, int buflen) {
    ssize_t written = 0;

    while (written < buflen) {
        int writtenThisTime = write(fd, buf + written, buflen - written);
        if (writtenThisTime < 0) {
            return writtenThisTime;
        }

        written = written + writtenThisTime;
    }

    return written;
}

int read_and_write(int lastFileDescriptor, char * fileNames[], int * fileDescriptors)
{
    int exit_status = EXIT_SUCCESS;

    while (true) {
        size_t len = 0;
        ssize_t read = 0;
        char* line = NULL;

        read = getline(&line, &len, stdin);
        if (read == -1) {
            break;
        }

        for (int i = 0; i <= lastFileDescriptor; i++) {
            int written = writeAll(fileDescriptors[i], line, strlen(line));
            if (written < 0) {
                err_msg("%s: Failed to write", fileNames[i]);
                return EXIT_FAILURE;
            }
        }
    }

    return exit_status;
}

int test_status_open_fd(int flags, char *file_name, int *fd, struct stat *status)
{
    int exit_status = EXIT_SUCCESS;

    if (access(file_name, F_OK) == 0) {
        if (access(file_name, W_OK) < 0) {
            err_msg("%s: Permission denied", argv[i]);
            return EXIT_FAILURE;
        }
    }

    if (lstat(file_name, status) < 0) {
        status->st_mode = 0;
    }

    if (S_ISDIR(status->st_mode)) {
        err_msg("%s: Is a directory", file_name);
        return EXIT_FAILURE;
    }

    int fd = open(file_name, flags, 0644);
    if (fd < 0) {
        err_msg("%s: Failed to open", file_name);
        return EXIT_FAILURE;
    }

    return EXIT_SUCCESS;
}

int main(int argc, char* argv[])
{
    struct stat status;
    int option;
    bool append = false;
    int exitStatus = EXIT_SUCCESS;

    while ((option = getopt(argc, argv, "a")) != -1) {
        switch (option)
        {
        case 'a':
            append = true;
            break;
        }
    }

    // We need to write in all the files given as parameter AND stdout.
    int numFileDescriptors = argc - optind + 1;

    int* fileDescriptors = (int*)malloc((numFileDescriptors + 1) * sizeof(*fileDescriptors));
    if (!fileDescriptors)
    {
        fprintf(stderr, "%s: In main() malloc of the array of file descriptors failed\n", argv[0]);
        return EXIT_FAILURE;
    }
    char** fileNames = (char**)malloc((numFileDescriptors + 1) * sizeof(*fileNames));
    if (!fileDescriptors)
    {
        fprintf(stderr, "%s: In main() malloc of the array of file descriptors failed\n", argv[0]);
        return EXIT_FAILURE;
    }

    int lastFileDescriptor = 0;
    fileDescriptors[0] = STDOUT_FILENO;
    fileNames[0] = "stdout";

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

    for (int i = optind; i < argc && exitStatus == EXIT_SUCCESS; i++) {
        int fd = 0;
        exitStatus = test_status_open_fd(flags, argv[i], &fd, &status);

        lastFileDescriptor = lastFileDescriptor + 1;
        fileDescriptors[lastFileDescriptor] = fd;
        fileNames[lastFileDescriptor] = argv[i];
    }

    if (exitStatus == EXIT_SUCCESS)
    {
        exitStatus = read_and_write(lastFileDescriptor, fileNames, fileDescriptors);
    }

    for (int i = 0; i <= lastFileDescriptor; i++) {
        close(fileDescriptors[i]);
    }

    free(fileDescriptors);
    free(fileNames);

    return exitStatus;
}