On Fri, 2014-08-29 at 15:40 +0200, Fridolin Pokorny wrote: > On Wed, 2014-08-27 at 17:02 +0100, Pádraig Brady wrote: > > On 08/27/2014 04:13 PM, Fridolin Pokorny wrote: > > > diff --git a/src/df.c b/src/df.c > > > index e907b94..156c7c1 100644 > > > --- a/src/df.c > > > +++ b/src/df.c > > > @@ -622,11 +622,11 @@ filter_mount_list (bool devices_only) > > > 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. */ > > > +#ifdef _linux_ > > > + if (me->me_dev == (dev_t) -1 && stat (me->me_mountdir, &buf) == -1) > > > +#else > > > if (-1 == stat (me->me_mountdir, &buf)) > > > +#endif > > > > Do we need the ifdef at all? > > Won't me_dev be -1 or valid? > > I wasn't sure about this; I don't know why testing for me_dev == -1 > wasn't already covered. This forced to call stat() even when me_dev was > set from dev_from_mount_options() in gnulib on non-Linux systems (or > specific calls on BeOS/Ultrix). These ifdefs should be removed in my > opinion. > > If ifdefs will be removed, this patch has nothing to do with mountinfo > on Linux. It simply lets me_dev to be populated from gnulib, if any, or > it tries to find some using stat() (which might not be always correct > in chroot).
Proposing modified patch and a simple test case. Have a nice day! Fridolin Pookrny
>From 80ea8041e1389262a4b1b4eec002330ab4054352 Mon Sep 17 00:00:00 2001 From: Fridolin Pokorny <[email protected]> Date: Wed, 27 Aug 2014 15:00:14 +0200 Subject: [PATCH 1/2] df: let me_dev be populated from gnulib * src/df.c (filter_mount_list, get_dev): Avoid using stat()s when me_dev is populated from Gnulib. This will give more accurate output when using df in chroot'ed environments. Device IDs are not determined by stat() call using mountpoint, so it is not possible to make a collision when same names are used (e.g. /home/ and CHROOT-ROOT/home/). --- src/df.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/df.c b/src/df.c index e907b94..abddb88 100644 --- a/src/df.c +++ b/src/df.c @@ -622,11 +622,7 @@ filter_mount_list (bool devices_only) 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)) + if (me->me_dev == (dev_t) -1 && stat (me->me_mountdir, &buf) == -1) { /* Stat failed - add ME to be able to complain about it later. */ buf.st_dev = me->me_dev; @@ -870,13 +866,14 @@ add_to_grand_total (struct field_values_t *bv, struct field_values_t *iv) If MOUNT_POINT is non-NULL, then DISK may be NULL -- certain systems may not be able to produce statistics in this case. ME_DUMMY and ME_REMOTE are the mount entry flags. + ME_DEV is mount entry device id or -1. Caller must set PROCESS_ALL to true when iterating over all entries, as when df is invoked with no non-option argument. See below for details. */ static void get_dev (char const *disk, char const *mount_point, char const* file, char const *stat_file, char const *fstype, - bool me_dummy, bool me_remote, + bool me_dummy, bool me_remote, dev_t me_dev, const struct fs_usage *force_fsu, bool process_all) { @@ -930,9 +927,9 @@ get_dev (char const *disk, char const *mount_point, char const* file, /* Ensure we don't output incorrect stats for over-mounted directories. Discard stats when the device name doesn't match. */ struct stat sb; - if (stat (stat_file, &sb) == 0) + if (me_dev != (dev_t) -1 || stat (stat_file, &sb) == 0) { - char const * devname = devname_for_dev (sb.st_dev); + char const * devname = devname_for_dev (me_dev); if (devname && ! STREQ (devname, disk)) { fstype = "-"; @@ -1212,7 +1209,7 @@ get_disk (char const *disk) { get_dev (best_match->me_devname, best_match->me_mountdir, file, NULL, best_match->me_type, best_match->me_dummy, - best_match->me_remote, NULL, false); + best_match->me_remote, best_match->me_dev, NULL, false); return true; } else if (eclipsed_device) @@ -1307,7 +1304,7 @@ get_point (const char *point, const struct stat *statp) if (best_match) get_dev (best_match->me_devname, best_match->me_mountdir, point, point, best_match->me_type, best_match->me_dummy, best_match->me_remote, - NULL, false); + best_match->me_dev, NULL, false); else { /* We couldn't find the mount entry corresponding to POINT. Go ahead and @@ -1318,7 +1315,8 @@ get_point (const char *point, const struct stat *statp) char *mp = find_mount_point (point, statp); if (mp) { - get_dev (NULL, mp, point, NULL, NULL, false, false, NULL, false); + get_dev (NULL, mp, point, NULL, NULL, false, false, + (dev_t) -1, NULL, false); free (mp); } } @@ -1349,7 +1347,7 @@ get_all_entries (void) for (me = mount_list; me; me = me->me_next) get_dev (me->me_devname, me->me_mountdir, NULL, NULL, me->me_type, - me->me_dummy, me->me_remote, NULL, true); + me->me_dummy, me->me_remote, me->me_dev, NULL, true); } /* Add FSTYPE to the list of file system types to display. */ @@ -1699,8 +1697,8 @@ main (int argc, char **argv) { if (print_grand_total) get_dev ("total", - (field_data[SOURCE_FIELD].used ? "-" : "total"), - NULL, NULL, NULL, false, false, &grand_fsu, false); + (field_data[SOURCE_FIELD].used ? "-" : "total"), NULL, + NULL, NULL, false, false, (dev_t) -1, &grand_fsu, false); print_table (); } -- 1.9.3
>From a8700d7e24e47c366f19dbd58a186432535d6702 Mon Sep 17 00:00:00 2001 From: Fridolin Pokorny <[email protected]> Date: Tue, 2 Sep 2014 14:04:43 +0200 Subject: [PATCH 2/2] df: add a test for /proc/self/mountinfo support * tests/df/chroot.sh: Add a test case. * tests/df/no-mtab-status.sh: Skip this test when /proc/self/mountinfo is supported. * tests/local.mk (all_root_tests): Mention the test. --- tests/df/chroot.sh | 59 ++++++++++++++++++++++++++++++++++++++++++++++ tests/df/no-mtab-status.sh | 3 +++ tests/local.mk | 1 + 3 files changed, 63 insertions(+) create mode 100755 tests/df/chroot.sh diff --git a/tests/df/chroot.sh b/tests/df/chroot.sh new file mode 100755 index 0000000..ca33d5f --- /dev/null +++ b/tests/df/chroot.sh @@ -0,0 +1,59 @@ +#!/bin/sh +# make sure 'df' uses /proc/self/mountinfo if available + +# Copyright (C) 2000-2014 Free Software Foundation, Inc. + +# 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=.}/tests/init.sh" +print_ver_ df +require_root_ + +cleanup_() { + umount lib + [ -d lib64 ] && umount lib64 +} + +grep '^#define MOUNTED_PROC_MOUNTINFO 1' $CONFIG_HEADER > /dev/null \ + || skip_ "/proc/self/mountinfo is not supported" + +mkdir lib || framework_failure_ +mount --bind /lib lib || framework_failure_ +[ -d /lib64 ] && { + mkdir lib64 || framework_failure_ + mount --bind /lib64 lib64 || framework_failure_ +} + +# place binary into chroot +cp "$abs_top_builddir/src/df" df || framework_failure_ + +# nothing to read +chroot . ./df -a && fail=1 + +# df should always use /proc/self/mountinfo, there's no /etc/mtab +mkdir -p proc/self/ || framework_failure_ +cat <<\EOF > proc/self/mountinfo || framework_failure_ +36 35 98:0 /mnt1 /mnt2 rw,noatime master:1 - ext3 /dev/root rw,errors=continue +EOF +chroot . ./df -a || fail=1 + +# fallback - /etc/mtab +rm -f proc/self/mountinfo +mkdir etc/ +cat <<\EOF > etc/mtab || framework_failure_ +/ /mnt ext3 rw,noatime 0 0 +EOF +chroot . ./df -a || fail=1 + +Exit $fail diff --git a/tests/df/no-mtab-status.sh b/tests/df/no-mtab-status.sh index 9ea2036..fcf589d 100755 --- a/tests/df/no-mtab-status.sh +++ b/tests/df/no-mtab-status.sh @@ -29,6 +29,9 @@ grep '^#define HAVE_MNTENT_H 1' $CONFIG_HEADER > /dev/null \ grep '^#define HAVE_GETMNTENT 1' $CONFIG_HEADER > /dev/null \ || skip_ "getmntent is not used on this system" +grep '^#define MOUNTED_PROC_MOUNTINFO 1' $CONFIG_HEADER > /dev/null \ + && skip_ "configured to use /proc/self/mountinfo" + # Simulate "mtab" failure. cat > k.c <<'EOF' || framework_failure_ #include <stdio.h> diff --git a/tests/local.mk b/tests/local.mk index e0f1f84..5c2d956 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -116,6 +116,7 @@ all_root_tests = \ tests/dd/skip-seek-past-dev.sh \ tests/df/problematic-chars.sh \ tests/df/over-mount-device.sh \ + tests/df/chroot.sh \ tests/du/bind-mount-dir-cycle.sh \ tests/id/setgid.sh \ tests/install/install-C-root.sh \ -- 1.9.3
