On Sat, Jun 07, 2025 at 06:20:48AM +0100, Al Viro wrote:
> On Fri, Jun 06, 2025 at 06:45:02PM +0100, Al Viro wrote:
> > On Fri, Jun 06, 2025 at 09:58:26AM +0200, Christian Brauner wrote:
> >
> > > Fwiw, check_mnt() is a useless name for this function that's been
> > > bothering me forever.
> >
> > Point, but let's keep the renaming (s/check_mnt/our_mount/, for
> > example) separate.
>
> Modified and force-pushed.
>
> It does pass xfstests without regressions. kselftests... AFAICS,
> no regressions either, but the damn thing is a mess. Example:
>
> # # set_layers_via_fds.c:711:set_layers_via_detached_mount_fds:Expected layers_found[i] (0) == true (1)
> # # set_layers_via_fds.c:39:set_layers_via_detached_mount_fds:Expected rmdir("/set_layers_via_fds") (-1) == 0 (0)
> # # set_layers_via_detached_mount_fds: Test terminated by assertion
> # # FAIL set_layers_via_fds.set_layers_via_detached_mount_fds
>
> Not a regression, AFAICT; the underlying problem is that mount options
> are shown incorrectly in the tested case. Still present after overlayfs
> merge. mount does succeed, but... in options we see this:
> rw,relatime,lowerdir+=/,lowerdir+=/,lowerdir+=/,lowerdir+=/,datadir+=/,datadir+=/,datadir+=/,upperdir=/upper,workdir=/work,redirect_dir=on,uuid=on,metacopy=on
>
> And it's a perfectly expected result - you are giving fsconfig(2) empty
> path on a detached tree, created with OPEN_TREE_CLONE. I.e. it *is*
> an empty path in the mount tree the sucker's in. What could d_path()
> produce other than "/"?
Sigh. There's no need to get all high and mighty about this. For once I
actually do write extensive selftests and they do actually catch a lot
of bugs. It's a joke how little selftests we have given the importance
of our apis. Nobody ever gives a flying fsck to review selftests when
they're posted because nobody seems to actually care.
The simple thing here to do is to point out that there's an issue in the
tests and that this should be fixed and maybe ask why.
The answer to that is that the getline checked in
TEST_F(set_layers_via_fds, set_layers_via_detached_mount_fds)
is a simple copy-and-paste error from
TEST_F(set_layers_via_fds, set_layers_via_fds)
that should just be removed and that's the end of that problem.
What sort of odd assumption is it that I'm not aware that a detached
tree doesn't resolve to /tmp. It's clearly a simple bug.
I actually had a patch to fix this I probably just forgot to paste it
during the merge window that added support for this.
> Note, BTW that it really does create set_layers_via_fds in root (WTF?) and
> running that sucker again yields a predictable fun result - mkdir() failing
> with EEXIST...
It's clearly a cleanup issue in FIXTURE_TEARDOWN(). Either fix it or ask
me to fix it.
> IMO that kind of stuff should be dealt with by creating a temporary directory
> somewhere in /tmp, mounting tmpfs on it, then doing all creations, etc.
> inside that. Then umount -l /tmp/<whatever>; rmdir /tmp/<whatever> will
> clean the things up.
Sorry, that's just wishful thinking at best and out of touch with how
these apis are used. The fact that you need a private assembly in some
hidden away directory followed by a umount is a complete waste of system
calls for a start. It's also inherently unclean unless you also bring
mount namespaces into the mix. Being able to use detached mount trees is
simple and clean especially for the overlayfs layer case.
There's enough cases where you don't ever want to leak the mounts into
an actually accessible mount namespace.
