[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#16539: More details on df command output for you
From: |
Bernhard Voelker |
Subject: |
bug#16539: More details on df command output for you |
Date: |
Wed, 18 Jun 2014 03:19:04 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 |
On 06/17/2014 06:03 PM, Pádraig Brady wrote:
> From 0a4b8027049f6746a237c9fc34a0e0a4afdcfc62 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?P=C3=A1draig=20Brady?= <address@hidden>
> 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.
Cool: finally GNU df fulfills the first sentence of the POSIX spec ;-)
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/df.html
The df utility shall write the amount of available space ... for
file systems on which the invoking user has appropriate read access.
I think we already discussed whether a warning would be better than
a line with "-"s (or even silently omit the line) - and seeing the
result is pleasing:
Before:
/usr/bin/df: ‘/root/backup’: Permission denied
After:
/dev/sda3 - - - - /root/backup
> * src/df.c (device_list): A new global list updated from ...
... from what?
> (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
s/plaeholder/placeholder/
> can't access a system specified mount point.
> (devname_for_dev): A new function to search the mount <-> dev mapping.
get_all_entries was also changed.
> * test/df/skip-duplicates.sh: Adjust accordingly.
> * NEWS: Mention the bug fixes.
>
> Discussed at: http://bugs.gnu.org/16539
> 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;
Maybe I'm old-fashioned, but I'd have separated the struct definition
from the variable definition ... well, personal style.
> @@ -610,20 +610,21 @@ excluded_fstype (const char *fstype)
> me_mountdir wins. */
>
> static void
> -filter_mount_list (void)
> +filter_mount_list (bool devices_only)
The parameter name isn't very intuitive (to me).
Would it harm to add a sentence in the function description?
> @@ -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;
> + }
Is it possible that errno is leaking from one get_dev() invocation
to another, i.e. that one failure eclipses another?
> + }
> + 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);
Shorter?
+ filter_mount_list (show_all_fs);
> 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"
my Thunderbird suggests: s/overmounted/over-mounted/
> +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
Great, we're getting closer!
Finally, I have an edge case on my PC which is not yet covered:
I have a backup partition mounted on /root/backup (which is not
accessible by a normal user), and a read-only bind-mount of it:
$ mount /dev/sda1 /root/backup
$ mount --bind /root/backup /media/backup
$ mount -o ro,remount,bind /media/backup
(The 3rd command is just there for setting the read-only flag).
df-8.21 perfectly skipped the inaccessible entry and went ahead
with the read-only copy
$ /usr/bin/df /dev/sda3
Filesystem 1K-blocks Used Available Use% Mounted on
/dev/sda3 155396036 124425132 23054156 85% /media/backup
while the latest one from git plus these 2 patches is falling over:
$ src/df /dev/sda3
src/df: ‘/root/backup’: Permission denied
This is a little regression.
Thus said, I'm not 100% happy with the filtering yet.
I don't have an idea right now how to solve it. Maybe in a
couple of days ... I have to think about it.
Thanks again & have a nice day,
Berny
- bug#16539: More details on df command output for you, Pádraig Brady, 2014/06/17
- bug#16539: More details on df command output for you,
Bernhard Voelker <=
- bug#16539: More details on df command output for you, Pádraig Brady, 2014/06/18
- bug#16539: More details on df command output for you, Pádraig Brady, 2014/06/18
- bug#16539: More details on df command output for you, Bernhard Voelker, 2014/06/19
- bug#16539: More details on df command output for you, Pádraig Brady, 2014/06/24
- bug#16539: More details on df command output for you, Bernhard Voelker, 2014/06/25
- bug#16539: More details on df command output for you, Pádraig Brady, 2014/06/25
- bug#16539: More details on df command output for you, Bernhard Voelker, 2014/06/25