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

[Bernhard Voelker]

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

df-deduplicating.patch
Description: Text Data