At 01/25/2017 06:24 PM, Eryu Guan wrote:
On Wed, Jan 25, 2017 at 05:30:32PM +0800, Qu Wenruo wrote:
At 01/25/2017 05:20 PM, Eryu Guan wrote:
On Tue, Jan 24, 2017 at 10:50:29PM -0800, Omar Sandoval wrote:
From: Omar Sandoval <osan...@fb.com>
This is a regression test for "Btrfs: disable xattr operations on
subvolume directories". On v4.9, it will result in an aborted
transaction.
Signed-off-by: Omar Sandoval <osan...@fb.com>
Looks good to me overall, tested with 4.10-rc5 kernel with and without
the proposed kernel fix, results are all as expected. Some minor issues
inline.
---
tests/btrfs/047 | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
tests/btrfs/047.out | 2 ++
tests/btrfs/group | 1 +
3 files changed, 72 insertions(+)
create mode 100644 tests/btrfs/047
Better to add new test with mode 755, otherwise git diff shows mode
changes after tests being run.
create mode 100644 tests/btrfs/047.out
diff --git a/tests/btrfs/047 b/tests/btrfs/047
new file mode 100644
index 00000000..c3222f8b
--- /dev/null
+++ b/tests/btrfs/047
@@ -0,0 +1,69 @@
+#! /bin/bash
+# FS QA Test 047
+#
+# Test that we can't set xattrs on read-only subvolume placeholder directories.
+# Regression test for Btrfs: disable xattr operations on subvolume directories.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2017 Facebook. 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
+. ./common/attr
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+_supported_fs btrfs
+_supported_os Linux
+_require_attrs
+_require_scratch
+
+_scratch_mkfs >/dev/null 2>&1
+_scratch_mount
+
+_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/parent"
+_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/parent/child"
+_run_btrfs_util_prog subvolume snapshot "$SCRATCH_MNT/parent"
"$SCRATCH_MNT/snapshot"
Please avoid using "run_check" (called by _run_btrfs_util_prog) if
possible. We can use $BTRFS_UTIL_PROG directly and throw away stdout or
append it to $seqres.full, and stderr from these commands will break the
golden image.
Isn't it designed to use "run_check" to break the test case if it returns
any thing other than 0?
The whole fstests is built upon the golden image matching mechanism,
it's designed to let you don't have to check the return value of
commands. IMO, this run_check should not be introduced in the first
place. I agree that this "golden image matching" has some problems, e.g.
we have to write lots of filters to make the outputs deterministic, it
has both up and down sides, as well as the "check return value" way. But
that's the way fstests has chosen to go since the beginning.
However run_check makes the test exits early, which is quite handy for
stress test.
IMHO any stderr output should be unexpected in this case.
Yes, so any unexpected output breaks & fails the test, and we can see
why it fails from the diff output. And run_check "hides" this info and
makes it harder to parse the result, e.g.
-setfattr: SCRATCH_MNT/snapshot/child: Operation not supported
+failed: '/usr/local/bin/btrfs filesystem sync /mnt/testarea/scratch'
+(see /root/workspace/xfstests/results//btrfs/btrfs/047.full for details)
And without run_check:
-setfattr: SCRATCH_MNT/snapshot/child: Operation not supported
+ERROR: sync ioctl failed on '/mnt/testarea/scratch': No such file or directory
We know why and where it fails.
True, it makes the output more user friendly.
However IMHO it's a problem of current run_check, why not to modify
run_check to redirect stderr to stdout?
Then everyone should be happy.
Thanks,
Qu
Thanks,
Eryu
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
