Ensure atomic creation of a cached appliance

Cached appliances are discovered by their predictable path. Previously we were
creating a cached appliance directly in this predictable path. This had at least
2 undesirable effects:

* Interrupting appliance creation would leave a corrupt appliance
* 2 processes could simultaneously attempt to create the same appliance, causing
  corruption.

This patch causes the cached appliance to be created in a temporary directory,
and then renamed to the predictable path. As rename is an atomic operation, this
makes the whole creation atomic.

This patch also changes the predictable path to have a prefix of 'guestfs.'.
This will make it simpler for system administrators to clean up old cached
appliances.

This patch resolves RHBZ#639405

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 970068d..699e90b 100644 (file)
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -111,6 +111,7 @@ perl/lib/Sys/Guestfs/Lib.pm
 php/extension/guestfs_php.c
 python/guestfs-py.c
 regressions/rhbz501893.c
 php/extension/guestfs_php.c
 python/guestfs-py.c
 regressions/rhbz501893.c

+regressions/test-launch-race.pl

 regressions/test-lvm-mapping.pl
 regressions/test-noexec-stack.pl
 ruby/ext/guestfs/_guestfs.c
 regressions/test-lvm-mapping.pl
 regressions/test-noexec-stack.pl
 ruby/ext/guestfs/_guestfs.c

diff --git a/regressions/Makefile.am b/regressions/Makefile.am
index b7e2816..c9156f9 100644 (file)
--- a/regressions/Makefile.am
+++ b/regressions/Makefile.am
@@ -38,6 +38,7 @@ TESTS = \
        test-find0.sh \
        test-guestfish-a.sh \
        test-guestfish-d.sh \
        test-find0.sh \
        test-guestfish-a.sh \
        test-guestfish-d.sh \

+       test-launch-race.pl \

        test-luks.sh \
        test-lvm-filtering.sh \
        test-lvm-mapping.pl \
        test-luks.sh \
        test-lvm-filtering.sh \
        test-lvm-mapping.pl \

diff --git a/regressions/test-launch-race.pl b/regressions/test-launch-race.pl
new file mode 100755 (executable)
index 0000000..941eff6
--- /dev/null
+++ b/regressions/test-launch-race.pl
@@ -0,0 +1,60 @@
+#!/usr/bin/perl
+# Copyright (C) 2010 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+
+# Test that 2 simultaneous launches in a clean cache directory will both succeed
+
+use strict;
+use warnings;
+
+use File::Temp qw(tempdir);
+use POSIX;
+
+use Sys::Guestfs;
+
+# Use a temporary TMPDIR to ensure it's clean
+my $tmpdir = tempdir (CLEANUP => 1);
+$ENV{TMPDIR} = $tmpdir;
+
+my $testimg = $tmpdir.'/test.img';
+system ("touch $testimg");
+
+my $pid = fork();
+die ("fork failed: $!") if ($pid < 0);
+
+if ($pid == 0) {
+  my $g = Sys::Guestfs->new ();
+  $g->add_drive ($testimg);
+  $g->launch ();
+  _exit (0);
+}
+
+my $g = Sys::Guestfs->new ();
+$g->add_drive ($testimg);
+$g->launch ();
+$g = undef;
+
+waitpid ($pid, 0) or die ("waitpid: $!");
+die ("child failed") unless ($? == 0);
+
+# Check that only 1 temporary cache directory was created
+my $dh;
+opendir ($dh, $tmpdir) or die ("Failed to open $tmpdir: $!");
+my @cachedirs = grep { /^guestfs\./ } readdir ($dh);
+closedir ($dh) or die ("Failed to close $tmpdir: $!");
+
+my $ncachedirs = scalar(@cachedirs);
+die ("Expected 1 cachedir, found $ncachedirs") unless ($ncachedirs == 1);

diff --git a/src/appliance.c b/src/appliance.c
index 8c8ddd0..7dc42c1 100644 (file)
--- a/src/appliance.c
+++ b/src/appliance.c
@@ -18,6 +18,8 @@
 
 #include <config.h>
 
 
 #include <config.h>
 

+#include <errno.h>
+#include <dirent.h>

 #include <stdio.h>
 #include <stdlib.h>
 #include <stdarg.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdarg.h>


@@ -249,9 +251,9 @@ check_for_cached_appliance (guestfs_h *g,
 {
   const char *tmpdir = guestfs_tmpdir ();
 
 {
   const char *tmpdir = guestfs_tmpdir ();
 

-  size_t len = strlen (tmpdir) + strlen (checksum) + 2;
+  size_t len = strlen (tmpdir) + strlen (checksum) + 10;

   char cachedir[len];
   char cachedir[len];

-  snprintf (cachedir, len, "%s/%s", tmpdir, checksum);
+  snprintf (cachedir, len, "%s/guestfs.%s", tmpdir, checksum);

 
   /* Touch the directory to prevent it being deleting in a rare race
    * between us doing the checks and a tmp cleaner running.  Note this
 
   /* Touch the directory to prevent it being deleting in a rare race
    * between us doing the checks and a tmp cleaner running.  Note this


@@ -342,28 +344,105 @@ build_supermin_appliance (guestfs_h *g,
     guestfs___print_timestamped_message (g, "begin building supermin appliance");
 
   const char *tmpdir = guestfs_tmpdir ();
     guestfs___print_timestamped_message (g, "begin building supermin appliance");
 
   const char *tmpdir = guestfs_tmpdir ();

-  size_t cdlen = strlen (tmpdir) + strlen (checksum) + 2;
-  char cachedir[cdlen];
-  snprintf (cachedir, cdlen, "%s/%s", tmpdir, checksum);

 
 

-  /* Don't worry about this failing, because the
-   * febootstrap-supermin-helper command will fail if the directory
-   * doesn't exist.  Note the directory might already exist, eg. if a
-   * tmp cleaner has removed the existing appliance but not the
-   * directory itself.
-   */
-  (void) mkdir (cachedir, 0755);
+  size_t tmpcdlen = strlen (tmpdir) + 16;
+  char tmpcd[tmpcdlen];
+  snprintf (tmpcd, tmpcdlen, "%s/guestfs.XXXXXX", tmpdir);
+
+  if (NULL == mkdtemp (tmpcd)) {
+    error (g, _("failed to create temporary cache directory: %m"));
+    return -1;
+  }

 
   if (g->verbose)
     guestfs___print_timestamped_message (g, "run febootstrap-supermin-helper");
 
 
   if (g->verbose)
     guestfs___print_timestamped_message (g, "run febootstrap-supermin-helper");
 

-  int r = run_supermin_helper (g, supermin_path, cachedir, cdlen);
+  int r = run_supermin_helper (g, supermin_path, tmpcd, tmpcdlen);

   if (r == -1)
     return -1;
 
   if (g->verbose)
     guestfs___print_timestamped_message (g, "finished building supermin appliance");
 
   if (r == -1)
     return -1;
 
   if (g->verbose)
     guestfs___print_timestamped_message (g, "finished building supermin appliance");
 

+  size_t cdlen = strlen (tmpdir) + strlen (checksum) + 10;
+  char cachedir[cdlen];
+  snprintf (cachedir, cdlen, "%s/guestfs.%s", tmpdir, checksum);
+
+  /* Make the temporary directory world readable */
+  if (chmod (tmpcd, 0755) == -1) {
+    error (g, "chmod %s: %m", tmpcd);
+  }
+
+  /* Try to rename the temporary directory to its non-temporary name */
+  if (rename (tmpcd, cachedir) == -1) {
+    /* If the cache directory now exists, we may have been racing with another
+     * libguestfs process. Check the new directory and use it if it's valid. */
+    if (errno == ENOTEMPTY || errno == EEXIST) {
+      /* Appliance cache consists of 2 files and a symlink in the cache
+       * directory. Delete them first. */
+      DIR *dir = opendir (tmpcd);
+      if (dir == NULL) {
+        error (g, "opendir %s: %m", tmpcd);
+        return -1;
+      }
+
+      int fd = dirfd (dir);
+      if (fd == -1) {
+        error (g, "dirfd: %m");
+        closedir (dir);
+        return -1;
+      }
+
+      struct dirent *dirent;
+      for (;;) {
+        errno = 0;
+        dirent = readdir (dir);
+
+        if (dirent == NULL) {
+          break;
+        }
+
+        /* Check that dirent is a file so we don't try to delete . and .. */
+        struct stat st;
+        if (fstatat (fd, dirent->d_name, &st, AT_SYMLINK_NOFOLLOW) == -1) {
+          error (g, "fstatat %s: %m", dirent->d_name);
+          return -1;
+        }
+
+        if (!S_ISDIR(st.st_mode)) {
+          if (unlinkat (fd, dirent->d_name, 0) == -1) {
+            error (g, "unlinkat %s: %m", dirent->d_name);
+            closedir (dir);
+            return -1;
+          }
+        }
+      }
+
+      if (errno != 0) {
+        error (g, "readdir %s: %m", tmpcd);
+        closedir (dir);
+        return -1;
+      }
+
+      closedir (dir);
+
+      /* Delete the temporary cache directory itself. */
+      if (rmdir (tmpcd) == -1) {
+        error (g, "rmdir %s: %m", tmpcd);
+        return -1;
+      }
+
+      /* Check the new cache directory, and return it if valid */
+      return check_for_cached_appliance (g, supermin_path, checksum,
+                                         kernel, initrd, appliance);
+    }
+
+    else {
+      error (g, _("error renaming temporary cache directory: %m"));
+      return -1;
+    }
+  }
+

   *kernel = safe_malloc (g, cdlen + 8 /* / + "kernel" + \0 */);
   *initrd = safe_malloc (g, cdlen + 8 /* / + "initrd" + \0 */);
   *appliance = safe_malloc (g, cdlen + 6 /* / + "root" + \0 */);
   *kernel = safe_malloc (g, cdlen + 8 /* / + "kernel" + \0 */);
   *initrd = safe_malloc (g, cdlen + 8 /* / + "initrd" + \0 */);
   *appliance = safe_malloc (g, cdlen + 6 /* / + "root" + \0 */);