Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS
On 11/17/2014 10:22 PM, Dave Chinner wrote:
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 o...@plexistor.com
---
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
Here's the patch resent with whitespace fixed, at least as a temporary
solution...
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.
Signed-off-by: Omer Zilberberg o...@plexistor.com
---
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
--
Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS
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.
Signed-off-by: Omer Zilberberg o...@plexistor.com
---
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
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-cifs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS
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 o...@plexistor.com
---
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
da...@fromorbit.com
--
To unsubscribe from this list: send the line unsubscribe linux-cifs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS
On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote:
On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner da...@fromorbit.com 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 eg...@redhat.com 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 eg...@redhat.com
---
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 21
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.
--
Dave Chinner
da...@fromorbit.com
--
To unsubscribe from this list: send the line unsubscribe linux-cifs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS
On Sat, Nov 15, 2014 at 01:35:33PM +0800, Eryu Guan wrote:
On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote:
On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner da...@fromorbit.com 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 eg...@redhat.com 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 eg...@redhat.com
---
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.
You're right, I missed that _scratch_mkfs is also called by ./check,
and if there's no SCRATCH_MNT set in local.config, only SCRATCH_DEV is
set, _scratch_mkfs can be dangerous.
As I asked earlier in this thread: why isn't get_next_config()
catching this?
I propose this patch, which skips _scratch_cleanup_files if called by check
[root@hp-dl388eg8-01 xfstests]# git diff
diff --git a/common/rc b/common/rc
index 435f74f..254fb66 100644
--- a/common/rc
+++ b/common/rc
@@ -554,6 +554,11 @@ _mkfs_dev()
# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
_scratch_cleanup_files()
{
+ # do nothing if called by check, variables are not fully valided yet
+ # SCRATCH_MNT can be empty, which is dangerous
+ if [ $iam == check ]; then
+ return
+ fi
Again, this is the wrong place to try to fix this - we haven't fixed
the landmine that has left us running with an invalid config. IOWs,
by the time _scratch_mkfs is called from *anywhere* we should have
fully validated the environment to be correct and valid. Parsing and
validating the config we have loaded from the config file is the job
of get_next_config(), yes?
Cheers,
Dave.
--
Dave Chinner
da...@fromorbit.com
--
To unsubscribe from this list: send the line unsubscribe linux-cifs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS
On Mon, Nov 17, 2014 at 04:41:41PM +1100, Dave Chinner wrote:
On Sat, Nov 15, 2014 at 01:35:33PM +0800, Eryu Guan wrote:
On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote:
On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner da...@fromorbit.com 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 eg...@redhat.com 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 eg...@redhat.com
---
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.
You're right, I missed that _scratch_mkfs is also called by ./check,
and if there's no SCRATCH_MNT set in local.config, only SCRATCH_DEV is
set, _scratch_mkfs can be dangerous.
As I asked earlier in this thread: why isn't get_next_config()
catching this?
I think I see the problem, this patch catches the empty SCRATCH_MNT
and works for me:
eguan@dhcp-13-216:~/workspace/src/xfstests$ git diff
diff --git a/common/config b/common/config
index 1cb08c0..409f1b8 100644
--- a/common/config
+++ b/common/config
@@ -464,7 +464,7 @@ get_next_config() {
exit 1
fi
- if [ ! -z $SCRATCH_MNT -a ! -d $SCRATCH_MNT ]; then
+ if [ ! -z $SCRATCH_MNT -o ! -d $SCRATCH_MNT ]; then
echo common/config: Error: \$SCRATCH_MNT ($SCRATCH_MNT) is not
a directory
exit 1
fi
If it looks sane I will send out a patch.
I propose this patch, which skips _scratch_cleanup_files if called by check
[root@hp-dl388eg8-01 xfstests]# git diff
diff --git a/common/rc b/common/rc
index 435f74f..254fb66 100644
--- a/common/rc
+++ b/common/rc
@@ -554,6 +554,11 @@ _mkfs_dev()
# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
_scratch_cleanup_files()
{
+ # do nothing if called by check, variables are not fully valided yet
+ # SCRATCH_MNT can be empty, which is dangerous
+ if [ $iam == check ]; then
+ return
+ fi
Again, this is the wrong place to try to fix this - we haven't fixed
the landmine that has left us running with an invalid config. IOWs,
by the time _scratch_mkfs is called from *anywhere* we should have
fully validated the environment to be correct and valid. Parsing and
validating the config we have loaded from the config file is the job
of get_next_config(), yes?
Yes, that makes more sense, thanks for the explanation!
Thanks,
Eryu
Cheers,
Dave.
--
Dave Chinner
da...@fromorbit.com
--
To unsubscribe from this list: send the line unsubscribe fstests in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-cifs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS
On Mon, Nov 17, 2014 at 02:06:03PM +0800, Eryu Guan wrote:
On Mon, Nov 17, 2014 at 04:41:41PM +1100, Dave Chinner wrote:
On Sat, Nov 15, 2014 at 01:35:33PM +0800, Eryu Guan wrote:
On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote:
On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner da...@fromorbit.com
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 eg...@redhat.com 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 eg...@redhat.com
---
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.
You're right, I missed that _scratch_mkfs is also called by ./check,
and if there's no SCRATCH_MNT set in local.config, only SCRATCH_DEV is
set, _scratch_mkfs can be dangerous.
As I asked earlier in this thread: why isn't get_next_config()
catching this?
I think I see the problem, this patch catches the empty SCRATCH_MNT
and works for me:
eguan@dhcp-13-216:~/workspace/src/xfstests$ git diff
diff --git a/common/config b/common/config
index 1cb08c0..409f1b8 100644
--- a/common/config
+++ b/common/config
@@ -464,7 +464,7 @@ get_next_config() {
exit 1
fi
- if [ ! -z $SCRATCH_MNT -a ! -d $SCRATCH_MNT ]; then
+ if [ ! -z $SCRATCH_MNT -o ! -d $SCRATCH_MNT ]; then
if (not an empty string OR not a directory)
That doesn't work if you define $SCRATCH_MNT - the directory check
fails on an empty string.
As it is, this isn't solving the underlying problem - that empty
string check is to avoid failing configs that are not configured
with a scratch device. i.e. if you define SCRATCH_MNT, it must be a
directory. However, there's no check that if you define SCRATCH_DEV
you've also defined a SCRATCH_MNT...
IOWs, the underlying problem here is that SCRATCH_DEV is defined,
but SCRATCH_MNT is not and we are not catching that issue. FYI, the
setup script is used for checking these sorts of things. e.g. with
an empty config:
$ sudo ./setup
Warning: need to define parameters for host test2
or set variables:
TEST_DIR TEST_DEV
$
And you'll note that it catches when we only have one of the two
necessary TEST_D* variables set. However, even with XFS as the
probe fstyp, it doesn't error out if I don't define a SCRATCH_MNT:
$ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test SCRATCH_DEV=/dev/vdb
./setup
TEST: DIR=/mnt/test DEV=/dev/vda rt=[] log=[]
TAPE: dev=[] rmt=[] rmtirix=[@]
SCRATCH: MNT= DEV=/dev/vdb rt=[/dev/vdc] log=[/dev/vdd]
VARIABLES: external=no largeblk=no fstyp=xfs
large_scratch_dev=no attrsecure=no
$
But it should fail with a sane error message indicating that we
caught the invalid config and didn't leave check to fail randomly
wiht some downstream error like:
$ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test SCRATCH_DEV=/dev/vdb ./check
generic/120
check: failed to mount $SCRATCH_DEV using specified options
Passed all 0 tests
$
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
$
So, really, all we need to do is ensure that SCRATCH_DEV/SCRATCH_MNT
are sanely configured...
Cheers,
Dave.
--
Dave Chinner
da...@fromorbit.com
--
To unsubscribe from this list: send the line unsubscribe linux-cifs in
the body of a message to
Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS
On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner da...@fromorbit.com 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 eg...@redhat.com 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 eg...@redhat.com
---
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
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
}
--
Thanks,
Steve
--
To unsubscribe from this list: send the line unsubscribe linux-cifs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS
On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote:
On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner da...@fromorbit.com 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 eg...@redhat.com 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 eg...@redhat.com
---
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.
You're right, I missed that _scratch_mkfs is also called by ./check,
and if there's no SCRATCH_MNT set in local.config, only SCRATCH_DEV is
set, _scratch_mkfs can be dangerous.
[root@hp-dl388eg8-01 xfstests]# ./check -nfs generic/001
FSTYP -- nfs
PLATFORM -- Linux/x86_64 hp-dl388eg8-01 3.18.0-rc4+
MKFS_OPTIONS -- -bsize=4096 localhost:/mnt/nfsscratch
MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 localhost:/mnt/nfsscratch
our local _scratch_mkfs routine ...
mount: can't find localhost:/mnt/nfsscratch in /etc/fstab
rm -rf /*
umount: localhost:/mnt/nfsscratch: mountpoint not found
check: failed to mkfs $SCRATCH_DEV using specified options
Passed all 0 tests
(I replaced the real rm -rf ... by echo 'rm -rf ...' in above test)
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
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
}
I propose this patch, which skips _scratch_cleanup_files if called by check
[root@hp-dl388eg8-01 xfstests]# git diff
diff --git a/common/rc b/common/rc
index 435f74f..254fb66 100644
--- a/common/rc
+++ b/common/rc
@@ -554,6 +554,11 @@ _mkfs_dev()
# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
_scratch_cleanup_files()
{
+ # do nothing if called by check, variables are not fully valided yet
+ # SCRATCH_MNT can be empty, which is dangerous
+ if [ $iam == check ]; then
+ return
+ fi
_scratch_mount
rm -rf $SCRATCH_MNT/*
_scratch_unmount
Thanks,
Eryu
--
To unsubscribe from this list: send the line unsubscribe linux-cifs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS
There should be a check to make sure SCRATCH_MNT exists before you
wipe the whole disk
+# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS
+_scratch_cleanup_files()
+{
+ _scratch_mount
+ rm -rf $SCRATCH_MNT/*
+ _scratch_unmount
+}
+
so if no SCRATCH_MNT then this does rm -rf/*
right ... (and wipes out your whole system ...)
On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan eg...@redhat.com 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 eg...@redhat.com
---
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
+}
+
_scratch_mkfs()
{
case $FSTYP in
@@ -558,7 +566,9 @@ _scratch_mkfs()
_scratch_mkfs_xfs $*
;;
nfs*)
- # do nothing for nfs
+ # unable to re-create NFS, just remove all files in $SCRATCH_MNT to
+ # avoid EEXIST caused by the leftover files created in previous runs
+_scratch_cleanup_files
;;
cifs)
# do nothing for cifs
@@ -1032,8 +1042,14 @@ _require_scratch_nocheck()
{
case $FSTYP in
nfs*)
- _notrun requires a scratch device
-;;
+ echo $SCRATCH_DEV | grep -q :/ /dev/null 21
+ 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
+ ;;
cifs)
_notrun requires a scratch device
;;
--
1.9.3
--
To unsubscribe from this list: send the line unsubscribe linux-nfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Steve
--
To unsubscribe from this list: send the line unsubscribe linux-cifs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS
On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote:
On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan eg...@redhat.com 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 eg...@redhat.com
---
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()
Cheers,
Dave.
--
Dave Chinner
da...@fromorbit.com
--
To unsubscribe from this list: send the line unsubscribe linux-cifs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
