Re: du bogus warning with bind mounts

[Jim Meyering]

Ondrej Oprala wrote:
>>From d8eab7abdf19b668065ff8be3700c9966eefdc39 Mon Sep 17 00:00:00 2001
> From: Ondrej Oprala <address@hidden>
> Date: Thu, 9 Aug 2012 17:34:09 +0200
> Subject: [PATCH] du: Fix an issue with bogus warnings on bind-mounted
>  directories
>
> * NEWS: Mention the fix.
> * 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/bind-mount-dir-cycle: Add a test for the fix.

Thanks.

Please remember to build with --enable-gcc-warnings.
With that, compilation failed due to a shadowing warning
with the new di_set global shadowing a local by the same name.

Here's a log entry I'm adding:

  (di_files): Rename global from di_set, now that there are two.

In comments don't mention "table" when speaking about sets.
The fact that we're currently using a hash table to implement
sets is irrelevant and best not mentioned.  Otherwise, if we
ever change the underlying data structure, the comments would
become erroneous.

In fill_mount_table, there is no need to use xmalloc to allocate a struct stat.
Also, combine declarations and initializations and move declarations
into innermost scope.

I had to remove the warning-upon-failed-stat that I had recommended,
because otherwise, when I run the test, I would get this failure:

    + diff -u exp out
    --- exp 2012-08-17 10:53:52.832808872 +0200
    +++ out 2012-08-17 10:53:52.835808713 +0200
    @@ -1 +1,2 @@
    +du: warning: unable to stat /run/user/meyering/gvfs: Permission denied
     du: mount point 'a' already traversed

I'm squashing the following into your change set and will post
the result shortly.

=====================================================================
diff --git a/src/du.c b/src/du.c
index bfe543e..1a11e14 100644
--- a/src/du.c
+++ b/src/du.c
@@ -61,10 +61,11 @@ extern bool fts_debug;
 # define FTS_CROSS_CHECK(Fts)
 #endif

-/* A set of dev/ino pairs.  */
-static struct di_set *di_set;
+/* A set of dev/ino pairs to help identify files and directories
+   whose sizes have already been counted.  */
+static struct di_set *di_files;

-/* A hash table for mount points.  */
+/* A set containing a dev/ino pair for each local mount point directory.  */
 static struct di_set *di_mnt;

 /* Keep track of the preceding "level" (depth in hierarchy)
@@ -337,9 +338,9 @@ Mandatory arguments to long options are mandatory for short 
options too.\n\
   exit (status);
 }

-/* Try to insert the INO/DEV pair into the di_set table.
+/* Try to insert the INO/DEV pair into DI_SET.
    Return true if the pair is successfully inserted,
-   false if the pair is already in the table.  */
+   false if the pair was already there.  */
 static bool
 hash_ins (struct di_set *di_set, ino_t ino, dev_t dev)
 {
@@ -465,7 +466,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 (di_set, sb->st_ino, sb->st_dev)))
+              && ! hash_ins (di_files, 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
@@ -480,15 +481,13 @@ process_file (FTS *fts, FTSENT *ent)
           return true;
         }

-      /*Check if dir is already in the mount table.  */
-      if (S_ISDIR (sb->st_mode))
+      /* If this is a directory that is already in our set of mount point
+         dev/inode pairs, then diagnose it and avoid the cycle.  */
+      if (info == FTS_D && di_set_lookup (di_mnt, sb->st_dev, sb->st_ino))
         {
-          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;
-            }
+          fts_set (fts, ent, FTS_SKIP);
+          error (0, 0, _("mount point %s already traversed"), quote (file));
+          return false;
         }

       switch (info)
@@ -638,25 +637,25 @@ du_files (char **files, int bit_flags)
   return ok;
 }

-/* Fill the di_mnt table with dev/ino pairs of mount points.  */
+/* Fill the di_mnt set with local mount point dev/ino pairs.  */

 static void
 fill_mount_table (void)
 {
-  struct mount_entry *mnt_ent;
-  struct mount_entry *mnt_free;
-  struct stat *buf = xmalloc (sizeof *buf);
-
-  mnt_ent = read_file_system_list (false);
+  struct mount_entry *mnt_ent = read_file_system_list (false);
   while (mnt_ent)
     {
+      struct mount_entry *mnt_free;
       if (!mnt_ent->me_remote && !mnt_ent->me_dummy)
         {
-          if (!stat (mnt_ent->me_mountdir, buf))
-            hash_ins (di_mnt, buf->st_ino, buf->st_dev);
+          struct stat buf;
+          if (!stat (mnt_ent->me_mountdir, &buf))
+            hash_ins (di_mnt, buf.st_ino, buf.st_dev);
           else
-            error (0, errno, _("warning: unable to stat %s"),
-                   mnt_ent->me_mountdir);
+            {
+              /* Ignore stat failure.  False positives are too common.
+                 E.g., "Permission denied" on /run/user/<name>/gvfs.  */
+            }
         }

       mnt_free = mnt_ent;
@@ -667,7 +666,6 @@ fill_mount_table (void)
       free (mnt_free->me_type);
       free (mnt_free);
     }
-  free (buf);
 }

 int
@@ -976,8 +974,8 @@ main (int argc, char **argv)

   fill_mount_table ();

-  di_set = di_set_alloc ();
-  if (!di_set)
+  di_files = di_set_alloc ();
+  if (!di_files)
     xalloc_die ();

   /* If not hashing everything, process_file won't find cycles on its
@@ -1055,7 +1053,7 @@ main (int argc, char **argv)
  argv_iter_done:

   argv_iter_free (ai);
-  di_set_free (di_set);
+  di_set_free (di_files);
   di_set_free (di_mnt);

   if (files_from && (ferror (stdin) || fclose (stdin) != 0) && ok)