On 01/24/2013 05:42 PM, Ondrej Oprala wrote:
The patch addresses df not showing the root filesystem line (
https://bugzilla.redhat.com/show_bug.cgi?id=887763 ). It's based on
Pádraig Brady's idea of favoring certain patterns
when deduplicating the list of mount entries
( http://lists.gnu.org/archive/html/coreutils/2013-01/msg00080.html ).
Hi Ondrej,
thanks for working on this.
BTW: did you see my comment on that bug?
https://bugzilla.redhat.com/show_bug.cgi?id=887763#c12
I consider this issue to be a bug in the user's environment or
even the kernel:
Another mount point is probably shadowed and therefore has the
same device number as that of the "/" mount point. And because
it is coming earlier in /etc/mtab, the "/" entry is skipped.
Or that file system has been umounted without removing the
entry from /etc/mtab (well, more unlikely).
Did someone analyse this? (I cannot reproduce it here.)
Re. your patch:
The implementation has some flaws:
src/df.c: In function 'dev_approved':
src/df.c:613:1: error: function might be candidate for attribute 'pure' if it
is known to return normally
[-Werror=suggest-attribute=pure]
+static int
+dev_examine (struct mount_entry *me)
This function should return void.
{
struct stat buf;
- if (-1 == stat (mount_dir, &buf))
- return false;
+ if (-1 == stat (me->me_mountdir, &buf))
+ return -1;
This introduces a severe bug, because df won't neither
report un-stat()able file systems nor will it exit(EXIT_FAILURE)
to indicate the failure.
Test case: mount something on /root/mnt which a normal user
cannot access, and then run "df" or "df -a" as a such a normal user.
struct devlist *devlist = devlist_head;
for ( ; devlist; devlist = devlist->next)
if (devlist->dev_num == buf.st_dev)
- return true;
+ {
+ if ((!strchr (devlist->me->me_devname, '/')
+ && strchr (me->me_devname, '/'))
+ || (strchr (devlist->me->me_devname, '/')
+ && strchr (me->me_devname, '/')
+ && strlen (devlist->me->me_mountdir) > strlen (me->me_mountdir)))
+ devlist->me = me;
+
+ return 0;
+ }
Looping thru the devlist can be avoided completely if me->me_devname
does not contain a '/'. Re-arranging the conditions may help.
@@ -803,8 +824,6 @@ get_dev (char const *disk, char const *mount_point,
/* No arguments nor "df -a", then check if df has to ... */
if (!show_rootfs && STREQ (disk, ROOTFS))
return; /* ... skip rootfs: (unless -trootfs is given. */
- if (dev_examined (mount_point, disk))
- return; /* ... skip duplicate entries (bind mounts). */
}
Also the first condition can be removed here from get_dev.
@@ -1133,9 +1152,19 @@ get_all_entries (void)
{
struct mount_entry *me;
+ if (!show_rootfs && !show_all_fs && !show_listed_fs)
show_listed_fs can never be true in that function.
Another usability issue:
For "df -t rootfs -t ext4", the duplicate ext4 bind mounts
are not suppressed any longer, because dev_examine() does not
run in this case. I think such duplicities should only be shown
for "du -a".
In the end, I started to slightly dislike our approach
of that devlist thing being spread over several places
(including the IF_LINTED loop to free it).
Then I played with the code a bit, too, and found a quite
central solution: a new function filter_mount_list() cares
about it all, and is just called by get_all_entries().
Sorry if I am steamrolling you by the review results and
proposing a new patch in the same moment - I wanted this
issue to just move faster.
Have a nice day,
Berny