Libguestfs - Oct 2017 - [PATCH v2 0/2] lib: Allow db_dump package to be a weak dependency

Previously posted:
https://www.redhat.com/archives/libguestfs/2017-October/msg00032.html

This takes a completely different approach.  It turns out that POSIX /
the shell already defines a special exit code 127 for ‘command not
found’.  We can make a small adjustment to lib/command.c to return
this exit code in that case.

Then we just have to modify the db_dump code to test for this exit
code.

I also made db_dump unconditional, allowing the packager to either
build without db_dump present at all, and/or to make db_dump into a
weak runtime dependency which will be silently ignored if not present
by the mechanism above.

Rich.

If running the external command fails in "argv mode" (ie. when
not using the shell), then exit with either 126 or 127 as defined
by POSIX.

This is mostly the same as what bash does, see
execute_cmd.c:shell_execve in the bash sources.

Note: saving errno around perror(3) if necessary, otherwise you will
see different behaviour between verbose and non-verbose mode.  In
non-verbose mode, perror(3) tried to print to a closed file
descriptor, failing and overwriting errno.  That took some time to
debug.
---
 lib/command.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/command.c b/lib/command.c
index 3d8bc7dbf..36f953dcf 100644
--- a/lib/command.c
+++ b/lib/command.c
@@ -539,7 +539,7 @@ static void
 run_child (struct command *cmd)
 {
   struct sigaction sa;
-  int i, fd, max_fd, r;
+  int i, err, fd, max_fd, r;
   char status_string[80];
 #ifdef HAVE_SETRLIMIT
   struct child_rlimits *child_rlimit;
@@ -614,8 +614,12 @@ run_child (struct command *cmd)
   switch (cmd->style) {
   case COMMAND_STYLE_EXECV:
     execvp (cmd->argv.argv[0], cmd->argv.argv);
+    err = errno;
     perror (cmd->argv.argv[0]);
-    _exit (EXIT_FAILURE);
+    /* These error codes are defined in POSIX and meant to be the
+     * same as the shell.
+     */
+    _exit (err == ENOENT ? 127 : 126);
 
   case COMMAND_STYLE_SYSTEM:
     r = system (cmd->string.str);
-- 
2.13.2

We do this by defining DB_DUMP unconditionally and then testing the
special exit code given by the shell if the command is not found (see
http://www.tldp.org/LDP/abs/html/exitcodes.html).

Packagers may either:

(1) Provide db_dump as a build requirement, but make it a weak
dependency at runtime, or:

(2) Not provide db_dump at build time, but define DB_DUMP to its
expected path when configuring, eg:

  DB_DUMP=/usr/bin/db_dump ./configure

which will then be compiled into the library and used if available at
runtime (or ignored as in case (1) if not available at runtime).
---
 lib/dbdump.c        | 11 +++++++----
 lib/inspect-apps.c  |  8 --------
 m4/guestfs-progs.m4 |  8 ++------
 3 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/lib/dbdump.c b/lib/dbdump.c
index 7c17ce6b3..3ec11f2bd 100644
--- a/lib/dbdump.c
+++ b/lib/dbdump.c
@@ -31,8 +31,6 @@
 #include "guestfs.h"
 #include "guestfs-internal.h"
 
-#if defined(DB_DUMP)
-
 static void read_db_dump_line (guestfs_h *g, void *datav, const char *line,
size_t len);
 static unsigned char *convert_hex_to_binary (guestfs_h *g, const char *hex,
size_t hexlen, size_t *binlen_rtn);
 
@@ -75,6 +73,13 @@ guestfs_int_read_db_dump (guestfs_h *g,
 
   if (r == -1)
     return -1;
+  if (WIFEXITED (r) && WEXITSTATUS (r) == 127) {
+    /* Allow the command to be missing at runtime so that packagers can
+     * use weak dependencies.  In this case it's not an error, we
+     * return an empty list.
+     */
+    return 0;
+  }
   if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) {
     guestfs_int_external_command_failed (g, r, DB_DUMP, NULL);
     return -1;
@@ -225,5 +230,3 @@ convert_hex_to_binary (guestfs_h *g, const char *hex, size_t
hexlen,
   *binlen_rtn = binlen;
   return bin;
 }
-
-#endif /* defined(DB_DUMP) */
diff --git a/lib/inspect-apps.c b/lib/inspect-apps.c
index f0cf16b38..57428a3ba 100644
--- a/lib/inspect-apps.c
+++ b/lib/inspect-apps.c
@@ -45,9 +45,7 @@
 #include "guestfs-internal-actions.h"
 #include "structs-cleanups.h"
 
-#ifdef DB_DUMP
 static struct guestfs_application2_list *list_applications_rpm (guestfs_h *g,
const char *root);
-#endif
 static struct guestfs_application2_list *list_applications_deb (guestfs_h *g,
const char *root);
 static struct guestfs_application2_list *list_applications_pacman (guestfs_h
*g, const char *root);
 static struct guestfs_application2_list *list_applications_apk (guestfs_h *g,
const char *root);
@@ -121,11 +119,9 @@ guestfs_impl_inspect_list_applications2 (guestfs_h *g,
const char *root)
 
   if (STREQ (type, "linux") || STREQ (type, "hurd")) {
     if (STREQ (package_format, "rpm")) {
-#ifdef DB_DUMP
       ret = list_applications_rpm (g, root);
       if (ret == NULL)
         return NULL;
-#endif
     }
     else if (STREQ (package_format, "deb")) {
       ret = list_applications_deb (g, root);
@@ -163,8 +159,6 @@ guestfs_impl_inspect_list_applications2 (guestfs_h *g, const
char *root)
   return ret;
 }
 
-#ifdef DB_DUMP
-
 /* This data comes from the Name database, and contains the application
  * names and the first 4 bytes of each link field.
  */
@@ -409,8 +403,6 @@ list_applications_rpm (guestfs_h *g, const char *root)
   return NULL;
 }
 
-#endif /* defined DB_DUMP */
-
 static struct guestfs_application2_list *
 list_applications_deb (guestfs_h *g, const char *root)
 {
diff --git a/m4/guestfs-progs.m4 b/m4/guestfs-progs.m4
index 1f0a4c1f6..1455be550 100644
--- a/m4/guestfs-progs.m4
+++ b/m4/guestfs-progs.m4
@@ -66,12 +66,8 @@ AM_CONDITIONAL([HAVE_PO4A], [test "x$PO4A" !=
"xno"])
 dnl Check for db_dump, db_load (optional).
 GUESTFS_FIND_DB_TOOL([DB_DUMP], [dump])
 GUESTFS_FIND_DB_TOOL([DB_LOAD], [load])
-if test "x$DB_DUMP" != "xno"; then
-    AC_DEFINE_UNQUOTED([DB_DUMP],["$DB_DUMP"],[Name of db_dump
program.])
-fi
-if test "x$DB_LOAD" != "xno"; then
-    AC_DEFINE_UNQUOTED([DB_LOAD],["$DB_LOAD"],[Name of db_load
program.])
-fi
+AC_DEFINE_UNQUOTED([DB_DUMP],["$DB_DUMP"],[Name of db_dump program.])
+AC_DEFINE_UNQUOTED([DB_LOAD],["$DB_LOAD"],[Name of db_load program.])
 
 dnl Check for netpbm programs (optional).
 AC_PATH_PROGS([PBMTEXT],[pbmtext],[no])
-- 
2.13.2

On Friday, 6 October 2017 15:52:16 CET Richard W.M. Jones
wrote:
> If running the external command fails in "argv mode" (ie. when
> not using the shell), then exit with either 126 or 127 as defined
> by POSIX.
> 
> This is mostly the same as what bash does, see
> execute_cmd.c:shell_execve in the bash sources.
> 
> Note: saving errno around perror(3) if necessary, otherwise you will
> see different behaviour between verbose and non-verbose mode.  In
> non-verbose mode, perror(3) tried to print to a closed file
> descriptor, failing and overwriting errno.  That took some time to
> debug.
> ---
LGTM.

Thanks,
-- 
Pino Toscano