Libguestfs - Nov 2014 - Re: [PATCH 1/1] inspect: Fix a bug in the *BSD root detection

The assumption that Linux will map the MBR partition to /dev/sda1
and the BSD 'a' partition to /dev/sda5 is not always correct.

Signed-off-by: Nikos Skalkotos <skalkoto@grnet.gr>
---
 src/guestfs-internal.h |  1 +
 src/inspect-fs.c       | 55 +++++++++++++++++++++++++++++++++-----------------
 src/inspect.c          |  6 ++++++
 3 files changed, 43 insertions(+), 19 deletions(-)

diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index fd0c4a1..2460d25 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -746,6 +746,7 @@ extern int guestfs___is_file_nocase (guestfs_h *g, const
char *);
 extern int guestfs___is_dir_nocase (guestfs_h *g, const char *);
 extern int guestfs___check_for_filesystem_on (guestfs_h *g,
                                               const char *mountable);
+extern void guestfs___check_for_dublicated_bsd_root(guestfs_h *g);
 extern int guestfs___parse_unsigned_int (guestfs_h *g, const char *str);
 extern int guestfs___parse_unsigned_int_ignore_trailing (guestfs_h *g, const
char *str);
 extern int guestfs___parse_major_minor (guestfs_h *g, struct inspect_fs *fs);
diff --git a/src/inspect-fs.c b/src/inspect-fs.c
index 539d814..99a8658 100644
--- a/src/inspect-fs.c
+++ b/src/inspect-fs.c
@@ -47,7 +47,7 @@
  * multiple threads call into the libguestfs API functions below
  * simultaneously.
  */
-static pcre *re_first_partition;
+static pcre *re_primary_partition;
 static pcre *re_major_minor;
 
 static void compile_regexps (void) __attribute__((constructor));
@@ -68,14 +68,14 @@ compile_regexps (void)
     }                                                                   \
   } while (0)
 
-  COMPILE (re_first_partition, "^/dev/(?:h|s|v)d.1$", 0);
+  COMPILE (re_primary_partition, "^/dev/(?:h|s|v)d.[1234]$", 0);
   COMPILE (re_major_minor, "(\\d+)\\.(\\d+)", 0);
 }
 
 static void
 free_regexps (void)
 {
-  pcre_free (re_first_partition);
+  pcre_free (re_primary_partition);
   pcre_free (re_major_minor);
 }
 
@@ -84,6 +84,39 @@ static int check_filesystem (guestfs_h *g, const char
*mountable,
                              int whole_device);
 static int extend_fses (guestfs_h *g);
 
+/* On *BSD systems, sometimes /dev/sda[1234] is a shadow of the real root
+ * filesystem that is probably /dev/sda5
+ * (see: http://www.freebsd.org/doc/handbook/disk-organization.html)
+ */
+void
+guestfs___check_for_dublicated_bsd_root(guestfs_h *g)
+{
+  size_t i;
+  bool is_primary, is_bsd;
+  struct inspect_fs *fs, *bsd_primary = NULL;
+
+  for (i = 0; i < g->nr_fses; ++i) {
+    fs = &g->fses[i];
+
+    is_primary = match (g, fs->mountable, re_primary_partition);
+    is_bsd = ((fs->type == OS_TYPE_FREEBSD) || \
+              (fs->type == OS_TYPE_NETBSD) || \
+              (fs->type == OS_TYPE_OPENBSD));
+
+    if (fs->is_root && is_primary && is_bsd) {
+      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;
+      bsd_primary->format = OS_FORMAT_UNKNOWN;
+      return;
+    }
+  }
+}
+
 /* Find out if 'device' contains a filesystem.  If it does, add
  * another entry in g->fses.
  */
@@ -207,14 +240,6 @@ 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) {
-    /* Ignore /dev/sda1 which is a shadow of the real root filesystem
-     * that is probably /dev/sda5 (see:
-     * http://www.freebsd.org/doc/handbook/disk-organization.html)
-     */
-    if (m->im_type == MOUNTABLE_DEVICE &&
-        match (g, m->im_device, re_first_partition))
-      return 0;
-
     fs->is_root = 1;
     fs->format = OS_FORMAT_INSTALLED;
     if (guestfs___check_freebsd_root (g, fs) == -1)
@@ -225,14 +250,6 @@ 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) {
-    /* Ignore /dev/sda1 which is a shadow of the real root filesystem
-     * that is probably /dev/sda5 (see:
-     * http://www.freebsd.org/doc/handbook/disk-organization.html)
-     */
-    if (m->im_type == MOUNTABLE_DEVICE &&
-        match (g, m->im_device, re_first_partition))
-      return 0;
-
     fs->is_root = 1;
     fs->format = OS_FORMAT_INSTALLED;
     if (guestfs___check_netbsd_root (g, fs) == -1)
diff --git a/src/inspect.c b/src/inspect.c
index 9248b06..048b059 100644
--- a/src/inspect.c
+++ b/src/inspect.c
@@ -64,6 +64,12 @@ guestfs__inspect_os (guestfs_h *g)
     }
   }
 
+  /* Check if the same filesystem was listed twice as root in g->fses.
+   * This may happen for the *BSD root partition where an MBR partition
+   * is a shadow of the real root partition probably /dev/sda5
+   */
+  guestfs___check_for_dublicated_bsd_root(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.
-- 
2.1.3

How about the attached patch?  It's basically the same as your patch
but I moved the code between files and tidied up some whitespace
issues.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Friday 28 November 2014 14:31:01 Richard W.M. Jones
wrote:
> How about the attached patch?  It's basically the same as your patch
> but I moved the code between files and tidied up some whitespace
> issues.
Present in both the patches:
> +/* On *BSD systems, sometimes /dev/sda[1234] is a shadow of the real root
> + * filesystem that is probably /dev/sda5
> + * (see: http://www.freebsd.org/doc/handbook/disk-organization.html)
> + */
> +static void
> +check_for_duplicated_bsd_root (guestfs_h *g)
> +{
> +  size_t i;
> +  bool is_primary, is_bsd;
> +  struct inspect_fs *fs, *bsd_primary = NULL;
> +
> +  for (i = 0; i < g->nr_fses; ++i) {
> +    fs = &g->fses[i];
> +
> +    is_primary = match (g, fs->mountable, re_primary_partition);
> +    is_bsd > +      fs->type == OS_TYPE_FREEBSD ||
> +      fs->type == OS_TYPE_NETBSD ||
> +      fs->type == OS_TYPE_OPENBSD;
> +
> +    if (fs->is_root && is_primary && is_bsd) {
> +      bsd_primary = fs;
> +      continue;
> +    }
This will run the regexp matching for every filesystem found; what
about inlining the match call as last part of the if, like:

   is_bsd      fs->type == OS_TYPE_FREEBSD ||
     fs->type == OS_TYPE_NETBSD ||
     fs->type == OS_TYPE_OPENBSD;

   if (fs->is_root && is_bsd && is_primary
       && match (g, fs->mountable, re_primary_partition)) {
     bsd_primary = fs;
     continue;
   }

This way it is done only for *BSD filesystems, and is_primary is
used only in that if anyway.

Also, is_bsd and fs could be declared just inside the for, as they are
not needed outside of it.

-- 
Pino Toscano