Error handling using forever-loop

One of the biggest hassles in programming is handling error conditions. It’s also one of the most important parts to get right because improperly handled errors lead to security defects and general unhappiness of your users.

Error-handling code sucks because it complicates the flow of the code that does real work. Consequently, most programming articles leave it as an exercise to the reader. This often has tragic consequences as many programmers leave error-handling as an exercise to end-users. sigh

Here is some typical code which handles errors, but poorly. Lots of projects I “ported” early in my career contained similar code. Note: Porting is the process of converting a game or program which runs on one platform (like the PlayStation) to another (Windows).

/* Duplicated error-handling code; not recommended */
char * LoadFile_duplicated_code (char * fname) {
  int length;
  char * data;
  FILE * file;

  if (NULL == (file = fopen(fname, "rb")))
    return NULL;

  if (fseek(file, 0, SEEK_END)) {
    fclose(file);
    return NULL;
  }

  if (-1 == (length = ftell(file))) {
    fclose(file);
    return NULL;
  }

  if (NULL == (data = (char *) malloc(length + 1))) {
    fclose(file);
    return NULL;
  }

  if (fseek(file, 0, SEEK_SET)) {
    free(data);
    fclose(file);
    return NULL;
  }

  if (length != fread(data, 1, length, file)) {
    free(data);
    fclose(file);
    return NULL;
  }

  // Cleanup
  fclose(file);

  // NULL terminate the result
  data[length] = 0;

  // Success!
  return data;
}

The code above isn’t terrible, inasmuch as it handles all error conditions. I’ve seen much worse code that didn’t check for errors at all!

Here’s another solution that uses goto statements. I wrote code in this style early in my career; if Warcraft and its tools are ever open-sourced I expect you’ll see code just like this:

/* cleanup using gotos; not recommended */
char * LoadFile_goto_cleanup (char * fname) {
  int length;
  char * data;
  FILE * file;

  if (NULL == (file = fopen(fname, "rb")))
    return NULL;

  if (fseek(file, 0, SEEK_END))
    goto err1;

  if (-1 == (length = ftell(file)))
    goto err1;

  if (NULL == (data = (char *) malloc(length + 1)))
    goto err1;

  if (fseek(file, 0, SEEK_SET))
    goto err2;

  if (length != fread(data, 1, length, file))
    goto err2;

  // Cleanup
  fclose(file);

  // NULL terminate the result
  data[length] = 0;

  // Success!
  return data;

err2:
  free(data);
err1:
  fclose(file);
  return NULL;
}

Using gotos for cleanup is effective but is viewed as anathema by many programmers due to Edsger Dijkstra’s famous dictum, “Goto Considered Harmful”. I used this style for several years but eventually came up with a solution I like better: the forever-loop pattern.

/* centralize cleanup code: recommended */
char * LoadFile_central_cleanup (char * fname) {
  int length;
  char * data = NULL;
  FILE * file = NULL;
  char * result = NULL;

  for (;;) {
    if (NULL == (file = fopen(fname, "rb")))
      break;

    if (fseek(file, 0, SEEK_END))
      break;

    if (-1 == (length = ftell(file)))
      break;

    if (NULL == (data = (char *) malloc(length + 1)))
      break;

    if (fseek(file, 0, SEEK_SET))
      break;

    if (length != fread(data, 1, length, file))
      break;

    // NULL terminate the result
    data[length] = 0;

    // Success!
    result = data;
    data = NULL;
    break;
  }

  if (data) free(data);
  if (file) fclose(file);
  return result;
}

The chief advantage is that the error-handling code doesn’t obscure the real work of the function. Further, the cleanup code is in the main path so it gets executed each time the function runs and is therefore more likely to be correct, always a bonus since most error code isn’t thoroughly tested.

I hope this missive helps improve your code-fu. Good luck!