On 05/13/2014 11:20 PM, Bernhard Voelker wrote: > On 05/12/2014 05:10 PM, Pádraig Brady wrote: >> I've attached 4 patches for df to: >> >> df: also deduplicate virtual file systems >> df: fix handling of symlinks in mount list >> df: ignore non file system entries in /proc/mounts >> maint: avoid clang -Wtautological-constant-out-of-range-compare warning > > They are look good to me - great work, thanks! > AFAIR, the last one was also warned about by Coverity (but > I can't check now as their website is down for maintenance). > >> Not included is your "overmount change" or the "d)" adjustment above, >> as I was unsure how you wanted to handle exactly. > > To recap, this was the problem: > > $ mount /dev/sda5 /tmp/mnt > $ mount -o loop tmp.img /tmp/mnt > > df - even including your 4 patches - shows wrong results: > > $ src/df | grep mnt > /dev/sda5 59365 1308 53471 3% /tmp/mnt > > $ src/df -a | grep mnt > /dev/sda5 59365 1308 53471 3% /tmp/mnt > /dev/loop0 59365 1308 53471 3% /tmp/mnt > > The idea was to trust the order of /proc/mounts > > $ tail -n2 /proc/mounts > /dev/sda5 /tmp/mnt ext2 rw,relatime 0 0 > /dev/loop0 /tmp/mnt ext3 rw,relatime,data=ordered 0 0 > > i.e., loop through the mount list leaving only the > last unique (maybe canonicalized) mount point. > > Another special case is when a mount point is not reachable > at all anymore: > > /dev/sda5 /tmp/mnt/some/subdir ext2 rw,relatime 0 0 > /dev/loop0 /tmp/mnt ext3 rw,relatime,data=ordered 0 0 > > I recommend discarding eclipsed mounts in normal output, > and output "-" values with -a.
The attached (on top of df-fix-last-device.diff) should put "-" placeholder values in the needed cases. That should be it for df fixes for coreutils-8.23. cheers, Pádraig.
>From 0a4b8027049f6746a237c9fc34a0e0a4afdcfc62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]> Date: Wed, 4 Jun 2014 00:09:11 +0100 Subject: [PATCH] df: output placeholder values for inaccessible mount points A system provided mount entry may be unavailable due to TOCTOU race, or if another device has been overmounted at that position, or due to access permissions. In all these cases output "-" placeholder values rather than either producing an error, or in the overmount case outputting values for the wrong device. * src/df.c (device_list): A new global list updated from ... (filter_mount_list): Adjust to take a parameter as to whether update the global mount list, or only the mount <-> device ID mapping. (get_dev): Use the device ID mapping to ensure we're not outputting stats for the wrong device. Also output plaeholder values when we can't access a system specified mount point. (devname_for_dev): A new function to search the mount <-> dev mapping. * test/df/skip-duplicates.sh: Adjust accordingly. * NEWS: Mention the bug fixes. Discussed at: http://bugs.gnu.org/16539 --- NEWS | 6 ++- src/df.c | 106 ++++++++++++++++++++++++++++++++----------- tests/df/skip-duplicates.sh | 9 +++- 3 files changed, 89 insertions(+), 32 deletions(-) diff --git a/NEWS b/NEWS index 5d1fe99..7ba145e 100644 --- a/NEWS +++ b/NEWS @@ -44,8 +44,10 @@ GNU coreutils NEWS -*- outline -*- [These dd bugs were present in "the beginning".] - df now elides duplicates for virtual file systems like tmpfs, and will - display the correct device name for directories mounted multiple times. + df now elides duplicates for virtual file systems like tmpfs. + Displays the correct device details for points mounted multiple times. + Displays placeholder values for inaccessible file systems, + rather than error messages or values for the wrong file system. [These bugs were present in "the beginning".] head --bytes=-N and --lines=-N now handles devices more diff --git a/src/df.c b/src/df.c index 10047ce..c7da208 100644 --- a/src/df.c +++ b/src/df.c @@ -45,12 +45,12 @@ /* Filled with device numbers of examined file systems to avoid duplicities in output. */ -struct devlist +static struct devlist { dev_t dev_num; struct mount_entry *me; struct devlist *next; -}; +} *device_list; /* If true, show even file systems with zero size or uninteresting types. */ @@ -610,20 +610,21 @@ excluded_fstype (const char *fstype) me_mountdir wins. */ static void -filter_mount_list (void) +filter_mount_list (bool devices_only) { struct mount_entry *me; - /* Store of already-processed device numbers. */ - struct devlist *devlist_head = NULL; - - /* Sort all 'wanted' entries into the list devlist_head. */ + /* Sort all 'wanted' entries into the list device_list. */ for (me = mount_list; me;) { struct stat buf; struct devlist *devlist; struct mount_entry *discard_me = NULL; + /* TODO: On Linux we might avoid this stat() and another in get_dev() + by using the device IDs available from /proc/self/mountinfo. + read_file_system_list() could populate me_dev from those + for efficiency and accuracy. */ if (-1 == stat (me->me_mountdir, &buf)) { /* Stat failed - add ME to be able to complain about it later. */ @@ -632,7 +633,7 @@ filter_mount_list (void) else { /* If we've already seen this device... */ - for (devlist = devlist_head; devlist; devlist = devlist->next) + for (devlist = device_list; devlist; devlist = devlist->next) if (devlist->dev_num == buf.st_dev) break; @@ -661,8 +662,9 @@ filter_mount_list (void) if (discard_me) { - me = me->me_next; - free_mount_entry (discard_me); + me = me->me_next; + if (! devices_only) + free_mount_entry (discard_me); } else { @@ -670,26 +672,46 @@ filter_mount_list (void) devlist = xmalloc (sizeof *devlist); devlist->me = me; devlist->dev_num = buf.st_dev; - devlist->next = devlist_head; - devlist_head = devlist; + devlist->next = device_list; + device_list = devlist; me = me->me_next; } } /* Finally rebuild the mount_list from the devlist. */ - mount_list = NULL; - while (devlist_head) + if (! devices_only) { + mount_list = NULL; + while (device_list) + { + /* Add the mount entry. */ + me = device_list->me; + me->me_next = mount_list; + mount_list = me; + /* Free devlist entry and advance. */ + struct devlist *devlist = device_list->next; + free (device_list); + device_list = devlist; + } + } +} + +/* Search a mount entry list for device id DEV. + Return the corresponding device name if found or NULL if not. */ + +static char const * _GL_ATTRIBUTE_PURE +devname_for_dev (dev_t dev) +{ + struct devlist *dl = device_list; + + while (dl) { - /* Add the mount entry. */ - me = devlist_head->me; - me->me_next = mount_list; - mount_list = me; - /* Free devlist entry and advance. */ - struct devlist *devlist = devlist_head->next; - free (devlist_head); - devlist_head = devlist; + if (dl->dev_num == dev) + return dl->me->me_devname; + dl = dl->next; } + + return NULL; } /* Return true if N is a known integer value. On many file systems, @@ -878,9 +900,37 @@ get_dev (char const *disk, char const *mount_point, char const* file, fsu = *force_fsu; else if (get_fs_usage (stat_file, disk, &fsu)) { - error (0, errno, "%s", quote (stat_file)); - exit_status = EXIT_FAILURE; - return; + /* If we can't access a system provided entry due + to it not being present (now), or due to permissions, + just output placeholder values rather than failing. */ + if (process_all && (errno == EACCES || errno == ENOENT)) + { + fstype = "-"; + fsu.fsu_blocksize = fsu.fsu_blocks = fsu.fsu_bfree = + fsu.fsu_bavail = fsu.fsu_files = fsu.fsu_ffree = UINTMAX_MAX; + } + else + { + error (0, errno, "%s", quote (stat_file)); + exit_status = EXIT_FAILURE; + return; + } + } + else if (process_all && show_all_fs) + { + /* Ensure we don't output incorrect stats for overmounted directories. + Discard stats when the device name doesn't match. */ + struct stat sb; + if (stat (stat_file, &sb) == 0) + { + char const * devname = devname_for_dev (sb.st_dev); + if (devname && ! STREQ (devname, disk)) + { + fstype = "-"; + fsu.fsu_blocksize = fsu.fsu_blocks = fsu.fsu_bfree = + fsu.fsu_bavail = fsu.fsu_files = fsu.fsu_ffree = UINTMAX_MAX; + } + } } if (fsu.fsu_blocks == 0 && !show_all_fs && !show_listed_fs) @@ -1230,8 +1280,10 @@ get_all_entries (void) { struct mount_entry *me; - if (!show_all_fs) - filter_mount_list (); + if (! show_all_fs) + filter_mount_list (false); + else + filter_mount_list (true); for (me = mount_list; me; me = me->me_next) get_dev (me->me_devname, me->me_mountdir, NULL, NULL, me->me_type, diff --git a/tests/df/skip-duplicates.sh b/tests/df/skip-duplicates.sh index a620e73..1b19149 100755 --- a/tests/df/skip-duplicates.sh +++ b/tests/df/skip-duplicates.sh @@ -96,9 +96,10 @@ test -f x || skip_ "internal test failure: maybe LD_PRELOAD doesn't work?" LD_PRELOAD=./k.so df -T >out || fail=1 test $(wc -l <out) -eq $(expr 1 + $unique_entries) || { fail=1; cat out; } -# Ensure we fail when unable to stat invalid entries -LD_PRELOAD=./k.so CU_TEST_DUPE_INVALID=1 df -T >out && fail=1 -test $(wc -l <out) -eq $(expr 1 + $unique_entries) || { fail=1; cat out; } +# Ensure we don't fail when unable to stat (currently) unavailable entries +# instead outputting placeholder information +LD_PRELOAD=./k.so CU_TEST_DUPE_INVALID=1 df -T >out || fail=1 +test $(wc -l <out) -eq $(expr 2 + $unique_entries) || { fail=1; cat out; } # df should also prefer "/fsname" over "fsname" if test "$unique_entries" = 2; then @@ -113,6 +114,8 @@ test $(grep -c 'virtfs2.*fstype2' <out) -eq 1 || { fail=1; cat out; } # Ensure that filtering duplicates does not affect -a processing. LD_PRELOAD=./k.so df -a >out || fail=1 test $(wc -l <out) -eq 6 || { fail=1; cat out; } +# Ensure placeholder "-" values used for the overmounted "virtfs" +test $(grep -c 'virtfs *-' <out) -eq 1 || { fail=1; cat out; } # Ensure that filtering duplicates does not affect # argument processing (now without the fake getmntent()). -- 1.7.7.6
