Libguestfs - Dec 2016 - [PATCH 1/2] inspect: fstab: Canonicalize paths appearing in fstab.

For example, converts "///usr//local//" -> "/usr/local".
---
 src/inspect-fs-unix.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/src/inspect-fs-unix.c b/src/inspect-fs-unix.c
index a1a757c..0fea9c8 100644
--- a/src/inspect-fs-unix.c
+++ b/src/inspect-fs-unix.c
@@ -89,6 +89,7 @@ static char *resolve_fstab_device (guestfs_h *g, const char
*spec,
                                    Hash_table *md_map,
                                    enum inspect_os_type os_type);
 static int inspect_with_augeas (guestfs_h *g, struct inspect_fs *fs, const char
**configfiles, int (*f) (guestfs_h *, struct inspect_fs *));
+static void canonical_mountpoint (char *mp);
 
 /* Hash structure for uuid->path lookups */
 typedef struct md_uuid {
@@ -1286,6 +1287,9 @@ check_fstab (guestfs_h *g, struct inspect_fs *fs)
     if (mp == NULL)
       return -1;
 
+    /* Canonicalize the path, so "///usr//local//" ->
"/usr/local" */
+    canonical_mountpoint (mp);
+
     /* Ignore certain mountpoints. */
     if (STRPREFIX (mp, "/dev/") ||
         STREQ (mp, "/dev") ||
@@ -2081,3 +2085,51 @@ make_augeas_path_expression (guestfs_h *g, const char
**configfiles)
   debug (g, "augeas pathexpr = %s", ret);
   return ret;
 }
+
+/* Canonicalize the path, so "///usr//local//" ->
"/usr/local"
+ *
+ * The path is modified in place because the result is always
+ * the same length or shorter than the argument passed.
+ */
+static void
+drop_char (char *mp)
+{
+  size_t len = strlen (mp);
+  memmove (&mp[0], &mp[1], len);
+}
+
+static void
+canonical_mountpoint_recursive (char *mp)
+{
+  if (mp[0] == '\0')
+    return;
+
+  /* Remove trailing slashes. */
+  if (mp[0] == '/' && mp[1] == '\0') {
+    mp[0] = '\0';
+    return;
+  }
+
+  /* Replace multiple slashes with single slashes. */
+  if (mp[0] == '/' && mp[1] == '/') {
+    drop_char (mp);
+    canonical_mountpoint_recursive (mp);
+    return;
+  }
+
+  canonical_mountpoint_recursive (&mp[1]);
+}
+
+static void
+canonical_mountpoint (char *mp)
+{
+  /* Collapse multiple leading slashes into a single slash ... */
+  while (mp[0] == '/' && mp[1] == '/')
+    drop_char (mp);
+
+  /* ... and then continue, skipping the leading slash. */
+  if (mp[0] == '/')
+    canonical_mountpoint_recursive (&mp[1]);
+  else
+    canonical_mountpoint_recursive (mp);
+}
-- 
2.9.3

---
 src/inspect-fs-unix.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/src/inspect-fs-unix.c b/src/inspect-fs-unix.c
index 0fea9c8..b2c5ce1 100644
--- a/src/inspect-fs-unix.c
+++ b/src/inspect-fs-unix.c
@@ -455,6 +455,7 @@ guestfs_int_check_linux_root (guestfs_h *g, struct
inspect_fs *fs)
 {
   int r;
   char *major, *minor;
+  size_t i;
 
   fs->type = OS_TYPE_LINUX;
 
@@ -768,9 +769,6 @@ guestfs_int_check_linux_root (guestfs_h *g, struct
inspect_fs *fs)
 
  skip_release_checks:;
 
-  /* Determine the architecture. */
-  check_architecture (g, fs);
-
   /* We already know /etc/fstab exists because it's part of the test
    * for Linux root above.  We must now parse this file to determine
    * which filesystems are used by the operating system and how they
@@ -780,6 +778,27 @@ guestfs_int_check_linux_root (guestfs_h *g, struct
inspect_fs *fs)
   if (inspect_with_augeas (g, fs, configfiles, check_fstab) == -1)
     return -1;
 
+  /* Some sysadmins like to mount /usr separately.  Unfortunately if
+   * this has been done and the distro uses UsrMove or equivalent,
+   * then there will no arch-specific binaries on the root filesystem
+   * and so check_architecture will fail (RHBZ#1401474).  See if there
+   * is a separate /usr (found by check_fstab above) and mount it.
+   * This filesystem will be unmounted by guestfs_umount_all in the
+   * caller.
+   */
+  for (i = 0; i < fs->nr_fstab; ++i) {
+    const char *mp = fs->fstab[i].mountpoint;
+
+    if (STREQ (mp, "/usr")) {
+      guestfs_push_error_handler (g, NULL, NULL);
+      guestfs_mount_ro (g, fs->fstab[i].mountable, mp);
+      guestfs_pop_error_handler (g);
+    }
+  }
+
+  /* Determine the architecture. */
+  check_architecture (g, fs);
+
   /* Determine hostname. */
   if (check_hostname_unix (g, fs) == -1)
     return -1;
-- 
2.9.3

On Tuesday, 6 December 2016 09:46:25 CET Richard W.M. Jones
wrote:
> For example, converts "///usr//local//" ->
"/usr/local".
> ---
>  src/inspect-fs-unix.c | 52
+++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/src/inspect-fs-unix.c b/src/inspect-fs-unix.c
> index a1a757c..0fea9c8 100644
> --- a/src/inspect-fs-unix.c
> +++ b/src/inspect-fs-unix.c
> @@ -89,6 +89,7 @@ static char *resolve_fstab_device (guestfs_h *g, const
char *spec,
>                                     Hash_table *md_map,
>                                     enum inspect_os_type os_type);
>  static int inspect_with_augeas (guestfs_h *g, struct inspect_fs *fs, const
char **configfiles, int (*f) (guestfs_h *, struct inspect_fs *));
> +static void canonical_mountpoint (char *mp);
>  
>  /* Hash structure for uuid->path lookups */
>  typedef struct md_uuid {
> @@ -1286,6 +1287,9 @@ check_fstab (guestfs_h *g, struct inspect_fs *fs)
>      if (mp == NULL)
>        return -1;
>  
> +    /* Canonicalize the path, so "///usr//local//" ->
"/usr/local" */
> +    canonical_mountpoint (mp);
> +
>      /* Ignore certain mountpoints. */
>      if (STRPREFIX (mp, "/dev/") ||
>          STREQ (mp, "/dev") ||
> @@ -2081,3 +2085,51 @@ make_augeas_path_expression (guestfs_h *g, const
char **configfiles)
>    debug (g, "augeas pathexpr = %s", ret);
>    return ret;
>  }
> +
> +/* Canonicalize the path, so "///usr//local//" ->
"/usr/local"
> + *
> + * The path is modified in place because the result is always
> + * the same length or shorter than the argument passed.
> + */
> +static void
> +drop_char (char *mp)
> +{
> +  size_t len = strlen (mp);
> +  memmove (&mp[0], &mp[1], len);
> +}
> +
> +static void
> +canonical_mountpoint_recursive (char *mp)
> +{
> +  if (mp[0] == '\0')
> +    return;
> +
> +  /* Remove trailing slashes. */
> +  if (mp[0] == '/' && mp[1] == '\0') {
> +    mp[0] = '\0';
> +    return;
> +  }
> +
> +  /* Replace multiple slashes with single slashes. */
> +  if (mp[0] == '/' && mp[1] == '/') {
> +    drop_char (mp);
> +    canonical_mountpoint_recursive (mp);
> +    return;
> +  }
> +
> +  canonical_mountpoint_recursive (&mp[1]);
> +}
> +
> +static void
> +canonical_mountpoint (char *mp)
> +{
> +  /* Collapse multiple leading slashes into a single slash ... */
> +  while (mp[0] == '/' && mp[1] == '/')
> +    drop_char (mp);
> +
> +  /* ... and then continue, skipping the leading slash. */
> +  if (mp[0] == '/')
> +    canonical_mountpoint_recursive (&mp[1]);
> +  else
> +    canonical_mountpoint_recursive (mp);
> +}
This implementation looks a bit inefficient to me (recursive, moving
bytes and using strlen for every char removed, etc). What about the
following implementation?

  void canonicalize(char *s)
  {
    size_t len = strlen(s);
    char *orig = s;

    s = strchr(s, '/');
    while (s != NULL && *s != 0) {
      char *pos = s + 1;
      char *p = pos;
      while (*p == '/')
        ++p;
      if (p - pos > 0) {
        memmove(pos, p, len - (p - orig) + 1);
        len -= p - pos;
      }

      s = strchr(pos, '/');
    }
    orig[len] = 0;
  }

The only behaviour change with the committed implementation is that it
does not remove trailing slashes, but IMHO they could be left there
(or removed after the canonicalization function, just once).

-- 
Pino Toscano

On Wed, Dec 07, 2016 at 06:07:00PM +0100, Pino Toscano
wrote:
> This implementation looks a bit inefficient to me (recursive, moving
It's inefficient (O(n^2)), but not because of recursion -- GCC can
turn the code into a loop using TCO.
> bytes and using strlen for every char removed, etc). What about the
> following implementation?
> 
>   void canonicalize(char *s)
>   {
>     size_t len = strlen(s);
>     char *orig = s;
> 
>     s = strchr(s, '/');
>     while (s != NULL && *s != 0) {
>       char *pos = s + 1;
>       char *p = pos;
>       while (*p == '/')
>         ++p;
>       if (p - pos > 0) {
>         memmove(pos, p, len - (p - orig) + 1);
>         len -= p - pos;
>       }
> 
>       s = strchr(pos, '/');
>     }
>     orig[len] = 0;
>   }
> 
> The only behaviour change with the committed implementation is that it
> does not remove trailing slashes, but IMHO they could be left there
> (or removed after the canonicalization function, just once).
I think it needs to handle the trailing '/'.

The bigger problem here is if we find an fstab that has something like
/etc/../usr/ or a path containing symlinks, but I don't think there's
any good way to deal with that.

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