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.

About these ads

6 Comments

Filed under Uncategorized

6 responses to “Using __attribute__((cleanup)) in libguestfs

  1. Saggi Mizrahi

    Why not use talloc[1] instead of some obscure compiler feature?
    I know it’s not exactly the same thing but talloc has a much more complete feature set.

    http://talloc.samba.org/talloc/doc/html/index.html

    • rich

      I have no objections at all to talloc, but it would require an even more substantial change for us to properly use a pool-based allocator.

  2. C. M.

    @Rich, we do something very similar to this at my employer. Typically we don’t use cleanup attributes for pointers — instead, we use them for on-stack data structures. E.g., something like this:

    {
    struct foo_map FOO_MAP_INIT_CLEAN(baz);

    foo_map_insert(&baz, bizz, blat);

    // implicit call to foo_map_clean(&baz);
    return
    }

    Where FOO_MAP_INIT_CLEAN(BAZ) expands to something like __attribute__((cleanup(foo_map_clean))) BAZ = FOO_MAP_INITIALIZER;

    FOO_MAP_INITIALIZER is usually just the zero struct, {}.

    It feels a little bit gross to be using (essentially) C++ destructors in C, but we don’t do any automatic I/O on cleanup: we stick to freeing heap memory only.

    • C. M.

      As a follow-up: we use this extensively both in userspace C and in the kernel. We’re pretty happy with it, but yes, it would be nice if GCC’s optimizer would inline the function calls. I haven’t inspected generated code to see what the result is, but I wouldn’t be surprised if you were right about every cleanup being a function call.

      • rich

        I should say it cannot be inlined because of the structure of this particular piece of code in libguestfs. I don’t think it’s a limitation of GCC or anything like that.

  3. C. M.

    Ah, I see. Well, our lawyers are scared of the GPLv3, so we are stuck using a very old GCC 4.2.2 which lacks many of the nice optimizations that come in more recent GCCs.

    However, reviewing our codebase, we define the cleanup functions as ‘inline’, which in GCC 4.2 means always inline, and it looks like generated code does not actually invoke function calls; cleanup is done in place.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s