Libguestfs - Jun 2013 - [PATCH 1/3] inspection: Refactor windows systemroot detection to allow re-use

This change refactors guestfs___has_windows_systemroot to
guestfs___get_windows_systemroot. The new function returns a
dynamically allocated char * which must be freed.

The new function is no less efficient than before, as it returns the
result of guestfs___case_sensitive_path_silently, which is required
anyway. The new code is slightly more efficient than before, as it
re-uses the result of this testing in guestfs___check_windows_root
rather than running it again.
---
 src/guestfs-internal.h   |  4 +--
 src/inspect-fs-windows.c | 72 +++++++++++++++++++++++-------------------------
 src/inspect-fs.c         |  8 ++++--
 3 files changed, 43 insertions(+), 41 deletions(-)

diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 6e97948..bc13b3c 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -668,8 +668,8 @@ extern int guestfs___check_hurd_root (guestfs_h *g, struct
inspect_fs *fs);
 
 /* inspect-fs-windows.c */
 extern char *guestfs___case_sensitive_path_silently (guestfs_h *g, const char
*);
-extern int guestfs___has_windows_systemroot (guestfs_h *g);
-extern int guestfs___check_windows_root (guestfs_h *g, struct inspect_fs *fs);
+extern char * guestfs___get_windows_systemroot (guestfs_h *g);
+extern int guestfs___check_windows_root (guestfs_h *g, struct inspect_fs *fs,
char *windows_systemroot);
 
 /* inspect-fs-cd.c */
 extern int guestfs___check_installer_root (guestfs_h *g, struct inspect_fs
*fs);
diff --git a/src/inspect-fs-windows.c b/src/inspect-fs-windows.c
index a979775..8ddea95 100644
--- a/src/inspect-fs-windows.c
+++ b/src/inspect-fs-windows.c
@@ -101,60 +101,58 @@ static char *map_registry_disk_blob (guestfs_h *g, const
void *blob);
  * "/Program Files" and "/System Volume Information". 
Those would
  * *not* be Windows root disks.  (RHBZ#674130)
  */
-static const char *systemroots[] -  { "/windows", "/winnt",
"/win32", "/win", NULL };
 
-int
-guestfs___has_windows_systemroot (guestfs_h *g)
+static int is_systemroot (guestfs_h *const g, const char *const systemroot)
 {
-  size_t i;
   char path[256];
 
-  for (i = 0; i < sizeof systemroots / sizeof systemroots[0]; ++i) {
-    CLEANUP_FREE char *systemroot -      guestfs___case_sensitive_path_silently
(g, systemroots[i]);
-    if (!systemroot)
-      continue;
+  snprintf (path, sizeof path, "%s/system32", systemroot);
+  if (!guestfs___is_dir_nocase (g, path))
+    return 0;
 
-    snprintf (path, sizeof path, "%s/system32", systemroot);
-    if (!guestfs___is_dir_nocase (g, path))
-      continue;
+  snprintf (path, sizeof path, "%s/system32/config", systemroot);
+  if (!guestfs___is_dir_nocase (g, path))
+    return 0;
 
-    snprintf (path, sizeof path, "%s/system32/config", systemroot);
-    if (!guestfs___is_dir_nocase (g, path))
-      continue;
+  snprintf (path, sizeof path, "%s/system32/cmd.exe", systemroot);
+  if (!guestfs___is_file_nocase (g, path))
+    return 0;
+
+  return 1;
+}
+
+char *
+guestfs___get_windows_systemroot (guestfs_h *g)
+{
+  /* Check a predefined list of common windows system root locations */
+  static const char *systemroots[] +    { "/windows",
"/winnt", "/win32", "/win", NULL };
 
-    snprintf (path, sizeof path, "%s/system32/cmd.exe", systemroot);
-    if (!guestfs___is_file_nocase (g, path))
+  for (size_t i = 0; i < sizeof systemroots / sizeof systemroots[0]; ++i) {
+    char *systemroot +      guestfs___case_sensitive_path_silently (g,
systemroots[i]);
+    if (!systemroot)
       continue;
 
-    return (int)i;
+    if (is_systemroot (g, systemroot)) {
+      debug (g, "windows %%SYSTEMROOT%% = %s", systemroot);
+
+      return systemroot;
+    } else {
+      free (systemroot);
+    }
   }
 
-  return -1; /* not found */
+  return NULL; /* not found */
 }
 
 int
-guestfs___check_windows_root (guestfs_h *g, struct inspect_fs *fs)
+guestfs___check_windows_root (guestfs_h *g, struct inspect_fs *fs,
+                              char *const systemroot)
 {
-  int i;
-  char *systemroot;
-
   fs->type = OS_TYPE_WINDOWS;
   fs->distro = OS_DISTRO_WINDOWS;
 
-  i = guestfs___has_windows_systemroot (g);
-  if (i == -1) {
-    error (g, "check_windows_root: has_windows_systemroot unexpectedly
returned -1");
-    return -1;
-  }
-
-  systemroot = guestfs_case_sensitive_path (g, systemroots[i]);
-  if (!systemroot)
-    return -1;
-
-  debug (g, "windows %%SYSTEMROOT%% = %s", systemroot);
-
   /* Freed by guestfs___free_inspect_info. */
   fs->windows_systemroot = systemroot;
 
@@ -179,7 +177,7 @@ check_windows_arch (guestfs_h *g, struct inspect_fs *fs)
   char cmd_exe[len];
   snprintf (cmd_exe, len, "%s/system32/cmd.exe",
fs->windows_systemroot);
 
-  /* Should exist because of previous check above in has_windows_systemroot. */
+  /* Should exist because of previous check above in get_windows_systemroot. */
   CLEANUP_FREE char *cmd_exe_path = guestfs_case_sensitive_path (g, cmd_exe);
   if (!cmd_exe_path)
     return -1;
diff --git a/src/inspect-fs.c b/src/inspect-fs.c
index d220634..0473e92 100644
--- a/src/inspect-fs.c
+++ b/src/inspect-fs.c
@@ -175,6 +175,9 @@ check_filesystem (guestfs_h *g, const char *mountable,
                   const struct guestfs_internal_mountable *m,
                   int whole_device)
 {
+  /* Not CLEANUP_FREE, as it will be cleaned up with inspection info */
+  char *windows_systemroot = NULL;
+
   if (extend_fses (g) == -1)
     return -1;
 
@@ -274,10 +277,11 @@ check_filesystem (guestfs_h *g, const char *mountable,
            guestfs_is_dir (g, "/spool") > 0)
     ;
   /* Windows root? */
-  else if (guestfs___has_windows_systemroot (g) >= 0) {
+  else if ((windows_systemroot = guestfs___get_windows_systemroot (g)) != NULL)
+  {
     fs->is_root = 1;
     fs->format = OS_FORMAT_INSTALLED;
-    if (guestfs___check_windows_root (g, fs) == -1)
+    if (guestfs___check_windows_root (g, fs, windows_systemroot) == -1)
       return -1;
   }
   /* Windows volume with installed applications (but not root)? */
-- 
1.8.2.1

---
 src/guestfs-internal.h |  2 ++
 src/match.c            | 32 +++++++++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index bc13b3c..0d76d1f 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -589,11 +589,13 @@ extern int guestfs___match (guestfs_h *g, const char *str,
const pcre *re);
 extern char *guestfs___match1 (guestfs_h *g, const char *str, const pcre *re);
 extern int guestfs___match2 (guestfs_h *g, const char *str, const pcre *re,
char **ret1, char **ret2);
 extern int guestfs___match3 (guestfs_h *g, const char *str, const pcre *re,
char **ret1, char **ret2, char **ret3);
+extern int guestfs___match6 (guestfs_h *g, const char *str, const pcre *re,
char **ret1, char **ret2, char **ret3, char **ret4, char **ret5, char **ret6);
 
 #define match guestfs___match
 #define match1 guestfs___match1
 #define match2 guestfs___match2
 #define match3 guestfs___match3
+#define match6 guestfs___match6
 
 /* proto.c */
 extern int guestfs___send (guestfs_h *g, int proc_nr, uint64_t progress_hint,
uint64_t optargs_bitmask, xdrproc_t xdrp, char *args);
diff --git a/src/match.c b/src/match.c
index b31721c..86c0a31 100644
--- a/src/match.c
+++ b/src/match.c
@@ -1,5 +1,5 @@
 /* libguestfs
- * Copyright (C) 2010-2012 Red Hat Inc.
+ * Copyright (C) 2010-2013 Red Hat Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -103,3 +103,33 @@ guestfs___match3 (guestfs_h *g, const char *str, const pcre
*re,
 
   return 1;
 }
+
+/* Match a regular expression which contains exactly six captures. */
+int
+guestfs___match6 (guestfs_h *g, const char *str, const pcre *re,
+                  char **ret1, char **ret2, char **ret3, char **ret4,
+                  char **ret5, char **ret6)
+{
+  size_t len = strlen (str);
+  int vec[30], r;
+
+  r = pcre_exec (re, NULL, str, len, 0, 0, vec, 30);
+  if (r == PCRE_ERROR_NOMATCH)
+    return 0;
+
+  *ret1 = NULL;
+  *ret2 = NULL;
+  *ret3 = NULL;
+  *ret4 = NULL;
+  *ret5 = NULL;
+  *ret6 = NULL;
+
+  if (r > 1) *ret1 = safe_strndup (g, &str[vec[2]], vec[3]-vec[2]);
+  if (r > 2) *ret2 = safe_strndup (g, &str[vec[4]], vec[5]-vec[4]);
+  if (r > 3) *ret3 = safe_strndup (g, &str[vec[6]], vec[7]-vec[6]);
+  if (r > 4) *ret4 = safe_strndup (g, &str[vec[8]], vec[9]-vec[8]);
+  if (r > 5) *ret5 = safe_strndup (g, &str[vec[10]], vec[11]-vec[10]);
+  if (r > 6) *ret6 = safe_strndup (g, &str[vec[12]], vec[13]-vec[12]);
+
+  return 1;
+}
-- 
1.8.2.1

Support arbitrary windows system root for pre-vista systems where
boot.ini is on the same partition as the system root.
---
 src/inspect-fs-windows.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/src/inspect-fs-windows.c b/src/inspect-fs-windows.c
index 8ddea95..61b2f3b 100644
--- a/src/inspect-fs-windows.c
+++ b/src/inspect-fs-windows.c
@@ -50,6 +50,8 @@
  * simultaneously.
  */
 static pcre *re_windows_version;
+static pcre *re_boot_ini_os_header;
+static pcre *re_boot_ini_os;
 
 static void compile_regexps (void) __attribute__((constructor));
 static void free_regexps (void) __attribute__((destructor));
@@ -70,12 +72,16 @@ compile_regexps (void)
   } while (0)
 
   COMPILE (re_windows_version, "^(\\d+)\\.(\\d+)", 0);
+  COMPILE (re_boot_ini_os_header, "^\\[operating systems\\]\\s*$",
0);
+  COMPILE (re_boot_ini_os,
"^(multi|scsi)\\((\\d+)\\)disk\\((\\d+)\\)rdisk\\((\\d+)\\)partition\\((\\d+)\\)([^=]+)=",
0);
 }
 
 static void
 free_regexps (void)
 {
   pcre_free (re_windows_version);
+  pcre_free (re_boot_ini_os_header);
+  pcre_free (re_boot_ini_os);
 }
 
 static int check_windows_arch (guestfs_h *g, struct inspect_fs *fs);
@@ -143,6 +149,86 @@ guestfs___get_windows_systemroot (guestfs_h *g)
     }
   }
 
+  /* If the fs contains boot.ini, check it for non-standard
+   * systemroot locations */
+  CLEANUP_FREE char *boot_ini_path +    guestfs___case_sensitive_path_silently
(g, "/boot.ini");
+  if (boot_ini_path) {
+    CLEANUP_FREE_STRING_LIST char **boot_ini +      guestfs_read_lines (g,
boot_ini_path);
+    if (!boot_ini) {
+      debug (g, "error reading %s", boot_ini_path);
+      return NULL;
+    }
+
+    int found_os = 0;
+    for (char **i = boot_ini; *i != NULL; i++) {
+      CLEANUP_FREE char *controller_type = NULL;
+      CLEANUP_FREE char *controller = NULL;
+      CLEANUP_FREE char *disk = NULL;
+      CLEANUP_FREE char *rdisk = NULL;
+      CLEANUP_FREE char *partition = NULL;
+      CLEANUP_FREE char *path = NULL;
+
+      char *line = *i;
+
+      if (!found_os) {
+        if (match (g, line, re_boot_ini_os_header)) {
+          found_os = 1;
+          continue;
+        }
+      }
+
+      /* See http://support.microsoft.com/kb/102873 for a discussion
+       * of what this line means */
+      if (match6 (g, line, re_boot_ini_os, &controller_type,
+                  &controller, &disk, &rdisk, &partition,
&path))
+      {
+        /* The Windows system root may be on any disk. However, there
+         * are currently (at least) 2 practical problems preventing us
+         * from locating it on another disk:
+         *
+         * 1. We don't have enough metadata about the disks we were
+         * given to know if what controller they were on and what
+         * index they had.
+         *
+         * 2. The way inspection of filesystems currently works, we
+         * can't mark another filesystem, which we may have already
+         * inspected, to be inspected for a specific Windows system
+         * root.
+         *
+         * Solving 1 properly would require a new API at a minimum. We
+         * might be able to fudge something practical without this,
+         * though, e.g. by looking at the <partition>th partition of
+         * every disk for the specific windows root.
+         *
+         * Solving 2 would probably require a significant refactoring
+         * of the way filesystems are inspected. We should probably do
+         * this some time.
+         *
+         * For the moment, we ignore all partition information and
+         * assume the system root is on the current partition. In
+         * practice, this will normally be correct.
+         */
+
+        /* Swap backslashes for forward slashes in the system root
+         * path */
+        for (char *j = path; *j != '\0'; j++) {
+          if (*j == '\\') *j = '/';
+        }
+
+        char *systemroot = guestfs___case_sensitive_path_silently (g, path);
+        if (systemroot && is_systemroot (g, systemroot)) {
+          debug (g, "windows %%SYSTEMROOT%% = %s", systemroot);
+
+          return systemroot;
+        } else {
+          free (systemroot);
+        }
+      }
+    }
+  }
+
   return NULL; /* not found */
 }
 
-- 
1.8.2.1

On Wed, Jun 05, 2013 at 04:56:03PM +0100, Matthew Booth
wrote:
> Support arbitrary windows system root for pre-vista systems where
> boot.ini is on the same partition as the system root.
              ^^^^^^^^^^^^^^^^^^^^^

I was just coming back to make this point.  Is this a common
configuration?  All the guests I can remember have had a separate boot
partition, except Windows XP.

Anyway, it seems this is an improvement, and there are no obvious
problems and it passes make check & make check-valgrind, so I have
pushed it.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
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/