On Wed, Feb 17, 2021 at 12:36 AM Rob Landley <r...@landley.net> 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 yochi...@google.com
_______________________________________________ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net