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

[Mattias Andrée]

On Thu, 7 Apr 2016 11:47:36 +0200
Bernhard Voelker <address@hidden> wrote:

> 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

I don't think this is a problem. For reliable
script you need to check whether the test
failed or was successful. That is, in the case
of test, you have to consider whether test
exited with value 0, 1, or >1.

> 
> * 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.

Perhaps it is better to check errno != ENOTDIR when
opendir fails, but the race itself is not important.

> 
> 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.

Yes, the shells that implement test needs to be updated.
But until then, it can use the stand-alone by simply
running `env test` instead of `test`. You really should
be doing that anyway unless you need one of the operators
that are only implemented in the shell, the shells'
implementations are not that good.

> 
> Have a nice day,
> Berny

pgpcH3Y_aHmjr.pgp
Description: OpenPGP digital signature