From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx1.redhat.com ([209.132.183.28]:38880 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752512AbdCaKqc (ORCPT ); Fri, 31 Mar 2017 06:46:32 -0400 From: David Howells In-Reply-To: <20170331101941.GL22845@eguan.usersys.redhat.com> References: <20170331101941.GL22845@eguan.usersys.redhat.com> <15267.1490891560@warthog.procyon.org.uk> Subject: Re: [PATCH] xfstests: Add first statx test MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <11251.1490957189.1@warthog.procyon.org.uk> Date: Fri, 31 Mar 2017 11:46:29 +0100 Message-ID: <11252.1490957189@warthog.procyon.org.uk> Sender: fstests-owner@vger.kernel.org To: Eryu Guan Cc: dhowells@redhat.com, linux-xfs , Andreas Dilger , Christoph Hellwig , linux-fsdevel@vger.kernel.org, Eric Sandeen , fstests@vger.kernel.org List-ID: Eryu Guan 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