There is still one weird case left in df, albeit not related to previous issues, $ adb shell df Filesystem 1K-blocks Used Available Use% Mounted on /dev/block/dm-1 964296 961376 2920 100% / tmpfs 2004636 1060 2003576 1% /dev $ adb shell df /dev/block/dm-1 Filesystem 1K-blocks Used Available Use% Mounted on tmpfs 2004636 1060 2003576 1% /dev
This is due to the st_dev of /dev/block/dm-1 equals to that of /dev. However /dev/block/dm-1 is a special device, so df should be checking its st_rdev, not st_dev. Only when /dev/block/dm-1 is not mounted, should we check its st_dev... 😵 On Wed, Feb 17, 2021 at 4:27 AM Yi-yo Chiang <[email protected]> wrote: > > > On Wed, Feb 17, 2021 at 12:36 AM Rob Landley <[email protected]> wrote: > >> On 2/16/21 3:01 AM, Yi-yo Chiang wrote:> > I think the main purpose >> of the >> original patch is to show mounts whose >> > > stat()/statvfs() failed. >> > >> > And the current one should do that? (I don't have any mount points >> I can't stat >> > because my laptop doesn't do crazy things with linux security >> modules, maybe I >> > can put one in a directory I can't read or something to simulate >> that...) >> > >> > >> > Well the cases that I found actually don't stem from Android security >> models, >> > they're actually from the complex bind mounts and over mounts on >> Android..:( I >> > simulated the "weird cases" on my linux workstation by: >> > 1. mkdir -p a/b >> > 2. mount <tmpfs> on a/b >> > 3. mount <tmpfs> on a >> > 4. check output of df [-a] # stat a/b would fail, because (2) is over >> mounted by (3) >> >> Which would only fail if the second tmpfs didn't have a "b" subdirectory. >> If it >> did, you'd presumably get a redundant view of the other filesystem? >> >> > 5. mkdir a/b >> > 6. check output of df [-a] # stat a/b would "success", however the >> st_dev should >> > be of (3), not (2) >> > 7. umount a >> > 8. mount --bind a/b a >> > 9. etc etc >> >> Hmmm... with the debian host df I get: >> >> $ sudo mount -t tmpfs uno b/c >> $ sudo mount -t tmpfs dos b >> $ mkdir b/c >> $ df >> ... >> dos 8168980 0 8168980 0% >> /home/landley/toybox/clean2/b >> >> It edits "uno" out entirely. (Which is what my old one was doing...) >> >> And if I rmdir b/c there's... no change. They're both there in /proc >> though: >> >> uno /home/landley/toybox/clean2/b/c tmpfs rw,relatime 0 0 >> dos /home/landley/toybox/clean2/b tmpfs rw,relatime 0 0 >> >> Hmmm... >> >> Ok, I taught toybox df -a to show those, _and_ used the existing overmount >> cancel logic to hide the false duplicate when a directory for the mount >> point >> exists. >> >> Is that good enough or do you want them to show up without -a? >> > > TBH I too am a bit confused with all these testing and comparing output > back and forth, and I may have confused some commands and stuff :/ > Anyway I think the behaviour now looks good. "df" hides all > un-stat-able and synth mounts, and "df -a" shows everything in > /proc/mounts, right? > > >> > I think it does too, but haven't tested it, which is why I asked. >> > >> > >> > Just tested on an Android bench, spotted a few problems: >> > 1. show_mt is too aggressively filtering out synthetic filesystems. >> Would >> > "!mt->statvfs.f_blocks && *mt->device != '/'" be a viable heuristic to >> filter >> > out synth fs? >> >> How is / a synthetic filesystem? Shouldn't it be initramfs? I've tested >> that it >> shows initramfs before... yeah, it's still there in mkroot: >> > > It's a heuristic, I thought devices whose name looks like a path (starts > with '/') probably isn't a synthetic FS. > So, !mt->statvfs.f_blocks && *mt->device != '/' > => device has no block and doesn't start with '/', so assume it's synth-fs > Looking at your "uno&dos" example above, I feel my heuristic to be very > fragile. Perhaps a better way is > !mt->statvfs.f_blocks && !major(mt->stat.st_dev) && mt->stat.st_dev > => device has no block, major number is 0 and device is not 0:0 (not a > failed stat()), then assume it's synth-fs > It doesn't matter much anyways since we don't really care why f_blocks is > zero (synth-fs or failed stat()), as we are hiding it no matter what. > > >> 8139cp 0000:00:02.0 eth0: link up, 100Mbps, full-duplex, lpa 0x05E1 >> random: fast init done >> Type exit when done. >> # df >> Filesystem 1K-blocks Used Available Use% Mounted on >> rootfs 29892 656 29236 3% / >> dev 29892 0 29892 0% /dev >> # >> >> > 2. df still skipping un-stat()-able mounts. >> >> Try now? >> >> > 3. `df` shows unstatable mounts, however `df <unstatable path/device>` >> shows >> > something such as: >> > >> > df: '/mnt/pass_through/0/emulated': Permission denied >> > Filesystem 1K-blocks Used Available Use% Mounted on >> > df: '/mnt/pass_through/0/emulated': Permission denied >> >> Hmmm... >> >> $ ./df b/c >> df: 'b/c': No such file or directory >> Filesystem 1K-blocks Used Available Use% Mounted on >> df: 'b/c': No such file or directory >> >> Ok, I can at least change that to not give the error message _twice_. But >> otherwise, at a design level, that's... sort of right? It _isn't_ there. >> >> There's a problem where if we can't stat it, we can't tell which one it >> is out >> of /proc/mounts because we can't match the device. And you can't do an >> exact >> match on the filename because "df ." is a directory under the mount >> point. I >> would have to add plumbing to xabspath() and then do a longest match >> fileunderdir(), which assumes that the /proc/mounts entry is always a >> cannonical >> absolute path? (I _think_ that's true in current kernels but would have >> to read >> the source)... >> >> > this is probably works as intended because the path is unstatable, but >> we could >> > do better by string matching the path/device with mount_points/device in >> > /proc/mounts. >> >> We could write a bunch of new code to handle this obscure corner case, >> yes. But >> why? We still wouldn't be able to tell anything _about_ the filesystem we >> can't >> access, and can only indirectly detect its _existence_. >> >> > I was thinking of a case, where stat(/dev/block/blah) failed but there is > a mounted entry of /dev/block/blah in /proc/mounts, then we should still > show the /proc/mounts line because the device name is an exact match. > But yes I agree with you now, this is still a flawed case because /dev & > /dev/block/ might be overmounted... We can never be sure what failed to > stat(/dev/block/blah) actually means, it could mean the user don't have > enough permission, or /dev/block/blah is hidden to the user etc etc. I > think having "df -a" to show inaccessible / shadowed mounts is good > enough. > > > And again, if there _is_ a directory there that's part of an enclosing >> filesystem, what should we say about it? >> >> $ df b/c >> Filesystem 1K-blocks Used Available Use% Mounted on >> uno 8168980 0 8168980 0% >> /home/landley/toybox/clean2/b/c >> $ ./toybox df b/c >> Filesystem 1K-blocks Used Available Use% Mounted on >> dos 8168980 0 8168980 0% /home/landley/toybox/clean2/b >> >> The debian one is giving the WRONG ANSWER. (It's dos, not uno.) Mine is >> giving >> what filesystem is actually providing that directory, and when there >> ISN'T such >> a directory, it says so. >> >> Sigh. I suppose I could make it so "df -a dirname" would show all the >> overmounts? Except that's always going to include rootfs in the list. >> Right now >> _exact_ overmounts are filtered out (which is why you don't get rootfs >> when >> /dev/sda1 is mounted on / because it's exactly occluded even thought it >> _is_ >> technically still there). >> >> > Sorry but I don't understand, I thought exact overmounts are shown already? > $ sudo mount a.img a/b > $ sudo mount b.img a/b > $ ./df -a > /dev/loop0 - - - - > /ssd2/external/toybox/a/b > /dev/loop1 2846 45 2521 2% > /ssd2/external/toybox/a/b > > > This is a design issue. What is the right behavior? >> >> > diff --git a/toys/posix/df.c b/toys/posix/df.c >> > index d8ba2986..a04db1d6 100644 >> > --- a/toys/posix/df.c >> > +++ b/toys/posix/df.c >> > @@ -93,7 +93,7 @@ static void show_mt(struct mtab_list *mt, int >> measuring) >> > } >> > >> > // If we don't have -a, skip synthetic filesystems >> > - if (!FLAG(a) && !mt->statvfs.f_blocks) return; >> > + if (!FLAG(a) && !mt->statvfs.f_blocks && *mt->device != '/') return; >> >> This means you won't show network mounts or tmpfs instances. (I.E. this >> would >> break df so my rootfs example above doesn't show rootfs.) >> >> > // Prepare filesystem display fields >> > *dsuapm = *mt->device == '/' ? xabspath(mt->device, 0) : 0; >> > @@ -189,7 +190,7 @@ void df_main(void) >> > // Measure the names then output the table (in filesystem creation >> order). >> > for (measuring = 1;;) { >> > for (mt = mtstart; mt; mt = mt->next) >> > - if (mt->stat.st_dev) show_mt(mt, measuring); >> > + show_mt(mt, measuring); >> >> I hadn't made it this far down in the email before I already did that. :) >> >> > if (!measuring--) break; >> > print_header(); >> > } >> > >> > >> > >> > > The only question I have left, is it guaranteed that st_dev must >> be zero >> > or left >> > > unchanged when stat() fails? Or do we need to do something like, >> "if stat() / >> > > statvfs() fails, ensure st_dev is zero" in portability.c to >> ensure the caller >> > > knows that stat(mount point) failed? >> > >> > Linux leaves it alone. I can't speak for Apple. >> > >> > That's good enough for me, but I can't say for Apple (I don't own an >> Apple >> > station and have never tested on one.) >> >> I had one for a while but never got the apple devkit to work because my >> apple ID >> glitched basically immediately and an apple guru spent an hour trying to >> unbork >> it and couldn't. (I break everything.) Thus I couldn't load xcroak to >> actually >> COMPILE anything on said mac. (It then sat on a shelf for 6 months and I >> eventually mailed it to a college student on twitter who needed a new >> computer...) >> >> Rob >> > > > -- > > Yi-yo Chiang > Software Engineer > [email protected] > -- Yi-yo Chiang Software Engineer [email protected]
_______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
