Libguestfs - Dec 2016 - [PATCH 0/5] Improve inspection of /usr filesystems

Hi,

this patch series improves the way /usr filesystems are handled: tag
them appropriately, so later on we can find them and merge results they
contain directly back for the root filesystem.

The series includes also a new private debug API, and its usage to fix
the resolution of /dev/mapper/.. devices found in fstab; without it,
LVM /usr filesystems are not recognized as belonging to their roots.
Maybe a better API for this could be added, but since it's something
only related to the appliance then can stay internal for now. (Better
suggestions always welcome, of course.)

Thanks,

Pino Toscano (5):
  inspect: change is_root flag into enum
  inspect: mark CoreOS /usr partitions with own USR role
  daemon: debug: new "exists" subcommand
  inspect: fix existance check of /dev/mapper devices
  inspect: gather info from /usr filesystems as well (RHBZ#1401474)

 daemon/debug.c         | 39 ++++++++++++++++++++++
 src/guestfs-internal.h | 13 ++++++--
 src/inspect-fs-cd.c    |  2 +-
 src/inspect-fs-unix.c  | 42 ++++++++++++++++++++++-
 src/inspect-fs.c       | 26 ++++++++-------
 src/inspect.c          | 90 +++++++++++++++++++++++++++++++++++++++++++++-----
 6 files changed, 187 insertions(+), 25 deletions(-)

-- 
2.7.4

Introduce a new enum to classify the role of a filesystem, if available.
This will help later on when doing operations on non-root filesystems,
like detecting particular mountpoints such as /usr.

The new enum has only "root" as known role, which replaces the is_root
flag.
---
 src/guestfs-internal.h | 11 ++++++++---
 src/inspect-fs-cd.c    |  2 +-
 src/inspect-fs.c       | 20 ++++++++++----------
 src/inspect.c          | 17 +++++++++--------
 4 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index f2f2a97..861ca75 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -633,15 +633,20 @@ enum inspect_os_package_management {
   OS_PACKAGE_MANAGEMENT_XBPS,
 };
 
+enum inspect_os_role {
+  OS_ROLE_UNKNOWN = 0,
+  OS_ROLE_ROOT,
+};
+
 /**
  * The inspection code maintains one of these structures per mountable
  * filesystem found in the disk image.  The struct (or structs) which
- * have the C<is_root> flag set are inspection roots, each
- * corresponding to a single guest.  Note that a filesystem can be
+ * have the C<role> attribute set to C<OS_ROLE_ROOT> are inspection
roots,
+ * each corresponding to a single guest.  Note that a filesystem can be
  * shared between multiple guests.
  */
 struct inspect_fs {
-  int is_root;
+  enum inspect_os_role role;
   char *mountable;
   enum inspect_os_type type;
   enum inspect_os_distro distro;
diff --git a/src/inspect-fs-cd.c b/src/inspect-fs-cd.c
index 10e9d54..278386e 100644
--- a/src/inspect-fs-cd.c
+++ b/src/inspect-fs-cd.c
@@ -525,7 +525,7 @@ guestfs_int_check_installer_iso (guestfs_h *g, struct
inspect_fs *fs,
 
   /* Otherwise we matched an ISO, so fill in the fs fields. */
   fs->mountable = safe_strdup (g, device);
-  fs->is_root = 1;
+  fs->role = OS_ROLE_ROOT;
   if (osinfo->is_installer)
     fs->format = OS_FORMAT_INSTALLER;
   fs->type = osinfo->type;
diff --git a/src/inspect-fs.c b/src/inspect-fs.c
index 5e5b004..1951678 100644
--- a/src/inspect-fs.c
+++ b/src/inspect-fs.c
@@ -164,7 +164,7 @@ check_filesystem (guestfs_h *g, const char *mountable,
            is_dir_bin &&
            guestfs_is_file (g, "/etc/freebsd-update.conf") > 0
&&
            guestfs_is_file (g, "/etc/fstab") > 0) {
-    fs->is_root = 1;
+    fs->role = OS_ROLE_ROOT;
     fs->format = OS_FORMAT_INSTALLED;
     if (guestfs_int_check_freebsd_root (g, fs) == -1)
       return -1;
@@ -175,7 +175,7 @@ check_filesystem (guestfs_h *g, const char *mountable,
            guestfs_is_file (g, "/netbsd") > 0 &&
            guestfs_is_file (g, "/etc/fstab") > 0 &&
            guestfs_is_file (g, "/etc/release") > 0) {
-    fs->is_root = 1;
+    fs->role = OS_ROLE_ROOT;
     fs->format = OS_FORMAT_INSTALLED;
     if (guestfs_int_check_netbsd_root (g, fs) == -1)
       return -1;
@@ -186,7 +186,7 @@ check_filesystem (guestfs_h *g, const char *mountable,
            guestfs_is_file (g, "/bsd") > 0 &&
            guestfs_is_file (g, "/etc/fstab") > 0 &&
            guestfs_is_file (g, "/etc/motd") > 0) {
-    fs->is_root = 1;
+    fs->role = OS_ROLE_ROOT;
     fs->format = OS_FORMAT_INSTALLED;
     if (guestfs_int_check_openbsd_root (g, fs) == -1)
       return -1;
@@ -195,7 +195,7 @@ check_filesystem (guestfs_h *g, const char *mountable,
   else if (guestfs_is_file (g, "/hurd/console") > 0 &&
            guestfs_is_file (g, "/hurd/hello") > 0 &&
            guestfs_is_file (g, "/hurd/null") > 0) {
-    fs->is_root = 1;
+    fs->role = OS_ROLE_ROOT;
     fs->format = OS_FORMAT_INSTALLED; /* XXX could be more specific */
     if (guestfs_int_check_hurd_root (g, fs) == -1)
       return -1;
@@ -206,7 +206,7 @@ check_filesystem (guestfs_h *g, const char *mountable,
            guestfs_is_file (g, "/service/vm") > 0 &&
            guestfs_is_file (g, "/etc/fstab") > 0 &&
            guestfs_is_file (g, "/etc/version") > 0) {
-    fs->is_root = 1;
+    fs->role = OS_ROLE_ROOT;
     fs->format = OS_FORMAT_INSTALLED;
     if (guestfs_int_check_minix_root (g, fs) == -1)
       return -1;
@@ -217,7 +217,7 @@ check_filesystem (guestfs_h *g, const char *mountable,
             is_symlink_to (g, "/bin", "usr/bin") > 0)
&&
            (guestfs_is_file (g, "/etc/fstab") > 0 ||
             guestfs_is_file (g, "/etc/hosts") > 0)) {
-    fs->is_root = 1;
+    fs->role = OS_ROLE_ROOT;
     fs->format = OS_FORMAT_INSTALLED;
     if (guestfs_int_check_linux_root (g, fs) == -1)
       return -1;
@@ -228,7 +228,7 @@ check_filesystem (guestfs_h *g, const char *mountable,
            guestfs_is_dir (g, "/home") > 0 &&
            guestfs_is_dir (g, "/usr") > 0 &&
            guestfs_is_file (g, "/etc/coreos/update.conf") > 0) {
-    fs->is_root = 1;
+    fs->role = OS_ROLE_ROOT;
     fs->format = OS_FORMAT_INSTALLED;
     if (guestfs_int_check_coreos_root (g, fs) == -1)
       return -1;
@@ -263,7 +263,7 @@ check_filesystem (guestfs_h *g, const char *mountable,
   /* Windows root? */
   else if ((windows_systemroot = guestfs_int_get_windows_systemroot (g)) !=
NULL)
     {
-      fs->is_root = 1;
+      fs->role = OS_ROLE_ROOT;
       fs->format = OS_FORMAT_INSTALLED;
       if (guestfs_int_check_windows_root (g, fs, windows_systemroot) == -1)
 	return -1;
@@ -278,7 +278,7 @@ check_filesystem (guestfs_h *g, const char *mountable,
   /* FreeDOS? */
   else if (guestfs_int_is_dir_nocase (g, "/FDOS") > 0 &&
            guestfs_int_is_file_nocase (g, "/FDOS/FREEDOS.BSS") >
0) {
-    fs->is_root = 1;
+    fs->role = OS_ROLE_ROOT;
     fs->format = OS_FORMAT_INSTALLED;
     fs->type = OS_TYPE_DOS;
     fs->distro = OS_DISTRO_FREEDOS;
@@ -306,7 +306,7 @@ check_filesystem (guestfs_h *g, const char *mountable,
             guestfs_is_file (g, "/amd64/txtsetup.sif") > 0 ||
             guestfs_is_file (g, "/freedos/freedos.ico") > 0 ||
             guestfs_is_file (g, "/boot/loader.rc") > 0)) {
-    fs->is_root = 1;
+    fs->role = OS_ROLE_ROOT;
     fs->format = OS_FORMAT_INSTALLER;
     if (guestfs_int_check_installer_root (g, fs) == -1)
       return -1;
diff --git a/src/inspect.c b/src/inspect.c
index 9a60aa0..ae5c457 100644
--- a/src/inspect.c
+++ b/src/inspect.c
@@ -113,7 +113,7 @@ collect_coreos_inspection_info (guestfs_h *g)
   for (i = 0; i < g->nr_fses; ++i) {
     struct inspect_fs *fs = &g->fses[i];
 
-    if (fs->distro == OS_DISTRO_COREOS && fs->is_root)
+    if (fs->distro == OS_DISTRO_COREOS && fs->role ==
OS_ROLE_ROOT)
       root = fs;
   }
 
@@ -123,7 +123,7 @@ collect_coreos_inspection_info (guestfs_h *g)
   for (i = 0; i < g->nr_fses; ++i) {
     struct inspect_fs *fs = &g->fses[i];
 
-    if (fs->distro != OS_DISTRO_COREOS || fs->is_root != 0)
+    if (fs->distro != OS_DISTRO_COREOS || fs->role == OS_ROLE_ROOT)
       continue;
 
     /* CoreOS is designed to contain 2 /usr partitions (USR-A, USR-B):
@@ -168,15 +168,16 @@ check_for_duplicated_bsd_root (guestfs_h *g)
       fs->type == OS_TYPE_NETBSD ||
       fs->type == OS_TYPE_OPENBSD;
 
-    if (fs->is_root && is_bsd &&
+    if (fs->role == OS_ROLE_ROOT && is_bsd &&
         match (g, fs->mountable, re_primary_partition)) {
       bsd_primary = fs;
       continue;
     }
 
-    if (fs->is_root && bsd_primary && bsd_primary->type
== fs->type) {
-      /* remove the is root flag from the bsd_primary */
-      bsd_primary->is_root = 0;
+    if (fs->role == OS_ROLE_ROOT && bsd_primary &&
+        bsd_primary->type == fs->type) {
+      /* remove the root role from the bsd_primary */
+      bsd_primary->role = OS_ROLE_UNKNOWN;
       bsd_primary->format = OS_FORMAT_UNKNOWN;
       return;
     }
@@ -202,7 +203,7 @@ guestfs_impl_inspect_get_roots (guestfs_h *g)
    * list in this case.
    */
   for (i = 0; i < g->nr_fses; ++i) {
-    if (g->fses[i].is_root)
+    if (g->fses[i].role == OS_ROLE_ROOT)
       guestfs_int_add_string (g, &ret, g->fses[i].mountable);
   }
   guestfs_int_end_stringsbuf (g, &ret);
@@ -704,7 +705,7 @@ guestfs_int_search_for_root (guestfs_h *g, const char *root)
 
   for (i = 0; i < g->nr_fses; ++i) {
     struct inspect_fs *fs = &g->fses[i];
-    if (fs->is_root && STREQ (root, fs->mountable))
+    if (fs->role == OS_ROLE_ROOT && STREQ (root, fs->mountable))
       return fs;
   }
 
-- 
2.7.4

Add a new inspect role for "/usr" partitions, and use that to mark the
/usr partition in CoreOS: this additional role allows to ease its lookup
later on, when merging its results into those of the root.
---
 src/guestfs-internal.h | 1 +
 src/inspect-fs-unix.c  | 1 +
 src/inspect.c          | 2 +-
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 861ca75..d10191d 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -636,6 +636,7 @@ enum inspect_os_package_management {
 enum inspect_os_role {
   OS_ROLE_UNKNOWN = 0,
   OS_ROLE_ROOT,
+  OS_ROLE_USR,
 };
 
 /**
diff --git a/src/inspect-fs-unix.c b/src/inspect-fs-unix.c
index a1a757c..c833304 100644
--- a/src/inspect-fs-unix.c
+++ b/src/inspect-fs-unix.c
@@ -1013,6 +1013,7 @@ guestfs_int_check_coreos_usr (guestfs_h *g, struct
inspect_fs *fs)
 
   fs->type = OS_TYPE_LINUX;
   fs->distro = OS_DISTRO_COREOS;
+  fs->role = OS_ROLE_USR;
 
   if (guestfs_is_file_opts (g, "/lib/os-release",
                             GUESTFS_IS_FILE_OPTS_FOLLOWSYMLINKS, 1, -1) > 0)
{
diff --git a/src/inspect.c b/src/inspect.c
index ae5c457..9055226 100644
--- a/src/inspect.c
+++ b/src/inspect.c
@@ -123,7 +123,7 @@ collect_coreos_inspection_info (guestfs_h *g)
   for (i = 0; i < g->nr_fses; ++i) {
     struct inspect_fs *fs = &g->fses[i];
 
-    if (fs->distro != OS_DISTRO_COREOS || fs->role == OS_ROLE_ROOT)
+    if (fs->distro != OS_DISTRO_COREOS || fs->role != OS_ROLE_USR)
       continue;
 
     /* CoreOS is designed to contain 2 /usr partitions (USR-A, USR-B):
-- 
2.7.4

Easy way to check whether a file in the appliance exists.
---
 daemon/debug.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/daemon/debug.c b/daemon/debug.c
index c646be7..2193fe6 100644
--- a/daemon/debug.c
+++ b/daemon/debug.c
@@ -67,6 +67,7 @@ static char *debug_core_pattern (const char *subcmd, size_t
argc, char *const *c
 static char *debug_device_speed (const char *subcmd, size_t argc, char *const
*const argv);
 static char *debug_env (const char *subcmd, size_t argc, char *const *const
argv);
 static char *debug_error (const char *subcmd, size_t argc, char *const *const
argv);
+static char *debug_exists (const char *subcmd, size_t argc, char *const *const
argv);
 static char *debug_fds (const char *subcmd, size_t argc, char *const *const
argv);
 static char *debug_ldd (const char *subcmd, size_t argc, char *const *const
argv);
 static char *debug_ls (const char *subcmd, size_t argc, char *const *const
argv);
@@ -90,6 +91,7 @@ static struct cmd cmds[] = {
   { "device_speed", debug_device_speed },
   { "env", debug_env },
   { "error", debug_error },
+  { "exists", debug_exists },
   { "fds", debug_fds },
   { "ldd", debug_ldd },
   { "ls", debug_ls },
@@ -933,6 +935,43 @@ do_debug_upload (const char *filename, int mode)
   return 0;
 }
 
+/* Check whether a file in the appliance exists.
+ * An empty string is returned if the file does not exist,
+ * an "ok" string when the file exists, and an error in case of other
+ * stat errors.
+ */
+static char *
+debug_exists (const char *subcmd, size_t argc, char *const *const argv)
+{
+  char *ret;
+  int r;
+  struct stat buf;
+
+  if (argc != 1) {
+    reply_with_error ("exists: one argument expected");
+    return NULL;
+  }
+
+  r = stat (argv[0], &buf);
+
+  if (r == -1) {
+    if (errno != ENOENT && errno != ENOTDIR) {
+      reply_with_perror ("stat: %s", argv[0]);
+      return NULL;
+    }
+    else
+      ret = strdup ("");
+  } else
+    ret = strdup ("ok");
+
+  if (NULL == ret) {
+    reply_with_perror ("strdup");
+    return NULL;
+  }
+
+  return ret;
+}
+
 /* This function is identical to debug_upload. */
 /* Has one FileIn parameter. */
 int
-- 
2.7.4

When checking for the existance of /dev/mapper devices found in the
fstab of a filesystem, using guestfs_exists means they are checked as
files in the guest, while they really appear as devices on the
appliance. Resort using a debug API to check whether a file in the
appliance exists, instead.

Fixes commit 96b6504b09461aeb6850bb2e7b870a0a4c2f5edf.
---
 src/inspect-fs-unix.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/inspect-fs-unix.c b/src/inspect-fs-unix.c
index c833304..7b54a4a 100644
--- a/src/inspect-fs-unix.c
+++ b/src/inspect-fs-unix.c
@@ -1806,6 +1806,19 @@ resolve_fstab_device_diskbyid (guestfs_h *g, const char
*part,
   return 0;
 }
 
+static bool
+dev_mapper_exists (guestfs_h *g, const char *device)
+{
+  CLEANUP_FREE char *ret = NULL;
+  const char *const args[] = { device, NULL };
+
+  guestfs_push_error_handler (g, NULL, NULL);
+  ret = guestfs_debug (g, "exists", (char **) args);
+  guestfs_pop_error_handler (g);
+
+  return ret && ret[0] != 0;
+}
+
 /* Resolve block device name to the libguestfs device name, eg.
  * /dev/xvdb1 => /dev/vdb1; and /dev/mapper/VG-LV => /dev/VG/LV.  This
  * assumes that disks were added in the same order as they appear to
@@ -1820,7 +1833,7 @@ resolve_fstab_device (guestfs_h *g, const char *spec,
Hash_table *md_map,
   char *type, *slice, *disk, *part;
   int r;
 
-  if (STRPREFIX (spec, "/dev/mapper/") && guestfs_exists (g,
spec) > 0) {
+  if (STRPREFIX (spec, "/dev/mapper/") && dev_mapper_exists
(g, spec) > 0) {
     /* LVM2 does some strange munging on /dev/mapper paths for VGs and
      * LVs which contain '-' character:
      *
-- 
2.7.4

Flag the filesystems for Linux /usr properly as USR role, and detect
some data out of it, like the distro information from an os-release
(if present), and the architecture (since the binaries used for our
architecture check will be available there only).

Later on, collect the results in a way similar to what is done for
CoreOS: for each non-CoreOS root, try to find its /usr filesystem, and
if found then merge what is missing from root; in the last case, also
override the distro inspection data (version, product name) if available
in /usr.
---
 src/guestfs-internal.h |  1 +
 src/inspect-fs-unix.c  | 26 ++++++++++++++++++
 src/inspect-fs.c       |  6 +++--
 src/inspect.c          | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 104 insertions(+), 2 deletions(-)

diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index d10191d..fbbfb90 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -864,6 +864,7 @@ extern void guestfs_int_merge_fs_inspections (guestfs_h *g,
struct inspect_fs *d
 
 /* inspect-fs-unix.c */
 extern int guestfs_int_check_linux_root (guestfs_h *g, struct inspect_fs *fs);
+extern int guestfs_int_check_linux_usr (guestfs_h *g, struct inspect_fs *fs);
 extern int guestfs_int_check_freebsd_root (guestfs_h *g, struct inspect_fs
*fs);
 extern int guestfs_int_check_netbsd_root (guestfs_h *g, struct inspect_fs *fs);
 extern int guestfs_int_check_openbsd_root (guestfs_h *g, struct inspect_fs
*fs);
diff --git a/src/inspect-fs-unix.c b/src/inspect-fs-unix.c
index 7b54a4a..bb8e0ef 100644
--- a/src/inspect-fs-unix.c
+++ b/src/inspect-fs-unix.c
@@ -786,6 +786,32 @@ guestfs_int_check_linux_root (guestfs_h *g, struct
inspect_fs *fs)
   return 0;
 }
 
+/* The currently mounted device looks like a Linux /usr. */
+int
+guestfs_int_check_linux_usr (guestfs_h *g, struct inspect_fs *fs)
+{
+  int r;
+
+  fs->type = OS_TYPE_LINUX;
+  fs->role = OS_ROLE_USR;
+
+  if (guestfs_is_file_opts (g, "/lib/os-release",
+                            GUESTFS_IS_FILE_OPTS_FOLLOWSYMLINKS, 1, -1) > 0)
{
+    r = parse_os_release (g, fs, "/lib/os-release");
+    if (r == -1)        /* error */
+      return -1;
+    if (r == 1)         /* ok - detected the release from this file */
+      goto skip_release_checks;
+  }
+
+ skip_release_checks:;
+
+  /* Determine the architecture. */
+  check_architecture (g, fs);
+
+  return 0;
+}
+
 /* The currently mounted device is known to be a FreeBSD root. */
 int
 guestfs_int_check_freebsd_root (guestfs_h *g, struct inspect_fs *fs)
diff --git a/src/inspect-fs.c b/src/inspect-fs.c
index 1951678..9f7630b 100644
--- a/src/inspect-fs.c
+++ b/src/inspect-fs.c
@@ -245,8 +245,10 @@ check_filesystem (guestfs_h *g, const char *mountable,
            is_dir_bin &&
            is_dir_share &&
            guestfs_is_dir (g, "/local") > 0 &&
-           guestfs_is_file (g, "/etc/fstab") == 0)
-    ;
+           guestfs_is_file (g, "/etc/fstab") == 0) {
+    if (guestfs_int_check_linux_usr (g, fs) == -1)
+      return -1;
+  }
   /* CoreOS /usr? */
   else if (is_dir_bin &&
            is_dir_share &&
diff --git a/src/inspect.c b/src/inspect.c
index 9055226..5e904d2 100644
--- a/src/inspect.c
+++ b/src/inspect.c
@@ -46,6 +46,8 @@ COMPILE_REGEXP (re_primary_partition,
"^/dev/(?:h|s|v)d.[1234]$", 0)
 
 static void check_for_duplicated_bsd_root (guestfs_h *g);
 static void collect_coreos_inspection_info (guestfs_h *g);
+static void collect_linux_inspection_info (guestfs_h *g);
+static void collect_linux_inspection_info_for (guestfs_h *g, struct inspect_fs
*root);
 
 /**
  * The main inspection API.
@@ -88,6 +90,12 @@ guestfs_impl_inspect_os (guestfs_h *g)
    */
   check_for_duplicated_bsd_root (g);
 
+  /* For Linux guests with a separate /usr filesyste, merge some of the
+   * inspected information in that partition to the inspect_fs struct
+   * of the root filesystem.
+   */
+  collect_linux_inspection_info (g);
+
   /* At this point we have, in the handle, a list of all filesystems
    * found and data about each one.  Now we assemble the list of
    * filesystems which are root devices and return that to the user.
@@ -149,6 +157,71 @@ collect_coreos_inspection_info (guestfs_h *g)
 }
 
 /**
+ * Traverse through the filesystems and find the /usr filesystem for
+ * the specified C<root>: if found, merge its basic inspection details
+ * to the root when they were set (i.e. because the /usr had os-release
+ * or other ways to identify the OS).
+ */
+static void
+collect_linux_inspection_info_for (guestfs_h *g, struct inspect_fs *root)
+{
+  size_t i;
+  struct inspect_fs *usr = NULL;
+
+  for (i = 0; i < g->nr_fses; ++i) {
+    struct inspect_fs *fs = &g->fses[i];
+    size_t j;
+
+    if (!(fs->distro == root->distro || fs->distro ==
OS_DISTRO_UNKNOWN) ||
+        fs->role != OS_ROLE_USR)
+      continue;
+
+    for (j = 0; j < root->nr_fstab; ++j) {
+      if (STREQ (fs->mountable, root->fstab[j].mountable)) {
+        usr = fs;
+        goto got_usr;
+      }
+    }
+  }
+
+  if (usr == NULL)
+    return;
+
+ got_usr:
+  /* If the version information in /usr is not null, then most probably
+   * there was an os-release file there, so reset what is in root
+   * and pick the results from /usr.
+   */
+  if (!version_is_null (&usr->version)) {
+    root->distro = OS_DISTRO_UNKNOWN;
+    free (root->product_name);
+    root->product_name = NULL;
+  }
+
+  guestfs_int_merge_fs_inspections (g, root, usr);
+}
+
+/**
+ * Traverse through the filesystem list and find out if it contains
+ * the C</> and C</usr> filesystems of a Linux image (but not
CoreOS,
+ * for which there is a separate C<collect_coreos_inspection_info>).
+ * If this is the case, sum up all the collected information on each
+ * root fs from the respective /usr filesystems.
+ */
+static void
+collect_linux_inspection_info (guestfs_h *g)
+{
+  size_t i;
+
+  for (i = 0; i < g->nr_fses; ++i) {
+    struct inspect_fs *fs = &g->fses[i];
+
+    if (fs->distro != OS_DISTRO_COREOS && fs->role ==
OS_ROLE_ROOT)
+      collect_linux_inspection_info_for (g, fs);
+  }
+}
+
+/**
  * On *BSD systems, sometimes F</dev/sda[1234]> is a shadow of the
  * real root filesystem that is probably F</dev/sda5> (see:
  * L<http://www.freebsd.org/doc/handbook/disk-organization.html>)
-- 
2.7.4

On Tue, Dec 06, 2016 at 04:29:17PM +0100, Pino Toscano
wrote:
> Hi,
> 
> this patch series improves the way /usr filesystems are handled: tag
> them appropriately, so later on we can find them and merge results they
> contain directly back for the root filesystem.
> 
> The series includes also a new private debug API, and its usage to fix
> the resolution of /dev/mapper/.. devices found in fstab; without it,
> LVM /usr filesystems are not recognized as belonging to their roots.
> Maybe a better API for this could be added, but since it's something
> only related to the appliance then can stay internal for now. (Better
> suggestions always welcome, of course.)
Firstly, I still think we need:

  [PATCH 1/2] inspect: fstab: Canonicalize paths appearing in fstab.

although it's not related to the current patch series.

In regards to this series:
> Pino Toscano (5):
>   inspect: change is_root flag into enum
>   inspect: mark CoreOS /usr partitions with own USR role
These are fine.
>   daemon: debug: new "exists" subcommand
>   inspect: fix existance check of /dev/mapper devices
This is very ugly.  We really shouldn't be calling a debug API from
inspection code.
>   inspect: gather info from /usr filesystems as well (RHBZ#1401474)
I have specific comments on this patch.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On Tue, Dec 06, 2016 at 04:29:22PM +0100, Pino Toscano
wrote:
> +static void
> +collect_linux_inspection_info_for (guestfs_h *g, struct inspect_fs *root)
> +{
> +  size_t i;
> +  struct inspect_fs *usr = NULL;
> +
> +  for (i = 0; i < g->nr_fses; ++i) {
> +    struct inspect_fs *fs = &g->fses[i];
> +    size_t j;
> +
> +    if (!(fs->distro == root->distro || fs->distro ==
OS_DISTRO_UNKNOWN) ||
> +        fs->role != OS_ROLE_USR)
> +      continue;
> +
> +    for (j = 0; j < root->nr_fstab; ++j) {
> +      if (STREQ (fs->mountable, root->fstab[j].mountable)) {
> +        usr = fs;
> +        goto got_usr;
> +      }
> +    }
> +  }
> +
> +  if (usr == NULL)
I think this should be:

  assert (usr == NULL);

unless there's some way to reach this point and usr != NULL.
> +    return;
> +
> + got_usr:
> +  /* If the version information in /usr is not null, then most probably
> +   * there was an os-release file there, so reset what is in root
> +   * and pick the results from /usr.
> +   */
> +  if (!version_is_null (&usr->version)) {
> +    root->distro = OS_DISTRO_UNKNOWN;
> +    free (root->product_name);
> +    root->product_name = NULL;
> +  }
> +
> +  guestfs_int_merge_fs_inspections (g, root, usr);
> +}
Rich.

-- 
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

On Wednesday, 7 December 2016 08:29:54 CET Richard W.M. Jones
wrote:
> On Tue, Dec 06, 2016 at 04:29:17PM +0100, Pino Toscano wrote:
> > Hi,
> > 
> > this patch series improves the way /usr filesystems are handled: tag
> > them appropriately, so later on we can find them and merge results
they
> > contain directly back for the root filesystem.
> > 
> > The series includes also a new private debug API, and its usage to fix
> > the resolution of /dev/mapper/.. devices found in fstab; without it,
> > LVM /usr filesystems are not recognized as belonging to their roots.
> > Maybe a better API for this could be added, but since it's
something
> > only related to the appliance then can stay internal for now. (Better
> > suggestions always welcome, of course.)
> 
> Firstly, I still think we need:
> 
>   [PATCH 1/2] inspect: fstab: Canonicalize paths appearing in fstab.
> 
> although it's not related to the current patch series.
This was not an issue in the data of RHBZ#140147, do you have an example
of guest with such paths in fstab? So far I never seen one myself.
> In regards to this series:
> 
> > Pino Toscano (5):
> >   inspect: change is_root flag into enum
> >   inspect: mark CoreOS /usr partitions with own USR role
> 
> These are fine.
Pushed, thanks.
> >   daemon: debug: new "exists" subcommand
> >   inspect: fix existance check of /dev/mapper devices
> 
> This is very ugly.  We really shouldn't be calling a debug API from
> inspection code.
As mentioned above, any better suggestion is welcome. I didn't want to
add a new public API for inspecting files in the appliance itself --
what about something like internal_exists?

Thanks,
-- 
Pino Toscano

On Tue, Dec 06, 2016 at 04:29:21PM +0100, Pino Toscano
wrote:
> @@ -1820,7 +1833,7 @@ resolve_fstab_device (guestfs_h *g, const char *spec,
Hash_table *md_map,
>    char *type, *slice, *disk, *part;
>    int r;
>  
> -  if (STRPREFIX (spec, "/dev/mapper/") && guestfs_exists
(g, spec) > 0) {
> +  if (STRPREFIX (spec, "/dev/mapper/") &&
dev_mapper_exists (g, spec) > 0) {
...
    device = guestfs_lvm_canonical_lv_name (g, spec);
  }


I think really the problem is with the call to
guestfs_lvm_canonical_lv_name.

Can we not replace this code with:

if (STRPREFIX (spec, "/dev/mapper/")) {
...
  guestfs_push_error_handler (g, NULL, NULL);
  device = guestfs_lvm_canonical_lv_name (g, spec);
  guestfs_pop_error_handler (g);
  if (device == NULL) {
    if (guestfs_last_errno (g) == ENOENT)
      // ignore error
    else {
      guestfs_int_error_errno (/* copy the handle error message & errno here
*/);
      return -1;
  }
}

?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v