2
\$\begingroup\$

I implemented the tee command from linux. The program implements the -a option

Reads standard input until end-of-file, write a copy of the input to standard output and to the file named in argv[1].

This is another implementation of the tee command. I posted another code trying to implement the command, but I think I found a better implementation.

Please criticize the implementation

code:

#define _GNU_SOURCE
#include <errno.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <stdio.h> 
#include <stdlib.h>

#ifndef BUF_SIZE        
#define BUF_SIZE 1024
#endif


static void usage(int exit_code) {
    printf("Usage: mytee [-a] FILE\n");
    exit(exit_code);
}

int main(int argc, char *argv[]) {
  int outputFd, openFlags;
  mode_t filePerms;
  int aflag = 0;
  char buf[BUF_SIZE + 1];
  ssize_t numRead;
  int c;

  while ((c = getopt(argc, argv, "ha")) != -1) {
    switch (c) {
      case 'h': usage(0); break;
      case 'a': aflag = 1; break;
    }
  }

  if (optind >= argc) {
    usage(1);
  }

  openFlags = O_CREAT | O_WRONLY;
  if (aflag == 1) {
    openFlags |= O_APPEND;
  } else {
    openFlags |= O_TRUNC;
  }
  filePerms = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP |
    S_IROTH | S_IWOTH;      /* rw-rw-rw- */
  outputFd = open(argv[optind], openFlags, filePerms);
  if (outputFd == -1) {
    perror("open");
    exit(1);
  }

  while ((numRead = read(STDIN_FILENO, buf, BUF_SIZE)) > 0) {
    buf[numRead] = '\0';
    // write to stdout
    printf("%s", buf);
    // write to output file (only up to numRead!)
    if (write(outputFd, buf, numRead) != numRead) {
      perror("write");
      exit(1);
    }
  }

  if (numRead == -1) {
    perror("read");
    exit(1);
  }

  if (close(outputFd) == -1) {
    perror("close output file");
    exit(1);
  }

  exit(0);
}
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$
  • I see no reason to use system calls. Here it on;y hurts portability. After all, a conforming implementation is not required to provide open, read, write, etc. However, fopen, fread, frwite are required, better stick to stdio.

  • printf("%s", buf); immediately restricts this program to text files. Better not treat stdout any differently from the output file. Yet again, fwrite to both.

  • Failing after write(outputFd, buf, numRead) != numRead is too pessimistic. write is allowed to transfer fewer bytes than requested, and it is not an error. Just keep writing unless a real error (write(...) = -1) occurs.

  • Hardcoding the program name in a usage message is a poor practice. At least in Unix-like systems, the same executable may have different names (and behave differently depending on the name; for a concrete example check vi, vim, view). A right way is to use argv[0].

\$\endgroup\$
1
\$\begingroup\$

General Observations

The code looks like it has been copied and pasted from another implementation. One of the reasons I say this is that the exit(STATUS) function has been called repeatedly in main(int argc, char* argv[]). A second reason I say this is that there are only 2 functions, main() and usage(int exit_code). The code within main() is obviously broken up into logic blocks that could be functions.

The code is not portable, unistd.h is not readily available on Windows, and types that might come from it such as mode_t and ssize_t don't compile on Windows. The function getopt() is also not portable, but that could be developed as a utility/library function.

Using `exit(STATUS) in Main

There is never a good reason to call exit(STATUS) main(), one simply returns from main(), exit() only needs to be called from functions that are not the main entry point of the program. Calling exit() in general should only be a worst case scenario, it is okay to call exit() in an application or a shell program, but in an operating system or a device driver it is a really bad idea. The C programming language is primarily used for operating systems development, device driver development and embedded device development, calling exit() is never a good idea in these environments so it is better not to develop the habit.

Readability and Maintainability

There are 2 standard macros defined for exit status and they are part of the C programming standard. These 2 macros are EXIT_SUCCESS and EXIT_FAILURE, and they are defined in stdlib.h which is included in this program. They would make the code much more readable that exit(0) and exit(1). The definitions are system dependent and that makes the code portable.

Functions / Complexity

The function main() 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.

\$\endgroup\$
2
  • \$\begingroup\$ "calling exit() is never a good idea in these environments so it is better not to develop the habit" Do you expect someone who develops this habit to attempt to call exit from a device driver or operating system? \$\endgroup\$ Commented May 22, 2022 at 9:41
  • \$\begingroup\$ @thedefault. I've been programming in C since 1984, you might not believe all the things I've seen software engineers early in their career do. In this particular case I believe I'm dealing with someone who is trying to teach themselves how to write Unix/Linux code in C, possibly for operating systems and trying my best to teach what I know. \$\endgroup\$
    – pacmaninbw
    Commented May 22, 2022 at 14:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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