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

Commit Message

Steve French Nov. 14, 2014, 5:02 p.m. UTC

On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <[e-mail]> wrote:
> On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote:
>> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <[e-mail]> wrote:
>> > This commit disables tests requires scratch dev running on NFS
>> >
>> > c041421 xfstests: stop special casing nfs and udf
>> >
>> > Now re-enable them to get a larger test coverage on NFS.
>> >
>> > Signed-off-by: Eryu Guan <[e-mail]>
>> > ---
>> >  common/rc | 22 +++++++++++++++++++---
>> >  1 file changed, 19 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/common/rc b/common/rc
>> > index 747cf72..ae03712 100644
>> > --- a/common/rc
>> > +++ b/common/rc
>> > @@ -551,6 +551,14 @@ _mkfs_dev()
>> >      rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd
>> >  }
>> >
>> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
>> > +_scratch_cleanup_files()
>> > +{
>> > +       _scratch_mount
>> > +       rm -rf $SCRATCH_MNT/*
>> > +       _scratch_unmount
>> > +}
>>
>> There should be a check to make sure SCRATCH_MNT exists before you
>> wipe the whole disk ....
>>
>> so if no SCRATCH_MNT then this does rm -rf/*
>> right ... (and wipes out your whole system ...)
>
> You can't get to that function until after all the checks that
> SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that
> is only called in tests after all the startup checks validate
> devices and mounts exist. i.e. see common/config::get_next_config()

Well, I reproduced it easily enough again today (after taking a
snapshot of the VM)
by simply running generic/120 against NFS with SCRATCH_MNT not
specified in local.config
Dros also ran into this problem.

The patch below fixes it for me but it wasn't immediately obvious how to best
return info to the user (ie print messages that make sense here -
"echo" seems to be supressed
in common/rc and notrun exits and logs it to a file but not to the
screen in this case)

sfrench@ubuntu:~/xfstests$ git diff -a

Comments

Dave Chinner Nov. 17, 2014, 5:34 a.m. UTC | #1

On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote:
> On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <[e-mail]> wrote:
> > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote:
> >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <[e-mail]> wrote:
> >> > This commit disables tests requires scratch dev running on NFS
> >> >
> >> > c041421 xfstests: stop special casing nfs and udf
> >> >
> >> > Now re-enable them to get a larger test coverage on NFS.
> >> >
> >> > Signed-off-by: Eryu Guan <[e-mail]>
> >> > ---
> >> >  common/rc | 22 +++++++++++++++++++---
> >> >  1 file changed, 19 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/common/rc b/common/rc
> >> > index 747cf72..ae03712 100644
> >> > --- a/common/rc
> >> > +++ b/common/rc
> >> > @@ -551,6 +551,14 @@ _mkfs_dev()
> >> >      rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd
> >> >  }
> >> >
> >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
> >> > +_scratch_cleanup_files()
> >> > +{
> >> > +       _scratch_mount
> >> > +       rm -rf $SCRATCH_MNT/*
> >> > +       _scratch_unmount
> >> > +}
> >>
> >> There should be a check to make sure SCRATCH_MNT exists before you
> >> wipe the whole disk ....
> >>
> >> so if no SCRATCH_MNT then this does rm -rf/*
> >> right ... (and wipes out your whole system ...)
> >
> > You can't get to that function until after all the checks that
> > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that
> > is only called in tests after all the startup checks validate
> > devices and mounts exist. i.e. see common/config::get_next_config()
> 
> Well, I reproduced it easily enough again today (after taking a
> snapshot of the VM)
> by simply running generic/120 against NFS with SCRATCH_MNT not
> specified in local.config
> Dros also ran into this problem.

tests/generic/120 does this:

....
_require_scratch
_scratch_mkfs ....

So we call:

_require_scratch
  _require_scratch_nocheck

	case "$FSTYP" in
        nfs*)                                                                    
                echo $SCRATCH_DEV | grep -q ":/" > /dev/null 2>&1                
                if [ -z "$SCRATCH_DEV" -o "$?" != "0" ]; then                    
                        _notrun "this test requires a valid \$SCRATCH_DEV"       
                fi                                                               
                if [ ! -d "$SCRATCH_MNT" ]; then                                 
                        _notrun "this test requires a valid \$SCRATCH_MNT"       
                fi                                                               

If $SCRATCH_MNT is empty, then it should fail right there. An empty
string gives:

$ if [ ! -d "" ]; then
> echo foo
> fi
foo
$

before it gets anywhere near _scratch_mkfs. So, why isn't
generic/120 aborting during the _require_scratch check?

Cheers,

Dave.

Patch

diff --git a/common/rc b/common/rc
index d5e3aff..866244b 100644
--- a/common/rc
+++ b/common/rc
@@ -555,6 +555,9 @@  _mkfs_dev()
 _scratch_cleanup_files()
 {
        _scratch_mount
+       if ! [ "$SCRATCH_MNT" ]; then
+               _notrun "this test requires a \$SCRATCH_MNT to be specified"
+       fi
        rm -rf $SCRATCH_MNT/*
        _scratch_unmount
 }