Re: [PATCH] xfstests: Add first statx test

[Eryu Guan <eguan@redhat.com>]

On Fri, Mar 31, 2017 at 11:46:29AM +0100, David Howells wrote:
> Eryu Guan <eguan@redhat.com> wrote:
> 
> > Also, we need to detect if the filesystem in test supports statx(2) or
> > not, and call _notrun to exit immediately and skip the test, so test
> > doesn't fail when running on older kernels where don't have statx
> > support. e.g. a new _require rule like
> 
> All filesystems support statx.  What changes is whether they provide any extra
> data.  What does matter is the kernel version (4.11-rc1 minimum).

xfstests is run not only with latest upstream kernel, but also with many
distro kernels, like RHEL, by distro testers, where there's no statx
syscall support at all (at this moment). So test should _notrun instead
of failing there.

If not providing extra data will fail the test, we need to require the
filesystem to be tested to provide such extra data too.

> 
> > > +failed=0
> > 
> > This is not needed, "status" is sufficient.
> 
> The script generated by new says:
> 
> 	status=1	# failure is the default!
> 
> I presume I'm allowed to change the default.

Yes, you can change the default simply by "status=0". But in xfstests we
rarely do that, we usually set status=0 just before exit if everything
went well.

> 
> > No need to check if mkfifo's status (and all later similar commands like
> > mkmod, mkdir, ln etc.). ...
> 
> But there's no point doing the stat on them if they didn't succeed.

Then just let statx(2) fail and complain about the missing file, the
test fails anyway. The whole point is that with golden image matching,
you don't have to check return value of every command, that's the
methodology xfstests has adopted. And occasionally, exercising such
error paths help find unexpected bugs too :)

> 
> > ... The test harness compares the test output with the predefined golden
> > output file (420.out in this case) and fails the test if the output doesn't
> > match. So any error message from mkfifo, mknod, mkdir commands will break
> > golden image and fails the test.
> 
> The status variable would be redundant then.

Some tests can't rely on the golden image match, they rely on status
variable. So test passes only if a) golden image matches, b) status=0

> 
> > And prefix or suffix the test file name with current test sequence
> > number would be good, to avoid file name conflicts with test files from
> > other tests.
> 
> Do I increment this for each use?  Or is it per call of the test script?  Or
> is it the number of the script (ie. 420 in this case)?

There's a pre-defined $seq variable (as showed in my example, not
included in this reply), you can just use $seq directly.

> 
> > If nc is really needed, we should check the existence of it before
> > starting the actual test, so test won't fail because of lack of nc
> > command. i.e. define NC_PROG in common/config and call _require_command
> > to check the existence of it. e.g.
> 
> Then I run $NC_PROG rather than nc?

Yes, that's perferred.

> 
> > > +if [ $failed = 1 ]
> > > +then
> > > +    echo "Test script failed"
> > > +    exit
> > > +fi
> > 
> > This is not needed either.
> 
> You're looking at version 1.  This is gone in version 2.
> 
> > I noticed you've already updated group to 'auto quick' :)
> 
> Someone needs to fix ./new.  Also it would be good if ./new either didn't
> require the suite to be built or explicitly said that the suite should be
> built before running if you run it without doing so first.

Yes, and the document really should be improved, so people could have an
easy start with it.

Thanks,
Eryu