Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS

[Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>]

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-/8YdC2HfS5554TAoqtyWWQ@public.gmane.org>
> ---
>  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-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org