Libguestfs - Nov 2018 - [PATCH v2 REPOST] lib: Allow db_dump package to be a weak dependency

We went around the houses a few times last year in order to try
to fix this old Debian bug:

https://bugzilla.redhat.com/show_bug.cgi?id=1409024

My last attempt was:

https://www.redhat.com/archives/libguestfs/2017-October/msg00058.html

which I believe was neither reviewed nor rejected, so I'm reposting
the same patch again, simply rebased against current git.

Rich.

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 757d50d88..a36b9ad2d 100644
--- a/m4/guestfs-progs.m4
+++ b/m4/guestfs-progs.m4
@@ -65,12 +65,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.19.0.rc0

On Friday, 2 November 2018 15:27:17 CET Richard W.M. Jones
wrote:
> 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).
I still do think this approach (hardcoding paths at build time) is not
a correct idea.  It might work for tools part of the same suite (say,
hardcode the path of virt-get-kernel in virt-builder), but it gets
messy for tools that you do not really control.

I originally posted a better solution for this:
https://www.redhat.com/archives/libguestfs/2017-February/msg00043.html
which apparently was rejected with doubtful reasons.
Yes, I still do think a proper runtime check, instead of wild
hardcoding is better, and it avoids the workaround proposed in form of
this patch.

-- 
Pino Toscano