From f2460699ab7f972b1981d072164a04820c52b0c6 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Thu, 28 Oct 2010 15:19:14 +0100 Subject: [PATCH] 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 --- po/POTFILES.in | 1 + regressions/Makefile.am | 1 + regressions/test-launch-race.pl | 60 +++++++++++++++++++++++ src/appliance.c | 105 +++++++++++++++++++++++++++++++++++----- 4 files changed, 154 insertions(+), 13 deletions(-) create mode 100755 regressions/test-launch-race.pl diff --git a/po/POTFILES.in b/po/POTFILES.in index 970068d..699e90b 100644 --- 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 +regressions/test-launch-race.pl 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 --- 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-launch-race.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 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 --- a/src/appliance.c +++ b/src/appliance.c @@ -18,6 +18,8 @@ #include +#include +#include #include #include #include @@ -249,9 +251,9 @@ check_for_cached_appliance (guestfs_h *g, { const char *tmpdir = guestfs_tmpdir (); - size_t len = strlen (tmpdir) + strlen (checksum) + 2; + size_t len = strlen (tmpdir) + strlen (checksum) + 10; 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 @@ -342,28 +344,105 @@ build_supermin_appliance (guestfs_h *g, 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"); - 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"); + 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 */); -- 1.8.3.1