On 08/16/2012 08:43 AM, Bernhard Voelker wrote:
On 08/15/2012 05:41 PM, Ondrej Oprala wrote:
Hi,
I've written a little fix for this bugzilla
https://bugzilla.redhat.com/show_bug.cgi?id=709351
which I think could solve the duplicity output problem. The idea is that
df would keep a linked
list of device numbers it already went through and not count in
(get_dev) those filesystems again
when running into them as bind mounts.
Cheers,
Ondrej.
Thanks for working on that.
+/* Global device number linked list. */
+static struct devlist *devlist;
+
+/* Help pointer to devlist's first entry. */
+static struct devlist *devlist_head;
+
I think we don't need both as static global variable.
The former is only used as a loop variable, so I'd define
it where it's needed.
+/* Add a device number of the soon-to-be examined
+ filesystem to the global list devlist. */
+
+static void
+devlist_add (dev_t dev_num)
+{
+ devlist->dev_num = dev_num;
+ devlist->dev_next = xmalloc (sizeof *devlist);
+ devlist = devlist->dev_next;
+ devlist->dev_num = 0;
+ devlist->dev_next = NULL;
+ devlist = devlist_head;
+}
Together with the new code in main, you allocate one item
more than what is needed. If the loop in dev_examinded()
would run "while (devlist)", then this wouldn't be necessary.
+/* Check if the device was already examined. */
+
+static bool
+dev_examined (char *mount_dir)
+{
+ dev_t dev_num;
+ struct stat *buf = alloca (sizeof *buf);
Isn't the use of alloca discouraged? A simple "struct stat buf"
would be sufficient.
+ stat (mount_dir, buf);
+
+ dev_num = buf->st_dev;
+ while (devlist->dev_num)
+ {
+ if (devlist->dev_num == dev_num)
+ {
+ devlist = devlist_head;
+ return true;
+ }
+ devlist = devlist->dev_next;
+ }
+
+ devlist_add (dev_num);
+ return false;
+}
...
@@ -830,8 +882,11 @@ get_all_entries (void)
struct mount_entry *me;
for (me = mount_list; me; me = me->me_next)
- get_dev (me->me_devname, me->me_mountdir, NULL, me->me_type,
- me->me_dummy, me->me_remote, NULL, true);
+ if (!dev_examined (me->me_mountdir))
+ {
+ get_dev (me->me_devname, me->me_mountdir, NULL, me->me_type,
+ me->me_dummy, me->me_remote, NULL, true);
+ }
}
...
@@ -1132,7 +1187,21 @@ main (int argc, char **argv)
get_entry (argv[i], &stats[i - optind]);
}
else
- get_all_entries ();
+ {
+ devlist = xmalloc (sizeof *devlist);
+ devlist->dev_num = 0;
+ devlist->dev_next = NULL;
+ devlist_head = devlist;
See my comment above: allocating one more than actually needed.
+
+ get_all_entries ();
+
+ while (devlist_head)
+ {
+ devlist = devlist_head->dev_next;
+ free (devlist_head);
+ devlist_head = devlist;
+ }
+ }
if (print_grand_total && file_systems_processed)
{
There's no test, although I personally think it's hard to
write a test ... well, maybe by faking /etc/mtab, i.e. by
intercepting getmntent() in another LD_PRELOAD'ed test? ;-)
The practical result is good, although - as a user - I'd expect
that the "rootfs" line would disappear instead of the line showing
the device which is mounted on '/'. E.g. on my PC, I now see
rootfs 12G 7.2G 3.9G 66% /
instead of
/dev/sda1 12G 7.2G 3.9G 66% /
Even the new findmnt command of util-linux shows the device:
$ ~/ul/findmnt --df
SOURCE FSTYPE SIZE USED AVAIL USE% TARGET
...
/dev/sda1 ext4 11.5G 7.1G 4.4G 62% /
...
Have a nice day,
Berny
I've amended my previous commit to include a test faking an mtab file.
It's very similar to the no-mtab-status test and expands on the getmntent
function replacement by returning an identical made-up structure in the
first 2 calls
of the function, with '/' as the mount dir. Since the df command still stats
the '/', I couldn't put in hard values as expected output.The test then
checks
the number of df's lines on output (should be header
+ 1 line of output based on mtab).
I hope it's satisfactory.
Cheers,
Ondrej.
From ff993b6e8367b424e2bf38e3c3c1d0d787f4489c Mon Sep 17 00:00:00 2001
From: Ondrej Oprala <[email protected]>
Date: Wed, 15 Aug 2012 17:30:40 +0200
Subject: [PATCH] df: Fix outputting filesystem duplicities caused by
bind-mounts
* NEWS: Mention the fix.
* src/df.c (devlist_add): Add another entry to the global
device number list.
(dev_examined): Check if a filesystem has been examined already.
* tests/df/df-dup-remove.sh: Add a new test simulating mtab entries.
* tests/local.mk: Add the test to the list.
---
NEWS | 3 ++
src/df.c | 70 +++++++++++++++++++++++++++++++++++++++++++++--
tests/df/df-dup-remove.sh | 69 ++++++++++++++++++++++++++++++++++++++++++++++
tests/local.mk | 1 +
4 files changed, 141 insertions(+), 2 deletions(-)
create mode 100755 tests/df/df-dup-remove.sh
diff --git a/NEWS b/NEWS
index f63df92..083c3ed 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,9 @@ GNU coreutils NEWS -*-
outline -*-
** Bug fixes
+ df now properly outputs filesystem information with bind mounts
+ present on the system.
+
du no longer emits a "disk-corrupted"-style diagnostic when it detects
a directory cycle that is due to a bind-mounted directory. Instead,
it detects this precise type of cycle, diagnoses it as such and
diff --git a/src/df.c b/src/df.c
index 83fef77..a885e01 100644
--- a/src/df.c
+++ b/src/df.c
@@ -43,6 +43,17 @@
proper_name ("David MacKenzie"), \
proper_name ("Paul Eggert")
+/* Filled with device numbers of examined filesystems to avoid
+ duplicities in output. */
+struct devlist
+{
+ dev_t dev_num;
+ struct devlist *dev_next;
+};
+
+/* Device list help pointer. */
+struct devlist *devlist_head;
+
/* If true, show inode information. */
static bool inode_format;
@@ -823,6 +834,48 @@ get_entry (char const *name, struct stat const *statp)
get_point (name, statp);
}
+/* Add a device number of the soon-to-be examined
+ filesystem to the global list devlist. */
+
+static void
+devlist_add (dev_t dev_num)
+{
+ struct devlist *devlist = devlist_head;
+ while (devlist->dev_next)
+ devlist = devlist->dev_next;
+
+ if (!devlist->dev_num)
+ devlist->dev_num = dev_num;
+ else
+ {
+ devlist->dev_next = xmalloc (sizeof *devlist);
+ devlist = devlist->dev_next;
+ devlist->dev_num = dev_num;
+ devlist->dev_next = NULL;
+ }
+}
+
+/* Check if the device was already examined. */
+
+static bool
+dev_examined (struct devlist *devlist, char *mount_dir)
+{
+ dev_t dev_num;
+ struct stat buf;
+ stat (mount_dir, &buf);
+
+ dev_num = buf.st_dev;
+ while (devlist)
+ {
+ if (devlist->dev_num == dev_num)
+ return true;
+ devlist = devlist->dev_next;
+ }
+
+ devlist_add (dev_num);
+ return false;
+}
+
/* Show all mounted file systems, except perhaps those that are of
an unselected type or are empty. */
@@ -830,10 +883,23 @@ static void
get_all_entries (void)
{
struct mount_entry *me;
+ struct devlist *devlist = xmalloc (sizeof *devlist);
+ devlist_head = devlist;
+
+ devlist->dev_num = 0;
+ devlist->dev_next = NULL;
for (me = mount_list; me; me = me->me_next)
- get_dev (me->me_devname, me->me_mountdir, NULL, me->me_type,
- me->me_dummy, me->me_remote, NULL, true);
+ if (!dev_examined (devlist, me->me_mountdir))
+ get_dev (me->me_devname, me->me_mountdir, NULL, me->me_type,
+ me->me_dummy, me->me_remote, NULL, true);
+
+ while (devlist)
+ {
+ devlist = devlist->dev_next;
+ free (devlist_head);
+ devlist_head = devlist;
+ }
}
/* Add FSTYPE to the list of file system types to display. */
diff --git a/tests/df/df-dup-remove.sh b/tests/df/df-dup-remove.sh
new file mode 100755
index 0000000..d097d4a
--- /dev/null
+++ b/tests/df/df-dup-remove.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+# Test df's behaviour when the mount list contains duplicit entries.
+# This test is skipped on systems that lack LD_PRELOAD support; that's fine.
+
+# Copyright (C) 2012 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"; path_prepend_ ./src
+print_ver_ df
+
+df || skip_ "df fails"
+
+# Simulate "mtab" failure.
+cat > k.c <<'EOF' || framework_failure_
+#include <stdio.h>
+#include <mntent.h>
+
+struct mntent *getmntent (FILE *fp)
+{
+ /* Prove that LD_PRELOAD works. */
+ static int done = 0;
+ if (!done)
+ {
+ fclose (fopen ("x", "w"));
+ ++done;
+ }
+ /* Now simulate an mtab file with
+ two entries. */
+
+ static struct mntent mntent;
+
+ while (done++ < 3)
+ {
+ mntent.mnt_fsname = "fsname";
+ mntent.mnt_dir = "/";
+ mntent.mnt_type = "-";
+
+ return &mntent;
+ }
+ return NULL;
+}
+EOF
+
+# Then compile/link it:
+gcc --std=gnu99 -shared -fPIC -ldl -O2 k.c -o k.so \
+ || skip_ "getmntent hack does not work on this platform"
+
+# Test if LD_PRELOAD works:
+LD_PRELOAD=./k.so df
+test -f x || skip_ "internal test failure: maybe LD_PRELOAD doesn't work?"
+
+#The fake mtab file should only contain 2 entries, both
+#having the same device number; thus the output should
+#consist of a header and one entry
+test $(LD_PRELOAD=./k.so df | wc -l) -eq 2 || fail=1
+
+Exit $fail
diff --git a/tests/local.mk b/tests/local.mk
index 2440667..6ae9576 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -453,6 +453,7 @@ all_tests = \
tests/df/unreadable.sh \
tests/df/total-unprocessed.sh \
tests/df/no-mtab-status.sh \
+ tests/df/df-dup-remove.sh \
tests/dd/direct.sh \
tests/dd/misc.sh \
tests/dd/nocache.sh \
--
1.7.11.4
