* [PATCH 0/6 v3] xfstests: some small fixes and mkfs validation test
@ 2016-07-14 12:43 Jan Tulak
2016-07-14 12:43 ` [PATCH 1/6] xfstests: Fix installation for extended names Jan Tulak
` (7 more replies)
0 siblings, 8 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-14 12:43 UTC (permalink / raw)
To: fstests; +Cc: Jan Tulak
This set is a merge of all my recent patches. The reason for the merge is that
the patches are sequentially dependent and making a set is the safest way of
ensuring their correct order after a merge.
One patch, "filename handling - fix early wildcard expansion", was
dropped for now. Patch "add _require_xfs_mkfs_validation to common/rc" is new,
and update of xfs/096 and input validation tests were changed.
Jan Tulak (6):
xfstests: Fix installation for extended names
fstests: filename handling for extended names in ./check was on a
wrong place
xfstests: remove unused variable
xfstests: add _require_xfs_mkfs_validation to common/rc
xfstests: update xfs/096 for new behaviour
xfstests: Add mkfs input validation tests
check | 26 +--
common/rc | 29 ++++
include/buildrules | 31 ++++
tests/btrfs/Makefile | 4 +-
tests/cifs/Makefile | 4 +-
tests/ext4/Makefile | 4 +-
tests/f2fs/Makefile | 4 +-
tests/generic/Makefile | 4 +-
tests/overlay/Makefile | 4 +-
tests/shared/Makefile | 4 +-
tests/udf/Makefile | 4 +-
tests/xfs/096 | 1 +
tests/xfs/400-input-validation | 338 +++++++++++++++++++++++++++++++++++++
tests/xfs/400-input-validation.out | 2 +
tests/xfs/Makefile | 4 +-
tests/xfs/group | 1 +
16 files changed, 433 insertions(+), 31 deletions(-)
create mode 100755 tests/xfs/400-input-validation
create mode 100644 tests/xfs/400-input-validation.out
--
2.5.5
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/6] xfstests: Fix installation for extended names
2016-07-14 12:43 [PATCH 0/6 v3] xfstests: some small fixes and mkfs validation test Jan Tulak
@ 2016-07-14 12:43 ` Jan Tulak
2016-07-14 12:43 ` [PATCH 2/6] fstests: filename handling for extended names in ./check was on a wrong place Jan Tulak
` (6 subsequent siblings)
7 siblings, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-14 12:43 UTC (permalink / raw)
To: fstests; +Cc: Jan Tulak
xfstests supports extended test names like 314-foo-bar, but installation of
these tests was skipped (not matching a regexp). So this patch fixes the
makefiles in tests/*/
The include/buildrules change was written by Dave Chinner.
Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
include/buildrules | 31 +++++++++++++++++++++++++++++++
tests/btrfs/Makefile | 4 ++--
tests/cifs/Makefile | 4 ++--
tests/ext4/Makefile | 4 ++--
tests/f2fs/Makefile | 4 ++--
tests/generic/Makefile | 4 ++--
tests/overlay/Makefile | 4 ++--
tests/shared/Makefile | 4 ++--
tests/udf/Makefile | 4 ++--
tests/xfs/Makefile | 4 ++--
10 files changed, 49 insertions(+), 18 deletions(-)
diff --git a/include/buildrules b/include/buildrules
index c8a7c47..1269c8c 100644
--- a/include/buildrules
+++ b/include/buildrules
@@ -100,3 +100,34 @@ MAKEDEP := $(MAKEDEPEND) $(CFLAGS)
cp /dev/null .dep; \
fi
+# Gather files for installing into two lists:
+# TESTS with executable scripts and OUTFILES with all the test outputs.
+# Makefile has a very small matching, so we have to go this long way.
+
+# Start with all test related files:
+ALLFILES = $(wildcard [0-9]??*)
+
+# Now build a list of known output files. Unfortunately, the
+# multiple output test files are poorly handled as makefiles don't
+# handle wildcarded multi-suffix matching. Hence we separate the
+# processing of these right now.
+EXTENDED_OUTFILES = $(wildcard [0-9]??*.out.*)
+BASIC_OUTFILES = $(wildcard [0-9]??*.out)
+OUTFILES = $(EXTENDED_OUTFILES) $(BASIC_OUTFILES)
+
+# Strip suffix to get matching tests. We want to strip from the
+# first "." to the end, but makefiles don't have a built in
+# operative for that. So:
+#
+# Hack: strip the multiple segments after .out with repeated basename calls.
+EXTFILTER1 = $(basename $(EXTENDED_OUTFILES))
+EXTFILTER2 = $(basename $(EXTFILTER1))
+EXTFILTER3 = $(basename $(EXTFILTER2))
+EXTFILTER4 = $(basename $(EXTFILTER3))
+
+# final filter list
+FILTER = $(basename $(EXTFILTER4) $(BASIC_OUTFILES))
+
+# finally, select the test files by filtering against against the
+# stripped output files and sort them to remove duplicates.
+TESTS = $(sort $(filter $(ALLFILES), $(FILTER)))
diff --git a/tests/btrfs/Makefile b/tests/btrfs/Makefile
index e1a5be1..2d93642 100644
--- a/tests/btrfs/Makefile
+++ b/tests/btrfs/Makefile
@@ -12,9 +12,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
diff --git a/tests/cifs/Makefile b/tests/cifs/Makefile
index 9176e5c..0c5cf3b 100644
--- a/tests/cifs/Makefile
+++ b/tests/cifs/Makefile
@@ -12,9 +12,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
diff --git a/tests/ext4/Makefile b/tests/ext4/Makefile
index 7a3c8e1..beb1541 100644
--- a/tests/ext4/Makefile
+++ b/tests/ext4/Makefile
@@ -12,9 +12,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
diff --git a/tests/f2fs/Makefile b/tests/f2fs/Makefile
index 4d00e9e..d13bca3 100644
--- a/tests/f2fs/Makefile
+++ b/tests/f2fs/Makefile
@@ -13,9 +13,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
diff --git a/tests/generic/Makefile b/tests/generic/Makefile
index 9529fb8..3878d05 100644
--- a/tests/generic/Makefile
+++ b/tests/generic/Makefile
@@ -12,9 +12,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
diff --git a/tests/overlay/Makefile b/tests/overlay/Makefile
index 63c9878..b07f892 100644
--- a/tests/overlay/Makefile
+++ b/tests/overlay/Makefile
@@ -12,9 +12,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
diff --git a/tests/shared/Makefile b/tests/shared/Makefile
index cbd87f9..8a83278 100644
--- a/tests/shared/Makefile
+++ b/tests/shared/Makefile
@@ -12,9 +12,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
diff --git a/tests/udf/Makefile b/tests/udf/Makefile
index 1d96658..c9c9f1b 100644
--- a/tests/udf/Makefile
+++ b/tests/udf/Makefile
@@ -12,9 +12,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
diff --git a/tests/xfs/Makefile b/tests/xfs/Makefile
index db94be0..d64800e 100644
--- a/tests/xfs/Makefile
+++ b/tests/xfs/Makefile
@@ -12,9 +12,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
--
2.5.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/6] fstests: filename handling for extended names in ./check was on a wrong place
2016-07-14 12:43 [PATCH 0/6 v3] xfstests: some small fixes and mkfs validation test Jan Tulak
2016-07-14 12:43 ` [PATCH 1/6] xfstests: Fix installation for extended names Jan Tulak
@ 2016-07-14 12:43 ` Jan Tulak
2016-07-14 12:43 ` [PATCH 3/6] xfstests: remove unused variable Jan Tulak
` (5 subsequent siblings)
7 siblings, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-14 12:43 UTC (permalink / raw)
To: fstests; +Cc: Jan Tulak
The code handling "./check foo/123", when the real test is "foo/123-bar-baz"
was moved to the earliest position, so everything working with the test name or
path will know the full name. Thus, no "123" and "123-bar-baz" mix is possible.
An example of this issue is $testname.notrun file. When _notrun "foo" was run
during ./check foo/$name command, it created $name.notrun. But few lines later,
it wanted $fullname.notrun. So if you did ./check foo/999, but the file was
999-bar-baz, then you got comparing outputs (and most likely a fail) instead of
a skip.
Another example of this mix is in xfstests output:
./check xfs/999
[...]
xfs/999 0s ... 0s
Ran: xfs/999-test-case
Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
UPDATE:
Just more explanation in the commit message.
check | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/check b/check
index 5be183f..ef6bd47 100755
--- a/check
+++ b/check
@@ -543,6 +543,20 @@ for section in $HOST_OPTIONS_SECTIONS; do
for seq in $list
do
err=false
+ if [ ! -f $seq ]; then
+ # Try to get full name in case the user supplied only seq id
+ # and the test has a name. A bit of hassle to find really
+ # the test and not its sample output or helping files.
+ bname=$(basename $seq)
+ full_seq=$(find $(dirname $seq) -name $bname* -executable |
+ awk '(NR == 1 || length < length(shortest)) { shortest = $0 }\
+ END { print shortest }')
+ if [ -f $full_seq ] \
+ && [ x$(echo $bname | grep -o "^$VALID_TEST_ID") != x ]; then
+ seq=$full_seq
+ seqnum=${full_seq#*/}
+ fi
+ fi
# the filename for the test and the name output are different.
# we don't include the tests/ directory in the name output.
@@ -566,19 +580,6 @@ for section in $HOST_OPTIONS_SECTIONS; do
if $showme; then
echo
continue
- elif [ ! -f $seq ]; then
- # Try to get full name in case the user supplied only seq id
- # and the test has a name. A bit of hassle to find really
- # the test and not its sample output or helping files.
- bname=$(basename $seq)
- full_seq=$(find $(dirname $seq) -name $bname* -executable |
- awk '(NR == 1 || length < length(shortest)) { shortest = $0 }\
- END { print shortest }')
- if [ -f $full_seq ] \
- && [ x$(echo $bname | grep -o "^$VALID_TEST_ID") != x ]; then
- seq=$full_seq
- seqnum=${full_seq#*/}
- fi
fi
if [ ! -f $seq ]; then
--
2.5.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 3/6] xfstests: remove unused variable
2016-07-14 12:43 [PATCH 0/6 v3] xfstests: some small fixes and mkfs validation test Jan Tulak
2016-07-14 12:43 ` [PATCH 1/6] xfstests: Fix installation for extended names Jan Tulak
2016-07-14 12:43 ` [PATCH 2/6] fstests: filename handling for extended names in ./check was on a wrong place Jan Tulak
@ 2016-07-14 12:43 ` Jan Tulak
2016-07-14 12:43 ` [PATCH 4/6] xfstests: add _require_xfs_mkfs_validation to common/rc Jan Tulak
` (4 subsequent siblings)
7 siblings, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-14 12:43 UTC (permalink / raw)
To: fstests; +Cc: Jan Tulak
After the previous patch moved few lines of code, one seqnum assignment is now
immediately overwritten by another. Remove the useless one.
Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
check | 1 -
1 file changed, 1 deletion(-)
diff --git a/check b/check
index ef6bd47..58d8869 100755
--- a/check
+++ b/check
@@ -554,7 +554,6 @@ for section in $HOST_OPTIONS_SECTIONS; do
if [ -f $full_seq ] \
&& [ x$(echo $bname | grep -o "^$VALID_TEST_ID") != x ]; then
seq=$full_seq
- seqnum=${full_seq#*/}
fi
fi
--
2.5.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 4/6] xfstests: add _require_xfs_mkfs_validation to common/rc
2016-07-14 12:43 [PATCH 0/6 v3] xfstests: some small fixes and mkfs validation test Jan Tulak
` (2 preceding siblings ...)
2016-07-14 12:43 ` [PATCH 3/6] xfstests: remove unused variable Jan Tulak
@ 2016-07-14 12:43 ` Jan Tulak
2016-07-14 14:21 ` Eryu Guan
2016-07-14 15:57 ` [PATCH v2] " Jan Tulak
2016-07-14 12:43 ` [PATCH 5/6] xfstests: update xfs/096 for new behaviour Jan Tulak
` (3 subsequent siblings)
7 siblings, 2 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-14 12:43 UTC (permalink / raw)
To: fstests; +Cc: Jan Tulak
Add a simple way to skip a test if it is (or is not) run on mkfs correctly
validating inputs.
Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
common/rc | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/common/rc b/common/rc
index 0c68e4f..72f9901 100644
--- a/common/rc
+++ b/common/rc
@@ -3843,6 +3843,35 @@ _get_fs_sysfs_attr()
cat /sys/fs/${FSTYP}/${dname}/${attr}
}
+# Skip if we are running an older binary without the stricter input checks.
+# Make multiple checks to be sure that there is no regression on the one
+# selected feature check, which would skew the result.
+_require_xfs_mkfs_validation()
+{
+ _require_scratch
+ $MKFS_XFS_PROG -f -N -s size=2s $SCRATCH_DEV >/dev/null 2>&1
+ sum=$?
+ $MKFS_XFS_PROG -f -N -l version=2,su=$((256 * 1024 + 4096)) $SCRATCH_DEV >/dev/null 2>&1
+ sum=`expr $sum + $?`
+
+ if [ "$sum" -eq 0 ]; then
+ _notrun "Requires newer mkfs with stricter input checks."
+ fi
+}
+
+# The oposite of _require_xfs_mkfs_validation.
+_require_xfs_mkfs_without_validation()
+{
+ _require_scratch
+ $MKFS_XFS_PROG -f -N -s size=2s $SCRATCH_DEV >/dev/null 2>&1
+ sum=$?
+ $MKFS_XFS_PROG -f -N -l version=2,su=$((256 * 1024 + 4096)) $SCRATCH_DEV >/dev/null 2>&1
+ sum=`expr $sum + $?`
+
+ if [ "$sum" -eq 2 ]; then
+ _notrun "Requires older mkfs without stricter input checks."
+ fi
+}
init_rc
################################################################################
--
2.5.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 5/6] xfstests: update xfs/096 for new behaviour
2016-07-14 12:43 [PATCH 0/6 v3] xfstests: some small fixes and mkfs validation test Jan Tulak
` (3 preceding siblings ...)
2016-07-14 12:43 ` [PATCH 4/6] xfstests: add _require_xfs_mkfs_validation to common/rc Jan Tulak
@ 2016-07-14 12:43 ` Jan Tulak
2016-07-14 12:43 ` [PATCH 6/6] xfstests: Add mkfs input validation tests Jan Tulak
` (2 subsequent siblings)
7 siblings, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-14 12:43 UTC (permalink / raw)
To: fstests; +Cc: Jan Tulak
Because we recently changed how mkfs behaves when it gets incorrect/invalid
values, add a feature check to run this test only on older binaries, which
accepts invalid sunit values.
Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
UPDATE:
Throw away all changes and replace them with a single
_require_xfs_mkfs_without_validation from common/rc.
Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
tests/xfs/096 | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/xfs/096 b/tests/xfs/096
index f949e83..64754b2 100755
--- a/tests/xfs/096
+++ b/tests/xfs/096
@@ -105,6 +105,7 @@ _supported_fs xfs
_supported_os IRIX Linux
_require_scratch
_require_v2log
+_require_xfs_mkfs_without_validation
# choose .out file based on internal/external log
rm -f $seqfull.out
--
2.5.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 6/6] xfstests: Add mkfs input validation tests
2016-07-14 12:43 [PATCH 0/6 v3] xfstests: some small fixes and mkfs validation test Jan Tulak
` (4 preceding siblings ...)
2016-07-14 12:43 ` [PATCH 5/6] xfstests: update xfs/096 for new behaviour Jan Tulak
@ 2016-07-14 12:43 ` Jan Tulak
2016-07-16 9:33 ` Eryu Guan
2016-07-16 10:28 ` [PATCH 0/6 v3] xfstests: some small fixes and mkfs validation test Eryu Guan
2016-07-18 8:47 ` [PATCH v2] xfstests: Fix installation for extended names Jan Tulak
7 siblings, 1 reply; 30+ messages in thread
From: Jan Tulak @ 2016-07-14 12:43 UTC (permalink / raw)
To: fstests; +Cc: Jan Tulak, Dave Chinner
mkfs.xfs does not do a very good job of input validation. This test
is designed to exercise the input validation and test good/bad
combinations of options being set. It will not pass on an old
mkfs.xfs binary - it is designed to be the test case for an input
validation cleanup (merged in spring/summer 2016).
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
CHANGES:
Use common/rc feature check for mkfs version testing.
Cheers,
Jan
Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
tests/xfs/400-input-validation | 338 +++++++++++++++++++++++++++++++++++++
tests/xfs/400-input-validation.out | 2 +
tests/xfs/group | 1 +
3 files changed, 341 insertions(+)
create mode 100755 tests/xfs/400-input-validation
create mode 100644 tests/xfs/400-input-validation.out
diff --git a/tests/xfs/400-input-validation b/tests/xfs/400-input-validation
new file mode 100755
index 0000000..83a4ff9
--- /dev/null
+++ b/tests/xfs/400-input-validation
@@ -0,0 +1,338 @@
+#! /bin/bash
+# FS QA Test No. xfs/400
+#
+# mkfs.xfs input validation test. Designed to break mkfs.xfs if it doesn't
+# filter garbage input or invalid option combinations correctly.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2016 Red Hat, Inc. All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+#-----------------------------------------------------------------------
+#
+
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1 # failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+ cd /
+ rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs xfs
+_supported_os Linux
+_require_scratch
+_require_xfs_mkfs_validation
+
+
+
+rm -f $seqres.full
+echo silence is golden
+
+# clear out any options to mkfs first. We want to test realtime and external log
+# devices if we can, but we also want to control them ourselves.
+logdev=$SCRATCH_LOGDEV
+rtdev=$SCRATCH_RTDEV
+
+MKFS_OPTIONS=
+SCRATCH_LOGDEV=
+SCRATCH_RTDEV=
+
+# limit the image size of the filesystem being created to something small
+fssize=$((4 * 1024 * 1024 * 1024))
+logsize=$((4 * 1024 * 1024 * 100))
+fsimg=$TEST_DIR/$seq.img
+
+do_mkfs_pass()
+{
+ echo >> $seqres.full
+ echo "pass expected $*" >> $seqres.full
+ $MKFS_XFS_PROG -f -N $* >> $seqres.full 2>&1
+ [ $? -ne 0 ] && echo "fail $*"
+}
+
+do_mkfs_fail()
+{
+ echo >> $seqres.full
+ echo "fail expected $*" >> $seqres.full
+ $MKFS_XFS_PROG -f -N $* >> $seqres.full 2>&1
+ [ $? -eq 0 ] && echo "pass $*"
+}
+
+reset_fsimg()
+{
+ rm -f $fsimg
+ $XFS_IO_PROG -f -c "truncate $fssize" $fsimg
+}
+
+reset_fsimg
+
+do_mkfs_pass $SCRATCH_DEV
+
+# basic "should fail" options
+
+# specifying sector sizes in sectors or blocks or garbage
+do_mkfs_fail -s size=2s $SCRATCH_DEV
+do_mkfs_fail -d sectsize=2s $SCRATCH_DEV
+do_mkfs_fail -l sectsize=2s $SCRATCH_DEV
+do_mkfs_fail -s size=2b $SCRATCH_DEV
+do_mkfs_fail -d sectsize=2b $SCRATCH_DEV
+do_mkfs_fail -l sectsize=2b $SCRATCH_DEV
+
+do_mkfs_fail -s size=grot $SCRATCH_DEV
+do_mkfs_fail -s size=2yerk $SCRATCH_DEV
+do_mkfs_fail -d sectsize=blah $SCRATCH_DEV
+do_mkfs_fail -d sectsize=2foo $SCRATCH_DEV
+do_mkfs_fail -l sectsize=nggh $SCRATCH_DEV
+do_mkfs_fail -l sectsize=2nggh $SCRATCH_DEV
+
+# conflicting sector/block sizes
+do_mkfs_fail -s size=512 -d sectsize=1024 $SCRATCH_DEV
+do_mkfs_fail -s size=512 -l sectsize=1024 $SCRATCH_DEV
+do_mkfs_fail -d sectsize=2048 -l sectsize=1024 $SCRATCH_DEV
+
+do_mkfs_fail -b size=512 -s size=1024 $SCRATCH_DEV
+do_mkfs_fail -b size=512 -d sectsize=1024 $SCRATCH_DEV
+do_mkfs_fail -b size=512 -l sectsize=1024 $SCRATCH_DEV
+
+# specifying block sizes in sectors without specifying sector size
+# or in blocks or garbage
+do_mkfs_fail -b size=2s $SCRATCH_DEV
+do_mkfs_fail -b size=2b $SCRATCH_DEV
+do_mkfs_fail -b size=nfi $SCRATCH_DEV
+do_mkfs_fail -b size=4096nfi $SCRATCH_DEV
+do_mkfs_fail -n size=2s $SCRATCH_DEV
+do_mkfs_fail -n size=2b $SCRATCH_DEV
+do_mkfs_fail -n size=nfi $SCRATCH_DEV
+do_mkfs_fail -n size=4096nfi $SCRATCH_DEV
+
+# bad label length
+do_mkfs_fail -L thisiswaytoolong $SCRATCH_DEV
+
+# basic "should pass" data section tests
+do_mkfs_pass $SCRATCH_DEV
+do_mkfs_pass -d name=$SCRATCH_DEV
+do_mkfs_pass -d size=$fssize $SCRATCH_DEV
+do_mkfs_pass -d agcount=32 $SCRATCH_DEV
+do_mkfs_pass -d agsize=32m $SCRATCH_DEV
+do_mkfs_pass -d agsize=32M $SCRATCH_DEV
+do_mkfs_pass -d agsize=1g $SCRATCH_DEV
+do_mkfs_pass -d agsize=$((32 * 1024 * 1024)) $SCRATCH_DEV
+do_mkfs_pass -b size=4096 -d agsize=8192b $SCRATCH_DEV
+do_mkfs_pass -d sectsize=512,agsize=65536s $SCRATCH_DEV
+do_mkfs_pass -s size=512 -d agsize=65536s $SCRATCH_DEV
+do_mkfs_pass -d noalign $SCRATCH_DEV
+do_mkfs_pass -d sunit=0,swidth=0 $SCRATCH_DEV
+do_mkfs_pass -d sunit=8,swidth=8 $SCRATCH_DEV
+do_mkfs_pass -d sunit=8,swidth=64 $SCRATCH_DEV
+do_mkfs_pass -d su=0,sw=0 $SCRATCH_DEV
+do_mkfs_pass -d su=4096,sw=1 $SCRATCH_DEV
+do_mkfs_pass -d su=4k,sw=1 $SCRATCH_DEV
+do_mkfs_pass -d su=4K,sw=8 $SCRATCH_DEV
+do_mkfs_pass -b size=4096 -d su=1b,sw=8 $SCRATCH_DEV
+do_mkfs_pass -d sectsize=512,su=8s,sw=8 $SCRATCH_DEV
+do_mkfs_pass -s size=512 -d su=8s,sw=8 $SCRATCH_DEV
+
+# invalid data section tests
+do_mkfs_fail -d size=${fssize}b $SCRATCH_DEV
+do_mkfs_fail -d size=${fssize}s $SCRATCH_DEV
+do_mkfs_fail -d size=${fssize}yerk $SCRATCH_DEV
+do_mkfs_fail -d agsize=8192b $SCRATCH_DEV
+do_mkfs_fail -d agsize=65536s $SCRATCH_DEV
+do_mkfs_fail -d agsize=32Mbsdfsdo $SCRATCH_DEV
+do_mkfs_fail -d agsize=1GB $SCRATCH_DEV
+do_mkfs_fail -d agcount=1k $SCRATCH_DEV
+do_mkfs_fail -d agcount=6b $SCRATCH_DEV
+do_mkfs_fail -d agcount=32,agsize=32m $SCRATCH_DEV
+do_mkfs_fail -d sunit=0,swidth=64 $SCRATCH_DEV
+do_mkfs_fail -d sunit=64,swidth=0 $SCRATCH_DEV
+do_mkfs_fail -d sunit=64,swidth=64,noalign $SCRATCH_DEV
+do_mkfs_fail -d sunit=64k,swidth=64 $SCRATCH_DEV
+do_mkfs_fail -d sunit=64,swidth=64m $SCRATCH_DEV
+do_mkfs_fail -d su=0,sw=64 $SCRATCH_DEV
+do_mkfs_fail -d su=4096,sw=0 $SCRATCH_DEV
+do_mkfs_fail -d su=4097,sw=1 $SCRATCH_DEV
+do_mkfs_fail -d su=4096,sw=64,noalign $SCRATCH_DEV
+do_mkfs_fail -d su=4096,sw=64s $SCRATCH_DEV
+do_mkfs_fail -d su=4096s,sw=64 $SCRATCH_DEV
+do_mkfs_fail -d su=4096b,sw=64 $SCRATCH_DEV
+do_mkfs_fail -d su=4096garabge,sw=64 $SCRATCH_DEV
+do_mkfs_fail -d su=4096,sw=64,sunit=64,swidth=64 $SCRATCH_DEV
+do_mkfs_fail -d sectsize=10,agsize=65536s $SCRATCH_DEV
+do_mkfs_fail -d sectsize=512s,agsize=65536s $SCRATCH_DEV
+
+reset_fsimg
+
+# file section, should pass
+do_mkfs_pass $fsimg
+do_mkfs_pass -d file=0 $SCRATCH_DEV
+do_mkfs_pass -d size=$fssize,file=1,name=$fsimg
+do_mkfs_pass -d size=$fssize,file $fsimg
+do_mkfs_pass -d size=$fssize $fsimg
+do_mkfs_pass -d size=$fssize,name=$fsimg
+do_mkfs_pass -d size=$((fssize/2)) $fsimg
+# again this one, to check that we didn't truncated the file
+do_mkfs_pass -d size=$fssize $fsimg
+rm -f $fsimg
+do_mkfs_pass -d file,size=$fssize $fsimg
+
+reset_fsimg
+
+# file section, should fail
+do_mkfs_fail -d file=1 $SCRATCH_DEV
+do_mkfs_fail -d file $fsimg # no size given
+rm -f $fsimg
+do_mkfs_fail $fsimg
+do_mkfs_fail -d size=$fssize $fsimg
+
+reset_fsimg
+
+# log section, should pass
+do_mkfs_pass -l size=$logsize -d size=$fssize $SCRATCH_DEV
+do_mkfs_pass -l agnum=2 $SCRATCH_DEV
+do_mkfs_pass -l size=4096b $SCRATCH_DEV
+do_mkfs_pass -l sectsize=512 $SCRATCH_DEV
+do_mkfs_pass -l sunit=64 $SCRATCH_DEV
+do_mkfs_pass -l sunit=64 -d sunit=8,swidth=8 $SCRATCH_DEV
+do_mkfs_pass -l sunit=8 $SCRATCH_DEV
+do_mkfs_pass -l su=$((4096*10)) $SCRATCH_DEV
+do_mkfs_pass -b size=4096 -l su=10b $SCRATCH_DEV
+do_mkfs_pass -l sectsize=512,su=$((4096*10)) $SCRATCH_DEV
+do_mkfs_pass -l internal $SCRATCH_DEV
+$XFS_IO_PROG -f -c "truncate $logsize" $fsimg
+do_mkfs_pass -l logdev=$fsimg $SCRATCH_DEV
+do_mkfs_pass -l name=$fsimg $SCRATCH_DEV
+do_mkfs_pass -l lazy-count=0 -m crc=0 $SCRATCH_DEV
+do_mkfs_pass -l lazy-count=1 -m crc=0 $SCRATCH_DEV
+do_mkfs_pass -l version=1 -m crc=0 $SCRATCH_DEV
+do_mkfs_pass -l version=2 -m crc=0 $SCRATCH_DEV
+do_mkfs_pass -l version=2 $SCRATCH_DEV
+
+# log section, should fail
+do_mkfs_fail -l size=${fssize}b $SCRATCH_DEV
+do_mkfs_fail -l size=${fssize}s $SCRATCH_DEV
+do_mkfs_fail -l size=${fssize}yerk $SCRATCH_DEV
+do_mkfs_fail -l agnum=1k $SCRATCH_DEV
+do_mkfs_fail -l agnum=6b $SCRATCH_DEV
+do_mkfs_fail -l agnum=32 $SCRATCH_DEV
+do_mkfs_fail -l sunit=0 $SCRATCH_DEV
+do_mkfs_fail -l sunit=63 $SCRATCH_DEV
+do_mkfs_fail -l su=1 $SCRATCH_DEV
+do_mkfs_fail -l su=10b $SCRATCH_DEV
+do_mkfs_fail -l su=10s $SCRATCH_DEV
+do_mkfs_fail -l su=$((4096*10+1)) $SCRATCH_DEV
+do_mkfs_fail -l sectsize=10,agsize=65536s $SCRATCH_DEV
+do_mkfs_fail -l sectsize=512s,agsize=65536s $SCRATCH_DEV
+do_mkfs_fail -l internal=0 $SCRATCH_DEV
+reset_fsimg
+do_mkfs_fail -l internal=1,logdev=$fsimg $SCRATCH_DEV
+do_mkfs_fail -l lazy-count=1garbage $SCRATCH_DEV
+do_mkfs_fail -l lazy-count=2 $SCRATCH_DEV
+do_mkfs_fail -l lazy-count=0 -m crc=1 $SCRATCH_DEV
+do_mkfs_fail -l version=1 -m crc=1 $SCRATCH_DEV
+do_mkfs_fail -l version=0 $SCRATCH_DEV
+
+
+
+# naming section, should pass
+do_mkfs_pass -n size=65536 $SCRATCH_DEV
+do_mkfs_pass -n log=15 $SCRATCH_DEV
+do_mkfs_pass -n version=2 $SCRATCH_DEV
+do_mkfs_pass -n version=ci $SCRATCH_DEV
+do_mkfs_pass -n ftype=0 -m crc=0 $SCRATCH_DEV
+do_mkfs_pass -n ftype=1 $SCRATCH_DEV
+
+# naming section, should fail
+do_mkfs_fail -n version=1 $SCRATCH_DEV
+do_mkfs_fail -n version=cid $SCRATCH_DEV
+do_mkfs_fail -n ftype=4 $SCRATCH_DEV
+do_mkfs_fail -n ftype=0 $SCRATCH_DEV
+
+reset_fsimg
+
+# metadata section, should pass
+do_mkfs_pass -m crc=1,finobt=1 $SCRATCH_DEV
+do_mkfs_pass -m crc=1,finobt=0 $SCRATCH_DEV
+do_mkfs_pass -m crc=0,finobt=0 $SCRATCH_DEV
+do_mkfs_pass -m crc=1 -n ftype=1 $SCRATCH_DEV
+do_mkfs_pass -m crc=0 -n ftype=1 $SCRATCH_DEV
+do_mkfs_pass -m crc=0 -n ftype=0 $SCRATCH_DEV
+
+# metadata section, should fail
+do_mkfs_fail -m crc=0,finobt=1 $SCRATCH_DEV
+do_mkfs_fail -m crc=1 -n ftype=0 $SCRATCH_DEV
+
+
+# realtime section, should pass
+do_mkfs_pass -r rtdev=$fsimg $SCRATCH_DEV
+do_mkfs_pass -r extsize=4k $SCRATCH_DEV
+do_mkfs_pass -r extsize=1G $SCRATCH_DEV
+do_mkfs_pass -r size=65536,rtdev=$fsimg $SCRATCH_DEV
+do_mkfs_pass -r noalign $SCRATCH_DEV
+
+
+# realtime section, should fail
+do_mkfs_fail -r rtdev=$SCRATCH_DEV
+do_mkfs_fail -r extsize=256 $SCRATCH_DEV
+do_mkfs_fail -r extsize=2G $SCRATCH_DEV
+do_mkfs_fail -r size=65536 $SCRATCH_DEV
+
+
+
+# inode section, should pass
+do_mkfs_pass -i size=256 -m crc=0 $SCRATCH_DEV
+do_mkfs_pass -i size=512 $SCRATCH_DEV
+do_mkfs_pass -i size=2048 $SCRATCH_DEV
+do_mkfs_pass -i log=10 $SCRATCH_DEV
+do_mkfs_pass -i perblock=2 $SCRATCH_DEV
+do_mkfs_pass -i maxpct=10 $SCRATCH_DEV
+do_mkfs_pass -i maxpct=100 $SCRATCH_DEV
+do_mkfs_pass -i maxpct=0 $SCRATCH_DEV
+do_mkfs_pass -i align=0 -m crc=0 $SCRATCH_DEV
+do_mkfs_pass -i align=1 -m crc=1 $SCRATCH_DEV
+do_mkfs_pass -i attr=1 -m crc=0 $SCRATCH_DEV
+do_mkfs_pass -i attr=2 $SCRATCH_DEV
+do_mkfs_pass -i projid32bit $SCRATCH_DEV
+do_mkfs_pass -i sparse=0 $SCRATCH_DEV
+do_mkfs_pass -i sparse -m crc $SCRATCH_DEV
+
+
+# inode section, should fail
+do_mkfs_fail -i size=256 -m crc $SCRATCH_DEV
+do_mkfs_fail -i size=128 $SCRATCH_DEV
+do_mkfs_fail -i size=513 $SCRATCH_DEV
+do_mkfs_fail -i size=4096 $SCRATCH_DEV
+do_mkfs_fail -i maxpct=110 $SCRATCH_DEV
+do_mkfs_fail -i align=2 $SCRATCH_DEV
+do_mkfs_fail -i sparse -m crc=0 $SCRATCH_DEV
+do_mkfs_fail -i align=0 -m crc=1 $SCRATCH_DEV
+do_mkfs_fail -i attr=1 -m crc=1 $SCRATCH_DEV
+
+status=0
+exit
diff --git a/tests/xfs/400-input-validation.out b/tests/xfs/400-input-validation.out
new file mode 100644
index 0000000..7080553
--- /dev/null
+++ b/tests/xfs/400-input-validation.out
@@ -0,0 +1,2 @@
+QA output created by 400-input-validation
+silence is golden
diff --git a/tests/xfs/group b/tests/xfs/group
index 9f8ca0f..dd2aa5b 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -308,3 +308,4 @@
325 auto quick clone
326 auto quick clone
327 auto quick clone
+400-input-validation auto quick mkfs
--
2.5.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 4/6] xfstests: add _require_xfs_mkfs_validation to common/rc
2016-07-14 12:43 ` [PATCH 4/6] xfstests: add _require_xfs_mkfs_validation to common/rc Jan Tulak
@ 2016-07-14 14:21 ` Eryu Guan
2016-07-14 15:16 ` Jan Tulak
2016-07-14 15:57 ` [PATCH v2] " Jan Tulak
1 sibling, 1 reply; 30+ messages in thread
From: Eryu Guan @ 2016-07-14 14:21 UTC (permalink / raw)
To: Jan Tulak; +Cc: fstests
On Thu, Jul 14, 2016 at 02:43:32PM +0200, Jan Tulak wrote:
> Add a simple way to skip a test if it is (or is not) run on mkfs correctly
> validating inputs.
>
> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> ---
> common/rc | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/common/rc b/common/rc
> index 0c68e4f..72f9901 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3843,6 +3843,35 @@ _get_fs_sysfs_attr()
> cat /sys/fs/${FSTYP}/${dname}/${attr}
> }
>
> +# Skip if we are running an older binary without the stricter input checks.
> +# Make multiple checks to be sure that there is no regression on the one
> +# selected feature check, which would skew the result.
> +_require_xfs_mkfs_validation()
> +{
> + _require_scratch
I don't think you need $SCRATCH_DEV and _require_scratch in this helper,
a test file just does the work, e.g. creating an empty file somewhere
and using the "-d name=<filename>,size=<size>" option of mkfs.xfs to do
the test.
> + $MKFS_XFS_PROG -f -N -s size=2s $SCRATCH_DEV >/dev/null 2>&1
> + sum=$?
> + $MKFS_XFS_PROG -f -N -l version=2,su=$((256 * 1024 + 4096)) $SCRATCH_DEV >/dev/null 2>&1
> + sum=`expr $sum + $?`
> +
> + if [ "$sum" -eq 0 ]; then
> + _notrun "Requires newer mkfs with stricter input checks."
"newer mkfs" is not specific, it's not clear which version of xfsprogs
one should use to run this test. It'd be good to see specific version
info in this message if possible. (Sorry I didn't make my point clear in
previous reviews.)
Thanks,
Eryu
> + fi
> +}
> +
> +# The oposite of _require_xfs_mkfs_validation.
> +_require_xfs_mkfs_without_validation()
> +{
> + _require_scratch
> + $MKFS_XFS_PROG -f -N -s size=2s $SCRATCH_DEV >/dev/null 2>&1
> + sum=$?
> + $MKFS_XFS_PROG -f -N -l version=2,su=$((256 * 1024 + 4096)) $SCRATCH_DEV >/dev/null 2>&1
> + sum=`expr $sum + $?`
> +
> + if [ "$sum" -eq 2 ]; then
> + _notrun "Requires older mkfs without stricter input checks."
> + fi
> +}
> init_rc
>
> ################################################################################
> --
> 2.5.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/6] xfstests: add _require_xfs_mkfs_validation to common/rc
2016-07-14 14:21 ` Eryu Guan
@ 2016-07-14 15:16 ` Jan Tulak
0 siblings, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-14 15:16 UTC (permalink / raw)
To: Eryu Guan; +Cc: fstests
On Thu, Jul 14, 2016 at 4:21 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Thu, Jul 14, 2016 at 02:43:32PM +0200, Jan Tulak wrote:
>> Add a simple way to skip a test if it is (or is not) run on mkfs correctly
>> validating inputs.
>>
>> Signed-off-by: Jan Tulak <jtulak@redhat.com>
>> ---
>> common/rc | 29 +++++++++++++++++++++++++++++
>> 1 file changed, 29 insertions(+)
>>
>> diff --git a/common/rc b/common/rc
>> index 0c68e4f..72f9901 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -3843,6 +3843,35 @@ _get_fs_sysfs_attr()
>> cat /sys/fs/${FSTYP}/${dname}/${attr}
>> }
>>
>> +# Skip if we are running an older binary without the stricter input checks.
>> +# Make multiple checks to be sure that there is no regression on the one
>> +# selected feature check, which would skew the result.
>> +_require_xfs_mkfs_validation()
>> +{
>> + _require_scratch
>
> I don't think you need $SCRATCH_DEV and _require_scratch in this helper,
> a test file just does the work, e.g. creating an empty file somewhere
> and using the "-d name=<filename>,size=<size>" option of mkfs.xfs to do
> the test.
>
True, that works too. I'm doing some further small tinkering around
this patch, but I will send a new version soon.
>> + $MKFS_XFS_PROG -f -N -s size=2s $SCRATCH_DEV >/dev/null 2>&1
>> + sum=$?
>> + $MKFS_XFS_PROG -f -N -l version=2,su=$((256 * 1024 + 4096)) $SCRATCH_DEV >/dev/null 2>&1
>> + sum=`expr $sum + $?`
>> +
>> + if [ "$sum" -eq 0 ]; then
>> + _notrun "Requires newer mkfs with stricter input checks."
>
> "newer mkfs" is not specific, it's not clear which version of xfsprogs
> one should use to run this test. It'd be good to see specific version
> info in this message if possible. (Sorry I didn't make my point clear in
> previous reviews.)
>
All right. :-) How about these?
Requires older mkfs without strict input checks: the last supported
version of xfsprogs is 4.5.
Requires newer mkfs with stricter input checks: the oldest supported
version of xfsprogs is 4.7.
Thanks,
Jan
> Thanks,
> Eryu
>
>> + fi
>> +}
>> +
>> +# The oposite of _require_xfs_mkfs_validation.
>> +_require_xfs_mkfs_without_validation()
>> +{
>> + _require_scratch
>> + $MKFS_XFS_PROG -f -N -s size=2s $SCRATCH_DEV >/dev/null 2>&1
>> + sum=$?
>> + $MKFS_XFS_PROG -f -N -l version=2,su=$((256 * 1024 + 4096)) $SCRATCH_DEV >/dev/null 2>&1
>> + sum=`expr $sum + $?`
>> +
>> + if [ "$sum" -eq 2 ]; then
>> + _notrun "Requires older mkfs without stricter input checks."
>> + fi
>> +}
>> init_rc
>>
>> ################################################################################
>> --
>> 2.5.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe fstests" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2] xfstests: add _require_xfs_mkfs_validation to common/rc
2016-07-14 12:43 ` [PATCH 4/6] xfstests: add _require_xfs_mkfs_validation to common/rc Jan Tulak
2016-07-14 14:21 ` Eryu Guan
@ 2016-07-14 15:57 ` Jan Tulak
1 sibling, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-14 15:57 UTC (permalink / raw)
To: fstests; +Cc: eguan, Jan Tulak
Add a simple way to skip a test if it is (or is not) run on mkfs correctly
validating inputs.
Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
UPDATE:
* Use tmp file instead of scratch - we are using -N flag, so it doesn't
really matter.
* Make both _requires to use the same function for testing, so if it is ever
changed, the change will be reflected in both.
---
common/rc | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/common/rc b/common/rc
index 0c68e4f..c66d31d 100644
--- a/common/rc
+++ b/common/rc
@@ -3843,6 +3843,39 @@ _get_fs_sysfs_attr()
cat /sys/fs/${FSTYP}/${dname}/${attr}
}
+# Skip if we are running an older binary without the stricter input checks.
+# Make multiple checks to be sure that there is no regression on the one
+# selected feature check, which would skew the result.
+#
+# At first, make a common function that runs the tests and returns
+# number of failed cases.
+mkfs_validation_check()
+{
+ local cmd="$MKFS_XFS_PROG -f -N -d file,name=/tmp/foo,size=$((1024 * 1024 * 1024))"
+ $cmd -s size=2s >/dev/null 2>&1
+ local sum=$?
+ $cmd -l version=2,su=$((256 * 1024 + 4096)) >/dev/null 2>&1
+ sum=`expr $sum + $?`
+ return $sum
+}
+# Skip the test if all calls passed - mkfs accepts invalid input
+_require_xfs_mkfs_validation()
+{
+ mkfs_validation_check
+ if [ "$?" -eq 0 ]; then
+ _notrun "Requires newer mkfs with stricter input checks: the oldest supported version of xfsprogs is 4.7."
+ fi
+}
+
+# The oposite of _require_xfs_mkfs_validation.
+_require_xfs_mkfs_without_validation()
+{
+ mkfs_validation_check
+ if [ "$?" -ne 0 ]; then
+ _notrun "Requires older mkfs without strict input checks: the last supported version of xfsprogs is 4.5."
+ fi
+}
+
init_rc
################################################################################
--
2.5.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
2016-07-14 12:43 ` [PATCH 6/6] xfstests: Add mkfs input validation tests Jan Tulak
@ 2016-07-16 9:33 ` Eryu Guan
2016-07-17 23:30 ` Dave Chinner
0 siblings, 1 reply; 30+ messages in thread
From: Eryu Guan @ 2016-07-16 9:33 UTC (permalink / raw)
To: Jan Tulak; +Cc: fstests, Dave Chinner
On Thu, Jul 14, 2016 at 02:43:34PM +0200, Jan Tulak wrote:
> mkfs.xfs does not do a very good job of input validation. This test
> is designed to exercise the input validation and test good/bad
> combinations of options being set. It will not pass on an old
> mkfs.xfs binary - it is designed to be the test case for an input
> validation cleanup (merged in spring/summer 2016).
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Jan Tulak <jtulak@redhat.com>
>
> ---
> CHANGES:
> Use common/rc feature check for mkfs version testing.
>
> Cheers,
> Jan
>
> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> ---
> tests/xfs/400-input-validation | 338 +++++++++++++++++++++++++++++++++++++
> tests/xfs/400-input-validation.out | 2 +
> tests/xfs/group | 1 +
> 3 files changed, 341 insertions(+)
> create mode 100755 tests/xfs/400-input-validation
> create mode 100644 tests/xfs/400-input-validation.out
>
> diff --git a/tests/xfs/400-input-validation b/tests/xfs/400-input-validation
> new file mode 100755
> index 0000000..83a4ff9
> --- /dev/null
> +++ b/tests/xfs/400-input-validation
> @@ -0,0 +1,338 @@
> +#! /bin/bash
> +# FS QA Test No. xfs/400
> +#
> +# mkfs.xfs input validation test. Designed to break mkfs.xfs if it doesn't
> +# filter garbage input or invalid option combinations correctly.
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2016 Red Hat, Inc. All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> +#-----------------------------------------------------------------------
> +#
> +
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
[snip]
> +do_mkfs_fail -l internal=1,logdev=$fsimg $SCRATCH_DEV
> +do_mkfs_fail -l lazy-count=1garbage $SCRATCH_DEV
> +do_mkfs_fail -l lazy-count=2 $SCRATCH_DEV
> +do_mkfs_fail -l lazy-count=0 -m crc=1 $SCRATCH_DEV
> +do_mkfs_fail -l version=1 -m crc=1 $SCRATCH_DEV
This test fails in my DAX testing, where SCRATCH_DEV is ramdisk. The
mkfs itself should fail, but it passed. Log version 2 was used
automatically, instead of prompting "V2 logs always enabled for CRC
enabled filesytems"
[root@dhcp-66-86-11 xfstests]# mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
meta-data=/dev/ram0 isize=512 agcount=1, agsize=4096 blks
= sectsz=4096 attr=2, projid32bit=1
= crc=1 finobt=1, sparse=0
data = bsize=4096 blocks=4096, imaxpct=25
= sunit=0 swidth=0 blks
naming =version 2 bsize=4096 ascii-ci=0 ftype=1
log =internal log bsize=4096 blocks=1605, version=2
= sectsz=4096 sunit=1 blks, lazy-count=1
realtime =none extsz=4096 blocks=0, rtextents=0
Is it a mkfs.xfs bug or the test case should handle the special case?
Thanks,
Eryu
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/6 v3] xfstests: some small fixes and mkfs validation test
2016-07-14 12:43 [PATCH 0/6 v3] xfstests: some small fixes and mkfs validation test Jan Tulak
` (5 preceding siblings ...)
2016-07-14 12:43 ` [PATCH 6/6] xfstests: Add mkfs input validation tests Jan Tulak
@ 2016-07-16 10:28 ` Eryu Guan
2016-07-18 8:47 ` [PATCH v2] xfstests: Fix installation for extended names Jan Tulak
7 siblings, 0 replies; 30+ messages in thread
From: Eryu Guan @ 2016-07-16 10:28 UTC (permalink / raw)
To: Jan Tulak; +Cc: fstests
On Thu, Jul 14, 2016 at 02:43:28PM +0200, Jan Tulak wrote:
> This set is a merge of all my recent patches. The reason for the merge is that
> the patches are sequentially dependent and making a set is the safest way of
> ensuring their correct order after a merge.
>
> One patch, "filename handling - fix early wildcard expansion", was
> dropped for now. Patch "add _require_xfs_mkfs_validation to common/rc" is new,
> and update of xfs/096 and input validation tests were changed.
>
>
> Jan Tulak (6):
> xfstests: Fix installation for extended names
> fstests: filename handling for extended names in ./check was on a
> wrong place
> xfstests: remove unused variable
> xfstests: add _require_xfs_mkfs_validation to common/rc
> xfstests: update xfs/096 for new behaviour
> xfstests: Add mkfs input validation tests
I took patch 4 and 5 in this pull-request and left other extended name
related patches for now, due to the .cfg file installation issue.
Thanks,
Eryu
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
2016-07-16 9:33 ` Eryu Guan
@ 2016-07-17 23:30 ` Dave Chinner
2016-07-18 11:29 ` Jan Tulak
0 siblings, 1 reply; 30+ messages in thread
From: Dave Chinner @ 2016-07-17 23:30 UTC (permalink / raw)
To: Eryu Guan; +Cc: Jan Tulak, fstests
On Sat, Jul 16, 2016 at 05:33:58PM +0800, Eryu Guan wrote:
> On Thu, Jul 14, 2016 at 02:43:34PM +0200, Jan Tulak wrote:
> > +do_mkfs_fail -l lazy-count=1garbage $SCRATCH_DEV
> > +do_mkfs_fail -l lazy-count=2 $SCRATCH_DEV
> > +do_mkfs_fail -l lazy-count=0 -m crc=1 $SCRATCH_DEV
> > +do_mkfs_fail -l version=1 -m crc=1 $SCRATCH_DEV
>
> This test fails in my DAX testing, where SCRATCH_DEV is ramdisk. The
> mkfs itself should fail, but it passed. Log version 2 was used
> automatically, instead of prompting "V2 logs always enabled for CRC
> enabled filesytems"
>
> [root@dhcp-66-86-11 xfstests]# mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
> meta-data=/dev/ram0 isize=512 agcount=1, agsize=4096 blks
> = sectsz=4096 attr=2, projid32bit=1
> = crc=1 finobt=1, sparse=0
> data = bsize=4096 blocks=4096, imaxpct=25
> = sunit=0 swidth=0 blks
> naming =version 2 bsize=4096 ascii-ci=0 ftype=1
> log =internal log bsize=4096 blocks=1605, version=2
> = sectsz=4096 sunit=1 blks, lazy-count=1
> realtime =none extsz=4096 blocks=0, rtextents=0
>
> Is it a mkfs.xfs bug or the test case should handle the special case?
Looks like it might be a side effect of using a 4k sector size. v1
logs only supported 512 byte sectors, so it's entirely possible that
the sector size is silently overriding the log version
specification. Probably should be fixed in mkfs.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2] xfstests: Fix installation for extended names
2016-07-14 12:43 [PATCH 0/6 v3] xfstests: some small fixes and mkfs validation test Jan Tulak
` (6 preceding siblings ...)
2016-07-16 10:28 ` [PATCH 0/6 v3] xfstests: some small fixes and mkfs validation test Eryu Guan
@ 2016-07-18 8:47 ` Jan Tulak
7 siblings, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-18 8:47 UTC (permalink / raw)
To: fstests; +Cc: eguan, Jan Tulak
xfstests supports extended test names like 314-foo-bar, but installation of
these tests was skipped (not matching a regexp). So this patch fixes the
makefiles in tests/*/
The include/buildrules change was written by Dave Chinner.
Signed-off-by: Jan Tulak <jtulak@redhat.com>
---
UPDATE:
Add .cfg support
---
include/buildrules | 32 ++++++++++++++++++++++++++++++++
tests/btrfs/Makefile | 4 ++--
tests/cifs/Makefile | 4 ++--
tests/ext4/Makefile | 4 ++--
tests/f2fs/Makefile | 4 ++--
tests/generic/Makefile | 4 ++--
tests/overlay/Makefile | 4 ++--
tests/shared/Makefile | 4 ++--
tests/udf/Makefile | 4 ++--
tests/xfs/Makefile | 4 ++--
10 files changed, 50 insertions(+), 18 deletions(-)
diff --git a/include/buildrules b/include/buildrules
index c8a7c47..76b755e 100644
--- a/include/buildrules
+++ b/include/buildrules
@@ -100,3 +100,35 @@ MAKEDEP := $(MAKEDEPEND) $(CFLAGS)
cp /dev/null .dep; \
fi
+# Gather files for installing into two lists:
+# TESTS with executable scripts and OUTFILES with all the test outputs.
+# Makefile has a very small matching, so we have to go this long way.
+
+# Start with all test related files:
+ALLFILES = $(wildcard [0-9]??*)
+
+# Now build a list of known output files. Unfortunately, the
+# multiple output test files are poorly handled as makefiles don't
+# handle wildcarded multi-suffix matching. Hence we separate the
+# processing of these right now.
+EXTENDED_OUTFILES = $(wildcard [0-9]??*.out.*)
+EXTENDED_OUTFILE_CFGS = $(wildcard [0-9]??.cfg)
+BASIC_OUTFILES = $(wildcard [0-9]??*.out)
+OUTFILES = $(EXTENDED_OUTFILES) $(EXTENDED_OUTFILE_CFGS) $(BASIC_OUTFILES)
+
+# Strip suffix to get matching tests. We want to strip from the
+# first "." to the end, but makefiles don't have a built in
+# operative for that. So:
+#
+# Hack: strip the multiple segments after .out with repeated basename calls.
+EXTFILTER1 = $(basename $(EXTENDED_OUTFILES))
+EXTFILTER2 = $(basename $(EXTFILTER1))
+EXTFILTER3 = $(basename $(EXTFILTER2))
+EXTFILTER4 = $(basename $(EXTFILTER3))
+
+# final filter list
+FILTER = $(basename $(EXTFILTER4) $(BASIC_OUTFILES))
+
+# finally, select the test files by filtering against against the
+# stripped output files and sort them to remove duplicates.
+TESTS = $(sort $(filter $(ALLFILES), $(FILTER)))
diff --git a/tests/btrfs/Makefile b/tests/btrfs/Makefile
index e1a5be1..2d93642 100644
--- a/tests/btrfs/Makefile
+++ b/tests/btrfs/Makefile
@@ -12,9 +12,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
diff --git a/tests/cifs/Makefile b/tests/cifs/Makefile
index 9176e5c..0c5cf3b 100644
--- a/tests/cifs/Makefile
+++ b/tests/cifs/Makefile
@@ -12,9 +12,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
diff --git a/tests/ext4/Makefile b/tests/ext4/Makefile
index 7a3c8e1..beb1541 100644
--- a/tests/ext4/Makefile
+++ b/tests/ext4/Makefile
@@ -12,9 +12,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
diff --git a/tests/f2fs/Makefile b/tests/f2fs/Makefile
index 4d00e9e..d13bca3 100644
--- a/tests/f2fs/Makefile
+++ b/tests/f2fs/Makefile
@@ -13,9 +13,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
diff --git a/tests/generic/Makefile b/tests/generic/Makefile
index 9529fb8..3878d05 100644
--- a/tests/generic/Makefile
+++ b/tests/generic/Makefile
@@ -12,9 +12,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
diff --git a/tests/overlay/Makefile b/tests/overlay/Makefile
index 63c9878..b07f892 100644
--- a/tests/overlay/Makefile
+++ b/tests/overlay/Makefile
@@ -12,9 +12,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
diff --git a/tests/shared/Makefile b/tests/shared/Makefile
index cbd87f9..8a83278 100644
--- a/tests/shared/Makefile
+++ b/tests/shared/Makefile
@@ -12,9 +12,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
diff --git a/tests/udf/Makefile b/tests/udf/Makefile
index 1d96658..c9c9f1b 100644
--- a/tests/udf/Makefile
+++ b/tests/udf/Makefile
@@ -12,9 +12,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
diff --git a/tests/xfs/Makefile b/tests/xfs/Makefile
index db94be0..d64800e 100644
--- a/tests/xfs/Makefile
+++ b/tests/xfs/Makefile
@@ -12,9 +12,9 @@ include $(BUILDRULES)
install:
$(INSTALL) -m 755 -d $(TARGET_DIR)
- $(INSTALL) -m 755 [0-9]?? $(TARGET_DIR)
+ $(INSTALL) -m 755 $(TESTS) $(TARGET_DIR)
$(INSTALL) -m 644 group $(TARGET_DIR)
- $(INSTALL) -m 644 [0-9]??.* $(TARGET_DIR)
+ $(INSTALL) -m 644 $(OUTFILES) $(TARGET_DIR)
# Nothing.
install-dev install-lib:
--
2.5.5
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
2016-07-17 23:30 ` Dave Chinner
@ 2016-07-18 11:29 ` Jan Tulak
0 siblings, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-18 11:29 UTC (permalink / raw)
To: Dave Chinner; +Cc: Eryu Guan, fstests, xfs-oss
On Mon, Jul 18, 2016 at 1:30 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Sat, Jul 16, 2016 at 05:33:58PM +0800, Eryu Guan wrote:
>> On Thu, Jul 14, 2016 at 02:43:34PM +0200, Jan Tulak wrote:
>> > +do_mkfs_fail -l lazy-count=1garbage $SCRATCH_DEV
>> > +do_mkfs_fail -l lazy-count=2 $SCRATCH_DEV
>> > +do_mkfs_fail -l lazy-count=0 -m crc=1 $SCRATCH_DEV
>> > +do_mkfs_fail -l version=1 -m crc=1 $SCRATCH_DEV
>>
>> This test fails in my DAX testing, where SCRATCH_DEV is ramdisk. The
>> mkfs itself should fail, but it passed. Log version 2 was used
>> automatically, instead of prompting "V2 logs always enabled for CRC
>> enabled filesytems"
>>
>> [root@dhcp-66-86-11 xfstests]# mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
>> meta-data=/dev/ram0 isize=512 agcount=1, agsize=4096 blks
>> = sectsz=4096 attr=2, projid32bit=1
>> = crc=1 finobt=1, sparse=0
>> data = bsize=4096 blocks=4096, imaxpct=25
>> = sunit=0 swidth=0 blks
>> naming =version 2 bsize=4096 ascii-ci=0 ftype=1
>> log =internal log bsize=4096 blocks=1605, version=2
>> = sectsz=4096 sunit=1 blks, lazy-count=1
>> realtime =none extsz=4096 blocks=0, rtextents=0
>>
>> Is it a mkfs.xfs bug or the test case should handle the special case?
>
> Looks like it might be a side effect of using a 4k sector size. v1
> logs only supported 512 byte sectors, so it's entirely possible that
> the sector size is silently overriding the log version
> specification. Probably should be fixed in mkfs.
>
>
I tried to duplicate this, but in my config it didn't failed - how did
you create the ramdisk?
# modprobe brd rd_nr=1 rd_size=$((200*1024))
# blockdev --getbsz /dev/ram0
4096
# blockdev --getpbsz /dev/ram0
512
# blockdev --getss /dev/ram0
512
# mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
V2 logs always enabled for CRC enabled filesytems
Usage: mkfs.xfs
[snip]
Thanks, Jan
PS: cc-ing XFS list - if it is mkfs bug, it is better there than in fstests.
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
@ 2016-07-18 11:29 ` Jan Tulak
0 siblings, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-18 11:29 UTC (permalink / raw)
To: Dave Chinner; +Cc: Eryu Guan, fstests, xfs-oss
On Mon, Jul 18, 2016 at 1:30 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Sat, Jul 16, 2016 at 05:33:58PM +0800, Eryu Guan wrote:
>> On Thu, Jul 14, 2016 at 02:43:34PM +0200, Jan Tulak wrote:
>> > +do_mkfs_fail -l lazy-count=1garbage $SCRATCH_DEV
>> > +do_mkfs_fail -l lazy-count=2 $SCRATCH_DEV
>> > +do_mkfs_fail -l lazy-count=0 -m crc=1 $SCRATCH_DEV
>> > +do_mkfs_fail -l version=1 -m crc=1 $SCRATCH_DEV
>>
>> This test fails in my DAX testing, where SCRATCH_DEV is ramdisk. The
>> mkfs itself should fail, but it passed. Log version 2 was used
>> automatically, instead of prompting "V2 logs always enabled for CRC
>> enabled filesytems"
>>
>> [root@dhcp-66-86-11 xfstests]# mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
>> meta-data=/dev/ram0 isize=512 agcount=1, agsize=4096 blks
>> = sectsz=4096 attr=2, projid32bit=1
>> = crc=1 finobt=1, sparse=0
>> data = bsize=4096 blocks=4096, imaxpct=25
>> = sunit=0 swidth=0 blks
>> naming =version 2 bsize=4096 ascii-ci=0 ftype=1
>> log =internal log bsize=4096 blocks=1605, version=2
>> = sectsz=4096 sunit=1 blks, lazy-count=1
>> realtime =none extsz=4096 blocks=0, rtextents=0
>>
>> Is it a mkfs.xfs bug or the test case should handle the special case?
>
> Looks like it might be a side effect of using a 4k sector size. v1
> logs only supported 512 byte sectors, so it's entirely possible that
> the sector size is silently overriding the log version
> specification. Probably should be fixed in mkfs.
>
>
I tried to duplicate this, but in my config it didn't failed - how did
you create the ramdisk?
# modprobe brd rd_nr=1 rd_size=$((200*1024))
# blockdev --getbsz /dev/ram0
4096
# blockdev --getpbsz /dev/ram0
512
# blockdev --getss /dev/ram0
512
# mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
V2 logs always enabled for CRC enabled filesytems
Usage: mkfs.xfs
[snip]
Thanks, Jan
PS: cc-ing XFS list - if it is mkfs bug, it is better there than in fstests.
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
2016-07-18 11:29 ` Jan Tulak
@ 2016-07-18 11:47 ` Eryu Guan
-1 siblings, 0 replies; 30+ messages in thread
From: Eryu Guan @ 2016-07-18 11:47 UTC (permalink / raw)
To: Jan Tulak; +Cc: Dave Chinner, fstests, xfs-oss
On Mon, Jul 18, 2016 at 01:29:47PM +0200, Jan Tulak wrote:
> On Mon, Jul 18, 2016 at 1:30 AM, Dave Chinner <david@fromorbit.com> wrote:
> > On Sat, Jul 16, 2016 at 05:33:58PM +0800, Eryu Guan wrote:
> >> On Thu, Jul 14, 2016 at 02:43:34PM +0200, Jan Tulak wrote:
> >> > +do_mkfs_fail -l lazy-count=1garbage $SCRATCH_DEV
> >> > +do_mkfs_fail -l lazy-count=2 $SCRATCH_DEV
> >> > +do_mkfs_fail -l lazy-count=0 -m crc=1 $SCRATCH_DEV
> >> > +do_mkfs_fail -l version=1 -m crc=1 $SCRATCH_DEV
> >>
> >> This test fails in my DAX testing, where SCRATCH_DEV is ramdisk. The
> >> mkfs itself should fail, but it passed. Log version 2 was used
> >> automatically, instead of prompting "V2 logs always enabled for CRC
> >> enabled filesytems"
> >>
> >> [root@dhcp-66-86-11 xfstests]# mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
> >> meta-data=/dev/ram0 isize=512 agcount=1, agsize=4096 blks
> >> = sectsz=4096 attr=2, projid32bit=1
> >> = crc=1 finobt=1, sparse=0
> >> data = bsize=4096 blocks=4096, imaxpct=25
> >> = sunit=0 swidth=0 blks
> >> naming =version 2 bsize=4096 ascii-ci=0 ftype=1
> >> log =internal log bsize=4096 blocks=1605, version=2
> >> = sectsz=4096 sunit=1 blks, lazy-count=1
> >> realtime =none extsz=4096 blocks=0, rtextents=0
> >>
> >> Is it a mkfs.xfs bug or the test case should handle the special case?
> >
> > Looks like it might be a side effect of using a 4k sector size. v1
> > logs only supported 512 byte sectors, so it's entirely possible that
> > the sector size is silently overriding the log version
> > specification. Probably should be fixed in mkfs.
> >
> >
>
> I tried to duplicate this, but in my config it didn't failed - how did
> you create the ramdisk?
I think you need to test on a 4k sector size disk. I use scsi_debug to
simulate physical 4k sector disk to reproduce this:
[root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug
[root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_mb=128 physblk_exp=3
[root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --getss /dev/sdc
4096
4096
512
[root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=1 -m crc=1 /dev/sdc
meta-data=/dev/sdc isize=512 agcount=4, agsize=8192 blks
= sectsz=4096 attr=2, projid32bit=1
= crc=1 finobt=1, sparse=0
data = bsize=4096 blocks=32768, imaxpct=25
= sunit=0 swidth=0 blks
naming =version 2 bsize=4096 ascii-ci=0 ftype=1
log =internal log bsize=4096 blocks=1605, version=2
= sectsz=4096 sunit=1 blks, lazy-count=1
realtime =none extsz=4096 blocks=0, rtextents=0
If you remove the "physblk_exp=3" at modprobe time, mkfs failed as
expected.
Thanks,
Eryu
>
> # modprobe brd rd_nr=1 rd_size=$((200*1024))
> # blockdev --getbsz /dev/ram0
> 4096
> # blockdev --getpbsz /dev/ram0
> 512
> # blockdev --getss /dev/ram0
> 512
>
> # mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
> V2 logs always enabled for CRC enabled filesytems
> Usage: mkfs.xfs
> [snip]
>
> Thanks, Jan
>
> PS: cc-ing XFS list - if it is mkfs bug, it is better there than in fstests.
>
> --
> Jan Tulak
> jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
@ 2016-07-18 11:47 ` Eryu Guan
0 siblings, 0 replies; 30+ messages in thread
From: Eryu Guan @ 2016-07-18 11:47 UTC (permalink / raw)
To: Jan Tulak; +Cc: fstests, xfs-oss
On Mon, Jul 18, 2016 at 01:29:47PM +0200, Jan Tulak wrote:
> On Mon, Jul 18, 2016 at 1:30 AM, Dave Chinner <david@fromorbit.com> wrote:
> > On Sat, Jul 16, 2016 at 05:33:58PM +0800, Eryu Guan wrote:
> >> On Thu, Jul 14, 2016 at 02:43:34PM +0200, Jan Tulak wrote:
> >> > +do_mkfs_fail -l lazy-count=1garbage $SCRATCH_DEV
> >> > +do_mkfs_fail -l lazy-count=2 $SCRATCH_DEV
> >> > +do_mkfs_fail -l lazy-count=0 -m crc=1 $SCRATCH_DEV
> >> > +do_mkfs_fail -l version=1 -m crc=1 $SCRATCH_DEV
> >>
> >> This test fails in my DAX testing, where SCRATCH_DEV is ramdisk. The
> >> mkfs itself should fail, but it passed. Log version 2 was used
> >> automatically, instead of prompting "V2 logs always enabled for CRC
> >> enabled filesytems"
> >>
> >> [root@dhcp-66-86-11 xfstests]# mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
> >> meta-data=/dev/ram0 isize=512 agcount=1, agsize=4096 blks
> >> = sectsz=4096 attr=2, projid32bit=1
> >> = crc=1 finobt=1, sparse=0
> >> data = bsize=4096 blocks=4096, imaxpct=25
> >> = sunit=0 swidth=0 blks
> >> naming =version 2 bsize=4096 ascii-ci=0 ftype=1
> >> log =internal log bsize=4096 blocks=1605, version=2
> >> = sectsz=4096 sunit=1 blks, lazy-count=1
> >> realtime =none extsz=4096 blocks=0, rtextents=0
> >>
> >> Is it a mkfs.xfs bug or the test case should handle the special case?
> >
> > Looks like it might be a side effect of using a 4k sector size. v1
> > logs only supported 512 byte sectors, so it's entirely possible that
> > the sector size is silently overriding the log version
> > specification. Probably should be fixed in mkfs.
> >
> >
>
> I tried to duplicate this, but in my config it didn't failed - how did
> you create the ramdisk?
I think you need to test on a 4k sector size disk. I use scsi_debug to
simulate physical 4k sector disk to reproduce this:
[root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug
[root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_mb=128 physblk_exp=3
[root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --getss /dev/sdc
4096
4096
512
[root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=1 -m crc=1 /dev/sdc
meta-data=/dev/sdc isize=512 agcount=4, agsize=8192 blks
= sectsz=4096 attr=2, projid32bit=1
= crc=1 finobt=1, sparse=0
data = bsize=4096 blocks=32768, imaxpct=25
= sunit=0 swidth=0 blks
naming =version 2 bsize=4096 ascii-ci=0 ftype=1
log =internal log bsize=4096 blocks=1605, version=2
= sectsz=4096 sunit=1 blks, lazy-count=1
realtime =none extsz=4096 blocks=0, rtextents=0
If you remove the "physblk_exp=3" at modprobe time, mkfs failed as
expected.
Thanks,
Eryu
>
> # modprobe brd rd_nr=1 rd_size=$((200*1024))
> # blockdev --getbsz /dev/ram0
> 4096
> # blockdev --getpbsz /dev/ram0
> 512
> # blockdev --getss /dev/ram0
> 512
>
> # mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
> V2 logs always enabled for CRC enabled filesytems
> Usage: mkfs.xfs
> [snip]
>
> Thanks, Jan
>
> PS: cc-ing XFS list - if it is mkfs bug, it is better there than in fstests.
>
> --
> Jan Tulak
> jtulak@redhat.com / jan@tulak.me
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
2016-07-18 11:47 ` Eryu Guan
@ 2016-07-18 11:54 ` Jan Tulak
-1 siblings, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-18 11:54 UTC (permalink / raw)
To: Eryu Guan; +Cc: Dave Chinner, fstests, xfs-oss
On Mon, Jul 18, 2016 at 1:47 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Mon, Jul 18, 2016 at 01:29:47PM +0200, Jan Tulak wrote:
>> On Mon, Jul 18, 2016 at 1:30 AM, Dave Chinner <david@fromorbit.com> wrote:
>> > On Sat, Jul 16, 2016 at 05:33:58PM +0800, Eryu Guan wrote:
>> >> On Thu, Jul 14, 2016 at 02:43:34PM +0200, Jan Tulak wrote:
>> >> > +do_mkfs_fail -l lazy-count=1garbage $SCRATCH_DEV
>> >> > +do_mkfs_fail -l lazy-count=2 $SCRATCH_DEV
>> >> > +do_mkfs_fail -l lazy-count=0 -m crc=1 $SCRATCH_DEV
>> >> > +do_mkfs_fail -l version=1 -m crc=1 $SCRATCH_DEV
>> >>
>> >> This test fails in my DAX testing, where SCRATCH_DEV is ramdisk. The
>> >> mkfs itself should fail, but it passed. Log version 2 was used
>> >> automatically, instead of prompting "V2 logs always enabled for CRC
>> >> enabled filesytems"
>> >>
>> >> [root@dhcp-66-86-11 xfstests]# mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
>> >> meta-data=/dev/ram0 isize=512 agcount=1, agsize=4096 blks
>> >> = sectsz=4096 attr=2, projid32bit=1
>> >> = crc=1 finobt=1, sparse=0
>> >> data = bsize=4096 blocks=4096, imaxpct=25
>> >> = sunit=0 swidth=0 blks
>> >> naming =version 2 bsize=4096 ascii-ci=0 ftype=1
>> >> log =internal log bsize=4096 blocks=1605, version=2
>> >> = sectsz=4096 sunit=1 blks, lazy-count=1
>> >> realtime =none extsz=4096 blocks=0, rtextents=0
>> >>
>> >> Is it a mkfs.xfs bug or the test case should handle the special case?
>> >
>> > Looks like it might be a side effect of using a 4k sector size. v1
>> > logs only supported 512 byte sectors, so it's entirely possible that
>> > the sector size is silently overriding the log version
>> > specification. Probably should be fixed in mkfs.
>> >
>> >
>>
>> I tried to duplicate this, but in my config it didn't failed - how did
>> you create the ramdisk?
>
> I think you need to test on a 4k sector size disk. I use scsi_debug to
> simulate physical 4k sector disk to reproduce this:
>
> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug
> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_mb=128 physblk_exp=3
> [root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --getss /dev/sdc
> 4096
> 4096
> 512
> [root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=1 -m crc=1 /dev/sdc
> meta-data=/dev/sdc isize=512 agcount=4, agsize=8192 blks
> = sectsz=4096 attr=2, projid32bit=1
> = crc=1 finobt=1, sparse=0
> data = bsize=4096 blocks=32768, imaxpct=25
> = sunit=0 swidth=0 blks
> naming =version 2 bsize=4096 ascii-ci=0 ftype=1
> log =internal log bsize=4096 blocks=1605, version=2
> = sectsz=4096 sunit=1 blks, lazy-count=1
> realtime =none extsz=4096 blocks=0, rtextents=0
>
> If you remove the "physblk_exp=3" at modprobe time, mkfs failed as
> expected.
>
Ah, thanks. :-) Now I can reproduce it and see what happens.
Thanks.
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
@ 2016-07-18 11:54 ` Jan Tulak
0 siblings, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-18 11:54 UTC (permalink / raw)
To: Eryu Guan; +Cc: fstests, xfs-oss
On Mon, Jul 18, 2016 at 1:47 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Mon, Jul 18, 2016 at 01:29:47PM +0200, Jan Tulak wrote:
>> On Mon, Jul 18, 2016 at 1:30 AM, Dave Chinner <david@fromorbit.com> wrote:
>> > On Sat, Jul 16, 2016 at 05:33:58PM +0800, Eryu Guan wrote:
>> >> On Thu, Jul 14, 2016 at 02:43:34PM +0200, Jan Tulak wrote:
>> >> > +do_mkfs_fail -l lazy-count=1garbage $SCRATCH_DEV
>> >> > +do_mkfs_fail -l lazy-count=2 $SCRATCH_DEV
>> >> > +do_mkfs_fail -l lazy-count=0 -m crc=1 $SCRATCH_DEV
>> >> > +do_mkfs_fail -l version=1 -m crc=1 $SCRATCH_DEV
>> >>
>> >> This test fails in my DAX testing, where SCRATCH_DEV is ramdisk. The
>> >> mkfs itself should fail, but it passed. Log version 2 was used
>> >> automatically, instead of prompting "V2 logs always enabled for CRC
>> >> enabled filesytems"
>> >>
>> >> [root@dhcp-66-86-11 xfstests]# mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
>> >> meta-data=/dev/ram0 isize=512 agcount=1, agsize=4096 blks
>> >> = sectsz=4096 attr=2, projid32bit=1
>> >> = crc=1 finobt=1, sparse=0
>> >> data = bsize=4096 blocks=4096, imaxpct=25
>> >> = sunit=0 swidth=0 blks
>> >> naming =version 2 bsize=4096 ascii-ci=0 ftype=1
>> >> log =internal log bsize=4096 blocks=1605, version=2
>> >> = sectsz=4096 sunit=1 blks, lazy-count=1
>> >> realtime =none extsz=4096 blocks=0, rtextents=0
>> >>
>> >> Is it a mkfs.xfs bug or the test case should handle the special case?
>> >
>> > Looks like it might be a side effect of using a 4k sector size. v1
>> > logs only supported 512 byte sectors, so it's entirely possible that
>> > the sector size is silently overriding the log version
>> > specification. Probably should be fixed in mkfs.
>> >
>> >
>>
>> I tried to duplicate this, but in my config it didn't failed - how did
>> you create the ramdisk?
>
> I think you need to test on a 4k sector size disk. I use scsi_debug to
> simulate physical 4k sector disk to reproduce this:
>
> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug
> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_mb=128 physblk_exp=3
> [root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --getss /dev/sdc
> 4096
> 4096
> 512
> [root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=1 -m crc=1 /dev/sdc
> meta-data=/dev/sdc isize=512 agcount=4, agsize=8192 blks
> = sectsz=4096 attr=2, projid32bit=1
> = crc=1 finobt=1, sparse=0
> data = bsize=4096 blocks=32768, imaxpct=25
> = sunit=0 swidth=0 blks
> naming =version 2 bsize=4096 ascii-ci=0 ftype=1
> log =internal log bsize=4096 blocks=1605, version=2
> = sectsz=4096 sunit=1 blks, lazy-count=1
> realtime =none extsz=4096 blocks=0, rtextents=0
>
> If you remove the "physblk_exp=3" at modprobe time, mkfs failed as
> expected.
>
Ah, thanks. :-) Now I can reproduce it and see what happens.
Thanks.
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
2016-07-18 11:54 ` Jan Tulak
@ 2016-07-18 12:33 ` Jan Tulak
-1 siblings, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-18 12:33 UTC (permalink / raw)
To: Eryu Guan; +Cc: Dave Chinner, fstests, xfs-oss
On Mon, Jul 18, 2016 at 1:54 PM, Jan Tulak <jtulak@redhat.com> wrote:
> On Mon, Jul 18, 2016 at 1:47 PM, Eryu Guan <eguan@redhat.com> wrote:
>> On Mon, Jul 18, 2016 at 01:29:47PM +0200, Jan Tulak wrote:
>>> On Mon, Jul 18, 2016 at 1:30 AM, Dave Chinner <david@fromorbit.com> wrote:
>>> > On Sat, Jul 16, 2016 at 05:33:58PM +0800, Eryu Guan wrote:
>>> >> On Thu, Jul 14, 2016 at 02:43:34PM +0200, Jan Tulak wrote:
>>> >> > +do_mkfs_fail -l lazy-count=1garbage $SCRATCH_DEV
>>> >> > +do_mkfs_fail -l lazy-count=2 $SCRATCH_DEV
>>> >> > +do_mkfs_fail -l lazy-count=0 -m crc=1 $SCRATCH_DEV
>>> >> > +do_mkfs_fail -l version=1 -m crc=1 $SCRATCH_DEV
>>> >>
>>> >> This test fails in my DAX testing, where SCRATCH_DEV is ramdisk. The
>>> >> mkfs itself should fail, but it passed. Log version 2 was used
>>> >> automatically, instead of prompting "V2 logs always enabled for CRC
>>> >> enabled filesytems"
>>> >>
>>> >> [root@dhcp-66-86-11 xfstests]# mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
>>> >> meta-data=/dev/ram0 isize=512 agcount=1, agsize=4096 blks
>>> >> = sectsz=4096 attr=2, projid32bit=1
>>> >> = crc=1 finobt=1, sparse=0
>>> >> data = bsize=4096 blocks=4096, imaxpct=25
>>> >> = sunit=0 swidth=0 blks
>>> >> naming =version 2 bsize=4096 ascii-ci=0 ftype=1
>>> >> log =internal log bsize=4096 blocks=1605, version=2
>>> >> = sectsz=4096 sunit=1 blks, lazy-count=1
>>> >> realtime =none extsz=4096 blocks=0, rtextents=0
>>> >>
>>> >> Is it a mkfs.xfs bug or the test case should handle the special case?
>>> >
>>> > Looks like it might be a side effect of using a 4k sector size. v1
>>> > logs only supported 512 byte sectors, so it's entirely possible that
>>> > the sector size is silently overriding the log version
>>> > specification. Probably should be fixed in mkfs.
>>> >
>>> >
>>>
>>> I tried to duplicate this, but in my config it didn't failed - how did
>>> you create the ramdisk?
>>
>> I think you need to test on a 4k sector size disk. I use scsi_debug to
>> simulate physical 4k sector disk to reproduce this:
>>
>> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug
>> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_mb=128 physblk_exp=3
>> [root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --getss /dev/sdc
>> 4096
>> 4096
>> 512
>> [root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=1 -m crc=1 /dev/sdc
>> meta-data=/dev/sdc isize=512 agcount=4, agsize=8192 blks
>> = sectsz=4096 attr=2, projid32bit=1
>> = crc=1 finobt=1, sparse=0
>> data = bsize=4096 blocks=32768, imaxpct=25
>> = sunit=0 swidth=0 blks
>> naming =version 2 bsize=4096 ascii-ci=0 ftype=1
>> log =internal log bsize=4096 blocks=1605, version=2
>> = sectsz=4096 sunit=1 blks, lazy-count=1
>> realtime =none extsz=4096 blocks=0, rtextents=0
>>
>> If you remove the "physblk_exp=3" at modprobe time, mkfs failed as
>> expected.
>>
>
> Ah, thanks. :-) Now I can reproduce it and see what happens.
And the culprit is in mkfs, some forty lines before the crc & log version check:
2026 ⇥ } else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
2027 ⇥ ⇥ lsu = blocksize;
2028 ⇥ ⇥ sb_feat.log_version = 2;
2029 ⇥ }
The possible solutions I can think of are:
1) Make a more complicated check.
This would change just a line or two, but most likely, we would test
the same thing multiple times and added unnecessary complexity.
2) Move the crc checks into an earlier place.
The only value that can be changed in crc checks from default is
finobt, and finobt is not read nor modified between argument parsing
and the crc check. This looks like a simple and safe thing, but it
will move some ~60 lines. I tested moving the crc testing block right
behind this:
1968 ⇥ memset(&ft, 0, sizeof(ft));
1969 ⇥ get_topology(&xi, &ft, force_overwrite);
And it works. I didn't run full test suite yet, though.
3) Change the silent autoupdating of log version. The default is 2 and
if user explicitly states v1, then we should either warn or fail
entirely if it is not possible to make such fs.
4) Do nothing with mkfs and instead, update the test to check the
sector size and expect pass/fail...
But this issue boils down to the question "what is the correct order
of doing things"? Should we try to autosolve what we can at first, and
check for remaining issues after that? Or should we check for issues
with the input ASAP, even if it can be solved by updating the input to
match the physical device? Right now, it looks like "someone wrote it
that way a long time ago" mix of both.
Your ideas, guys?
Thanks,
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
@ 2016-07-18 12:33 ` Jan Tulak
0 siblings, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-18 12:33 UTC (permalink / raw)
To: Eryu Guan; +Cc: fstests, xfs-oss
On Mon, Jul 18, 2016 at 1:54 PM, Jan Tulak <jtulak@redhat.com> wrote:
> On Mon, Jul 18, 2016 at 1:47 PM, Eryu Guan <eguan@redhat.com> wrote:
>> On Mon, Jul 18, 2016 at 01:29:47PM +0200, Jan Tulak wrote:
>>> On Mon, Jul 18, 2016 at 1:30 AM, Dave Chinner <david@fromorbit.com> wrote:
>>> > On Sat, Jul 16, 2016 at 05:33:58PM +0800, Eryu Guan wrote:
>>> >> On Thu, Jul 14, 2016 at 02:43:34PM +0200, Jan Tulak wrote:
>>> >> > +do_mkfs_fail -l lazy-count=1garbage $SCRATCH_DEV
>>> >> > +do_mkfs_fail -l lazy-count=2 $SCRATCH_DEV
>>> >> > +do_mkfs_fail -l lazy-count=0 -m crc=1 $SCRATCH_DEV
>>> >> > +do_mkfs_fail -l version=1 -m crc=1 $SCRATCH_DEV
>>> >>
>>> >> This test fails in my DAX testing, where SCRATCH_DEV is ramdisk. The
>>> >> mkfs itself should fail, but it passed. Log version 2 was used
>>> >> automatically, instead of prompting "V2 logs always enabled for CRC
>>> >> enabled filesytems"
>>> >>
>>> >> [root@dhcp-66-86-11 xfstests]# mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
>>> >> meta-data=/dev/ram0 isize=512 agcount=1, agsize=4096 blks
>>> >> = sectsz=4096 attr=2, projid32bit=1
>>> >> = crc=1 finobt=1, sparse=0
>>> >> data = bsize=4096 blocks=4096, imaxpct=25
>>> >> = sunit=0 swidth=0 blks
>>> >> naming =version 2 bsize=4096 ascii-ci=0 ftype=1
>>> >> log =internal log bsize=4096 blocks=1605, version=2
>>> >> = sectsz=4096 sunit=1 blks, lazy-count=1
>>> >> realtime =none extsz=4096 blocks=0, rtextents=0
>>> >>
>>> >> Is it a mkfs.xfs bug or the test case should handle the special case?
>>> >
>>> > Looks like it might be a side effect of using a 4k sector size. v1
>>> > logs only supported 512 byte sectors, so it's entirely possible that
>>> > the sector size is silently overriding the log version
>>> > specification. Probably should be fixed in mkfs.
>>> >
>>> >
>>>
>>> I tried to duplicate this, but in my config it didn't failed - how did
>>> you create the ramdisk?
>>
>> I think you need to test on a 4k sector size disk. I use scsi_debug to
>> simulate physical 4k sector disk to reproduce this:
>>
>> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug
>> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_mb=128 physblk_exp=3
>> [root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --getss /dev/sdc
>> 4096
>> 4096
>> 512
>> [root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=1 -m crc=1 /dev/sdc
>> meta-data=/dev/sdc isize=512 agcount=4, agsize=8192 blks
>> = sectsz=4096 attr=2, projid32bit=1
>> = crc=1 finobt=1, sparse=0
>> data = bsize=4096 blocks=32768, imaxpct=25
>> = sunit=0 swidth=0 blks
>> naming =version 2 bsize=4096 ascii-ci=0 ftype=1
>> log =internal log bsize=4096 blocks=1605, version=2
>> = sectsz=4096 sunit=1 blks, lazy-count=1
>> realtime =none extsz=4096 blocks=0, rtextents=0
>>
>> If you remove the "physblk_exp=3" at modprobe time, mkfs failed as
>> expected.
>>
>
> Ah, thanks. :-) Now I can reproduce it and see what happens.
And the culprit is in mkfs, some forty lines before the crc & log version check:
2026 ⇥ } else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
2027 ⇥ ⇥ lsu = blocksize;
2028 ⇥ ⇥ sb_feat.log_version = 2;
2029 ⇥ }
The possible solutions I can think of are:
1) Make a more complicated check.
This would change just a line or two, but most likely, we would test
the same thing multiple times and added unnecessary complexity.
2) Move the crc checks into an earlier place.
The only value that can be changed in crc checks from default is
finobt, and finobt is not read nor modified between argument parsing
and the crc check. This looks like a simple and safe thing, but it
will move some ~60 lines. I tested moving the crc testing block right
behind this:
1968 ⇥ memset(&ft, 0, sizeof(ft));
1969 ⇥ get_topology(&xi, &ft, force_overwrite);
And it works. I didn't run full test suite yet, though.
3) Change the silent autoupdating of log version. The default is 2 and
if user explicitly states v1, then we should either warn or fail
entirely if it is not possible to make such fs.
4) Do nothing with mkfs and instead, update the test to check the
sector size and expect pass/fail...
But this issue boils down to the question "what is the correct order
of doing things"? Should we try to autosolve what we can at first, and
check for remaining issues after that? Or should we check for issues
with the input ASAP, even if it can be solved by updating the input to
match the physical device? Right now, it looks like "someone wrote it
that way a long time ago" mix of both.
Your ideas, guys?
Thanks,
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
2016-07-18 12:33 ` Jan Tulak
@ 2016-07-20 23:54 ` Dave Chinner
-1 siblings, 0 replies; 30+ messages in thread
From: Dave Chinner @ 2016-07-20 23:54 UTC (permalink / raw)
To: Jan Tulak; +Cc: Eryu Guan, fstests, xfs-oss
On Mon, Jul 18, 2016 at 02:33:29PM +0200, Jan Tulak wrote:
> On Mon, Jul 18, 2016 at 1:54 PM, Jan Tulak <jtulak@redhat.com> wrote:
> > On Mon, Jul 18, 2016 at 1:47 PM, Eryu Guan <eguan@redhat.com> wrote:
> >> On Mon, Jul 18, 2016 at 01:29:47PM +0200, Jan Tulak wrote:
> >>> On Mon, Jul 18, 2016 at 1:30 AM, Dave Chinner <david@fromorbit.com> wrote:
> >>> > On Sat, Jul 16, 2016 at 05:33:58PM +0800, Eryu Guan wrote:
> >>> >> On Thu, Jul 14, 2016 at 02:43:34PM +0200, Jan Tulak wrote:
> >>> >> > +do_mkfs_fail -l lazy-count=1garbage $SCRATCH_DEV
> >>> >> > +do_mkfs_fail -l lazy-count=2 $SCRATCH_DEV
> >>> >> > +do_mkfs_fail -l lazy-count=0 -m crc=1 $SCRATCH_DEV
> >>> >> > +do_mkfs_fail -l version=1 -m crc=1 $SCRATCH_DEV
> >>> >>
> >>> >> This test fails in my DAX testing, where SCRATCH_DEV is ramdisk. The
> >>> >> mkfs itself should fail, but it passed. Log version 2 was used
> >>> >> automatically, instead of prompting "V2 logs always enabled for CRC
> >>> >> enabled filesytems"
> >>> >>
> >>> >> [root@dhcp-66-86-11 xfstests]# mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
> >>> >> meta-data=/dev/ram0 isize=512 agcount=1, agsize=4096 blks
> >>> >> = sectsz=4096 attr=2, projid32bit=1
> >>> >> = crc=1 finobt=1, sparse=0
> >>> >> data = bsize=4096 blocks=4096, imaxpct=25
> >>> >> = sunit=0 swidth=0 blks
> >>> >> naming =version 2 bsize=4096 ascii-ci=0 ftype=1
> >>> >> log =internal log bsize=4096 blocks=1605, version=2
> >>> >> = sectsz=4096 sunit=1 blks, lazy-count=1
> >>> >> realtime =none extsz=4096 blocks=0, rtextents=0
> >>> >>
> >>> >> Is it a mkfs.xfs bug or the test case should handle the special case?
> >>> >
> >>> > Looks like it might be a side effect of using a 4k sector size. v1
> >>> > logs only supported 512 byte sectors, so it's entirely possible that
> >>> > the sector size is silently overriding the log version
> >>> > specification. Probably should be fixed in mkfs.
> >>> >
> >>> >
> >>>
> >>> I tried to duplicate this, but in my config it didn't failed - how did
> >>> you create the ramdisk?
> >>
> >> I think you need to test on a 4k sector size disk. I use scsi_debug to
> >> simulate physical 4k sector disk to reproduce this:
> >>
> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug
> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_mb=128 physblk_exp=3
> >> [root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --getss /dev/sdc
> >> 4096
> >> 4096
> >> 512
> >> [root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=1 -m crc=1 /dev/sdc
So this is an invalid filesystem configuration. It should be
detected as such during command line parsing and rejected before we
get anywhere near checking topology constraints. In mkfs
terms, it's a conflicting option set.
> And the culprit is in mkfs, some forty lines before the crc & log version check:
>
> 2026 ⇥ } else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
> 2027 ⇥ ⇥ lsu = blocksize;
> 2028 ⇥ ⇥ sb_feat.log_version = 2;
> 2029 ⇥ }
>
> The possible solutions I can think of are:
None of which really appeal because, IMO, they are trying to solve
the wrong problem.
The whole point of moving to table based command line option parsing
is that we can encode these sorts of conflicts into the option
table. The conflict resolution in the option table is currently not
complete - it can only encode and detect conflicts within a
suboption type, but not across suboption types (e.g. within -d
suboptions, but not between -d and -l suboptions).
This is simply because I never got as far as implementing this level
of conflict encoding/resolution. In essence, the conflict array
needs to define the sub option type, the suboption that is
in conflict and the value that it conflicts against. Hence the
conflicts table can then encode such things as "version 1 logs are
invalid for CRC enabled filesystems" and vice versa.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
@ 2016-07-20 23:54 ` Dave Chinner
0 siblings, 0 replies; 30+ messages in thread
From: Dave Chinner @ 2016-07-20 23:54 UTC (permalink / raw)
To: Jan Tulak; +Cc: Eryu Guan, fstests, xfs-oss
On Mon, Jul 18, 2016 at 02:33:29PM +0200, Jan Tulak wrote:
> On Mon, Jul 18, 2016 at 1:54 PM, Jan Tulak <jtulak@redhat.com> wrote:
> > On Mon, Jul 18, 2016 at 1:47 PM, Eryu Guan <eguan@redhat.com> wrote:
> >> On Mon, Jul 18, 2016 at 01:29:47PM +0200, Jan Tulak wrote:
> >>> On Mon, Jul 18, 2016 at 1:30 AM, Dave Chinner <david@fromorbit.com> wrote:
> >>> > On Sat, Jul 16, 2016 at 05:33:58PM +0800, Eryu Guan wrote:
> >>> >> On Thu, Jul 14, 2016 at 02:43:34PM +0200, Jan Tulak wrote:
> >>> >> > +do_mkfs_fail -l lazy-count=1garbage $SCRATCH_DEV
> >>> >> > +do_mkfs_fail -l lazy-count=2 $SCRATCH_DEV
> >>> >> > +do_mkfs_fail -l lazy-count=0 -m crc=1 $SCRATCH_DEV
> >>> >> > +do_mkfs_fail -l version=1 -m crc=1 $SCRATCH_DEV
> >>> >>
> >>> >> This test fails in my DAX testing, where SCRATCH_DEV is ramdisk. The
> >>> >> mkfs itself should fail, but it passed. Log version 2 was used
> >>> >> automatically, instead of prompting "V2 logs always enabled for CRC
> >>> >> enabled filesytems"
> >>> >>
> >>> >> [root@dhcp-66-86-11 xfstests]# mkfs -t xfs -f -l version=1 -m crc=1 /dev/ram0
> >>> >> meta-data=/dev/ram0 isize=512 agcount=1, agsize=4096 blks
> >>> >> = sectsz=4096 attr=2, projid32bit=1
> >>> >> = crc=1 finobt=1, sparse=0
> >>> >> data = bsize=4096 blocks=4096, imaxpct=25
> >>> >> = sunit=0 swidth=0 blks
> >>> >> naming =version 2 bsize=4096 ascii-ci=0 ftype=1
> >>> >> log =internal log bsize=4096 blocks=1605, version=2
> >>> >> = sectsz=4096 sunit=1 blks, lazy-count=1
> >>> >> realtime =none extsz=4096 blocks=0, rtextents=0
> >>> >>
> >>> >> Is it a mkfs.xfs bug or the test case should handle the special case?
> >>> >
> >>> > Looks like it might be a side effect of using a 4k sector size. v1
> >>> > logs only supported 512 byte sectors, so it's entirely possible that
> >>> > the sector size is silently overriding the log version
> >>> > specification. Probably should be fixed in mkfs.
> >>> >
> >>> >
> >>>
> >>> I tried to duplicate this, but in my config it didn't failed - how did
> >>> you create the ramdisk?
> >>
> >> I think you need to test on a 4k sector size disk. I use scsi_debug to
> >> simulate physical 4k sector disk to reproduce this:
> >>
> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug
> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_mb=128 physblk_exp=3
> >> [root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --getss /dev/sdc
> >> 4096
> >> 4096
> >> 512
> >> [root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=1 -m crc=1 /dev/sdc
So this is an invalid filesystem configuration. It should be
detected as such during command line parsing and rejected before we
get anywhere near checking topology constraints. In mkfs
terms, it's a conflicting option set.
> And the culprit is in mkfs, some forty lines before the crc & log version check:
>
> 2026 ⇥ } else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
> 2027 ⇥ ⇥ lsu = blocksize;
> 2028 ⇥ ⇥ sb_feat.log_version = 2;
> 2029 ⇥ }
>
> The possible solutions I can think of are:
None of which really appeal because, IMO, they are trying to solve
the wrong problem.
The whole point of moving to table based command line option parsing
is that we can encode these sorts of conflicts into the option
table. The conflict resolution in the option table is currently not
complete - it can only encode and detect conflicts within a
suboption type, but not across suboption types (e.g. within -d
suboptions, but not between -d and -l suboptions).
This is simply because I never got as far as implementing this level
of conflict encoding/resolution. In essence, the conflict array
needs to define the sub option type, the suboption that is
in conflict and the value that it conflicts against. Hence the
conflicts table can then encode such things as "version 1 logs are
invalid for CRC enabled filesystems" and vice versa.
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] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
2016-07-20 23:54 ` Dave Chinner
@ 2016-07-21 14:24 ` Jan Tulak
-1 siblings, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-21 14:24 UTC (permalink / raw)
To: Dave Chinner; +Cc: Eryu Guan, fstests, xfs-oss
On Thu, Jul 21, 2016 at 1:54 AM, Dave Chinner <david@fromorbit.com> wrote:
>> >> I think you need to test on a 4k sector size disk. I use scsi_debug to
>> >> simulate physical 4k sector disk to reproduce this:
>> >>
>> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug
>> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_mb=128 physblk_exp=3
>> >> [root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --getss /dev/sdc
>> >> 4096
>> >> 4096
>> >> 512
>> >> [root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=1 -m crc=1 /dev/sdc
>
> So this is an invalid filesystem configuration. It should be
> detected as such during command line parsing and rejected before we
> get anywhere near checking topology constraints. In mkfs
> terms, it's a conflicting option set.
>
>> And the culprit is in mkfs, some forty lines before the crc & log version check:
>>
>> 2026 ⇥ } else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
>> 2027 ⇥ ⇥ lsu = blocksize;
>> 2028 ⇥ ⇥ sb_feat.log_version = 2;
>> 2029 ⇥ }
>>
>> The possible solutions I can think of are:
>
> None of which really appeal because, IMO, they are trying to solve
> the wrong problem.
>
> The whole point of moving to table based command line option parsing
> is that we can encode these sorts of conflicts into the option
> table. The conflict resolution in the option table is currently not
> complete - it can only encode and detect conflicts within a
> suboption type, but not across suboption types (e.g. within -d
> suboptions, but not between -d and -l suboptions).
>
> This is simply because I never got as far as implementing this level
> of conflict encoding/resolution. In essence, the conflict array
> needs to define the sub option type, the suboption that is
> in conflict and the value that it conflicts against. Hence the
> conflicts table can then encode such things as "version 1 logs are
> invalid for CRC enabled filesystems" and vice versa.
>
Ok, in long term, the correct way is to extend the conflicts table.
But what in the meantime? Are we going to let it be now until it is
fixed by the enhanced table?
And regarding my question at the end of the mail, I interpret your
answer as "if the arguments are wrong, fail ASAP and don't try to fix
it."
Thanks,
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
@ 2016-07-21 14:24 ` Jan Tulak
0 siblings, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-21 14:24 UTC (permalink / raw)
To: Dave Chinner; +Cc: Eryu Guan, fstests, xfs-oss
On Thu, Jul 21, 2016 at 1:54 AM, Dave Chinner <david@fromorbit.com> wrote:
>> >> I think you need to test on a 4k sector size disk. I use scsi_debug to
>> >> simulate physical 4k sector disk to reproduce this:
>> >>
>> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug
>> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_mb=128 physblk_exp=3
>> >> [root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --getss /dev/sdc
>> >> 4096
>> >> 4096
>> >> 512
>> >> [root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=1 -m crc=1 /dev/sdc
>
> So this is an invalid filesystem configuration. It should be
> detected as such during command line parsing and rejected before we
> get anywhere near checking topology constraints. In mkfs
> terms, it's a conflicting option set.
>
>> And the culprit is in mkfs, some forty lines before the crc & log version check:
>>
>> 2026 ⇥ } else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
>> 2027 ⇥ ⇥ lsu = blocksize;
>> 2028 ⇥ ⇥ sb_feat.log_version = 2;
>> 2029 ⇥ }
>>
>> The possible solutions I can think of are:
>
> None of which really appeal because, IMO, they are trying to solve
> the wrong problem.
>
> The whole point of moving to table based command line option parsing
> is that we can encode these sorts of conflicts into the option
> table. The conflict resolution in the option table is currently not
> complete - it can only encode and detect conflicts within a
> suboption type, but not across suboption types (e.g. within -d
> suboptions, but not between -d and -l suboptions).
>
> This is simply because I never got as far as implementing this level
> of conflict encoding/resolution. In essence, the conflict array
> needs to define the sub option type, the suboption that is
> in conflict and the value that it conflicts against. Hence the
> conflicts table can then encode such things as "version 1 logs are
> invalid for CRC enabled filesystems" and vice versa.
>
Ok, in long term, the correct way is to extend the conflicts table.
But what in the meantime? Are we going to let it be now until it is
fixed by the enhanced table?
And regarding my question at the end of the mail, I interpret your
answer as "if the arguments are wrong, fail ASAP and don't try to fix
it."
Thanks,
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
2016-07-21 14:24 ` Jan Tulak
@ 2016-07-21 22:40 ` Dave Chinner
-1 siblings, 0 replies; 30+ messages in thread
From: Dave Chinner @ 2016-07-21 22:40 UTC (permalink / raw)
To: Jan Tulak; +Cc: Eryu Guan, fstests, xfs-oss
On Thu, Jul 21, 2016 at 04:24:52PM +0200, Jan Tulak wrote:
> On Thu, Jul 21, 2016 at 1:54 AM, Dave Chinner <david@fromorbit.com> wrote:
> >> >> I think you need to test on a 4k sector size disk. I use scsi_debug to
> >> >> simulate physical 4k sector disk to reproduce this:
> >> >>
> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug
> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_mb=128 physblk_exp=3
> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --getss /dev/sdc
> >> >> 4096
> >> >> 4096
> >> >> 512
> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=1 -m crc=1 /dev/sdc
> >
> > So this is an invalid filesystem configuration. It should be
> > detected as such during command line parsing and rejected before we
> > get anywhere near checking topology constraints. In mkfs
> > terms, it's a conflicting option set.
> >
> >> And the culprit is in mkfs, some forty lines before the crc & log version check:
> >>
> >> 2026 ⇥ } else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
> >> 2027 ⇥ ⇥ lsu = blocksize;
> >> 2028 ⇥ ⇥ sb_feat.log_version = 2;
> >> 2029 ⇥ }
> >>
> >> The possible solutions I can think of are:
> >
> > None of which really appeal because, IMO, they are trying to solve
> > the wrong problem.
> >
> > The whole point of moving to table based command line option parsing
> > is that we can encode these sorts of conflicts into the option
> > table. The conflict resolution in the option table is currently not
> > complete - it can only encode and detect conflicts within a
> > suboption type, but not across suboption types (e.g. within -d
> > suboptions, but not between -d and -l suboptions).
> >
> > This is simply because I never got as far as implementing this level
> > of conflict encoding/resolution. In essence, the conflict array
> > needs to define the sub option type, the suboption that is
> > in conflict and the value that it conflicts against. Hence the
> > conflicts table can then encode such things as "version 1 logs are
> > invalid for CRC enabled filesystems" and vice versa.
> >
>
> Ok, in long term, the correct way is to extend the conflicts table.
Not long term. It's fairly simple to do.
1. Convert all the individual subopt parameter tables to an array of tables
with a defined index for each set of subopts,
2. add a value field to the parameter, and store the CLI value in
it when it is set
3. make the conflicts array in each subopts a structure like:
struct conflicts {
int subopt;
int index;
int invalid_value;
};
and convert all the existing conflicts to this format
4. Define cross-subopt conflicts like this:
.subopt_params = {
{ .index = M_CRC,
- .conflicts = { LAST_CONFLICT },
+ .conflicts = { { LOG, L_VERSION, 1 },
+ { LAST_CONFLICT, 0, 0 }, },
.minval = 0,
.maxval = 1,
.defaultval = 1,
},
And the L_VERSION subopt parameter will have a conflict like
+ .conflicts = { { META, M_CRC, 1 },
+ { LAST_CONFLICT, 0, 0 }, },
5. update the conflict lookup to do cross option lookups via
checking the relevant option conflict table. e.g by checking the
conflict[i].value against subopt[LOG].subopt_params[L_VERSION].value...
> But what in the meantime? Are we going to let it be now until it is
> fixed by the enhanced table?
IMO: fix it once, fix it properly.
> And regarding my question at the end of the mail, I interpret your
> answer as "if the arguments are wrong, fail ASAP and don't try to fix
> it."
The first step in any program shoul dbe to validate user supplied
inputs. Once they are validated and known good, you don't have to
add random code to handle invalid combinations - you can just assume
the inputs are valid to begin with and those corner cases don't need
to be handled.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
@ 2016-07-21 22:40 ` Dave Chinner
0 siblings, 0 replies; 30+ messages in thread
From: Dave Chinner @ 2016-07-21 22:40 UTC (permalink / raw)
To: Jan Tulak; +Cc: Eryu Guan, fstests, xfs-oss
On Thu, Jul 21, 2016 at 04:24:52PM +0200, Jan Tulak wrote:
> On Thu, Jul 21, 2016 at 1:54 AM, Dave Chinner <david@fromorbit.com> wrote:
> >> >> I think you need to test on a 4k sector size disk. I use scsi_debug to
> >> >> simulate physical 4k sector disk to reproduce this:
> >> >>
> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug
> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_mb=128 physblk_exp=3
> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --getss /dev/sdc
> >> >> 4096
> >> >> 4096
> >> >> 512
> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=1 -m crc=1 /dev/sdc
> >
> > So this is an invalid filesystem configuration. It should be
> > detected as such during command line parsing and rejected before we
> > get anywhere near checking topology constraints. In mkfs
> > terms, it's a conflicting option set.
> >
> >> And the culprit is in mkfs, some forty lines before the crc & log version check:
> >>
> >> 2026 ⇥ } else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
> >> 2027 ⇥ ⇥ lsu = blocksize;
> >> 2028 ⇥ ⇥ sb_feat.log_version = 2;
> >> 2029 ⇥ }
> >>
> >> The possible solutions I can think of are:
> >
> > None of which really appeal because, IMO, they are trying to solve
> > the wrong problem.
> >
> > The whole point of moving to table based command line option parsing
> > is that we can encode these sorts of conflicts into the option
> > table. The conflict resolution in the option table is currently not
> > complete - it can only encode and detect conflicts within a
> > suboption type, but not across suboption types (e.g. within -d
> > suboptions, but not between -d and -l suboptions).
> >
> > This is simply because I never got as far as implementing this level
> > of conflict encoding/resolution. In essence, the conflict array
> > needs to define the sub option type, the suboption that is
> > in conflict and the value that it conflicts against. Hence the
> > conflicts table can then encode such things as "version 1 logs are
> > invalid for CRC enabled filesystems" and vice versa.
> >
>
> Ok, in long term, the correct way is to extend the conflicts table.
Not long term. It's fairly simple to do.
1. Convert all the individual subopt parameter tables to an array of tables
with a defined index for each set of subopts,
2. add a value field to the parameter, and store the CLI value in
it when it is set
3. make the conflicts array in each subopts a structure like:
struct conflicts {
int subopt;
int index;
int invalid_value;
};
and convert all the existing conflicts to this format
4. Define cross-subopt conflicts like this:
.subopt_params = {
{ .index = M_CRC,
- .conflicts = { LAST_CONFLICT },
+ .conflicts = { { LOG, L_VERSION, 1 },
+ { LAST_CONFLICT, 0, 0 }, },
.minval = 0,
.maxval = 1,
.defaultval = 1,
},
And the L_VERSION subopt parameter will have a conflict like
+ .conflicts = { { META, M_CRC, 1 },
+ { LAST_CONFLICT, 0, 0 }, },
5. update the conflict lookup to do cross option lookups via
checking the relevant option conflict table. e.g by checking the
conflict[i].value against subopt[LOG].subopt_params[L_VERSION].value...
> But what in the meantime? Are we going to let it be now until it is
> fixed by the enhanced table?
IMO: fix it once, fix it properly.
> And regarding my question at the end of the mail, I interpret your
> answer as "if the arguments are wrong, fail ASAP and don't try to fix
> it."
The first step in any program shoul dbe to validate user supplied
inputs. Once they are validated and known good, you don't have to
add random code to handle invalid combinations - you can just assume
the inputs are valid to begin with and those corner cases don't need
to be handled.
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] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
2016-07-21 22:40 ` Dave Chinner
@ 2016-07-22 13:08 ` Jan Tulak
-1 siblings, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-22 13:08 UTC (permalink / raw)
To: Dave Chinner; +Cc: Eryu Guan, fstests, xfs-oss
On Fri, Jul 22, 2016 at 12:40 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Thu, Jul 21, 2016 at 04:24:52PM +0200, Jan Tulak wrote:
>> On Thu, Jul 21, 2016 at 1:54 AM, Dave Chinner <david@fromorbit.com> wrote:
>> >> >> I think you need to test on a 4k sector size disk. I use scsi_debug to
>> >> >> simulate physical 4k sector disk to reproduce this:
>> >> >>
>> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug
>> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_mb=128 physblk_exp=3
>> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --getss /dev/sdc
>> >> >> 4096
>> >> >> 4096
>> >> >> 512
>> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=1 -m crc=1 /dev/sdc
>> >
>> > So this is an invalid filesystem configuration. It should be
>> > detected as such during command line parsing and rejected before we
>> > get anywhere near checking topology constraints. In mkfs
>> > terms, it's a conflicting option set.
>> >
>> >> And the culprit is in mkfs, some forty lines before the crc & log version check:
>> >>
>> >> 2026 ⇥ } else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
>> >> 2027 ⇥ ⇥ lsu = blocksize;
>> >> 2028 ⇥ ⇥ sb_feat.log_version = 2;
>> >> 2029 ⇥ }
>> >>
>> >> The possible solutions I can think of are:
>> >
>> > None of which really appeal because, IMO, they are trying to solve
>> > the wrong problem.
>> >
>> > The whole point of moving to table based command line option parsing
>> > is that we can encode these sorts of conflicts into the option
>> > table. The conflict resolution in the option table is currently not
>> > complete - it can only encode and detect conflicts within a
>> > suboption type, but not across suboption types (e.g. within -d
>> > suboptions, but not between -d and -l suboptions).
>> >
>> > This is simply because I never got as far as implementing this level
>> > of conflict encoding/resolution. In essence, the conflict array
>> > needs to define the sub option type, the suboption that is
>> > in conflict and the value that it conflicts against. Hence the
>> > conflicts table can then encode such things as "version 1 logs are
>> > invalid for CRC enabled filesystems" and vice versa.
>> >
>>
>> Ok, in long term, the correct way is to extend the conflicts table.
>
> Not long term. It's fairly simple to do.
>
> 1. Convert all the individual subopt parameter tables to an array of tables
> with a defined index for each set of subopts,
> 2. add a value field to the parameter, and store the CLI value in
> it when it is set
> 3. make the conflicts array in each subopts a structure like:
>
> struct conflicts {
> int subopt;
> int index;
> int invalid_value;
> };
>
> and convert all the existing conflicts to this format
>
> 4. Define cross-subopt conflicts like this:
>
> .subopt_params = {
> { .index = M_CRC,
> - .conflicts = { LAST_CONFLICT },
> + .conflicts = { { LOG, L_VERSION, 1 },
> + { LAST_CONFLICT, 0, 0 }, },
> .minval = 0,
> .maxval = 1,
> .defaultval = 1,
> },
>
> And the L_VERSION subopt parameter will have a conflict like
>
> + .conflicts = { { META, M_CRC, 1 },
> + { LAST_CONFLICT, 0, 0 }, },
>
> 5. update the conflict lookup to do cross option lookups via
> checking the relevant option conflict table. e.g by checking the
> conflict[i].value against subopt[LOG].subopt_params[L_VERSION].value...
>
>> But what in the meantime? Are we going to let it be now until it is
>> fixed by the enhanced table?
>
> IMO: fix it once, fix it properly.
>
>> And regarding my question at the end of the mail, I interpret your
>> answer as "if the arguments are wrong, fail ASAP and don't try to fix
>> it."
>
> The first step in any program shoul dbe to validate user supplied
> inputs. Once they are validated and known good, you don't have to
> add random code to handle invalid combinations - you can just assume
> the inputs are valid to begin with and those corner cases don't need
> to be handled.
>
OK, thanks for showing the way, I will work on the changes.
In any case, to return to the beginning, the new test added by this
patch is all right.
Thanks,
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/6] xfstests: Add mkfs input validation tests
@ 2016-07-22 13:08 ` Jan Tulak
0 siblings, 0 replies; 30+ messages in thread
From: Jan Tulak @ 2016-07-22 13:08 UTC (permalink / raw)
To: Dave Chinner; +Cc: Eryu Guan, fstests, xfs-oss
On Fri, Jul 22, 2016 at 12:40 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Thu, Jul 21, 2016 at 04:24:52PM +0200, Jan Tulak wrote:
>> On Thu, Jul 21, 2016 at 1:54 AM, Dave Chinner <david@fromorbit.com> wrote:
>> >> >> I think you need to test on a 4k sector size disk. I use scsi_debug to
>> >> >> simulate physical 4k sector disk to reproduce this:
>> >> >>
>> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe -r scsi_debug
>> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# modprobe scsi_debug dev_size_mb=128 physblk_exp=3
>> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# blockdev --getbsz --getpbsz --getss /dev/sdc
>> >> >> 4096
>> >> >> 4096
>> >> >> 512
>> >> >> [root@dhcp-66-86-11 xfsprogs-dev]# mkfs -t xfs -l version=1 -m crc=1 /dev/sdc
>> >
>> > So this is an invalid filesystem configuration. It should be
>> > detected as such during command line parsing and rejected before we
>> > get anywhere near checking topology constraints. In mkfs
>> > terms, it's a conflicting option set.
>> >
>> >> And the culprit is in mkfs, some forty lines before the crc & log version check:
>> >>
>> >> 2026 ⇥ } else if (lsectorsize > XFS_MIN_SECTORSIZE && !lsu && !lsunit) {
>> >> 2027 ⇥ ⇥ lsu = blocksize;
>> >> 2028 ⇥ ⇥ sb_feat.log_version = 2;
>> >> 2029 ⇥ }
>> >>
>> >> The possible solutions I can think of are:
>> >
>> > None of which really appeal because, IMO, they are trying to solve
>> > the wrong problem.
>> >
>> > The whole point of moving to table based command line option parsing
>> > is that we can encode these sorts of conflicts into the option
>> > table. The conflict resolution in the option table is currently not
>> > complete - it can only encode and detect conflicts within a
>> > suboption type, but not across suboption types (e.g. within -d
>> > suboptions, but not between -d and -l suboptions).
>> >
>> > This is simply because I never got as far as implementing this level
>> > of conflict encoding/resolution. In essence, the conflict array
>> > needs to define the sub option type, the suboption that is
>> > in conflict and the value that it conflicts against. Hence the
>> > conflicts table can then encode such things as "version 1 logs are
>> > invalid for CRC enabled filesystems" and vice versa.
>> >
>>
>> Ok, in long term, the correct way is to extend the conflicts table.
>
> Not long term. It's fairly simple to do.
>
> 1. Convert all the individual subopt parameter tables to an array of tables
> with a defined index for each set of subopts,
> 2. add a value field to the parameter, and store the CLI value in
> it when it is set
> 3. make the conflicts array in each subopts a structure like:
>
> struct conflicts {
> int subopt;
> int index;
> int invalid_value;
> };
>
> and convert all the existing conflicts to this format
>
> 4. Define cross-subopt conflicts like this:
>
> .subopt_params = {
> { .index = M_CRC,
> - .conflicts = { LAST_CONFLICT },
> + .conflicts = { { LOG, L_VERSION, 1 },
> + { LAST_CONFLICT, 0, 0 }, },
> .minval = 0,
> .maxval = 1,
> .defaultval = 1,
> },
>
> And the L_VERSION subopt parameter will have a conflict like
>
> + .conflicts = { { META, M_CRC, 1 },
> + { LAST_CONFLICT, 0, 0 }, },
>
> 5. update the conflict lookup to do cross option lookups via
> checking the relevant option conflict table. e.g by checking the
> conflict[i].value against subopt[LOG].subopt_params[L_VERSION].value...
>
>> But what in the meantime? Are we going to let it be now until it is
>> fixed by the enhanced table?
>
> IMO: fix it once, fix it properly.
>
>> And regarding my question at the end of the mail, I interpret your
>> answer as "if the arguments are wrong, fail ASAP and don't try to fix
>> it."
>
> The first step in any program shoul dbe to validate user supplied
> inputs. Once they are validated and known good, you don't have to
> add random code to handle invalid combinations - you can just assume
> the inputs are valid to begin with and those corner cases don't need
> to be handled.
>
OK, thanks for showing the way, I will work on the changes.
In any case, to return to the beginning, the new test added by this
patch is all right.
Thanks,
Jan
--
Jan Tulak
jtulak@redhat.com / jan@tulak.me
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2016-07-22 13:08 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-14 12:43 [PATCH 0/6 v3] xfstests: some small fixes and mkfs validation test Jan Tulak
2016-07-14 12:43 ` [PATCH 1/6] xfstests: Fix installation for extended names Jan Tulak
2016-07-14 12:43 ` [PATCH 2/6] fstests: filename handling for extended names in ./check was on a wrong place Jan Tulak
2016-07-14 12:43 ` [PATCH 3/6] xfstests: remove unused variable Jan Tulak
2016-07-14 12:43 ` [PATCH 4/6] xfstests: add _require_xfs_mkfs_validation to common/rc Jan Tulak
2016-07-14 14:21 ` Eryu Guan
2016-07-14 15:16 ` Jan Tulak
2016-07-14 15:57 ` [PATCH v2] " Jan Tulak
2016-07-14 12:43 ` [PATCH 5/6] xfstests: update xfs/096 for new behaviour Jan Tulak
2016-07-14 12:43 ` [PATCH 6/6] xfstests: Add mkfs input validation tests Jan Tulak
2016-07-16 9:33 ` Eryu Guan
2016-07-17 23:30 ` Dave Chinner
2016-07-18 11:29 ` Jan Tulak
2016-07-18 11:29 ` Jan Tulak
2016-07-18 11:47 ` Eryu Guan
2016-07-18 11:47 ` Eryu Guan
2016-07-18 11:54 ` Jan Tulak
2016-07-18 11:54 ` Jan Tulak
2016-07-18 12:33 ` Jan Tulak
2016-07-18 12:33 ` Jan Tulak
2016-07-20 23:54 ` Dave Chinner
2016-07-20 23:54 ` Dave Chinner
2016-07-21 14:24 ` Jan Tulak
2016-07-21 14:24 ` Jan Tulak
2016-07-21 22:40 ` Dave Chinner
2016-07-21 22:40 ` Dave Chinner
2016-07-22 13:08 ` Jan Tulak
2016-07-22 13:08 ` Jan Tulak
2016-07-16 10:28 ` [PATCH 0/6 v3] xfstests: some small fixes and mkfs validation test Eryu Guan
2016-07-18 8:47 ` [PATCH v2] xfstests: Fix installation for extended names Jan Tulak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.