On Mon, Nov 17, 2014 at 04:53:50PM +0200, Omer Zilberberg wrote:
> > Another FYI: we can actually test any filesystem without a scratch
> > device configured:
> >
> > $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test ./check generic/120
> > FSTYP -- xfs (non-debug)
> > PLATFORM -- Linux/x86_64 test2 3.18.0-rc2-dgc+
> >
> > generic/120 16s ... [not run] this test requires a valid $SCRATCH_DEV
> > Not run: generic/120
> > Passed all 0 tests
> > $
>
> Please note that since commit 83ef157d, that is no longer true, because _require_test calls _is_block_dev with empty parameter, which prints usage:
> $ sudo TEST_DEV=/dev/sda TEST_DIR=/mnt/dev0 ./check generic/001
> FSTYP -- ext4
> PLATFORM -- Linux/x86_64 testvm 3.17.0
>
> generic/001 7s ... - output mismatch (see /opt/xfstests/results//generic/001.out.bad)
> --- tests/generic/001.out 2014-09-10 11:04:44.249185592 +0300
> +++ /opt/xfstests/results//generic/001.out.bad 2014-11-17 15:14:12.380061760 +0200
> @@ -1,4 +1,5 @@
> QA output created by 001
> +Usage: _is_block_dev dev
> cleanup
> setup ....................................
> iter 1 chain ... check ....................................
> ...
> (Run 'diff -u tests/generic/001.out /opt/xfstests/results//generic/001.out.bad' to see the entire diff)
> Ran: generic/001
> Failures: generic/001
> Failed 1 of 1 tests
>
> Here's a patch to fix that:
> ----
> Subject: [PATCH] _required_test: removed unneeded test for scratch_dev
>
> testing for scratch_dev in _required_test is unnecessary, since it is
> not required for these tests. Furthermore, if a scratch_dev is not given,
> all tests which are supposed to pass on test_dev fail, because
> _is_block_dev is given an empty string as scratch_dev, and prints usage.
I'm beginning to sound like a broken record. Why isn't this *invalid
config check* being done in the config parsing rather than on every
test that requires a scratch or test device?
i.e. removing the check is not the correct fix - checking it during
config parsing means none of these checks need to be done in
_require_test, nor in _require_scratch_nocheck.
i.e. in get_next_config() we already know the FSTYP, so we should
be checking that:
a) TEST_DEV is valid for fstyp
b) SCRATCH_DEV (if configured) is valid for fstyp
c) TEST_DEV != SCRATCH_DEV if they both exist and fstyp
indicates they should be block devices
And then we can pretty much assume that they are valid everywhere
else, and _require_scratch_nocheck() simply needs to check for a
non-empty string...
> Signed-off-by: Omer Zilberberg <omzg@xxxxxxxxxxxxx>
> ---
> common/rc | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/common/rc b/common/rc
> index d5e3aff..b863808 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1104,7 +1104,7 @@ _require_scratch()
> }
>
>
> -# this test needs a test partition - check we're ok & unmount it
> +# this test needs a test partition - check we're ok & mount if necessary
> #
> _require_test()
> {
> @@ -1138,10 +1138,6 @@ _require_test()
> then
> _notrun "this test requires a valid \$TEST_DEV"
> fi
> - if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ]
> - then
> - _notrun "this test requires a valid \$TEST_DEV"
> - fi
> if [ ! -d "$TEST_DIR" ]
> then
> _notrun "this test requires a valid \$TEST_DIR"
The patch is whitespace damaged. Please read:
https://www.kernel.org/doc/Documentation/email-clients.txt
and resend.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
