* [PATCH] xfstests: add function _require_fssum() @ 2014-02-24 11:56 Filipe David Borba Manana 2014-02-24 12:23 ` Dave Chinner 2014-02-25 13:46 ` [PATCH v2] " Filipe David Borba Manana 0 siblings, 2 replies; 7+ messages in thread From: Filipe David Borba Manana @ 2014-02-24 11:56 UTC (permalink / raw) To: xfs; +Cc: Filipe David Borba Manana, linux-btrfs To avoid repeating detection of fssum presence in many btrfs tests, as suggested by Dave Chinner. Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com> --- common/rc | 7 +++++++ tests/btrfs/007 | 5 +---- tests/btrfs/016 | 5 +---- tests/btrfs/030 | 5 +---- tests/btrfs/038 | 5 +---- tests/btrfs/039 | 5 +---- tests/btrfs/040 | 5 +---- tests/btrfs/041 | 5 +---- tests/btrfs/042 | 5 +---- 9 files changed, 15 insertions(+), 32 deletions(-) mode change 100644 => 100755 tests/btrfs/016 diff --git a/common/rc b/common/rc index 5df504c..cce05cc 100644 --- a/common/rc +++ b/common/rc @@ -2144,6 +2144,13 @@ _require_cp_reflink() _notrun "This test requires a cp with --reflink support." } +_require_fssum() +{ + HERE=`pwd` + FSSUM_PROG=$HERE/src/fssum + [ -x $FSSUM_PROG ] || _notrun "fssum not built" +} + # Given 2 files, verify that they have the same mapping but different # inodes - i.e. an undisturbed reflink # Silent if so, make noise if not diff --git a/tests/btrfs/007 b/tests/btrfs/007 index 5df9ccb..5430613 100755 --- a/tests/btrfs/007 +++ b/tests/btrfs/007 @@ -31,7 +31,6 @@ seq=`basename $0` seqres=$RESULT_DIR/$seq echo "QA output created by $seq" -here=`pwd` tmp=`mktemp -d` status=1 @@ -52,11 +51,9 @@ _need_to_be_root _supported_fs btrfs _supported_os Linux _require_scratch +_require_fssum _require_seek_data_hole -FSSUM_PROG=$here/src/fssum -[ -x $FSSUM_PROG ] || _notrun "fssum not built" - rm -f $seqres.full workout() diff --git a/tests/btrfs/016 b/tests/btrfs/016 old mode 100644 new mode 100755 index 6faead1..d04c21a --- a/tests/btrfs/016 +++ b/tests/btrfs/016 @@ -26,7 +26,6 @@ seq=`basename $0` seqres=$RESULT_DIR/$seq echo "QA output created by $seq" -here=`pwd` tmp=`mktemp -d` tmp_dir=send_temp_$seq @@ -51,9 +50,7 @@ trap "_cleanup ; exit \$status" 0 1 2 3 15 _supported_fs btrfs _supported_os Linux _require_scratch - -FSSUM_PROG=$here/src/fssum -[ -x $FSSUM_PROG ] || _notrun "fssum not built" +_require_fssum _scratch_mkfs > /dev/null 2>&1 diff --git a/tests/btrfs/030 b/tests/btrfs/030 index a9f5fb4..a76a410 100755 --- a/tests/btrfs/030 +++ b/tests/btrfs/030 @@ -36,7 +36,6 @@ seq=`basename $0` seqres=$RESULT_DIR/$seq echo "QA output created by $seq" -here=`pwd` tmp=`mktemp -d` status=1 # failure is the default! trap "_cleanup; exit \$status" 0 1 2 3 15 @@ -54,11 +53,9 @@ _cleanup() _supported_fs btrfs _supported_os Linux _require_scratch +_require_fssum _need_to_be_root -FSSUM_PROG=$here/src/fssum -[ -x $FSSUM_PROG ] || _notrun "fssum not built" - rm -f $seqres.full _scratch_mkfs >/dev/null 2>&1 diff --git a/tests/btrfs/038 b/tests/btrfs/038 index 8893696..4941d3e 100755 --- a/tests/btrfs/038 +++ b/tests/btrfs/038 @@ -32,7 +32,6 @@ seq=`basename $0` seqres=$RESULT_DIR/$seq echo "QA output created by $seq" -here=`pwd` tmp=`mktemp -d` status=1 # failure is the default! trap "_cleanup; exit \$status" 0 1 2 3 15 @@ -50,11 +49,9 @@ _cleanup() _supported_fs btrfs _supported_os Linux _require_scratch +_require_fssum _need_to_be_root -FSSUM_PROG=$here/src/fssum -[ -x $FSSUM_PROG ] || _notrun "fssum not built" - rm -f $seqres.full _scratch_mkfs >/dev/null 2>&1 diff --git a/tests/btrfs/039 b/tests/btrfs/039 index 41e09be..758b23c 100755 --- a/tests/btrfs/039 +++ b/tests/btrfs/039 @@ -35,7 +35,6 @@ seq=`basename $0` seqres=$RESULT_DIR/$seq echo "QA output created by $seq" -here=`pwd` tmp=`mktemp -d` status=1 # failure is the default! trap "_cleanup; exit \$status" 0 1 2 3 15 @@ -53,11 +52,9 @@ _cleanup() _supported_fs btrfs _supported_os Linux _require_scratch +_require_fssum _need_to_be_root -FSSUM_PROG=$here/src/fssum -[ -x $FSSUM_PROG ] || _notrun "fssum not built" - rm -f $seqres.full _scratch_mkfs >/dev/null 2>&1 diff --git a/tests/btrfs/040 b/tests/btrfs/040 index 77c4a84..dfd495c 100755 --- a/tests/btrfs/040 +++ b/tests/btrfs/040 @@ -35,7 +35,6 @@ seq=`basename $0` seqres=$RESULT_DIR/$seq echo "QA output created by $seq" -here=`pwd` tmp=`mktemp -d` status=1 # failure is the default! trap "_cleanup; exit \$status" 0 1 2 3 15 @@ -53,11 +52,9 @@ _cleanup() _supported_fs btrfs _supported_os Linux _require_scratch +_require_fssum _need_to_be_root -FSSUM_PROG=$here/src/fssum -[ -x $FSSUM_PROG ] || _notrun "fssum not built" - rm -f $seqres.full _scratch_mkfs >/dev/null 2>&1 diff --git a/tests/btrfs/041 b/tests/btrfs/041 index bfc0e4b..a960001 100755 --- a/tests/btrfs/041 +++ b/tests/btrfs/041 @@ -31,7 +31,6 @@ seq=`basename $0` seqres=$RESULT_DIR/$seq echo "QA output created by $seq" -here=`pwd` tmp=`mktemp -d` status=1 # failure is the default! trap "_cleanup; exit \$status" 0 1 2 3 15 @@ -49,11 +48,9 @@ _cleanup() _supported_fs btrfs _supported_os Linux _require_scratch +_require_fssum _need_to_be_root -FSSUM_PROG=$here/src/fssum -[ -x $FSSUM_PROG ] || _notrun "fssum not built" - rm -f $seqres.full _scratch_mkfs >/dev/null 2>&1 diff --git a/tests/btrfs/042 b/tests/btrfs/042 index 2bd5147..9aa8967 100755 --- a/tests/btrfs/042 +++ b/tests/btrfs/042 @@ -36,7 +36,6 @@ seq=`basename $0` seqres=$RESULT_DIR/$seq echo "QA output created by $seq" -here=`pwd` tmp=`mktemp -d` status=1 # failure is the default! trap "_cleanup; exit \$status" 0 1 2 3 15 @@ -54,11 +53,9 @@ _cleanup() _supported_fs btrfs _supported_os Linux _require_scratch +_require_fssum _need_to_be_root -FSSUM_PROG=$here/src/fssum -[ -x $FSSUM_PROG ] || _notrun "fssum not built" - rm -f $seqres.full _scratch_mkfs >/dev/null 2>&1 -- 1.7.9.5 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] xfstests: add function _require_fssum() 2014-02-24 11:56 [PATCH] xfstests: add function _require_fssum() Filipe David Borba Manana @ 2014-02-24 12:23 ` Dave Chinner 2014-02-24 13:22 ` Filipe David Manana 2014-02-25 13:46 ` [PATCH v2] " Filipe David Borba Manana 1 sibling, 1 reply; 7+ messages in thread From: Dave Chinner @ 2014-02-24 12:23 UTC (permalink / raw) To: Filipe David Borba Manana; +Cc: linux-btrfs, xfs On Mon, Feb 24, 2014 at 11:56:23AM +0000, Filipe David Borba Manana wrote: > To avoid repeating detection of fssum presence in many btrfs tests, as > suggested by Dave Chinner. > > Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com> > --- > common/rc | 7 +++++++ > tests/btrfs/007 | 5 +---- > tests/btrfs/016 | 5 +---- > tests/btrfs/030 | 5 +---- > tests/btrfs/038 | 5 +---- > tests/btrfs/039 | 5 +---- > tests/btrfs/040 | 5 +---- > tests/btrfs/041 | 5 +---- > tests/btrfs/042 | 5 +---- > 9 files changed, 15 insertions(+), 32 deletions(-) > mode change 100644 => 100755 tests/btrfs/016 > > diff --git a/common/rc b/common/rc > index 5df504c..cce05cc 100644 > --- a/common/rc > +++ b/common/rc > @@ -2144,6 +2144,13 @@ _require_cp_reflink() > _notrun "This test requires a cp with --reflink support." > } > > +_require_fssum() > +{ > + HERE=`pwd` > + FSSUM_PROG=$HERE/src/fssum > + [ -x $FSSUM_PROG ] || _notrun "fssum not built" > +} $here is defined by check to be the root of the xfstests instance that is running. There's 60+ tests that already us it. Hence: _require_fssum() { FSSUM_PROG=$here/src/fssum [ -x $FSSUM_PROG ] || _notrun "fssum not built" } Is all you need here. > + > # Given 2 files, verify that they have the same mapping but different > # inodes - i.e. an undisturbed reflink > # Silent if so, make noise if not > diff --git a/tests/btrfs/007 b/tests/btrfs/007 > index 5df9ccb..5430613 100755 > --- a/tests/btrfs/007 > +++ b/tests/btrfs/007 > @@ -31,7 +31,6 @@ seq=`basename $0` > seqres=$RESULT_DIR/$seq > echo "QA output created by $seq" > > -here=`pwd` > tmp=`mktemp -d` > status=1 Yeah, redefining $here is a bad thing to do :/ And I'd missed that this was being done in all the new btrfs tests, otherwise I would have pulled it up earlier. It also points out that the btrfs tests are using a non-standard $tmp directory - one that is in the xfstests source directory. That's a bad thing, too - tests should be using: tmp=/tmp/$$ to store small temporary files. If /tmp is too small for what a test needs, then the test should be using $TEST_DIR as the store for the temporary files to exercise the filesystem under test as much as possible. e.g. send image files build form snapshots of SCRATCH_DEV should be stored on TEST_DIR, not in $tmp; filesystem image files that are mounted by loopback should be stored on TEST_DIR or SCRATCH_MNT, not $tmp. And so on. i.e. the idea is that you direct as much of the IO to the test_DIR and SCRATCH_MNT as possible, not to the filesystem that is hosting $tmp or the xfstests source directory.... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfstests: add function _require_fssum() 2014-02-24 12:23 ` Dave Chinner @ 2014-02-24 13:22 ` Filipe David Manana 2014-02-24 22:22 ` Dave Chinner 0 siblings, 1 reply; 7+ messages in thread From: Filipe David Manana @ 2014-02-24 13:22 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-btrfs@vger.kernel.org, xfs@oss.sgi.com On Mon, Feb 24, 2014 at 12:23 PM, Dave Chinner <david@fromorbit.com> wrote: > On Mon, Feb 24, 2014 at 11:56:23AM +0000, Filipe David Borba Manana wrote: >> To avoid repeating detection of fssum presence in many btrfs tests, as >> suggested by Dave Chinner. >> >> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com> >> --- >> common/rc | 7 +++++++ >> tests/btrfs/007 | 5 +---- >> tests/btrfs/016 | 5 +---- >> tests/btrfs/030 | 5 +---- >> tests/btrfs/038 | 5 +---- >> tests/btrfs/039 | 5 +---- >> tests/btrfs/040 | 5 +---- >> tests/btrfs/041 | 5 +---- >> tests/btrfs/042 | 5 +---- >> 9 files changed, 15 insertions(+), 32 deletions(-) >> mode change 100644 => 100755 tests/btrfs/016 >> >> diff --git a/common/rc b/common/rc >> index 5df504c..cce05cc 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -2144,6 +2144,13 @@ _require_cp_reflink() >> _notrun "This test requires a cp with --reflink support." >> } >> >> +_require_fssum() >> +{ >> + HERE=`pwd` >> + FSSUM_PROG=$HERE/src/fssum >> + [ -x $FSSUM_PROG ] || _notrun "fssum not built" >> +} > > $here is defined by check to be the root of the xfstests instance > that is running. There's 60+ tests that already us it. Hence: > > _require_fssum() > { > FSSUM_PROG=$here/src/fssum > [ -x $FSSUM_PROG ] || _notrun "fssum not built" > } > > Is all you need here. Hum, doesn't work unless the test file defines $here. At least when running a single only (.e.g. ./check btrfs/041). > >> + >> # Given 2 files, verify that they have the same mapping but different >> # inodes - i.e. an undisturbed reflink >> # Silent if so, make noise if not >> diff --git a/tests/btrfs/007 b/tests/btrfs/007 >> index 5df9ccb..5430613 100755 >> --- a/tests/btrfs/007 >> +++ b/tests/btrfs/007 >> @@ -31,7 +31,6 @@ seq=`basename $0` >> seqres=$RESULT_DIR/$seq >> echo "QA output created by $seq" >> >> -here=`pwd` >> tmp=`mktemp -d` >> status=1 > > Yeah, redefining $here is a bad thing to do :/ See above :) > > And I'd missed that this was being done in all the new btrfs tests, > otherwise I would have pulled it up earlier. > > It also points out that the btrfs tests are using a non-standard > $tmp directory - one that is in the xfstests source directory. > That's a bad thing, too - tests should be using: > > tmp=/tmp/$$ > > to store small temporary files. > > If /tmp is too small for what a test needs, then the test should be > using $TEST_DIR as the store for the temporary files to exercise > the filesystem under test as much as possible. e.g. send image > files build form snapshots of SCRATCH_DEV should be stored on TEST_DIR, > not in $tmp; filesystem image files that are mounted by loopback > should be stored on TEST_DIR or SCRATCH_MNT, not $tmp. And so on. > > i.e. the idea is that you direct as much of the IO to the test_DIR > and SCRATCH_MNT as possible, not to the filesystem that is hosting > $tmp or the xfstests source directory.... Right. Sounds like a separate patch (to use TEST_DIR/$$ for e.g. as a place to store temporary test data). Thanks Dave > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com -- Filipe David Manana, "Reasonable men adapt themselves to the world. Unreasonable men adapt the world to themselves. That's why all progress depends on unreasonable men." _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfstests: add function _require_fssum() 2014-02-24 13:22 ` Filipe David Manana @ 2014-02-24 22:22 ` Dave Chinner 2014-02-24 23:08 ` Filipe David Manana 0 siblings, 1 reply; 7+ messages in thread From: Dave Chinner @ 2014-02-24 22:22 UTC (permalink / raw) To: Filipe David Manana; +Cc: linux-btrfs@vger.kernel.org, xfs@oss.sgi.com On Mon, Feb 24, 2014 at 01:22:36PM +0000, Filipe David Manana wrote: > On Mon, Feb 24, 2014 at 12:23 PM, Dave Chinner <david@fromorbit.com> wrote: > > On Mon, Feb 24, 2014 at 11:56:23AM +0000, Filipe David Borba Manana wrote: > >> To avoid repeating detection of fssum presence in many btrfs tests, as > >> suggested by Dave Chinner. > >> > >> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com> > >> --- > >> common/rc | 7 +++++++ > >> tests/btrfs/007 | 5 +---- > >> tests/btrfs/016 | 5 +---- > >> tests/btrfs/030 | 5 +---- > >> tests/btrfs/038 | 5 +---- > >> tests/btrfs/039 | 5 +---- > >> tests/btrfs/040 | 5 +---- > >> tests/btrfs/041 | 5 +---- > >> tests/btrfs/042 | 5 +---- > >> 9 files changed, 15 insertions(+), 32 deletions(-) > >> mode change 100644 => 100755 tests/btrfs/016 > >> > >> diff --git a/common/rc b/common/rc > >> index 5df504c..cce05cc 100644 > >> --- a/common/rc > >> +++ b/common/rc > >> @@ -2144,6 +2144,13 @@ _require_cp_reflink() > >> _notrun "This test requires a cp with --reflink support." > >> } > >> > >> +_require_fssum() > >> +{ > >> + HERE=`pwd` > >> + FSSUM_PROG=$HERE/src/fssum > >> + [ -x $FSSUM_PROG ] || _notrun "fssum not built" > >> +} > > > > $here is defined by check to be the root of the xfstests instance > > that is running. There's 60+ tests that already us it. Hence: > > > > _require_fssum() > > { > > FSSUM_PROG=$here/src/fssum > > [ -x $FSSUM_PROG ] || _notrun "fssum not built" > > } > > > > Is all you need here. > > Hum, doesn't work unless the test file defines $here. At least when > running a single only (.e.g. ./check btrfs/041). Right, I realised that it isn't exported from check, and the template for new tests defines it as: here=\`pwd\` which is essentially the same thing. Almost every test does it. $ git grep here=\`pwd\` |wc -l 364 And it works just fine when running a test as you describe above. Many of the _requires* functions just use $here directly, and does a lot of the common/* infrastructure. It is assumed that $here is set and valid and so if it wasn't set, lots of different things would break. Oh, I note that btrfs/034 and btrfs/037 don't set it, so they need fixing, too. Looking at it, though, we shoul djust export the value from check and remove the assignment that occurs in every test as they end up being the same value: $ sudo ./check generic/001 check_here=/home/dave/src/xfstests-dev FSTYP -- xfs (debug) PLATFORM -- Linux/x86_64 test2 3.14.0-rc3-dgc+ MKFS_OPTIONS -- -f -bsize=4096 /dev/vdb MOUNT_OPTIONS -- /dev/vdb /mnt/scratch generic/001 5s ... [failed, exit status 1] - output mismatch (see /home/dave/src/xfstests-dev/results//generic/001.out.bad) ..... $ cat /home/dave/src/xfstests-dev/results//generic/001.out.bad QA output created by 001 here=/home/dave/src/xfstests-dev $ > >> diff --git a/tests/btrfs/007 b/tests/btrfs/007 > >> index 5df9ccb..5430613 100755 > >> --- a/tests/btrfs/007 > >> +++ b/tests/btrfs/007 > >> @@ -31,7 +31,6 @@ seq=`basename $0` > >> seqres=$RESULT_DIR/$seq > >> echo "QA output created by $seq" > >> > >> -here=`pwd` > >> tmp=`mktemp -d` > >> status=1 > > > > Yeah, redefining $here is a bad thing to do :/ Right, my mistake. It needs to be defined for the entire test, not in a requires function. Hence I think we should just export it from check.... > > It also points out that the btrfs tests are using a non-standard > > $tmp directory - one that is in the xfstests source directory. > > That's a bad thing, too - tests should be using: > > > > tmp=/tmp/$$ > > > > to store small temporary files. > > > > If /tmp is too small for what a test needs, then the test should be > > using $TEST_DIR as the store for the temporary files to exercise > > the filesystem under test as much as possible. e.g. send image > > files build form snapshots of SCRATCH_DEV should be stored on TEST_DIR, > > not in $tmp; filesystem image files that are mounted by loopback > > should be stored on TEST_DIR or SCRATCH_MNT, not $tmp. And so on. > > > > i.e. the idea is that you direct as much of the IO to the test_DIR > > and SCRATCH_MNT as possible, not to the filesystem that is hosting > > $tmp or the xfstests source directory.... > > Right. Sounds like a separate patch (to use TEST_DIR/$$ for e.g. as a > place to store temporary test data). tmp should be set to /tmp/$$. That's where the *test harness* will direct temporary log files, mkfs output, etc. If you have temporary data for the *test*, and it's large, use TEST_DIR, not $tmp. $tmp is generally only for things like log files, time stamps, output files that need later processing because they can't be filtered directly, etc. Why? Because if the test corrupts $TEST_DIR and you've got all the test harness tmp files on TEST_DIR, how is the test harness going to work out what went wrong during the test? Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfstests: add function _require_fssum() 2014-02-24 22:22 ` Dave Chinner @ 2014-02-24 23:08 ` Filipe David Manana 2014-02-24 23:35 ` Dave Chinner 0 siblings, 1 reply; 7+ messages in thread From: Filipe David Manana @ 2014-02-24 23:08 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-btrfs@vger.kernel.org, xfs@oss.sgi.com On Mon, Feb 24, 2014 at 10:22 PM, Dave Chinner <david@fromorbit.com> wrote: > On Mon, Feb 24, 2014 at 01:22:36PM +0000, Filipe David Manana wrote: >> On Mon, Feb 24, 2014 at 12:23 PM, Dave Chinner <david@fromorbit.com> wrote: >> > On Mon, Feb 24, 2014 at 11:56:23AM +0000, Filipe David Borba Manana wrote: >> >> To avoid repeating detection of fssum presence in many btrfs tests, as >> >> suggested by Dave Chinner. >> >> >> >> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com> >> >> --- >> >> common/rc | 7 +++++++ >> >> tests/btrfs/007 | 5 +---- >> >> tests/btrfs/016 | 5 +---- >> >> tests/btrfs/030 | 5 +---- >> >> tests/btrfs/038 | 5 +---- >> >> tests/btrfs/039 | 5 +---- >> >> tests/btrfs/040 | 5 +---- >> >> tests/btrfs/041 | 5 +---- >> >> tests/btrfs/042 | 5 +---- >> >> 9 files changed, 15 insertions(+), 32 deletions(-) >> >> mode change 100644 => 100755 tests/btrfs/016 >> >> >> >> diff --git a/common/rc b/common/rc >> >> index 5df504c..cce05cc 100644 >> >> --- a/common/rc >> >> +++ b/common/rc >> >> @@ -2144,6 +2144,13 @@ _require_cp_reflink() >> >> _notrun "This test requires a cp with --reflink support." >> >> } >> >> >> >> +_require_fssum() >> >> +{ >> >> + HERE=`pwd` >> >> + FSSUM_PROG=$HERE/src/fssum >> >> + [ -x $FSSUM_PROG ] || _notrun "fssum not built" >> >> +} >> > >> > $here is defined by check to be the root of the xfstests instance >> > that is running. There's 60+ tests that already us it. Hence: >> > >> > _require_fssum() >> > { >> > FSSUM_PROG=$here/src/fssum >> > [ -x $FSSUM_PROG ] || _notrun "fssum not built" >> > } >> > >> > Is all you need here. >> >> Hum, doesn't work unless the test file defines $here. At least when >> running a single only (.e.g. ./check btrfs/041). > > Right, I realised that it isn't exported from check, and the template > for new tests defines it as: > > here=\`pwd\` > > which is essentially the same thing. Almost every test does it. > > $ git grep here=\`pwd\` |wc -l > 364 > > And it works just fine when running a test as you describe above. > Many of the _requires* functions just use $here directly, and does a > lot of the common/* infrastructure. It is assumed that $here is set > and valid and so if it wasn't set, lots of different things would > break. > > Oh, I note that btrfs/034 and btrfs/037 don't set it, so they need > fixing, too. > > Looking at it, though, we shoul djust export the value from check > and remove the assignment that occurs in every test as they end up > being the same value: > > $ sudo ./check generic/001 > check_here=/home/dave/src/xfstests-dev > FSTYP -- xfs (debug) > PLATFORM -- Linux/x86_64 test2 3.14.0-rc3-dgc+ > MKFS_OPTIONS -- -f -bsize=4096 /dev/vdb > MOUNT_OPTIONS -- /dev/vdb /mnt/scratch > > generic/001 5s ... [failed, exit status 1] - output mismatch (see /home/dave/src/xfstests-dev/results//generic/001.out.bad) > ..... > $ cat /home/dave/src/xfstests-dev/results//generic/001.out.bad > QA output created by 001 > here=/home/dave/src/xfstests-dev > $ > >> >> diff --git a/tests/btrfs/007 b/tests/btrfs/007 >> >> index 5df9ccb..5430613 100755 >> >> --- a/tests/btrfs/007 >> >> +++ b/tests/btrfs/007 >> >> @@ -31,7 +31,6 @@ seq=`basename $0` >> >> seqres=$RESULT_DIR/$seq >> >> echo "QA output created by $seq" >> >> >> >> -here=`pwd` >> >> tmp=`mktemp -d` >> >> status=1 >> > >> > Yeah, redefining $here is a bad thing to do :/ > > Right, my mistake. It needs to be defined for the entire test, not > in a requires function. Hence I think we should just export it from > check.... Hum, ok. So the decision is to let the tests explicitly define the variable "here", and not export "here" from the main check script. > >> > It also points out that the btrfs tests are using a non-standard >> > $tmp directory - one that is in the xfstests source directory. >> > That's a bad thing, too - tests should be using: >> > >> > tmp=/tmp/$$ >> > >> > to store small temporary files. >> > >> > If /tmp is too small for what a test needs, then the test should be >> > using $TEST_DIR as the store for the temporary files to exercise >> > the filesystem under test as much as possible. e.g. send image >> > files build form snapshots of SCRATCH_DEV should be stored on TEST_DIR, >> > not in $tmp; filesystem image files that are mounted by loopback >> > should be stored on TEST_DIR or SCRATCH_MNT, not $tmp. And so on. >> > >> > i.e. the idea is that you direct as much of the IO to the test_DIR >> > and SCRATCH_MNT as possible, not to the filesystem that is hosting >> > $tmp or the xfstests source directory.... >> >> Right. Sounds like a separate patch (to use TEST_DIR/$$ for e.g. as a >> place to store temporary test data). > > tmp should be set to /tmp/$$. That's where the *test harness* will > direct temporary log files, mkfs output, etc. If you have temporary > data for the *test*, and it's large, use TEST_DIR, not $tmp. > $tmp is generally only for things like log files, time stamps, > output files that need later processing because they can't be > filtered directly, etc. > > Why? Because if the test corrupts $TEST_DIR and you've got all the > test harness tmp files on TEST_DIR, how is the test harness going > to work out what went wrong during the test? Makes sense. All these btrfs tests are adding small files to /tmp, less than ~100Kb or so (certainly not megabytes), so that seems fine. Thanks Dave > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com -- Filipe David Manana, "Reasonable men adapt themselves to the world. Unreasonable men adapt the world to themselves. That's why all progress depends on unreasonable men." _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfstests: add function _require_fssum() 2014-02-24 23:08 ` Filipe David Manana @ 2014-02-24 23:35 ` Dave Chinner 0 siblings, 0 replies; 7+ messages in thread From: Dave Chinner @ 2014-02-24 23:35 UTC (permalink / raw) To: Filipe David Manana; +Cc: linux-btrfs@vger.kernel.org, xfs@oss.sgi.com On Mon, Feb 24, 2014 at 11:08:27PM +0000, Filipe David Manana wrote: > On Mon, Feb 24, 2014 at 10:22 PM, Dave Chinner <david@fromorbit.com> wrote: > > On Mon, Feb 24, 2014 at 01:22:36PM +0000, Filipe David Manana wrote: > >> On Mon, Feb 24, 2014 at 12:23 PM, Dave Chinner <david@fromorbit.com> wrote: > >> >> diff --git a/tests/btrfs/007 b/tests/btrfs/007 > >> >> index 5df9ccb..5430613 100755 > >> >> --- a/tests/btrfs/007 > >> >> +++ b/tests/btrfs/007 > >> >> @@ -31,7 +31,6 @@ seq=`basename $0` > >> >> seqres=$RESULT_DIR/$seq > >> >> echo "QA output created by $seq" > >> >> > >> >> -here=`pwd` > >> >> tmp=`mktemp -d` > >> >> status=1 > >> > > >> > Yeah, redefining $here is a bad thing to do :/ > > > > Right, my mistake. It needs to be defined for the entire test, not > > in a requires function. Hence I think we should just export it from > > check.... > > Hum, ok. So the decision is to let the tests explicitly define the > variable "here", and not export "here" from the main check script. Well, that's the status quo right now. What I'm suggesting is that we should just export it from check and get rid of it from each test as a separate cleanup. i.e. It always needs to be set to the same value (i.e. the root of the xfstests tree) so there is no reason why it should not be set up once in a central location. Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] xfstests: add function _require_fssum() 2014-02-24 11:56 [PATCH] xfstests: add function _require_fssum() Filipe David Borba Manana 2014-02-24 12:23 ` Dave Chinner @ 2014-02-25 13:46 ` Filipe David Borba Manana 1 sibling, 0 replies; 7+ messages in thread From: Filipe David Borba Manana @ 2014-02-25 13:46 UTC (permalink / raw) To: xfs; +Cc: Filipe David Borba Manana, linux-btrfs To avoid repeating detection of fssum presence in many btrfs tests, as suggested by Dave Chinner. Also exported the variable "here" from the main control script, to avoid repeating its declaration in every single testcase file. Also removed the declaration of "here" from btrfs test cases that require the fssum program only. Removing it from all other test cases will be a separate change. Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com> --- V2: Exported the variable "here" from the main control file, as suggested by Dave Chinner. Like this we avoid declaring it again in the new function and in many other places. check | 2 +- common/rc | 6 ++++++ tests/btrfs/007 | 5 +---- tests/btrfs/016 | 5 +---- tests/btrfs/030 | 5 +---- tests/btrfs/038 | 5 +---- tests/btrfs/039 | 5 +---- tests/btrfs/040 | 5 +---- tests/btrfs/041 | 5 +---- tests/btrfs/042 | 5 +---- 10 files changed, 15 insertions(+), 33 deletions(-) mode change 100644 => 100755 tests/btrfs/016 diff --git a/check b/check index 268417c..b619f6d 100755 --- a/check +++ b/check @@ -32,7 +32,7 @@ diff="diff -u" showme=false have_test_arg=false randomize=false -here=`pwd` +export here=`pwd` xfile="" # start the initialisation work now diff --git a/common/rc b/common/rc index 5df504c..0f50f49 100644 --- a/common/rc +++ b/common/rc @@ -2144,6 +2144,12 @@ _require_cp_reflink() _notrun "This test requires a cp with --reflink support." } +_require_fssum() +{ + FSSUM_PROG=$here/src/fssum + [ -x $FSSUM_PROG ] || _notrun "fssum not built" +} + # Given 2 files, verify that they have the same mapping but different # inodes - i.e. an undisturbed reflink # Silent if so, make noise if not diff --git a/tests/btrfs/007 b/tests/btrfs/007 index 5df9ccb..5430613 100755 --- a/tests/btrfs/007 +++ b/tests/btrfs/007 @@ -31,7 +31,6 @@ seq=`basename $0` seqres=$RESULT_DIR/$seq echo "QA output created by $seq" -here=`pwd` tmp=`mktemp -d` status=1 @@ -52,11 +51,9 @@ _need_to_be_root _supported_fs btrfs _supported_os Linux _require_scratch +_require_fssum _require_seek_data_hole -FSSUM_PROG=$here/src/fssum -[ -x $FSSUM_PROG ] || _notrun "fssum not built" - rm -f $seqres.full workout() diff --git a/tests/btrfs/016 b/tests/btrfs/016 old mode 100644 new mode 100755 index 6faead1..d04c21a --- a/tests/btrfs/016 +++ b/tests/btrfs/016 @@ -26,7 +26,6 @@ seq=`basename $0` seqres=$RESULT_DIR/$seq echo "QA output created by $seq" -here=`pwd` tmp=`mktemp -d` tmp_dir=send_temp_$seq @@ -51,9 +50,7 @@ trap "_cleanup ; exit \$status" 0 1 2 3 15 _supported_fs btrfs _supported_os Linux _require_scratch - -FSSUM_PROG=$here/src/fssum -[ -x $FSSUM_PROG ] || _notrun "fssum not built" +_require_fssum _scratch_mkfs > /dev/null 2>&1 diff --git a/tests/btrfs/030 b/tests/btrfs/030 index a9f5fb4..a76a410 100755 --- a/tests/btrfs/030 +++ b/tests/btrfs/030 @@ -36,7 +36,6 @@ seq=`basename $0` seqres=$RESULT_DIR/$seq echo "QA output created by $seq" -here=`pwd` tmp=`mktemp -d` status=1 # failure is the default! trap "_cleanup; exit \$status" 0 1 2 3 15 @@ -54,11 +53,9 @@ _cleanup() _supported_fs btrfs _supported_os Linux _require_scratch +_require_fssum _need_to_be_root -FSSUM_PROG=$here/src/fssum -[ -x $FSSUM_PROG ] || _notrun "fssum not built" - rm -f $seqres.full _scratch_mkfs >/dev/null 2>&1 diff --git a/tests/btrfs/038 b/tests/btrfs/038 index 8893696..4941d3e 100755 --- a/tests/btrfs/038 +++ b/tests/btrfs/038 @@ -32,7 +32,6 @@ seq=`basename $0` seqres=$RESULT_DIR/$seq echo "QA output created by $seq" -here=`pwd` tmp=`mktemp -d` status=1 # failure is the default! trap "_cleanup; exit \$status" 0 1 2 3 15 @@ -50,11 +49,9 @@ _cleanup() _supported_fs btrfs _supported_os Linux _require_scratch +_require_fssum _need_to_be_root -FSSUM_PROG=$here/src/fssum -[ -x $FSSUM_PROG ] || _notrun "fssum not built" - rm -f $seqres.full _scratch_mkfs >/dev/null 2>&1 diff --git a/tests/btrfs/039 b/tests/btrfs/039 index 41e09be..758b23c 100755 --- a/tests/btrfs/039 +++ b/tests/btrfs/039 @@ -35,7 +35,6 @@ seq=`basename $0` seqres=$RESULT_DIR/$seq echo "QA output created by $seq" -here=`pwd` tmp=`mktemp -d` status=1 # failure is the default! trap "_cleanup; exit \$status" 0 1 2 3 15 @@ -53,11 +52,9 @@ _cleanup() _supported_fs btrfs _supported_os Linux _require_scratch +_require_fssum _need_to_be_root -FSSUM_PROG=$here/src/fssum -[ -x $FSSUM_PROG ] || _notrun "fssum not built" - rm -f $seqres.full _scratch_mkfs >/dev/null 2>&1 diff --git a/tests/btrfs/040 b/tests/btrfs/040 index 77c4a84..dfd495c 100755 --- a/tests/btrfs/040 +++ b/tests/btrfs/040 @@ -35,7 +35,6 @@ seq=`basename $0` seqres=$RESULT_DIR/$seq echo "QA output created by $seq" -here=`pwd` tmp=`mktemp -d` status=1 # failure is the default! trap "_cleanup; exit \$status" 0 1 2 3 15 @@ -53,11 +52,9 @@ _cleanup() _supported_fs btrfs _supported_os Linux _require_scratch +_require_fssum _need_to_be_root -FSSUM_PROG=$here/src/fssum -[ -x $FSSUM_PROG ] || _notrun "fssum not built" - rm -f $seqres.full _scratch_mkfs >/dev/null 2>&1 diff --git a/tests/btrfs/041 b/tests/btrfs/041 index bfc0e4b..a960001 100755 --- a/tests/btrfs/041 +++ b/tests/btrfs/041 @@ -31,7 +31,6 @@ seq=`basename $0` seqres=$RESULT_DIR/$seq echo "QA output created by $seq" -here=`pwd` tmp=`mktemp -d` status=1 # failure is the default! trap "_cleanup; exit \$status" 0 1 2 3 15 @@ -49,11 +48,9 @@ _cleanup() _supported_fs btrfs _supported_os Linux _require_scratch +_require_fssum _need_to_be_root -FSSUM_PROG=$here/src/fssum -[ -x $FSSUM_PROG ] || _notrun "fssum not built" - rm -f $seqres.full _scratch_mkfs >/dev/null 2>&1 diff --git a/tests/btrfs/042 b/tests/btrfs/042 index 2bd5147..9aa8967 100755 --- a/tests/btrfs/042 +++ b/tests/btrfs/042 @@ -36,7 +36,6 @@ seq=`basename $0` seqres=$RESULT_DIR/$seq echo "QA output created by $seq" -here=`pwd` tmp=`mktemp -d` status=1 # failure is the default! trap "_cleanup; exit \$status" 0 1 2 3 15 @@ -54,11 +53,9 @@ _cleanup() _supported_fs btrfs _supported_os Linux _require_scratch +_require_fssum _need_to_be_root -FSSUM_PROG=$here/src/fssum -[ -x $FSSUM_PROG ] || _notrun "fssum not built" - rm -f $seqres.full _scratch_mkfs >/dev/null 2>&1 -- 1.7.9.5 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-02-25 13:46 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-02-24 11:56 [PATCH] xfstests: add function _require_fssum() Filipe David Borba Manana 2014-02-24 12:23 ` Dave Chinner 2014-02-24 13:22 ` Filipe David Manana 2014-02-24 22:22 ` Dave Chinner 2014-02-24 23:08 ` Filipe David Manana 2014-02-24 23:35 ` Dave Chinner 2014-02-25 13:46 ` [PATCH v2] " Filipe David Borba Manana
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox