[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Fri, Nov 28, 2014 at 03:42:58PM +0100, Pino Toscano wrote:
> 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.

Good points.

See updated patch below.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
From 282132c58bc81216a391fc62c28fc6f01b8c8fb6 Mon Sep 17 00:00:00 2001
From: Nikos Skalkotos <skalkoto grnet gr>
Date: Thu, 27 Nov 2014 18:30:33 +0200
Subject: [PATCH] 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/inspect-fs.c | 19 --------------
 src/inspect.c    | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 19 deletions(-)

diff --git a/src/inspect-fs.c b/src/inspect-fs.c
index 539d814..b023ffc 100644
--- a/src/inspect-fs.c
+++ b/src/inspect-fs.c
@@ -47,7 +47,6 @@
  * multiple threads call into the libguestfs API functions below
  * simultaneously.
  */
-static pcre *re_first_partition;
 static pcre *re_major_minor;
 
 static void compile_regexps (void) __attribute__((constructor));
@@ -68,14 +67,12 @@ compile_regexps (void)
     }                                                                   \
   } while (0)
 
-  COMPILE (re_first_partition, "^/dev/(?:h|s|v)d.1$", 0);
   COMPILE (re_major_minor, "(\\d+)\\.(\\d+)", 0);
 }
 
 static void
 free_regexps (void)
 {
-  pcre_free (re_first_partition);
   pcre_free (re_major_minor);
 }
 
@@ -207,14 +204,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 +214,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..0e98d41 100644
--- a/src/inspect.c
+++ b/src/inspect.c
@@ -32,11 +32,49 @@
 #include <endian.h>
 #endif
 
+#include "ignore-value.h"
+
 #include "guestfs.h"
 #include "guestfs-internal.h"
 #include "guestfs-internal-actions.h"
 #include "guestfs_protocol.h"
 
+/* Compile all the regular expressions once when the shared library is
+ * loaded.  PCRE is thread safe so we're supposedly OK here if
+ * multiple threads call into the libguestfs API functions below
+ * simultaneously.
+ */
+static pcre *re_primary_partition;
+
+static void compile_regexps (void) __attribute__((constructor));
+static void free_regexps (void) __attribute__((destructor));
+
+static void
+compile_regexps (void)
+{
+  const char *err;
+  int offset;
+
+#define COMPILE(re,pattern,options)                                     \
+  do {                                                                  \
+    re = pcre_compile ((pattern), (options), &err, &offset, NULL);      \
+    if (re == NULL) {                                                   \
+      ignore_value (write (2, err, strlen (err)));                      \
+      abort ();                                                         \
+    }                                                                   \
+  } while (0)
+
+  COMPILE (re_primary_partition, "^/dev/(?:h|s|v)d.[1234]$", 0);
+}
+
+static void
+free_regexps (void)
+{
+  pcre_free (re_primary_partition);
+}
+
+static void check_for_duplicated_bsd_root (guestfs_h *g);
+
 /* The main inspection code. */
 char **
 guestfs__inspect_os (guestfs_h *g)
@@ -64,6 +102,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
+   */
+  check_for_duplicated_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.
@@ -75,6 +119,40 @@ guestfs__inspect_os (guestfs_h *g)
   return ret;
 }
 
+/* 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;
+  struct inspect_fs *bsd_primary = NULL;
+
+  for (i = 0; i < g->nr_fses; ++i) {
+    bool is_bsd;
+    struct inspect_fs *fs = &g->fses[i];
+
+    is_bsd =
+      fs->type == OS_TYPE_FREEBSD ||
+      fs->type == OS_TYPE_NETBSD ||
+      fs->type == OS_TYPE_OPENBSD;
+
+    if (fs->is_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;
+      bsd_primary->format = OS_FORMAT_UNKNOWN;
+      return;
+    }
+  }
+}
+
 static int
 compare_strings (const void *vp1, const void *vp2)
 {
-- 
2.1.0


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]