On Sat, Apr 23, 2022 at 10:17:59AM +0300, Amir Goldstein wrote: > On Thu, Apr 21, 2022 at 9:05 PM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > > > On Thu, Apr 21, 2022 at 05:37:17PM +0200, Christoph Hellwig wrote: > > > On Thu, Apr 21, 2022 at 11:35:13PM +0800, Zorro Lang wrote: > > > > Sure, I won't do that wilfully, just try to ask how we can improve this > > > > huge and 'keep growing' idmapped-mounts.c, not tend to remove the whole > > > > idmapped-mount testing coverage :) > > > > > > It might just be time to split that file up into a few ones if there > > > is a sensible split. I'll let Christian think about that, though. > > > > Yep, I agree. I think we need to at least rename it to reflect is vfs > > generic nature and then split it into separate test binaries. > > I'll think about a good approach. > > The majority of test cases still do require_fs_allow_idmap and from > those who don't, most of them are variants for test cases that run > with and without idmapped mounts and possibly also in_userns. Just to clarify a few points in more detail to expand on the "grossly misnamed" theme: Given that this started out as a dedicated testsuite to provide almost obsessive testing of idmapped mounts the majority of tests is of course about idmapped mounts. It'd be rather concerning if it wasn't, in addition to also being misnamed. But to put numbers to it, we do have 74 test functions that each have a separate theme. Spread across the 74 test functions we do have ~120 test cases (Basically each fork() invocation in each of those 74 test functions can be considered a separate test case.). Of these 74 test functions we do have 12 generic vfs test functions, i.e. test functions that are not concerned with idmapped mounts. Just going by the number of test functions, not actual test cases that's almost 20% of the test suite concerned with generic vfs behavior. Plus, there's additional patchsets that extend the generic behavior which brings in another ~4-8 additional generic test functions with multiple test cases. And people will keep adding to it. > > And this new test case is no exception - there is still idmapping > involved, just not idmapped-mounts. The point was that there's a decent number of tests that aren't concerned with idmapped mounts. And this new test-case is only relevant for tmpfs mounted in a non-initial userns. So I'm not sure what this is trying to say. The argument is simply that there is a non-trivial number of tests that are not concerned with idmapped mounts. But the name of the test binary does imply that it is only concerned with idmapped mounts when it clearly isn't. So it is misnamed at this point causing understandable confusion. Iow, given that I and others have to respond to a patch or add a comment in the commit message of a patch to remind people that the thing they're patching doesn't just do what it says on the tin, we should probably rename it or do something else to improve the situation. It'd be concerning to have a can labeled "tomato soup" but to then discover that is also has spaghetti in it. That might be a nice suprise but I'd still better put it on the label. :) That's beside the point that the source file is 15k+ LOC strong which is just obscene. :) > > However you decide to break it up and/or rename the test binary > (I am not sure you must split the binary - only the source files), Rename we must imho; but I haven't yet decided whether or not we really should use separate binaries. I think I'd make that dependent on whether or not a good generic name can be found. :) > I think we need to be more consistent about the groups that the tests > that run this binary are in. > > 'perms' group is adequate, but adding to the 'idmapped' group and > maybe also to a new 'userns' group would be useful. > > BTW, the tests that use src/nsexec should also belong to the userns > group as does overlay/020, the only test that uses the 'unshare' tool. Good points.
