Re: [XFSTESTS 5/6] Add richacl tests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Dec 08, 2015 at 12:45:05AM +0100, Andreas Gruenbacher wrote:
> On Mon, Dec 7, 2015 at 10:11 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> > On Sun, Dec 06, 2015 at 06:31:20PM +0100, Andreas Gruenbacher wrote:
> >> Dave,
> >>
> >> On Tue, Nov 24, 2015 at 12:08 AM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> >> > On Wed, Nov 18, 2015 at 03:17:48PM +0100, Andreas Gruenbacher wrote:
> >> >> Add the Rich Access Control List tests from the richacl package.  The
> >> >> new tests require TEST_DEV and TEST_DIR to be set.
> >> >>
> >> >> When the check script is run, it first makes sure that the test
> >> >> filesystem has richacls enabled or disabled as appropriate: with the
> >> >> -richacl option, richacls must be enabled; without the -richacl option,
> >> >> richacls must be disabled.  If TEST_DEV has incorrect richacl support,
> >> >> the TEST_DEV filesystem is recreated.
> >> >
> >> > No, we don't recreate the testdev like this with the test harness.
> >> > The test device is intended to remain unchanged from run to run, so
> >> > give us long term aging of the test filesystem over months of
> >> > regression test running.
> >> >
> >> > If you want to test the testdev with richacls enabled, then you need
> >> > to manually create the testdev with richacls.  Any test that
> >> > specifically needs to enable richacls should use the scratch device
> >> > and use a _requires_scratch_richacls() test to check that the
> >> > scratch device can be created with richacl support.
> >>
> >> well, let's leave it to users to create their TEST_DEV with richacl
> >> support if they want the richacl tests to be run then. Those tests
> >> will already be skipped on unsuitable filesystems anyway.
> >
> > No, that's not the way xfstests works, or how developers really
> > expect it to run. If you have new functionality that requires
> > specific on-disk format requirements that are different to the
> > defaults that the developers userspace progs are using then you
> > should be using the scratch device for those tests after probing that
> 
> That should be possible. Those tests would then only be using
> SCRATCH_DEV / SCRATCH_MNT and not TEST_DEV / TEST_DIR at all. Any
> ideas how xfstests could be made to not always require TEST_DEV /
> TEST_DIR to be set?

That's not something I've ever considered - the test harness has
always assumed that you must have a valid test device to run
xfstests. Only recently did we add _require_test() to all the tests
that need the test device to move the mounting of the test device
from the harness to the tests themselves, but the harness itself
still has lots of code in it that assumes a test device must
exist....

Really, though, I fail to understand why this is a problem. It
takes 30s to configure test and scratch devices on loopback, and
then this is a total non-problem....

> > That you are using C code to detect whether an on-disk feature is
> > supported or not. This is done through running the userspace tools
> > to check they support the on-disk feature, then creating a
> > filesystem with those tools and mounting it to determine whether the
> > kernel supports the feature correctly as well.
> 
> Someone might run "check -g richacl" on a filesystem that doesn't
> support richacls. In that case, I want the richacl test cases to be
> skipped, with an explanation why; I don't want the test cases to just
> explode. That's the same as what the POSIX ACL related test cases do
> in common/attr:_require_acls().

Really? have you not noticed the copious use of _notrun() in all the
_require functions. here's a small selection for you:

generic/038      [not run] This test requires at least 10GB free on /mnt/scratch to run
generic/288 2s ... [not run] FITRIM not supported on /mnt/scratch
shared/002 0s ... [not run] This test requires a sane block device flush
xfs/016 9s ... [not run] Cannot mkfs for this test using MKFS_OPTIONS specified
xfs/035  [not run] No dump tape specified
xfs/095  [not run] not suitable for this OS: Linux
xfs/127  [not run] Reflink not supported by scratch filesystem type: xfs
xfs/131  [not run] External volumes not in use, skipped this test
xfs/186 2s ... [not run] attr v1 not supported on /dev/ram1
xfs/191  [not run] no mkfs support for NFS v4 ACLs
xfs/197  [not run] This test is only valid on 32 bit machines

And so on.

(Did you notice the xfs/191? You know, the old richacl test
execution wrapper. It's got all the mkfs/kernel/tool tests in it
that these days we wrap up into a _requires() function...)

> _require_acls() uses the chacl utility for probing for POSIX ACL
> support.

Yes, because posix acls don't have filesystem feature bits that can
be probed, nor do they have need for mkfs or fsck support. Welcome
to the new world - it's different to the old world.

> The require-richacls program serves the same purpose. I could
> probe using getfattr instead, but getfattr isn't necessarily
> installed, and I won't want to create an unnecessary dependency.

So you have to mkfs the filesytem, then mount it, then you can run
getfattr to check if richacls are supported. Yet mkfs can fail, the
mount can fail, and neither of them are going to give you nice
"richacls not supported" errors. See xfs/191, again.

> > There is no point in maintaining two test suites - you'll change
> > stuff in your richacl package test suite, and the xfstests stuff
> > will be ignored, because *you won't be using xfstests*.
> > The whole point of getting this stuff into xfstests is so that
> > there's one place to both run and maintain the tests.
> 
> I don't want to maintain two test suites, I want the same tests to
> work in both contexts.

Sure, xfstests becomes the master copy, because that's the one the
rest of the filesystem developer community needs you to maintain.

> >> What's the benefit? The tests don't need anything from common/, I want
> >> to keep them well separated so that updating them will remain easy.
> >
> > What's the benefit? That the tests all follow the same patterns, and
> > that means any xfstests user can modify and update the tests without
> > having to learn some new, weird frankenstein test infrastructure.
> 
> From the point of view of the xfstests infrastructure,

But from a maintenance perspective, as well as fs developers who
needs to understand the tests quickly, the tests need use the test
harness in the same way as all the other tests in xfstests. The same
logging, the same log files, the same expected results file, the
same method of determining pass or failure, etc.

Embed another test harness within the xfstests harness that does
it's own golden output matching, diffing, results aggregation and
reporting is just adding a whole load of code duplication
abstraction and bloat. It's unnecessary - the xfstests harness
provides all the functionality the richacl tests needs. It's just a
trivial mechanical conversion to change it all over. This isn't
difficult - you're making a mountain out of a molehill here...

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs



[Index of Archives]     [Linux XFS Devel]     [Linux Filesystem Development]     [Filesystem Testing]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux