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

[Jeff Layton]

On Thu, 2019-09-12 at 11:58 -0600, Andreas Dilger wrote:
> > 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?
> 

glibc already does this fallback.

> 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))
>         {
>           :
>           :
> 
> 

Yes, I think I'm going to rework the set to just add stat_for_ino and
fstat_for_ino functions and plug those into the existing code instead of
factoring out the loop detection like this.

I'll post a v2 set in a bit after I re-do some testing.

> > +        {
> > +          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
> 
> 
> 
> 
> 

-- 
Jeff Layton <address@hidden>