Re: [PATCH] canonicalize: add support for not resolving symlinks

[Jim Meyering]

Pádraig Brady wrote:
> This is for use in a proposed coreutils `realpath` command.
> Specifically by these options:
>
>   -L, --logical                resolve `..' components before symlinks
>   -s, --strip                  don't expand symlinks
...

Thanks.  This looks good.
Some nits:

> Subject: [PATCH] canonicalize: add support for not resolving symlinks
>
> This will initially be used by a new coreutils realpath command.
>
> * lib/canonicalize.h: Add the CAN_NOLINKS flag to
> indicate we don't want to follow symlinks.  Also
> provide CAN_MODE_MASK to aid setting these existing
> mutually exclusive values.
> * lib/canonicalize.c (canonicalize_filename_mode):
> Extract the flags from can_mode parameter, which
> are currently just used to select between stat()
> and lstat().  Also ensure that mutually exclusive
> values and flagged immediately as invalid.

s/and/are/

> * tests/test-canonicalize.c: Verify symlinks are
> not followed, and that invalid flag combinations
> are diagnosed.
> ---
>  ChangeLog                 |   16 ++++++++++++++++
>  lib/canonicalize.c        |   18 +++++++++++++++---
>  lib/canonicalize.h        |   10 +++++++++-
>  tests/test-canonicalize.c |   12 ++++++++++++
>  4 files changed, 52 insertions(+), 4 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index cc50b4a..06dfe77 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,19 @@
> +2011-12-29  Pádraig Brady  <address@hidden>
> +
> +     canonicalize: add support for not resolving symlinks
> +     * lib/canonicalize.h: Add the CAN_NOLINKS flag to
> +     indicate we don't want to follow symlinks.  Also
> +     provide CAN_MODE_MASK to aid setting these existing
> +     mutually exclusive values.
> +     * lib/canonicalize.c (canonicalize_filename_mode):
> +     Extract the flags from can_mode parameter, which
> +     are currently just used to select between stat()
> +     and lstat().  Also ensure that mutually exclusive
> +     values and flagged immediately as invalid.

s/and/are/

> +     * tests/test-canonicalize.c: Verify symlinks are
> +     not followed, and that invalid flag combinations
> +     are diagnosed.
> +
>  2011-12-25  Jim Meyering  <address@hidden>
>
>       gitlog-to-changelog: do not clump multi-paragraph entries
> diff --git a/lib/canonicalize.c b/lib/canonicalize.c
> index 4fe9f30..c73a963 100644
> --- a/lib/canonicalize.c
> +++ b/lib/canonicalize.c
> @@ -31,6 +31,8 @@
>  #include "xalloc.h"
>  #include "xgetcwd.h"
>
> +#define MULTIPLE_BITS_SET(i) (((i) & ((i) - 1)) != 0)
> +
>  /* In this file, we cannot handle file names longer than PATH_MAX.
>     On systems with no file name length limit, use a fallback.  */
>  #ifndef PATH_MAX
> @@ -82,8 +84,9 @@ seen_triple (Hash_table **ht, char const *filename, struct 
> stat const *st)
>  /* Return the canonical absolute name of file NAME, while treating
>     missing elements according to CAN_MODE.  A canonical name
>     does not contain any `.', `..' components nor any repeated file name
> -   separators ('/') or symlinks.  Whether components must exist
> -   or not depends on canonicalize mode.  The result is malloc'd.  */
> +   separators ('/') or, depdending on other CAN_MODE flags, symlinks.
> +   Whether components must exist or not depends on canonicalize mode.
> +   The result is malloc'd.  */
>
>  char *
>  canonicalize_filename_mode (const char *name, canonicalize_mode_t can_mode)
> @@ -95,6 +98,15 @@ canonicalize_filename_mode (const char *name, 
> canonicalize_mode_t can_mode)
>    size_t extra_len = 0;
>    Hash_table *ht = NULL;
>    int saved_errno;
> +  int can_flags = can_mode & ~CAN_MODE_MASK;
> +  can_mode &= CAN_MODE_MASK;
> +  bool logical = can_flags & CAN_NOLINKS;
> +
> +  if (MULTIPLE_BITS_SET (can_mode))
> +    {
> +      errno = EINVAL;
> +      return NULL;
> +    }
>
>    if (name == NULL)
>      {
> @@ -185,7 +197,7 @@ canonicalize_filename_mode (const char *name, 
> canonicalize_mode_t can_mode)
>            dest += end - start;
>            *dest = '\0';
>
> -          if (lstat (rname, &st) != 0)
> +          if ((logical?stat:lstat) (rname, &st) != 0)

Please add spaces around operators:

             if ((logical ? stat : lstat) (rname, &st) != 0)

>              {
>                saved_errno = errno;
>                if (can_mode == CAN_EXISTING)
> diff --git a/lib/canonicalize.h b/lib/canonicalize.h
> index 04ad79c..4bb5898 100644
> --- a/lib/canonicalize.h
> +++ b/lib/canonicalize.h
> @@ -19,6 +19,8 @@
>
>  #include <stdlib.h> /* for canonicalize_file_name */
>
> +#define CAN_MODE_MASK (CAN_EXISTING | CAN_ALL_BUT_LAST | CAN_MISSING)
> +
>  enum canonicalize_mode_t
>    {
>      /* All components must exist.  */
> @@ -28,7 +30,13 @@ enum canonicalize_mode_t
>      CAN_ALL_BUT_LAST = 1,
>
>      /* No requirements on components existence.  */
> -    CAN_MISSING = 2
> +    CAN_MISSING = 2,
> +
> +    /* Don't expand symlinks.  */
> +    CAN_NOLINKS = 4,
> +
> +    /* Modify the original (Not yet supported).  */
> +    CAN_NOALLOC = 8

Would it be better to add it along with the
change that adds support?

>    };
>  typedef enum canonicalize_mode_t canonicalize_mode_t;

...