The Good, the Bad and the Ugly, part 5

Last time, reporting about errno brought the example code to new depths of ugliness. It is about time to introduce some relief.

The most obvious target is the repeated use (abuse ?) of fprintf, which can be addressed by using a simple wrapper function. Even the simplest function, however, introduces a new word in the vocabulary used to describe the solution to the problem implemented by the program - to describe it to a fellow programmer (which might be your future self), that is: the compiler is just as happy with the code as it stands. In other words, each function introduces an abstraction and as such it is essential to pick its name judiciously, so that it is not always necessary to reach for the actual source of the function in order to understand what it does.

  /* Report an error on stderr */
  void err_printf(const char *format, ...) {
    va_list args;

    va_start(args, format);
    (void) vfprintf(stderr, format, args);
    va_end(args);
  }

Now, the somewhat verbose err_printf() might not appeal old UNIX hands, but this is my best effort at conveying what the function is supposed to do. I once read that the inability to come up with a name describing what a function does is a telltale sign that said function is attempting to do too many things at once; hopefully, the above function does not.

In doing its job, err_printf() embeds a decision: we are not interested in the return value of vfprintf, which is documented to be the same value that would be returned by the equivalent fprintf. In other words, the caller will get no information about the success or failure of the function; as briefly suggested last time, should he choose to invoke it, it will either be because he's genuinely uninterested in the outcome, or because he's confident that whatever the outcome subsequent execution of the program will satisfy his requirements.

Please note the above does not tell the reader of a non trivial fragment of code using err_printf() which of the above was the actual intent, i.e. if the programmer was merely uninterested in the outcome or actually confident in its non-relevance. The latter gets problematic if code maintenance adds code afterwards which depends on the successful execution of err_printf, such as a followup message with more details about the problem.

However, even by itself, err_printf brings some measurable improvement to the example code:

/*
 popen-test: experiment with popen(), Part 5.

 Copyright © 2006  Davide Bolcioni <dbolcion@libero.it>
                                                                              
 Distributed under the GPL,
 see http://www.gnu.org/copyleft/gpl.html.

*/

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <stdarg.h>
#include <sys/types.h>
#include <sys/wait.h>

void err_printf(const char* format, ...)
{
  va_list args;

  va_start(args, format);
  (void) vfprintf(stderr, format, args);
  va_end(args);
}

int main(int argc, char* argv[])
{
  int rc = EXIT_FAILURE; /* Optimist */

  if (argc > 1) {
    FILE* sink;

    if (fflush(NULL) != EOF) { /* all open files */
      sink = popen(argv[1], "w");

      if (sink) {
        int status;
        int i;

        /* NB: deliberately allow to test for no output sent */

        for (i = 2; i < argc; i++) {
          if (fputs(argv[i], sink) == EOF) {
            err_printf("%s: fputs(\"%s\") failed: ",
                       argv[0], argv[i]);
            perror("");
            break;
          }
        }

        status = pclose(sink);
    
        /* The return value is funny */

        if (status == -1) {
          err_printf("%s: pclose() failed: ",
                     argv[0]);
          perror("");
        }
        else if (WIFEXITED(status)) {
          rc = WEXITSTATUS(status);
          err_printf("%s: child did an exit(%d).\n",
                     argv[0], rc);
        }
        else if (WIFSIGNALED(status)) {
          err_printf("%s: child terminated by signal %d.\n",
                     argv[0], WTERMSIG(status));
        }
        else if (WIFSTOPPED(status)) {
          err_printf("%s: child stopped with signal %d.\n",
                     argv[0], WSTOPSIG(status));
        }
        else {
          err_printf(
            "%s: I don't think we're in Kansas anymore, Toto.\n",
            argv[0]
          );
        }
      }
      else {
        err_printf("%s: popen(\"%s\") failed: ",
                    argv[0], argv[1]);
        perror("");
      }
    }
    else {
      err_printf("%s: fflush(NULL) failed: ",
                 argv[0]);
      perror("");
    }
  }
  else {
    err_printf("Usage: %s <command> [<arg>]...\n",
               argv[0]);
  }

  return rc;
}

/*
 vim:sw=2:nowrap
*/

The more observant among you might notice that the above code has two problems: it falls afoul of the "what happens after" syndrome discussed above

      ...

      else {
        err_printf("%s: popen(\"%s\") failed: ",
                    argv[0], argv[1]);
        perror("");
      }

      ...

and might trash errno within err_printf(). This second problem is easily fixed¹:

  /* Report an error on stderr */
  void err_printf(const char *format, ...) {
    int saved_errno = errno;
    va_list args;
    

    va_start(args, format);
    (void) vfprintf(stderr, format, args);
    va_end(args);
    errno = saved_errno;
  }

which compares favorably against messing around with saving and restoring errno all over the program. It might be tempting to add perror() to the mix also:

  /* Report an error on stderr */
  void err_printf(const char *format, ...) {
    int saved_errno = errno;
    va_list args;
    

    va_start(args, format);
    (void) vfprintf(stderr, format, args);
    va_end(args);

    if (saved_errno) {
      errno = saved_errno;
      perror("");
    }

    errno = saved_errno;
  }

Since it's not exactly clear what perror() is going to print in case of no error, the above code studiously avoids to trigger that. The next installment will attempt to discuss if the above is really an improvement.

Note [1]: as long as err_printf() and all its clients are compiled with the same threading flags.