Libguestfs - Oct 2017 - [PATCH 2/2] lib/inspect-icon.c: Return error if guestfs_int_cmd_run has internal error.

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

---
 lib/inspect-apps.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/inspect-apps.c b/lib/inspect-apps.c
index f0cf16b38..020a3332c 100644
--- a/lib/inspect-apps.c
+++ b/lib/inspect-apps.c
@@ -122,9 +122,14 @@ 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;
+      /* Allow db_dump to be missing at runtime so that it can be
+       * installed as a weak dependency.
+       */
+      if (system (DB_DUMP " -V >/dev/null 2>&1") == 0) {
+        ret = list_applications_rpm (g, root);
+        if (ret == NULL)
+          return NULL;
+      }
 #endif
     }
     else if (STREQ (package_format, "deb")) {
-- 
2.13.2

This function returns -1 if things like fork(2) fail, so it is right
to return an error here.  If the PBMTEXT command fails then that's a
NOT_FOUND case.
---
 lib/inspect-icon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/inspect-icon.c b/lib/inspect-icon.c
index 533e9fb4b..0edd3d1b0 100644
--- a/lib/inspect-icon.c
+++ b/lib/inspect-icon.c
@@ -400,7 +400,7 @@ icon_cirros (guestfs_h *g, size_t *size_r)
   guestfs_int_cmd_add_string_quoted   (cmd, pngfile);
   r = guestfs_int_cmd_run (cmd);
   if (r == -1)
-    return NOT_FOUND;
+    return NULL;
   if (!WIFEXITED (r) || WEXITSTATUS (r) != 0)
     return NOT_FOUND;
 
-- 
2.13.2

On Thursday, 5 October 2017 15:41:31 CEST Richard W.M. Jones
wrote:
> ---
>  lib/inspect-apps.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/inspect-apps.c b/lib/inspect-apps.c
> index f0cf16b38..020a3332c 100644
> --- a/lib/inspect-apps.c
> +++ b/lib/inspect-apps.c
> @@ -122,9 +122,14 @@ 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;
> +      /* Allow db_dump to be missing at runtime so that it can be
> +       * installed as a weak dependency.
> +       */
> +      if (system (DB_DUMP " -V >/dev/null 2>&1") == 0)
{
> +        ret = list_applications_rpm (g, root);
> +        if (ret == NULL)
> +          return NULL;
> +      }
>  #endif
This approach has few issues:
a) DB_DUMP is not escaped, so a path with special characters will play
   badly with shell
b) `DB_DUMP -V` is executed every time RPM packages are extracted
c) there is no feedback to the users (not even a debug()) that DB_DUMP
   is not installed, and thus information on the installed RPM packages
   are not extracted
d) the same issue happens for the tools used to extract icons (netpbm,
   and icoutils)

I still do think my approach [1] is better in this regard, since it
already copes with all the points above.

Why should be optional tools searched at build time, hardcoded, and
then still need to be checked at runtime?  The concern that there is
no way to know which tools are used is sort of moot, since the internal
command API (in the library) logs program and arguments.

[1] https://www.redhat.com/archives/libguestfs/2017-February/msg00016.html

-- 
Pino Toscano

On Thursday, 5 October 2017 15:41:32 CEST Richard W.M. Jones
wrote:
> This function returns -1 if things like fork(2) fail, so it is right
> to return an error here.  If the PBMTEXT command fails then that's a
> NOT_FOUND case.
> ---
>  lib/inspect-icon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/inspect-icon.c b/lib/inspect-icon.c
> index 533e9fb4b..0edd3d1b0 100644
> --- a/lib/inspect-icon.c
> +++ b/lib/inspect-icon.c
> @@ -400,7 +400,7 @@ icon_cirros (guestfs_h *g, size_t *size_r)
>    guestfs_int_cmd_add_string_quoted   (cmd, pngfile);
>    r = guestfs_int_cmd_run (cmd);
>    if (r == -1)
> -    return NOT_FOUND;
> +    return NULL;
>    if (!WIFEXITED (r) || WEXITSTATUS (r) != 0)
>      return NOT_FOUND;
LGTM (it looks like this issue dates back to when the code was
introduced, even before CLEANUP_* & co).

-- 
Pino Toscano