coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] df (without any params) omits the root filesystem line


From: Ondrej Oprala
Subject: Re: [PATCH] df (without any params) omits the root filesystem line
Date: Sat, 26 Jan 2013 11:12:09 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0

On 01/25/2013 01:28 AM, Bernhard Voelker wrote:
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
Hi,

thanks for looking at the patch.
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.
No worries, your approach looks good.

Thanks,
Ondrej



reply via email to

[Prev in Thread] Current Thread [Next in Thread]