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
>From 3286aad80104f8443564f99df2ae6318c9f1134f Mon Sep 17 00:00:00 2001 From: Ondrej Oprala <[email protected]> Date: Fri, 25 Jan 2013 01:07:58 +0100 Subject: [PATCH] df: prefer fullpath entries when deduplicating * src/df.c (struct devlist): Add a new element for storing pointers to mount_entry structures. (devlist_head, dev_examined): Remove. (filter_mount_list): Add new function to filter out the rootfs entry (unless -trootfs is specified), and duplicities. The function favors entries with a '/' character in me_devname or those with the shortest me_mountdir string, if multiple entries fulfill the first condition. Use struct devlist to build up a list of entries already known, and finally rebuild the global mount_list. (get_all_entries): Call the above new function unless the -a option is specified. (get_dev): Remove the code for skipping rootfs and duplicities. * tests/df/skip-duplicates.sh: Add test cases. Co-authored-by: Bernhard Voelker <[email protected]> --- src/df.c | 116 +++++++++++++++++++++++++++++------------- tests/df/skip-duplicates.sh | 18 +++++-- 2 files changed, 94 insertions(+), 40 deletions(-) diff --git a/src/df.c b/src/df.c index 9523cc1..8711c3a 100644 --- a/src/df.c +++ b/src/df.c @@ -48,12 +48,10 @@ struct devlist { dev_t dev_num; + struct mount_entry *me; struct devlist *next; }; -/* Store of already-processed device numbers. */ -static struct devlist *devlist_head; - /* If true, show even file systems with zero size or uninteresting types. */ static bool show_all_fs; @@ -606,27 +604,87 @@ excluded_fstype (const char *fstype) return false; } -/* Check if the device was already examined. */ +/* Filter mount list by skipping duplicate entries. + In the case of duplicities - based on to the device number - the mount entry + with a '/' in its me_devname (i.e. not pseudo name like tmpfs) wins. + If both have a real devname (e.g. bind mounts), then that with the shorter + me_mountdir wins. + Finally, do not filter out a rootfs entry if -trootfs is specified. */ -static bool -dev_examined (char const *mount_dir, char const *devname) +static void +filter_mount_list (void) { - struct stat buf; - if (-1 == stat (mount_dir, &buf)) - return false; + struct mount_entry *me; - struct devlist *devlist = devlist_head; - for ( ; devlist; devlist = devlist->next) - if (devlist->dev_num == buf.st_dev) - return true; + /* Store of already-processed device numbers. */ + struct devlist *devlist_head = NULL; - /* Add the device number to the global list devlist. */ - devlist = xmalloc (sizeof *devlist); - devlist->dev_num = buf.st_dev; - devlist->next = devlist_head; - devlist_head = devlist; + /* Sort all 'wanted' entries into the list devlist_head. */ + for (me = mount_list; me; me = me->me_next) + { + struct stat buf; + struct devlist *devlist; - return false; + if (-1 == stat (me->me_mountdir, &buf)) + { + ; /* Stat failed - add ME to be able to complain about it later. */ + } + else + if (show_rootfs + && ( STREQ (me->me_mountdir, "/") + || STREQ (me->me_type, ROOTFS))) + { + /* Df should show rootfs (due to -trootfs). + Add this ME both if it is the rootfs entry itself or "/" + (as that must not replace the rootfs entry in the devlist). */ + ; + } + else + { + /* If the device name is a real path name ... */ + if (strchr (me->me_devname, '/')) + { + /* ... try to find its device number in the devlist. */ + for (devlist = devlist_head; devlist; devlist = devlist->next) + if (devlist->dev_num == buf.st_dev) + break; + + if (devlist) + { + /* Let the shorter mountdir win. */ + if ( !strchr (devlist->me->me_devname, '/') + || ( strlen (devlist->me->me_mountdir) + > strlen (me->me_mountdir))) + { + /* FIXME: free ME - the others are also not free()d. */ + devlist->me = me; + } + continue; /* ... with the loop over the mount_list. */ + } + } + } + + /* Add the device number to the global list devlist. */ + devlist = xmalloc (sizeof devlist); + devlist->me = me; + devlist->dev_num = buf.st_dev; + devlist->next = devlist_head; + devlist_head = devlist; + } + + /* Finally rebuild the mount_list from the devlist. */ + mount_list = NULL; + while (devlist_head) + { + /* Add the mount entry. */ + me = devlist_head->me; + me->me_next = mount_list; + mount_list = me; + /* Free devlist entry and advance. */ + struct devlist *devlist = devlist_head->next; + free (devlist_head); + devlist_head = devlist; + } } /* Return true if N is a known integer value. On many file systems, @@ -798,15 +856,6 @@ get_dev (char const *disk, char const *mount_point, if (!selected_fstype (fstype) || excluded_fstype (fstype)) return; - if (process_all && !show_all_fs && !show_listed_fs) - { - /* 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). */ - } - /* If MOUNT_POINT is NULL, then the file system is not mounted, and this program reports on the file system that the special file is on. It would be better to report on the unmounted file system, @@ -1133,6 +1182,9 @@ get_all_entries (void) { struct mount_entry *me; + if (!show_all_fs) + filter_mount_list (); + 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); @@ -1506,14 +1558,6 @@ main (int argc, char **argv) } IF_LINT (free (columns)); - IF_LINT ( - while (devlist_head) - { - struct devlist *devlist = devlist_head->next; - free (devlist_head); - devlist_head = devlist; - } - ); exit (exit_status); } diff --git a/tests/df/skip-duplicates.sh b/tests/df/skip-duplicates.sh index 31ab014..20f3910 100755 --- a/tests/df/skip-duplicates.sh +++ b/tests/df/skip-duplicates.sh @@ -39,10 +39,15 @@ struct mntent *getmntent (FILE *fp) static struct mntent mntent; - while (done++ < 3) + while (done++ < 4) { - mntent.mnt_fsname = "fsname"; - mntent.mnt_dir = "/"; + /* File system - Mounted on + fsname / + /fsname /root + /fsname / + */ + mntent.mnt_fsname = (done == 2) ? "fsname" : "/fsname"; + mntent.mnt_dir = (done == 3) ? "/root" : "/"; mntent.mnt_type = "-"; return &mntent; @@ -65,9 +70,14 @@ test -f x || skip_ "internal test failure: maybe LD_PRELOAD doesn't work?" LD_PRELOAD=./k.so df >out || fail=1 test $(wc -l <out) -eq 2 || { fail=1; cat out; } +# df should also prefer "/fsname" over "fsname" +test $(grep -c '/fsname' <out) -eq 1 || { fail=1; cat out; } +# ... and "/fsname" with '/' as Mounted on over '/root' +test $(grep -c '/root' <out) -eq 0 || { fail=1; cat out; } + # Ensure that filtering duplicates does not affect -a processing. LD_PRELOAD=./k.so df -a >out || fail=1 -test $(wc -l <out) -eq 3 || { fail=1; cat out; } +test $(wc -l <out) -eq 4 || { fail=1; cat out; } # Ensure that filtering duplcates does not affect # argument processing (now without the fake getmntent()). -- 1.7.7
