2:20 p.m.
From: "Richard W.M. Jones" <rjones(a)redhat.com>
Since we will be calling guestfs___build_appliance from the libvirt
code in future, there's no point having two places where we have to
acquire the lock. Push the lock down into this function instead.
Because "glthread/lock.h" includes <errno.h> we have to add this
header to the file too.
---
src/appliance.c | 28 +++++++++++++++++++++++++---
src/launch-appliance.c | 16 ++--------------
2 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/src/appliance.c b/src/appliance.c
index e42bec4..d206f3a 100644
--- a/src/appliance.c
+++ b/src/appliance.c
@@ -36,6 +36,8 @@
#include <sys/types.h>
#endif
+#include "glthread/lock.h"
+
#include "guestfs.h"
#include "guestfs-internal.h"
#include "guestfs-internal-actions.h"
@@ -58,6 +60,13 @@ static int hard_link_to_cached_appliance (guestfs_h *g, const char
*cachedir, ch
static int run_supermin_helper (guestfs_h *g, const char *supermin_path, const char
*cachedir, size_t cdlen);
static void print_febootstrap_command_line (guestfs_h *g, const char *argv[]);
+/* RHBZ#790721: It makes no sense to have multiple threads racing to
+ * build the appliance from within a single process, and the code
+ * isn't safe for that anyway. Therefore put a thread lock around
+ * appliance building.
+ */
+gl_lock_define_initialized (static, building_lock);
+
/* Locate or build the appliance.
*
* This function locates or builds the appliance as necessary,
@@ -136,11 +145,15 @@ guestfs___build_appliance (guestfs_h *g,
int r;
uid_t uid = geteuid ();
+ gl_lock_lock (building_lock);
+
/* Step (1). */
char *supermin_path;
r = find_path (g, contains_supermin_appliance, NULL, &supermin_path);
- if (r == -1)
+ if (r == -1) {
+ gl_lock_unlock (building_lock);
return -1;
+ }
if (r == 1) {
/* Step (2): calculate checksum. */
@@ -152,6 +165,7 @@ guestfs___build_appliance (guestfs_h *g,
if (r != 0) {
free (supermin_path);
free (checksum);
+ gl_lock_unlock (building_lock);
return r == 1 ? 0 : -1;
}
@@ -160,6 +174,7 @@ guestfs___build_appliance (guestfs_h *g,
kernel, initrd, appliance);
free (supermin_path);
free (checksum);
+ gl_lock_unlock (building_lock);
return r;
}
free (supermin_path);
@@ -168,8 +183,10 @@ guestfs___build_appliance (guestfs_h *g,
/* Step (5). */
char *path;
r = find_path (g, contains_fixed_appliance, NULL, &path);
- if (r == -1)
+ if (r == -1) {
+ gl_lock_unlock (building_lock);
return -1;
+ }
if (r == 1) {
size_t len = strlen (path);
@@ -181,13 +198,16 @@ guestfs___build_appliance (guestfs_h *g,
sprintf (*appliance, "%s/root", path);
free (path);
+ gl_lock_unlock (building_lock);
return 0;
}
/* Step (6). */
r = find_path (g, contains_old_style_appliance, NULL, &path);
- if (r == -1)
+ if (r == -1) {
+ gl_lock_unlock (building_lock);
return -1;
+ }
if (r == 1) {
size_t len = strlen (path);
@@ -198,11 +218,13 @@ guestfs___build_appliance (guestfs_h *g,
*appliance = NULL;
free (path);
+ gl_lock_unlock (building_lock);
return 0;
}
error (g, _("cannot find any suitable libguestfs supermin, fixed or old-style
appliance on LIBGUESTFS_PATH (search path: %s)"),
g->path);
+ gl_lock_unlock (building_lock);
return -1;
}
diff --git a/src/launch-appliance.c b/src/launch-appliance.c
index 79eae34..f10801c 100644
--- a/src/launch-appliance.c
+++ b/src/launch-appliance.c
@@ -23,13 +23,12 @@
#include <stdint.h>
#include <inttypes.h>
#include <unistd.h>
+#include <errno.h>
#include <fcntl.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <signal.h>
-#include "glthread/lock.h"
-
#include "guestfs.h"
#include "guestfs-internal.h"
#include "guestfs-internal-actions.h"
@@ -121,13 +120,6 @@ add_cmdline_shell_unquoted (guestfs_h *g, const char *options)
}
}
-/* RHBZ#790721: It makes no sense to have multiple threads racing to
- * build the appliance from within a single process, and the code
- * isn't safe for that anyway. Therefore put a thread lock around
- * appliance building.
- */
-gl_lock_define_initialized (static, building_lock);
-
static int
launch_appliance (guestfs_h *g, const char *arg)
{
@@ -150,12 +142,8 @@ launch_appliance (guestfs_h *g, const char *arg)
/* Locate and/or build the appliance. */
char *kernel = NULL, *initrd = NULL, *appliance = NULL;
- gl_lock_lock (building_lock);
- if (guestfs___build_appliance (g, &kernel, &initrd, &appliance) == -1) {
- gl_lock_unlock (building_lock);
+ if (guestfs___build_appliance (g, &kernel, &initrd, &appliance) == -1)
return -1;
- }
- gl_lock_unlock (building_lock);
TRACE0 (launch_build_appliance_end);
--
1.7.10.4