Libguestfs - Jan 2017 - [PATCH 0/3] library: improve handling of external tools

Hi,

the libguestfs library uses a number of external tools; for some of
them, we search for them at build time, enabling some feature only if
found, and later on assuming at runtime they are installed. However,
the situation is more complex than that:
- hardcoding the full path means that there is an incoherency in the
  way some of the tools are used, as some other tools (e.g. qemu-img)
  are just run as assumed to be in $PATH; also hardcoding means
  libguestfs cannot use different versions of those tools
- some of the tools are actually optional, and their runtime lack can
  be handled gracefully instead of throwing a "failed to execute"
error

The chosen approach is to build a process-wide cache in the libguestfs
library of paths of tools searched, even in case they are not available:
this way it is easier to just skip something at runtime if a program is
not there.  The patch series applies it for the libdb tools, and for
the tools used to extract icons from guests.

Thanks,

Pino Toscano (3):
  lib: add internal cache for external application
  lib: improve libdb tools handling at runtime (RHBZ#1409024)
  inspect: make netpbm and icoutils really optional runtime tools

 m4/guestfs-find-db-tool.m4 |   2 +-
 m4/guestfs_progs.m4        |  24 ------
 src/Makefile.am            |   1 +
 src/external-apps.c        | 190 +++++++++++++++++++++++++++++++++++++++++++++
 src/guestfs-internal.h     |   3 +
 src/inspect-apps.c         |  14 +++-
 src/inspect-icon.c         |  54 +++++++------
 7 files changed, 237 insertions(+), 51 deletions(-)
 create mode 100644 src/external-apps.c

-- 
2.7.4

Add a new simple internal API to check the availability of installed
commands in the system at runtime, and cache the result of of the search
(the full path, if found).

This API will ease the usage of optional external applications used by
the library, as it will be possible to avoid using a tool if it is not
available in the system.
---
 src/Makefile.am        |   1 +
 src/external-apps.c    | 190 +++++++++++++++++++++++++++++++++++++++++++++++++
 src/guestfs-internal.h |   3 +
 3 files changed, 194 insertions(+)
 create mode 100644 src/external-apps.c

diff --git a/src/Makefile.am b/src/Makefile.am
index c315c5d..3d8b119 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -95,6 +95,7 @@ libguestfs_la_SOURCES = \
 	dbdump.c \
 	drives.c \
 	errors.c \
+	external-apps.c \
 	event-string.c \
 	events.c \
 	file.c \
diff --git a/src/external-apps.c b/src/external-apps.c
new file mode 100644
index 0000000..2ed2bc2
--- /dev/null
+++ b/src/external-apps.c
@@ -0,0 +1,190 @@
+/* libguestfs
+ * Copyright (C) 2017 Red Hat Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include <assert.h>
+
+#include "glthread/lock.h"
+#include "hash.h"
+#include "hash-pjw.h"
+
+#include "guestfs.h"
+#include "guestfs-internal.h"
+
+static void free_cache (void) __attribute__((destructor));
+static bool hash_compare_strings (const void *a, const void *b);
+static void hash_free_strings (void *s);
+
+/* The actual cache of available applications. */
+static Hash_table *apps_ht;
+/* This lock protects access to the applications cache. */
+gl_lock_define_initialized (static, apps_lock);
+
+/* Dummy static object, representing a "not found" result
+ * (Hash_table does not accept NULL values for keys).
+ */
+static const char NOT_FOUND[] = "not_found";
+
+#if defined(__GNUC__) && GUESTFS_GCC_VERSION >= 40800 /* gcc >=
4.8.0 */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wstack-usage="
+#endif
+
+/**
+ * Check program exists and is executable on C<$PATH>.
+ */
+static int
+prog_exists (guestfs_h *g, const char *prog, char **ret_path)
+{
+  const char *pathc = getenv ("PATH");
+
+  if (!pathc)
+    return 0;
+
+  const size_t proglen = strlen (prog);
+  const char *elem;
+  char *saveptr;
+  const size_t len = strlen (pathc) + 1;
+  char path[len];
+  strcpy (path, pathc);
+
+  elem = strtok_r (path, ":", &saveptr);
+  while (elem) {
+    const size_t n = strlen (elem) + proglen + 2;
+    char testprog[n];
+
+    snprintf (testprog, n, "%s/%s", elem, prog);
+    if (access (testprog, X_OK) == 0) {
+      *ret_path = safe_strdup (g, testprog);
+      return 1;
+    }
+
+    elem = strtok_r (NULL, ":", &saveptr);
+  }
+
+  /* Not found. */
+  return 0;
+}
+
+/**
+ * Check whether C<cmd> is installed.
+ *
+ * This function checks in C<$PATH> whether the specified command
+ * C<cmd> is available.  If C<ret_path> is not C<NULL>, on
success
+ * it will contain a new string with the full path of the command.
+ *
+ * Returns C<-1> on failures, C<0> if C<cmd> is not
available, and
+ * C<1> when C<cmd> is available.
+ */
+int
+guestfs_int_find_cmd (guestfs_h *g, const char *cmd, char **ret_path)
+{
+  int ret = -1;
+  const void *value;
+  int r;
+  CLEANUP_FREE char *path = NULL;
+
+  if (ret_path)
+    *ret_path = NULL;
+
+  gl_lock_lock (apps_lock);
+
+  /* Create the hash table, if not existing already. */
+  if (apps_ht == NULL) {
+    /* We don't expect many applications searched, so start with
+     * a low value of potential items.
+     */
+    apps_ht = hash_initialize (10, NULL, hash_pjw, hash_compare_strings,
+                               hash_free_strings);
+    if (apps_ht == NULL) {
+      error (g, "failed to create the hash table for caching
applications");
+      goto out;
+    }
+  }
+
+  /* Check whether we already looked for CMD, setting the proper
+   * values depending on the results in the hash table.
+   */
+  value = hash_lookup (apps_ht, cmd);
+  if (value) {
+    if (value == NOT_FOUND) {
+      path = NULL;
+      ret = 0;
+    } else {
+      path = safe_strdup (g, (char *) value);
+      ret = 1;
+    }
+    goto out;
+  }
+
+  /* We didn't search for CMD yet, so look for it, and store the result
+   * in the hash table.
+   */
+  r = prog_exists (g, cmd, &path);
+  if (r < 0)
+    goto out;
+  else if (r == 0) {
+    value = hash_insert (apps_ht, NOT_FOUND);
+    ret = 0;
+  } else  {
+    value = hash_insert (apps_ht, safe_strdup (g, path));
+    ret = 1;
+  }
+  assert (value);
+
+ out:
+  gl_lock_unlock (apps_lock);
+
+  debug (g, "external app: %s, result = %d, %s", cmd, ret, ret > 0
? path : "-");
+
+  if (ret > 0 && ret_path) {
+    assert (path);
+    *ret_path = path;
+    path = NULL;
+  }
+
+  return ret;
+}
+
+#if defined(__GNUC__) && GUESTFS_GCC_VERSION >= 40800 /* gcc >=
4.8.0 */
+#pragma GCC diagnostic pop
+#endif
+
+void
+free_cache (void)
+{
+  if (apps_ht)
+    hash_free (apps_ht);
+}
+
+bool
+hash_compare_strings (const void *a, const void *b)
+{
+  return STREQ (a, b) ? true : false;
+}
+
+void
+hash_free_strings (void *s)
+{
+  free (s);
+}
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 40dad35..9797750 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -1007,4 +1007,7 @@ extern bool guestfs_int_version_cmp_ge (const struct
version *a, const struct ve
 #define version_init_null(v) guestfs_int_version_from_values (v, 0, 0, 0)
 #define version_is_null(v) ((v)->v_major == 0 && (v)->v_minor ==
0 && (v)->v_micro == 0)
 
+/* external-apps.c */
+extern int guestfs_int_find_cmd (guestfs_h *g, const char *cmd, char
**ret_path);
+
 #endif /* GUESTFS_INTERNAL_H_ */
-- 
2.7.4

When built with support for libdb tools (i.e. db_dump was found at
configure time), check for the existance of the db_dumb tool before
attempting to use it: this way the libdb tools can stay an optional
dependency at runtime, and their lack just gives no information on the
applications installed in an RPM-based distro.

Since we don't need the absolute path of db_dumb (nor db_load, used only
in tests), change the GUESTFS_FIND_DB_TOOL m4 macro to give the names
instead.
---
 m4/guestfs-find-db-tool.m4 |  2 +-
 src/inspect-apps.c         | 14 ++++++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/m4/guestfs-find-db-tool.m4 b/m4/guestfs-find-db-tool.m4
index ee20301..1026938 100644
--- a/m4/guestfs-find-db-tool.m4
+++ b/m4/guestfs-find-db-tool.m4
@@ -36,7 +36,7 @@ AC_DEFUN([GUESTFS_FIND_DB_TOOL],[
                 exe_list="$exe_list $exe"
             done
         done
-        AC_PATH_PROGS([]VARIABLE[], [$exe_list], [no])
+        AC_CHECK_PROGS([]VARIABLE[], [$exe_list], [no])
     ])
 
     popdef([VARIABLE])
diff --git a/src/inspect-apps.c b/src/inspect-apps.c
index b377f57..78c2711 100644
--- a/src/inspect-apps.c
+++ b/src/inspect-apps.c
@@ -22,6 +22,7 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <string.h>
+#include <libintl.h>
 
 #ifdef HAVE_ENDIAN_H
 #include <endian.h>
@@ -109,6 +110,7 @@ guestfs_impl_inspect_list_applications2 (guestfs_h *g, const
char *root)
 {
   struct guestfs_application2_list *ret = NULL;
   struct inspect_fs *fs = guestfs_int_search_for_root (g, root);
+  int r;
   if (!fs)
     return NULL;
 
@@ -122,9 +124,17 @@ guestfs_impl_inspect_list_applications2 (guestfs_h *g,
const char *root)
       switch (fs->package_format) {
       case OS_PACKAGE_FORMAT_RPM:
 #ifdef DB_DUMP
-        ret = list_applications_rpm (g, fs);
-        if (ret == NULL)
+        r = guestfs_int_find_cmd (g, DB_DUMP, NULL);
+        if (r < 0)
           return NULL;
+        if (r == 0) {
+          debug (g, _("inspection of RPM database cannot be done, as %s is
not available"),
+                    DB_DUMP);
+        } else {
+          ret = list_applications_rpm (g, fs);
+          if (ret == NULL)
+            return NULL;
+        }
 #endif
         break;
 
-- 
2.7.4

Check at runtime for the existency of all the tools needed to manipulate
the icons for CirrOS and Windows guests, gracefully handle their lack.
This also drops the requirement of looking for them and hardcoding
their full paths at configure time, since we are directly running them
as available in $PATH.

As result, the code extracting the icons of CirrOS and Windows guests
can be always compiled.
---
 m4/guestfs_progs.m4 | 24 ------------------------
 src/inspect-icon.c  | 54 +++++++++++++++++++++++++++++------------------------
 2 files changed, 30 insertions(+), 48 deletions(-)

diff --git a/m4/guestfs_progs.m4 b/m4/guestfs_progs.m4
index c63e62f..74780eb 100644
--- a/m4/guestfs_progs.m4
+++ b/m4/guestfs_progs.m4
@@ -73,30 +73,6 @@ if test "x$DB_LOAD" != "xno"; then
     AC_DEFINE_UNQUOTED([DB_LOAD],["$DB_LOAD"],[Name of db_load
program.])
 fi
 
-dnl Check for netpbm programs (optional).
-AC_PATH_PROGS([PBMTEXT],[pbmtext],[no])
-AC_PATH_PROGS([PNMTOPNG],[pnmtopng],[no])
-AC_PATH_PROGS([BMPTOPNM],[bmptopnm],[no])
-AC_PATH_PROGS([PAMCUT],[pamcut],[no])
-if test "x$PBMTEXT" != "xno"; then
-    AC_DEFINE_UNQUOTED([PBMTEXT],["$PBMTEXT"],[Name of pbmtext
program.])
-fi
-if test "x$PNMTOPNG" != "xno"; then
-    AC_DEFINE_UNQUOTED([PNMTOPNG],["$PNMTOPNG"],[Name of pnmtopng
program.])
-fi
-if test "x$BMPTOPNM" != "xno"; then
-    AC_DEFINE_UNQUOTED([BMPTOPNM],["$BMPTOPNM"],[Name of bmptopnm
program.])
-fi
-if test "x$PAMCUT" != "xno"; then
-    AC_DEFINE_UNQUOTED([PAMCUT],["$PAMCUT"],[Name of pamcut
program.])
-fi
-
-dnl Check for icoutils (optional).
-AC_PATH_PROGS([WRESTOOL],[wrestool],[no])
-if test "x$WRESTOOL" != "xno"; then
-    AC_DEFINE_UNQUOTED([WRESTOOL],["$WRESTOOL"],[Name of wrestool
program.])
-fi
-
 dnl Check for xzcat (required).
 AC_PATH_PROGS([XZCAT],[xzcat],[no])
 test "x$XZCAT" = "xno" && AC_MSG_ERROR([xzcat must
be installed])
diff --git a/src/inspect-icon.c b/src/inspect-icon.c
index 84d4e4a..5d7301f 100644
--- a/src/inspect-icon.c
+++ b/src/inspect-icon.c
@@ -28,14 +28,24 @@
 #include "guestfs-internal.h"
 #include "guestfs-internal-actions.h"
 
-/* External tools are required for some icon types.  Check we have them. */
-#if defined(PBMTEXT) && defined (PNMTOPNG)
-#define CAN_DO_CIRROS 1
-#endif
-#if defined(WRESTOOL) && defined(BMPTOPNM) && defined(PNMTOPNG)
&&	\
-  defined(PAMCUT)
-#define CAN_DO_WINDOWS 1
-#endif
+/* External tools are required for some icon types.  List their names
+ * here, so in case we need to change their names and/or set the right
+ * names at configure time, then it's easier to do.
+ */
+#define BMPTOPNM "bmptopnm"
+#define PAMCUT "pamcut"
+#define PBMTEXT "pbmtext"
+#define PNMTOPNG "pnmtopng"
+#define WRESTOOL "wrestool"
+
+#define check_cmd(_cmd_) \
+  do { \
+    const int cmd_res_ = guestfs_int_find_cmd (g, _cmd_, NULL); \
+    if (cmd_res_ < 0) \
+      return NULL; /* a real error */ \
+    if (cmd_res_ == 0) \
+      return NOT_FOUND; \
+  } while (0)
 
 /* All these icon_*() functions return the same way.  One of:
  *
@@ -58,14 +68,10 @@ static char *icon_debian (guestfs_h *g, struct inspect_fs
*fs, size_t *size_r);
 static char *icon_ubuntu (guestfs_h *g, struct inspect_fs *fs, size_t *size_r);
 static char *icon_mageia (guestfs_h *g, struct inspect_fs *fs, size_t *size_r);
 static char *icon_opensuse (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r);
-#if CAN_DO_CIRROS
 static char *icon_cirros (guestfs_h *g, struct inspect_fs *fs, size_t *size_r);
-#endif
 static char *icon_voidlinux (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r);
 static char *icon_altlinux (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r);
-#if CAN_DO_WINDOWS
 static char *icon_windows (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r);
-#endif
 
 /* Dummy static object. */
 static char *NOT_FOUND = (char *) "not_found";
@@ -156,9 +162,7 @@ guestfs_impl_inspect_get_icon (guestfs_h *g, const char
*root, size_t *size_r,
       break;
 
     case OS_DISTRO_CIRROS:
-#if CAN_DO_CIRROS
       r = icon_cirros (g, fs, &size);
-#endif
       break;
 
     case OS_DISTRO_VOID_LINUX:
@@ -194,13 +198,11 @@ guestfs_impl_inspect_get_icon (guestfs_h *g, const char
*root, size_t *size_r,
     break;
 
   case OS_TYPE_WINDOWS:
-#if CAN_DO_WINDOWS
     /* We don't know how to get high quality icons from a Windows guest,
      * so disable this if high quality was specified.
      */
     if (!highquality)
       r = icon_windows (g, fs, &size);
-#endif
     break;
 
   case OS_TYPE_FREEBSD:
@@ -391,8 +393,6 @@ icon_opensuse (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r)
   return get_png (g, fs, OPENSUSE_ICON, size_r, 2048);
 }
 
-#if CAN_DO_CIRROS
-
 /* Cirros's logo is a text file! */
 #define CIRROS_LOGO "/usr/share/cirros/logo"
 
@@ -406,6 +406,9 @@ icon_cirros (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r)
   CLEANUP_CMD_CLOSE struct command *cmd = guestfs_int_new_command (g);
   int r;
 
+  check_cmd (PBMTEXT);
+  check_cmd (PNMTOPNG);
+
   r = guestfs_is_file_opts (g, CIRROS_LOGO,
                             GUESTFS_IS_FILE_OPTS_FOLLOWSYMLINKS, 1, -1);
   if (r == -1)
@@ -445,8 +448,6 @@ icon_cirros (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r)
   return ret;
 }
 
-#endif /* CAN_DO_CIRROS */
-
 #define VOIDLINUX_ICON "/usr/share/void-artwork/void-logo.png"
 
 static char *
@@ -463,8 +464,6 @@ icon_altlinux (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r)
   return get_png (g, fs, ALTLINUX_ICON, size_r, 20480);
 }
 
-#if CAN_DO_WINDOWS
-
 /* Windows, as usual, has to be much more complicated and stupid than
  * anything else.
  *
@@ -491,6 +490,10 @@ icon_windows_xp (guestfs_h *g, struct inspect_fs *fs,
size_t *size_r)
   int r;
   char *ret;
 
+  check_cmd (WRESTOOL);
+  check_cmd (BMPTOPNM);
+  check_cmd (PNMTOPNG);
+
   /* Download %systemroot%\explorer.exe */
   filename = safe_asprintf (g, "%s/explorer.exe",
fs->windows_systemroot);
   filename_case = guestfs_case_sensitive_path (g, filename);
@@ -553,6 +556,11 @@ icon_windows_7 (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r)
   int r;
   char *ret;
 
+  check_cmd (WRESTOOL);
+  check_cmd (BMPTOPNM);
+  check_cmd (PAMCUT);
+  check_cmd (PNMTOPNG);
+
   for (i = 0; win7_explorer[i] != NULL; ++i) {
     CLEANUP_FREE char *filename = NULL;
 
@@ -661,5 +669,3 @@ icon_windows (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r)
   /* Not (yet) a supported version of Windows. */
   else return NOT_FOUND;
 }
-
-#endif /* CAN_DO_WINDOWS */
-- 
2.7.4