[PATCH v2 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 lib/Makefile.am | 1 + lib/external-apps.c | 190 +++++++++++++++++++++++++++++++++++++++++++++ lib/guestfs-internal.h | 3 + lib/inspect-apps.c | 14 +++- lib/inspect-icon.c | 54 +++++++------ m4/guestfs-find-db-tool.m4 | 2 +- m4/guestfs_progs.m4 | 24 ------ 7 files changed, 237 insertions(+), 51 deletions(-) create mode 100644 lib/external-apps.c -- 2.9.3
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. --- lib/Makefile.am | 1 + lib/external-apps.c | 190 +++++++++++++++++++++++++++++++++++++++++++++++++ lib/guestfs-internal.h | 3 + 3 files changed, 194 insertions(+) create mode 100644 lib/external-apps.c diff --git a/lib/Makefile.am b/lib/Makefile.am index e1ab1bf..2f53e83 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -85,6 +85,7 @@ libguestfs_la_SOURCES = \ dbdump.c \ drives.c \ errors.c \ + external-apps.c \ event-string.c \ events.c \ file.c \ diff --git a/lib/external-apps.c b/lib/external-apps.c new file mode 100644 index 0000000..2ed2bc2 --- /dev/null +++ b/lib/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/lib/guestfs-internal.h b/lib/guestfs-internal.h index 04d087b..b50b86f 100644 --- a/lib/guestfs-internal.h +++ b/lib/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.9.3
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. --- lib/inspect-apps.c | 14 ++++++++++++-- m4/guestfs-find-db-tool.m4 | 2 +- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/inspect-apps.c b/lib/inspect-apps.c index b377f57..78c2711 100644 --- a/lib/inspect-apps.c +++ b/lib/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; 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]) -- 2.9.3
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. --- lib/inspect-icon.c | 54 +++++++++++++++++++++++++++++------------------------ m4/guestfs_progs.m4 | 24 ------------------------ 2 files changed, 30 insertions(+), 48 deletions(-) diff --git a/lib/inspect-icon.c b/lib/inspect-icon.c index 84d4e4a..5d7301f 100644 --- a/lib/inspect-icon.c +++ b/lib/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 */ 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]) -- 2.9.3
On Thu, Feb 02, 2017 at 02:33:17PM +0100, Pino Toscano wrote:
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
There is reasoning here, see m4/guestfs_programs.m4: # Check for external programs required to either build or run # libguestfs. # # AC_CHECK_PROG(S) or AC_PATH_PROG(S)? # # Use AC_CHECK_PROG(S) for programs which are only used during build. # # Use AC_PATH_PROG(S) for program names which are compiled into the # binary and used at run time. The reason is so that we know which # programs the binary actually uses. As it says in the message, the main reason I like to hard code the full path is because I want to know that the user is running the same program that we tested against, ie. they didn't install some half-baked binary in /usr/local/bin. I think it's a mistake if we call qemu-img without the full path BTW ...
- some of the tools are actually optional, and their runtime lack can be handled gracefully instead of throwing a "failed to execute" error
Now, I do think this is a problem. But why don't we just test if the (full pathnamed) program exists using a simple stat or access? Rich.
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
lib/Makefile.am | 1 + lib/external-apps.c | 190 +++++++++++++++++++++++++++++++++++++++++++++ lib/guestfs-internal.h | 3 + lib/inspect-apps.c | 14 +++- lib/inspect-icon.c | 54 +++++++------ m4/guestfs-find-db-tool.m4 | 2 +- m4/guestfs_progs.m4 | 24 ------ 7 files changed, 237 insertions(+), 51 deletions(-) create mode 100644 lib/external-apps.c
-- 2.9.3
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
participants (2)
-
Pino Toscano -
Richard W.M. Jones