Using __attribute__((cleanup)) in libguestfs

After some discussion upstream (mostly on IRC I’m sad to say), I have pushed a patch to use GCC’s __attribute__((cleanup)) extension (also supported by LLVM, but not by anything else).

This attribute causes a destructor to run automatically when a variable goes out of scope. In libguestfs we’ve added some CLEANUP_* macros to make things a little bit simpler, so I’m going to use those macros in these examples:

{
  CLEANUP_FREE char *buf = NULL;
  ...
  if (asprintf (&buf, "%s/%s", tmpdir, filename) == -1) {
    reply_with_perror ("asprintf");
    return -1;
  }
  ...
  return 0;
}

The CLEANUP_FREE macro ensures that the equivalent of free (buf) is called on every exit path from the scope where the variable ‘buf’ is defined, which means on every return, but also on other exits such as goto or falling off the end.

For most functions, this may save a line or two. Occasionally it can make functions a lot simpler. Compare before:

static char *
icon_windows_xp (guestfs_h *g, struct inspect_fs *fs, size_t *size_r)
{
  char *filename = NULL;
  char *filename_case = NULL;
  char *filename_downloaded = NULL;
  char *pngfile = NULL;
  char *ret;
  struct command *cmd;
  int r;

  /* Download %systemroot%\explorer.exe */
  filename = safe_asprintf (g, "%s/explorer.exe", fs->windows_systemroot);
  filename_case = guestfs___case_sensitive_path_silently (g, filename);
  if (filename_case == NULL)
    goto not_found;

  filename_downloaded = guestfs___download_to_tmp (g, fs, filename_case,
                                                   "explorer.exe",
                                                   MAX_WINDOWS_EXPLORER_SIZE);
  if (filename_downloaded == NULL)
    goto not_found;

  pngfile = safe_asprintf (g, "%s/windows-xp-icon.png", g->tmpdir);

/*...*/
  if (r == -1)
    goto error;
  if (!WIFEXITED (r) || WEXITSTATUS (r) != 0)
    goto not_found;

  if (read_whole_file (g, pngfile, &ret, size_r) == -1)
    goto error;

  free (filename);
  free (filename_case);
  free (filename_downloaded);
  free (pngfile);
  return ret;

 error:
  free (filename);
  free (filename_case);
  free (filename_downloaded);
  free (pngfile);
  return NULL;

 not_found:
  free (filename);
  free (filename_case);
  free (filename_downloaded);
  free (pngfile);
  return NOT_FOUND;
}

and after:

static char *
icon_windows_xp (guestfs_h *g, struct inspect_fs *fs, size_t *size_r)
{
  CLEANUP_FREE char *filename = NULL;
  CLEANUP_FREE char *filename_case = NULL;
  CLEANUP_FREE char *filename_downloaded = NULL;
  CLEANUP_FREE char *pngfile = NULL;
  char *ret;
  struct command *cmd;
  int r;

  /* Download %systemroot%\explorer.exe */
  filename = safe_asprintf (g, "%s/explorer.exe", fs->windows_systemroot);
  filename_case = guestfs___case_sensitive_path_silently (g, filename);
  if (filename_case == NULL)
    return NOT_FOUND;

  filename_downloaded = guestfs___download_to_tmp (g, fs, filename_case,
                                                   "explorer.exe",
                                                   MAX_WINDOWS_EXPLORER_SIZE);
  if (filename_downloaded == NULL)
    return NOT_FOUND;

  pngfile = safe_asprintf (g, "%s/windows-xp-icon.png", g->tmpdir);

/*...*/
  if (r == -1)
    return NULL;
  if (!WIFEXITED (r) || WEXITSTATUS (r) != 0)
    return NOT_FOUND;

  if (read_whole_file (g, pngfile, &ret, size_r) == -1)
    return NULL;

  return ret;
}

It’s not all good news though. It’s almost certainly less efficient: a function call is made on every exit path, even ones which you could prove are irrelevant.

It may make the code more obscure, particularly for people who aren’t used to this feature.

It won’t work at all on non-GCC non-LLVM compilers, although I don’t think that is relevant to libguestfs.

There is some scope for error, especially double-freeing or accidentally freeing buffers which are returned from the function.