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

[Pino Toscano]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20161207/a349fbfe/attachment.sig>