Re: [PATCH 2/3] ls: use statx for loop detection if it's available

[Andreas Dilger]

> On Sep 11, 2019, at 7:51 AM, Jeff Layton <address@hidden> wrote:
> 
> * move loop detection routine into separate function
> * add a statx-enabled variant that is used when it's available. No need
>  for a full stat since all we care about is the dev/ino.
> * Since dev/ino should never change, set AT_STATX_DONT_SYNC
>  unconditionally.

See inline...

> @@ -2711,27 +2718,54 @@ queue_directory (char const *name, char const 
> *realname, bool command_line_arg)
>   pending_dirs = new;
> }
> 
> -/* Read directory NAME, and list the files in it.
> -   If REALNAME is nonzero, print its name instead of NAME;
> -   this is used for symbolic links to directories.
> -   COMMAND_LINE_ARG means this directory was mentioned on the command line.  
> */
> -
> -static void
> -print_dir (char const *name, char const *realname, bool command_line_arg)
> +#if USE_STATX
> +static bool
> +loop_detected (const char *name, DIR *dirp, bool command_line_arg)
> {
> -  DIR *dirp;
> -  struct dirent *next;
> -  uintmax_t total_blocks = 0;
> -  static bool first = true;
> -
> -  errno = 0;
> -  dirp = opendir (name);
> -  if (!dirp)
> +  if (LOOP_DETECT)
>     {
> -      file_failure (command_line_arg, _("cannot open directory %s"), name);
> -      return;
> -    }
> +      struct statx stx;
> +      int fd = dirfd (dirp);
> +      int flags = AT_STATX_DONT_SYNC;
> +      const char *pathname = name;
> +
> +      if (0 <= fd)
> +     {
> +       pathname = "";
> +       flags |= AT_EMPTY_PATH;
> +     }
> +      else
> +        {
> +       /* If dirfd failed, endure the overhead of statx by path. */
> +       fd = AT_FDCWD;
> +     }
> +
> +      if (statx (fd, pathname, flags, STATX_INO, &stx) < 0)
> +        {
> +          file_failure (command_line_arg,
> +                        _("cannot determine device and inode of %s"), name);

Should this have a runtime fallback to stat() if statx() is not implemented
on the running kernel, or is that already handled at another level?

In that case, rather than splitting loop_detected() into two nearly-identical
functions, it would be possible to do something like the following in a single
loop_detected() function:

#ifdef USE_STATX
      errno = 0;
      /* try statx() first since it is lightweight. use stat() if unavailable */
      if (statx (fd, pathname, flags, STATX_INO, &stx) == 0
          || (errno && errno != EOPNOTSUPP))
        {
          if (errno)
            goto error;
          ino = stx.stx_ino;
          dev = stx.stx_dev;
        }
      else
#endif
      if ((0 <= fd
           ? fstat (fd, &dir_stat)
           : stat (name, &dir_stat) == 0)
        {
          ino = dir_stat.st_ino;
          dev = dir_stat.st_dev;
        }
      else
        {
          closedir (dirp);
error:
          file_failure (command_line_arg,
                        _("cannot determine device and inode of %s"), name);
          return false;
        }

      if (visit_dir (dev, ino))
        {
          :
          :


> +        {
> +          return true;
> +        }
> 
> +      /* If we've already visited this dev/inode pair, warn that
> +         we've found a loop, and do not process this directory.  */
> +      dev_t dev = makedev (stx.stx_dev_major, stx.stx_dev_minor);
> +      if (visit_dir (dev, stx.stx_ino))
> +        {
> +          error (0, 0, _("%s: not listing already-listed directory"),
> +                 quotef (name));
> +          set_exit_status (true);
> +          return true;
> +        }
> +
> +      dev_ino_push (dev, stx.stx_ino);
> +    }
> +  return false;
> +}
> +#else
> +static bool
> +loop_detected (const char *name, DIR *dirp, bool command_line_arg)
> +{
>   if (LOOP_DETECT)
>     {
>       struct stat dir_stat;
> @@ -2744,8 +2778,7 @@ print_dir (char const *name, char const *realname, bool 
> command_line_arg)
>         {
>           file_failure (command_line_arg,
>                         _("cannot determine device and inode of %s"), name);
> -          closedir (dirp);
> -          return;
> +          return true;
>         }
> 
>       /* If we've already visited this dev/inode pair, warn that
> @@ -2754,13 +2787,42 @@ print_dir (char const *name, char const *realname, 
> bool command_line_arg)
>         {
>           error (0, 0, _("%s: not listing already-listed directory"),
>                  quotef (name));
> -          closedir (dirp);
>           set_exit_status (true);
> -          return;
> +          return true;
>         }
> 
>       dev_ino_push (dir_stat.st_dev, dir_stat.st_ino);
>     }
> +  return false;
> +}
> +#endif
> +
> +/* Read directory NAME, and list the files in it.
> +   If REALNAME is nonzero, print its name instead of NAME;
> +   this is used for symbolic links to directories.
> +   COMMAND_LINE_ARG means this directory was mentioned on the command line.  
> */
> +
> +static void
> +print_dir (char const *name, char const *realname, bool command_line_arg)
> +{
> +  DIR *dirp;
> +  struct dirent *next;
> +  uintmax_t total_blocks = 0;
> +  static bool first = true;
> +
> +  errno = 0;
> +  dirp = opendir (name);
> +  if (!dirp)
> +    {
> +      file_failure (command_line_arg, _("cannot open directory %s"), name);
> +      return;
> +    }
> +
> +  if (loop_detected(name, dirp, command_line_arg))
> +    {
> +      closedir (dirp);
> +      return;
> +    }
> 
>   clear_files ();
> 
> --
> 2.21.0
> 


Cheers, Andreas

signature.asc
Description: Message signed with OpenPGP