[PATCH libguestfs 1/4] appliance: Push appliance building lock into guestfs___build_appliance.

["Richard W.M. Jones" <rjones@xxxxxxxxxx>]

From: "Richard W.M. Jones" <rjones@xxxxxxxxxx>

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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list