4
\$\begingroup\$

We have modified the tee program with your suggestions from the previous pull request. Did I implement well? I would like to optimize the program even more, what else could I improve? What other possible issues are being implemented?

previous question Implementation of the "tee" utility program

#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>
#include <fcntl.h>
#include <sys/stat.h>
#include <errno.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 main(int argc, char* argv[])
{
    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 = malloc((numFileDescriptors) * sizeof(*fileDescriptors));
    if (fileDescriptors == NULL) {
        err_msg("Memory allocation failed");
        return 1;
    }

    char **fileNames = malloc((numFileDescriptors) * sizeof(*fileNames));
    if (fileNames == NULL) {
        err_msg("Memory allocation failed");
        return 1;
    }

    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; i++) {
        int fd = open(argv[i], flags, 0644);
        if (fd < 0) {
            err_msg("%s: %s", argv[i], strerror(errno));
            errCode = 1;
            continue;
        }

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

    while(true) {
        char line[MAXLINE];
        size_t len = 0;
        
        int c;
        while (len < sizeof(line) && ((c = getchar()) != EOF)) {
            if ((line[len++] = c) == '\n')
                break;
        }

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

        if (c == EOF)
            break;
    }

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

    free(fileDescriptors);
    free(fileNames);

    return errCode;
}

Update: a small question About what @pacmaninbw said. Possible reported bug:"you don't want to enter the following loop if the variable errCode is already set.."

I failed to reply to an error in this case. What I tried was to reduce the permissions on a file while using the tee. Example:

terminal 1:
tee file1.txt file2.txt
some input

terminal 2:
chmod 000 file1.txt

1 (still in tee):
some input 2

Close tee (no error).

chmod 664 file1.txt

diff file1.txt file2.txt

At diff, the files are identical.

man tee indicates the following ways in which errors can be treated:


   MODE determines behavior with write errors on the outputs:
       'warn' diagnose errors writing to any output

       'warn-nopipe'
              diagnose errors writing to any output not a pipe

       'exit' exit on error writing to any output

       'exit-nopipe'
              exit on error writing to any output not a pipe

       The default MODE for the -p option is 'warn-nopipe'.  The default oper‐
       ation when --output-error is not specified, is to exit  immediately  on
       error  writing  to a pipe, and diagnose errors writing to non pipe out‐
       puts.

Is it ok to implement the exit mode (the simplest) in which error is returned if any write fails?

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Thanks, Tony Speight and pacmaninbw \$\endgroup\$
    – Mike
    May 24 at 16:18

1 Answer 1

3
\$\begingroup\$

General Observations

You are obviously interested in improving your Linux and C programming skills and this is a good thing. The code in general is improving. The use of malloc() has definitely improved. The use of the conditional assignment is also an improvement. You have also decreased the lines of code in main, but this can be improved. Sorry for the rant on complexity, but there are reasons.

Consistency

How you open functions and blocks of code should be consistent throughout the code. The functions int writeAll(int fd, char* buf, int buflen) and int main(int argc, char* argv[]) are inconsistent. I'm talking about where the curly brace ({) is. The function int writeAll(int fd, char* buf, int buflen) is consistent with all the other code blocks, the function main() is not. While I personally prefer the style main() uses, if you want to be consistent the the code should look like:

int main(int argc, char* argv[]) {

Complexity / Keep it Simple

I've mentioned this in 2 other reviews; rather than pointing to the Single Responsibility Principle again, I'm going to point out another older programming principle, that actually predates most programming, called the KISS Principle. The KISS principle has various translations, but what it basically states is KEEP IT SIMPLE.

The first thing I learned studying software engineering was Step-wise Refinement, sometimes known as Top Down Design. This was before they started teaching us code. The science/art of programming is to keep breaking problems down into smaller and smaller pieces until each piece of the problem is very easy to solve. Step-wise Refinement is applicable to all programming languages; in object oriented programming you can take a mixed Top Down, Bottom Up design approach. Step-wise refinement is applicable to all engineering, and not just software engineering.

Robust software is software that is easy to expand. Before you start coding, you need to design the software so that you or whoever inherits the software can easily add new functionality. That is why I recommend more smaller functions to start with.

Some examples of why this is important: the Linux Tee command actually takes 6 flags; this is a simplified version of the Linux Tee command. If at some point you want to add more of the functionality the switch statement that parses the flags will grow much larger, and this will make the main() function much larger. If you add the --output-error flag (see below), you are going to need multiple functions just to handle the MODE.
A second reason is that some code is not portable. For instance the getopt() is a POSIX standard; it may now apply to all Linux/Unix operating systems, but that wasn't always the case. getopt() has never been ported to Windows, so to make the code truly portable you will have to write your own version.

From the tee man page

-a, --append append to the given FILEs, do not overwrite
-i, --ignore-interrupts ignore interrupt signals
-p diagnose errors writing to non pipes
--output-error[=MODE] set behavior on write error. See MODE below
--help display this help and exit
--version output version information and exit

MODE determines behavior with write errors on the outputs:
'warn' diagnose errors writing to any output

  'warn-nopipe'  
         diagnose errors writing to any output not a pipe  

  'exit' exit on error writing to any output  

  'exit-nopipe'  
         exit on error writing to any output not a pipe  

To implement more of the flags you might want a struct that contains fields for each of the flags. You might want to have a separate struct for the modes that is included by the flags struct.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.