Always define an else clause for portability #ifdefs

If you use #ifdef conditionals in your code to check for portability features, be sure to always define a catch-all else clause that actually does something, even if this something is to error out.

Consider the following code snippet, quoted from gamin's tests/testing.c file:

if (arg != NULL) {
#ifdef HAVE_SETENV
  setenv("GAM_CLIENT_ID", arg, 1);
#elif HAVE_PUTENV
  char *client_id = malloc (strlen (arg) + sizeof "GAM_CLIENT_ID=");
  if (client_id)
  {
      strcpy (client_id, "GAM_CLIENT_ID=");
      strcat (client_id, arg);
      putenv (client_id);
  }
#endif /* HAVE_SETENV */
}
ret = FAMOpen(&(testState.fc));

The FAMOpen method queries the GAM_CLIENT_ID environment variable to set up the connections parameters to the FAM server. If the variable is not defined, the connection will still work, even though it will use some default internal value. In the test code above, the variable is explicitly set to let the tests use a separate server instance.

Now, did you notice that we have to conditional branches? One for setenv and one for putenv? It seems reasonable to assume that one or the other must be present on any Unix system. Unfortunately, this is flawed:

What happens if the code forgets to include config.h?
What happens if the configure script fails to detect both setenv and putenv? This is not that uncommon, given how some configure scripts are written.
What happens if neither setenv nor putenv are available?

The answer to the three questions is: in the above code snippet, the code builds just fine but will misbehave at run time: neither HAVE_SETENV nor HAVE_PUTENV are defined, so the code will not be able to define the required environment variable. However, FAMOpen will later be called and it will not behave as expected because the variable has not been set.

Note that this code snippet is just an example. I have seen many more instances of this exact same problem with worse consequences than the above. Read: they were not part of the test code, but just part of the regular code path.

So how do you implement the above in a saner way? You have two alternatives:

Add an #else clause that contains a fallback implementation. In the case above, we could, for example, prefer to use setenv if present because it has a nicer interface, and fall back to putenv if not found.
This has a disadvantage though: if you forget to include config.h or the configure script cannot correctly detect one of the possible implementations (even when present), you will always use the fallback implementation.
Keep each possible implementation correctly protected by a conditional, but add a #else clause that raises an error at build time. This will make sure that you never forget to define at least one of the portability macros for any reason. This is the preferred approach.

Following the second suggestion above, the code would get the following structure:

#if defined(HAVE_SETENV)
setenv(...);
#elif defined(HAVE_PUTENV)
putenv(...);
#else
#   error "Don't know how to set environment variables."
#endif

With this code, we can be sure that the code will not build if none of the possible implementations are selected. We can later proceed to investigate why that happened.

Always define an else clause for portability #ifdefs

Subscribe

Featured posts

Archive