Re: [PATCH] xfstests: Add first statx test

[David Howells <dhowells@redhat.com>]

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).

> > +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.

> 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.

> ... 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.

> 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)?

> 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?

> > +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.

David