-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
duplicated fs, hiding cruft, porting issues #9
base: master
Are you sure you want to change the base?
Conversation
This allows filters that look at other items.
They were listed multiple times when bind mounts or btrfs/zfs subvolumes are in use. And in those cases, a single filesystem is often mounted many, many times. Filesystems whose backing store is not an absolute path are assumed to be unique. This is not always correct (they can be bind-mounted). In case of duplicates, the mount with a shorter path is considered more important.
On modern Debian-based systems there's usually 3 of them, on Fedora 23 4. I'm not sure what's the good place to put this code in, as you prefer to separate logic from presentation. It doesn't fit into is_mnt_ignore() as it's used by some distributions at least on linux, freebsd and hurd and that function is platform-specific.
It obeyed only is_mnt_ignore().
So does regular df.
This reuses the "linux" platform, as everything you use there is glibc rather than linux specific.
Like Hurd, this uses "linux" despite the real platform being FreeBSD, as it's libc that matters.
A kfreebsd, and I guess sometimes real FreeBSD, thingy.
Hi @kilobyte, Thanks for the patches, I will take a look at them. Cheers |
|
||
#include <stdio.h> | ||
#include <stdlib.h> | ||
#include <string.h> | ||
#include <unistd.h> | ||
|
||
#if defined(__linux__) || defined(__GLIBC__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to move the check down? I'd like it to be at the very top because this file is linux/glibc specific. But I'm being picky, I'll move it back at the top after merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The__GLIBC__
define comes from one of these headers, not from gcc, so the order does matter. I'm not sure what happens on Linux libcs other than glibc, like musl or dietlibc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll fire up a VM with Alpine Linux and see how it behaves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it builds fine in Alpine Linux which uses musl. I'll leave it as is then.
I went through your changes. I'm generally very OK with your changes, thanks a lot they will go in! 😃 |
Ok, I tested a bit further and I think it is filtering a bit too much now:
Without your changes, I get this:
And this is actually the same result as
I think we don't want to filter out more than |
I don't understand the concern about multiple loops. Is there any reason to stuff more logic into a single iteration instead of multiple, more readable loops? If it's about efficiency, then the cost is a few clock cycles in a loop with no more than a hundred or so iterations (on a really busy server with that many mounts), and because of cache locality it might be even faster... |
Elimination of /run mounts is intentional, as they're not supposed to have any non-trivial files in it, and thus are not interesting to the user. There's often many of them -- 2 on your system but at least 3 or 4 on Debian. Only reason to show them, IMHO, is when because of some error they're getting full anyway, that's why I special-cased them to show up when more than 50% filled. |
I think it'd be nice to apply that /run treatment to devtmpfs mounts as well, with the same logic -- they're both filesystems whose capability to store real files is only an implementation details. Thus, dfc should show them only if there's something unusual, ie, they're getting dangerously full. |
It would be really silly if about efficiency. No really, that's absolutely not the point. It's about clarity. It is more like the first two loops kinda do similar things, right?
On the other hand, this would mean that |
Another case where dfc is more spammy than df. First, the base state on one machine with this pull request applied:
udev shouldn't be shown, but so far, good. But let's run:
-- df doesn't show those, dfc does. Without my patches, it'd show copies of all the real filesystems, so at least that's gone -- but we still have all those duplicated udev and tmpfs bind-mounts that df filtered away. |
OK, I think it makes sense not to display certain file systems as you suggest. However, I feel like the behavior of automagically displaying a file system if its usage goes above 50% is counter intuitive. I would rather it being always or never displayed. Maybe an option could be added to hide file systems with usage below a certain threshold but I'm not even fond of this idea either. |
Here's a grab-bag of misc fixes and additions. I placed them in a single pull request as I prefer to cherry-pick over using GitHub's GUI, but can separate them if you wish. Apologies if I didn't notice something being already available another way -- I've learned about dfc just a few hours ago...
The commits are mostly independent, although some depend on fcd1920.
The changes are: