Re: [PATCH v2 1/2] test: Add unary operator -E: test that a file is an empty directory

[Bernhard Voelker]

On 04/07/2016 10:34 AM, Mattias Andrée wrote:
> diff --git a/src/test.c b/src/test.c
> index 8ac7467..eb9c43a 100644
> --- a/src/test.c
> +++ b/src/test.c
> @@ -27,6 +27,7 @@
>  #endif
>  
>  #include <config.h>
> +#include <dirent.h>
>  #include <stdio.h>
>  #include <sys/types.h>
>  
> @@ -179,6 +180,39 @@ get_mtime (char const *filename, struct timespec *mtime)
>    return ok;
>  }
>  
> +/* Return true iff DIR is empty. DIR must be a directory.  */
> +static bool
> +empty_p (char const *dirname)
> +{
> +  DIR *dir;
> +  struct dirent *de;
> +
> +  dir = opendir (dirname);
> +  if (!dir)
> +    {
> +      error (0, errno, "%s", dirname);
> +      test_exit (TEST_FAILURE);
> +    }
> +
> +  while (errno = 0, (de = readdir (dir)))
> +    {
> +      if (de->d_name[0] == '.')
> +        if (de->d_name[1 + (de->d_name[1] == '.')] == '\0')
> +          continue;
> +      closedir (dir);
> +      return 0;
> +    }
> +
> +  if (errno)
> +    {
> +      error (0, errno, "%s", dirname);
> +      closedir (dir);
> +      test_exit (TEST_FAILURE);
> +    }
> +  closedir (dir);
> +  return 1;
> +}
> +
>  /* Return true if S is one of the test command's binary operators.  */
>  static bool
>  binop (char const *s)
> @@ -457,6 +491,12 @@ unary_operator (void)
>        return (stat (argv[pos - 1], &stat_buf) == 0
>                && S_ISDIR (stat_buf.st_mode));
>  
> +    case 'E':                        /* File is an empty directory? */
> +      unary_advance ();
> +      return (stat (argv[pos - 1], &stat_buf) == 0
> +              && S_ISDIR (stat_buf.st_mode)
> +              && empty_p (argv[pos - 1]));
> +

While I find this feature quite useful - although I never needed such a
test yet -, I see the following problems:

* While the other file type based tests only use stat/lstat, this
one additionally needs opendir/readdir/closedir, i.e., the permissions
on the directory could slip into the game:

  $ mkdir /dev/shm/dir

  $ chmod =0000 /dev/shm/dir

  $ src/test -E /dev/shm/dir && echo OK || echo FAIL
  src/test: /dev/shm/dir: Permission denied
  FAIL

  $ src/test -d /dev/shm/dir && echo OK || echo FAIL
  OK

* The above stat/opendir is racy:

  $ while test -d /dev/shm/dir ; do \
      chmod =0000 /dev/shm/dir; \
      chmod =0700 /dev/shm/dir; \
    done &

  $ for f in $(seq 100); do \
      src/test -E /dev/shm/dir 2>/dev/null \
        && echo OK; \
    done \
      | grep -c OK
  49

Avoiding stat at all, and relying on opendir may be a better way.

However, as Eric pointed out, coreutils' test and \[ are not in use very
often, so the discussion and implementation should IMO start for the
builtins of the major shells first.

Have a nice day,
Berny