Re: du bogus warning with bind mounts

[Jim Meyering]

Ondrej Oprala wrote:
>  I've redone the patch to use mountlist instead of mntent, added the
> !remote && !dummy
> condition and created a test for it.

Great!  Thanks.

Some of this feedback is nit-picking, but there are more important
changes, too.

...
> Subject: [PATCH] du: Fix an issue with bogus warnings on bind-mounted
>  directories
>
> * NEWS: Mention the fix.
> * src/du.c: Add a function to read+stat mount points
> and properly check cyclic directory entries.

Please use this entry for "du.c" in the log:

  * src/du.c: Include "mountlist.h".
  (di_mnt): New global set.
  (fill_mount_table): New function.
  (hash_ins): Add HT parameter.
  (process_file): Look up each dir dev/ino pair in the new set.
  (main): Allocate, initialize, and free the new set.

> * tests/Makefile.am: Add a new file.
> * tests/du/cyclic-dir: Add a test for the fix.

Please rename the new test file to
bind-mount-dir-cycle

> ---
>  NEWS                |  3 +++
>  src/du.c            | 61 
> ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  tests/Makefile.am   |  1 +
>  tests/du/cyclic-dir | 40 +++++++++++++++++++++++++++++++++++
>  4 files changed, 102 insertions(+), 3 deletions(-)
>  create mode 100755 tests/du/cyclic-dir
>
> diff --git a/NEWS b/NEWS
> index ca4568a..8f85c2b 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -4,6 +4,9 @@ GNU coreutils NEWS                                    -*- 
> outline -*-
>
>  ** Bug fixes
>
> +  du no longer emits bogus warnings when traversing bind-mounted
> +  directory cycles.
> +
>    cksum now prints checksums atomically so that concurrent
>    processes will not intersperse their output.
>    [the bug dates back to the initial implementation]
> diff --git a/src/du.c b/src/du.c
> index 7333941..1f3a3e4 100644
> --- a/src/du.c
> +++ b/src/du.c
> @@ -27,6 +27,7 @@
>  #include <getopt.h>
>  #include <sys/types.h>
>  #include <assert.h>
> +#include "mountlist.h"

Please move this down so it's alphabetized with other "..."-includes.

>  #include "system.h"
>  #include "argmatch.h"
>  #include "argv-iter.h"
> @@ -60,9 +61,14 @@ extern bool fts_debug;
>  # define FTS_CROSS_CHECK(Fts)
>  #endif
>
> +#define MTAB "/etc/mtab"
> +

Not used.

>  /* A set of dev/ino pairs.  */
>  static struct di_set *di_set;
>
> +/* A hash table for mount points. */

s/\./. /

> +static struct di_set *di_mnt;
> +
>  /* Keep track of the preceding "level" (depth in hierarchy)
>     from one call of process_file to the next.  */
>  static size_t prev_level;
> @@ -333,11 +339,11 @@ Mandatory arguments to long options are mandatory for 
> short options too.\n\
>    exit (status);
>  }
>
> -/* Try to insert the INO/DEV pair into the global table, HTAB.
> +/* Try to insert the INO/DEV pair into the di_set table.
>     Return true if the pair is successfully inserted,
>     false if the pair is already in the table.  */
>  static bool
> -hash_ins (ino_t ino, dev_t dev)
> +hash_ins (struct di_set *di_set, ino_t ino, dev_t dev)
>  {
>    int inserted = di_set_insert (di_set, dev, ino);
>    if (inserted < 0)
> @@ -461,7 +467,7 @@ process_file (FTS *fts, FTSENT *ent)
>        if (excluded
>            || (! opt_count_all
>                && (hash_all || (! S_ISDIR (sb->st_mode) && 1 < sb->st_nlink))
> -              && ! hash_ins (sb->st_ino, sb->st_dev)))
> +              && ! hash_ins (di_set, sb->st_ino, sb->st_dev)))
>          {
>            /* If ignoring a directory in preorder, skip its children.
>               Ignore the next fts_read output too, as it's a postorder
> @@ -476,6 +482,17 @@ process_file (FTS *fts, FTSENT *ent)
>            return true;
>          }
>
> +      /*Check if dir is already in the mount table */

Make comment spacing and punctuation consistent:

         /* Check if dir is already in the mount table.  */

> +      if (S_ISDIR (sb->st_mode))
> +        {
> +          if (di_set_lookup (di_mnt, sb->st_dev, sb->st_ino))
> +            {
> +              fts_set (fts, ent, FTS_SKIP);
> +              error (0, 0, _("mount point %s already traversed"), quote 
> (file));
> +              return false;
> +            }
> +        }
> +
>        switch (info)
>          {
>          case FTS_D:
> @@ -623,6 +640,36 @@ du_files (char **files, int bit_flags)
>    return ok;
>  }
>
> +/* Fill the di_mnt table with dev/ino pairs
> + * of mount points  */

That comment belongs all on one line.

> +static void
> +fill_mount_table (void)
> +{
> +  struct mount_entry *mnt_ent, *mnt_free;

Declare each variable on its own line.

> +  struct stat *buf;
> +  buf = xmalloc (sizeof *buf);

Combine declaration and initialization.  Reduces duplication.

> +  mnt_ent = read_file_system_list (false);
> +  while (mnt_ent)
> +    {
> +      if (!mnt_ent->me_remote && !mnt_ent->me_dummy)
> +        {
> +          stat (mnt_ent->me_mountdir, buf);

If stat fails, we must not call hash_ins, since &buf is then undefined.
It _might_ deserve a warning, too.

> +          hash_ins (di_mnt, buf->st_ino, buf->st_dev);
> +        }
> +
> +      mnt_free = mnt_ent;
> +      mnt_ent = mnt_ent->me_next;
> +
> +      free (mnt_free->me_devname);
> +      free (mnt_free->me_mountdir);
> +      free (mnt_free->me_type);
> +      free (mnt_free);
> +    }
> +  free (buf);
> +}
> +
>  int
>  main (int argc, char **argv)
>  {
> @@ -922,6 +969,13 @@ main (int argc, char **argv)
>      xalloc_die ();
>
>    /* Initialize the set of dev,inode pairs.  */
> +
> +  di_mnt = di_set_alloc ();
> +  if (!di_mnt)
> +    xalloc_die ();
> +
> +  fill_mount_table ();
> +
>    di_set = di_set_alloc ();
>    if (!di_set)
>      xalloc_die ();
> @@ -1002,6 +1056,7 @@ main (int argc, char **argv)
>
>    argv_iter_free (ai);
>    di_set_free (di_set);
> +  di_set_free (di_mnt);
>
>    if (files_from && (ferror (stdin) || fclose (stdin) != 0) && ok)
>      error (EXIT_FAILURE, 0, _("error reading %s"), quote (files_from));
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index edc04b4..20c313d 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -32,6 +32,7 @@ root_tests =                                        \
>    cp/sparse-fiemap                           \
>    dd/skip-seek-past-dev                              \
>    df/problematic-chars                               \
> +  du/cyclic-dir                                      \
>    install/install-C-root                     \
>    ls/capability                                      \
>    ls/nameless-uid                            \
> diff --git a/tests/du/cyclic-dir b/tests/du/cyclic-dir
> new file mode 100755
> index 0000000..65569ce
> --- /dev/null
> +++ b/tests/du/cyclic-dir
> @@ -0,0 +1,40 @@
> +#!/bin/sh
> +# Ensure du properly handles bind mounts

s/bind mounts/bind-mount-induced directory cycles/

> +# Copyright (C) 2006-2012 Free Software Foundation, Inc.

s/2006-//

> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +. "${srcdir=.}/init.sh"; path_prepend_ ../src
> +print_ver_ rm
> +require_root_
> +
> +cleanup_()
> +{
> +  # When you take the undesirable shortcut of making /etc/mtab a link
> +  # to /proc/mounts, unmounting "$other_partition_tmpdir" would fail.
> +  # So, here we unmount a/b instead.
> +  umount a/b
> +  rm -rf "$other_partition_tmpdir"

This test does not require a temporary directory on a separate partition,
so remove the line above, as well as the use of other-fs-tmpdir below.

> +}
> +. "$abs_srcdir/other-fs-tmpdir"
> +
> +t=$other_partition_tmpdir
> +mkdir -p a/b $t/y
> +mount --bind $t a/b \
> +  || skip_ "This test requires mount with a working --bind option."
> +
> +du a && fail=1

Please record the output of du and compare against expected output.
This will ensure not just that du fails but will also show whether the
bind-mounted directory name is listed.  Of course, like other du
tests, you'll have to filter out the block counts before comparing,
since they are notoriously non-portable.

> +Exit $fail