Message ID | 20200911133855.38997-2-preichl@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | : generic: add test for boundary in xfs_attr_shortform_verify | expand |
On Fri, Sep 11, 2020 at 03:38:55PM +0200, Pavel Reichl wrote: > Add a regression test to check that the boundary test > for the fixed-offset parts of xfs_attr_sf_entry > in xfs_attr_shortform_verify is not off by one. > > This can be shown by: > > touch file > setfattr -n user.a file > > With help from Zorro. Thanks very much. > > Signed-off-by: Pavel Reichl <preichl@redhat.com> > --- > tests/generic/609 | 64 +++++++++++++++++++++++++++++++++++++++++++ > tests/generic/609.out | 8 ++++++ > tests/generic/group | 1 + > 3 files changed, 73 insertions(+) > create mode 100755 tests/generic/609 > create mode 100644 tests/generic/609.out > > diff --git a/tests/generic/609 b/tests/generic/609 > new file mode 100755 > index 00000000..fd632e86 > --- /dev/null > +++ b/tests/generic/609 > @@ -0,0 +1,64 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2020 Red Hat, Inc. All Rights Reserved. > +# > +# FS QA Test 609 > +# > +# Verify that metadata won't get corrupted when extended attribute > +# name of size one is set. You can specify this case cover which upstream fix(commit). > + > +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 > +. ./common/attr > + > +# remove previous $seqres.full before test > +rm -f $seqres.full > + > +# real QA test starts here > + > +_supported_fs generic > +_supported_os Linux > +_require_scratch > +_require_attrs > + > +TESTFILE="${SCRATCH_MNT}/testfile" > + > +echo "+ create scratch fs" > +_scratch_mkfs > /dev/null 2>&1 > + > +echo "+ mount fs image" I don't think such detailed log output for each of step are needed, but you can keep it if you'd like that :) > +_scratch_mount > + > +echo "+ make test file" > +touch "${TESTFILE}" > + > +echo "+ set file attributes" > +"${SETFATTR_PROG}" -n user.a "${TESTFILE}" > + > +echo 3 > /proc/sys/vm/drop_caches > +sleep 3 I think you'd better to add a comment to explain why these two lines are needed. > + > +echo "+ get file attributes" > +"${GETFATTR_PROG}" --absolute-names -n user.a "${TESTFILE}" | grep "user.a" From my testing, this step already can trigger an error if the bug isn't fixed. But due to this's a test for fs corruption, maybe a manual filesystem checking (_check_scratch_fs) is better at the end? > + > +echo "+ umount fs image" > +umount "${SCRATCH_MNT}" Use _scratch_unmount to umount SCRATCH_DEV > + > +status=0 > +exit > diff --git a/tests/generic/609.out b/tests/generic/609.out > new file mode 100644 > index 00000000..aab84553 > --- /dev/null > +++ b/tests/generic/609.out > @@ -0,0 +1,8 @@ > +QA output created by 609 > ++ create scratch fs > ++ mount fs image > ++ make test file > ++ set file attributes > ++ get file attributes > +user.a="" > ++ umount fs image > diff --git a/tests/generic/group b/tests/generic/group > index aa969bcb..66bfb9dd 100644 > --- a/tests/generic/group > +++ b/tests/generic/group > @@ -611,3 +611,4 @@ > 606 auto attr quick dax > 607 auto attr quick dax > 608 auto attr quick dax > +609 auto attr > -- > 2.26.2 >
On Sat, Sep 12, 2020 at 02:23:46AM +0800, Zorro Lang wrote: > On Fri, Sep 11, 2020 at 03:38:55PM +0200, Pavel Reichl wrote: > > Add a regression test to check that the boundary test > > for the fixed-offset parts of xfs_attr_sf_entry > > in xfs_attr_shortform_verify is not off by one. > > > > This can be shown by: > > > > touch file > > setfattr -n user.a file > > > > With help from Zorro. Thanks very much. > > > > Signed-off-by: Pavel Reichl <preichl@redhat.com> > > --- > > tests/generic/609 | 64 +++++++++++++++++++++++++++++++++++++++++++ > > tests/generic/609.out | 8 ++++++ > > tests/generic/group | 1 + > > 3 files changed, 73 insertions(+) > > create mode 100755 tests/generic/609 > > create mode 100644 tests/generic/609.out > > > > diff --git a/tests/generic/609 b/tests/generic/609 > > new file mode 100755 > > index 00000000..fd632e86 > > --- /dev/null > > +++ b/tests/generic/609 > > @@ -0,0 +1,64 @@ > > +#! /bin/bash > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright (c) 2020 Red Hat, Inc. All Rights Reserved. > > +# > > +# FS QA Test 609 > > +# > > +# Verify that metadata won't get corrupted when extended attribute > > +# name of size one is set. > > You can specify this case cover which upstream fix(commit). > > > + > > +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 > > +. ./common/attr > > + > > +# remove previous $seqres.full before test > > +rm -f $seqres.full > > + > > +# real QA test starts here > > + > > +_supported_fs generic > > +_supported_os Linux > > +_require_scratch > > +_require_attrs > > + > > +TESTFILE="${SCRATCH_MNT}/testfile" > > + > > +echo "+ create scratch fs" > > +_scratch_mkfs > /dev/null 2>&1 > > + > > +echo "+ mount fs image" > > I don't think such detailed log output for each of step are needed, but you can > keep it if you'd like that :) > > > +_scratch_mount > > + > > +echo "+ make test file" > > +touch "${TESTFILE}" > > + > > +echo "+ set file attributes" > > +"${SETFATTR_PROG}" -n user.a "${TESTFILE}" > > + > > +echo 3 > /proc/sys/vm/drop_caches > > +sleep 3 > > I think you'd better to add a comment to explain why these two lines are needed. Does _scratch_cycle_mount trigger it too? > > + > > +echo "+ get file attributes" > > +"${GETFATTR_PROG}" --absolute-names -n user.a "${TESTFILE}" | grep "user.a" > > From my testing, this step already can trigger an error if the bug isn't fixed. > But due to this's a test for fs corruption, maybe a manual filesystem checking > (_check_scratch_fs) is better at the end? > > > + > > +echo "+ umount fs image" > > +umount "${SCRATCH_MNT}" > > Use _scratch_unmount to umount SCRATCH_DEV > > > + > > +status=0 > > +exit > > diff --git a/tests/generic/609.out b/tests/generic/609.out > > new file mode 100644 > > index 00000000..aab84553 > > --- /dev/null > > +++ b/tests/generic/609.out > > @@ -0,0 +1,8 @@ > > +QA output created by 609 > > ++ create scratch fs > > ++ mount fs image > > ++ make test file > > ++ set file attributes > > ++ get file attributes > > +user.a="" > > ++ umount fs image > > diff --git a/tests/generic/group b/tests/generic/group > > index aa969bcb..66bfb9dd 100644 > > --- a/tests/generic/group > > +++ b/tests/generic/group > > @@ -611,3 +611,4 @@ > > 606 auto attr quick dax > > 607 auto attr quick dax > > 608 auto attr quick dax > > +609 auto attr "auto quick attr"? Assuming this test runs in under 30s and doesn't crash the kernel, right? --D > > -- > > 2.26.2 > > >
>>> 608 auto attr quick dax >>> +609 auto attr > > "auto quick attr"? Assuming this test runs in under 30s and doesn't > crash the kernel, right? > Yes, it runs about 3-4 seconds (~the sleep call), it does not crash the kernel. OK, I will add the quick group, I wasn't sure about the time limits. Thanks.
Hi, thanks for the comments. I tried to address them all in version 2 of this patch. Thanks. Pavel
On Fri, Sep 11, 2020 at 12:30:06PM -0700, Darrick J. Wong wrote: > On Sat, Sep 12, 2020 at 02:23:46AM +0800, Zorro Lang wrote: > > On Fri, Sep 11, 2020 at 03:38:55PM +0200, Pavel Reichl wrote: > > > Add a regression test to check that the boundary test > > > for the fixed-offset parts of xfs_attr_sf_entry > > > in xfs_attr_shortform_verify is not off by one. > > > > > > This can be shown by: > > > > > > touch file > > > setfattr -n user.a file > > > > > > With help from Zorro. Thanks very much. > > > > > > Signed-off-by: Pavel Reichl <preichl@redhat.com> > > > --- > > > tests/generic/609 | 64 +++++++++++++++++++++++++++++++++++++++++++ > > > tests/generic/609.out | 8 ++++++ > > > tests/generic/group | 1 + > > > 3 files changed, 73 insertions(+) > > > create mode 100755 tests/generic/609 > > > create mode 100644 tests/generic/609.out > > > > > > diff --git a/tests/generic/609 b/tests/generic/609 > > > new file mode 100755 > > > index 00000000..fd632e86 > > > --- /dev/null > > > +++ b/tests/generic/609 > > > @@ -0,0 +1,64 @@ > > > +#! /bin/bash > > > +# SPDX-License-Identifier: GPL-2.0 > > > +# Copyright (c) 2020 Red Hat, Inc. All Rights Reserved. > > > +# > > > +# FS QA Test 609 > > > +# > > > +# Verify that metadata won't get corrupted when extended attribute > > > +# name of size one is set. > > > > You can specify this case cover which upstream fix(commit). > > > > > + > > > +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 > > > +. ./common/attr > > > + > > > +# remove previous $seqres.full before test > > > +rm -f $seqres.full > > > + > > > +# real QA test starts here > > > + > > > +_supported_fs generic > > > +_supported_os Linux > > > +_require_scratch > > > +_require_attrs > > > + > > > +TESTFILE="${SCRATCH_MNT}/testfile" > > > + > > > +echo "+ create scratch fs" > > > +_scratch_mkfs > /dev/null 2>&1 > > > + > > > +echo "+ mount fs image" > > > > I don't think such detailed log output for each of step are needed, but you can > > keep it if you'd like that :) > > > > > +_scratch_mount > > > + > > > +echo "+ make test file" > > > +touch "${TESTFILE}" > > > + > > > +echo "+ set file attributes" > > > +"${SETFATTR_PROG}" -n user.a "${TESTFILE}" > > > + > > > +echo 3 > /proc/sys/vm/drop_caches > > > +sleep 3 > > > > I think you'd better to add a comment to explain why these two lines are needed. > > Does _scratch_cycle_mount trigger it too? Yes, cycle mount can make sure this change ondisk 100% :) > > > > + > > > +echo "+ get file attributes" > > > +"${GETFATTR_PROG}" --absolute-names -n user.a "${TESTFILE}" | grep "user.a" > > > > From my testing, this step already can trigger an error if the bug isn't fixed. > > But due to this's a test for fs corruption, maybe a manual filesystem checking > > (_check_scratch_fs) is better at the end? > > > > > + > > > +echo "+ umount fs image" > > > +umount "${SCRATCH_MNT}" > > > > Use _scratch_unmount to umount SCRATCH_DEV > > > > > + > > > +status=0 > > > +exit > > > diff --git a/tests/generic/609.out b/tests/generic/609.out > > > new file mode 100644 > > > index 00000000..aab84553 > > > --- /dev/null > > > +++ b/tests/generic/609.out > > > @@ -0,0 +1,8 @@ > > > +QA output created by 609 > > > ++ create scratch fs > > > ++ mount fs image > > > ++ make test file > > > ++ set file attributes > > > ++ get file attributes > > > +user.a="" > > > ++ umount fs image > > > diff --git a/tests/generic/group b/tests/generic/group > > > index aa969bcb..66bfb9dd 100644 > > > --- a/tests/generic/group > > > +++ b/tests/generic/group > > > @@ -611,3 +611,4 @@ > > > 606 auto attr quick dax > > > 607 auto attr quick dax > > > 608 auto attr quick dax > > > +609 auto attr > > "auto quick attr"? Assuming this test runs in under 30s and doesn't > crash the kernel, right? > > --D > > > > -- > > > 2.26.2 > > > > > >
diff --git a/tests/generic/609 b/tests/generic/609 new file mode 100755 index 00000000..fd632e86 --- /dev/null +++ b/tests/generic/609 @@ -0,0 +1,64 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2020 Red Hat, Inc. All Rights Reserved. +# +# FS QA Test 609 +# +# Verify that metadata won't get corrupted when extended attribute +# name of size one is set. + +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 +. ./common/attr + +# remove previous $seqres.full before test +rm -f $seqres.full + +# real QA test starts here + +_supported_fs generic +_supported_os Linux +_require_scratch +_require_attrs + +TESTFILE="${SCRATCH_MNT}/testfile" + +echo "+ create scratch fs" +_scratch_mkfs > /dev/null 2>&1 + +echo "+ mount fs image" +_scratch_mount + +echo "+ make test file" +touch "${TESTFILE}" + +echo "+ set file attributes" +"${SETFATTR_PROG}" -n user.a "${TESTFILE}" + +echo 3 > /proc/sys/vm/drop_caches +sleep 3 + +echo "+ get file attributes" +"${GETFATTR_PROG}" --absolute-names -n user.a "${TESTFILE}" | grep "user.a" + +echo "+ umount fs image" +umount "${SCRATCH_MNT}" + +status=0 +exit diff --git a/tests/generic/609.out b/tests/generic/609.out new file mode 100644 index 00000000..aab84553 --- /dev/null +++ b/tests/generic/609.out @@ -0,0 +1,8 @@ +QA output created by 609 ++ create scratch fs ++ mount fs image ++ make test file ++ set file attributes ++ get file attributes +user.a="" ++ umount fs image diff --git a/tests/generic/group b/tests/generic/group index aa969bcb..66bfb9dd 100644 --- a/tests/generic/group +++ b/tests/generic/group @@ -611,3 +611,4 @@ 606 auto attr quick dax 607 auto attr quick dax 608 auto attr quick dax +609 auto attr
Add a regression test to check that the boundary test for the fixed-offset parts of xfs_attr_sf_entry in xfs_attr_shortform_verify is not off by one. This can be shown by: touch file setfattr -n user.a file With help from Zorro. Thanks very much. Signed-off-by: Pavel Reichl <preichl@redhat.com> --- tests/generic/609 | 64 +++++++++++++++++++++++++++++++++++++++++++ tests/generic/609.out | 8 ++++++ tests/generic/group | 1 + 3 files changed, 73 insertions(+) create mode 100755 tests/generic/609 create mode 100644 tests/generic/609.out