generic: test that xattrs can have slashes in their names

PATCH Superseded Darrick J. Wong 3 participants 6 messages Jan 4, 2019 – Jan 7, 2019

lore

Share & tools

lore

Darrick J. Wong

author

2019-01-04 21:23 UTC #0 lore raw

PATCH 1/1 generic: test that xattrs can have slashes in their names

From: Darrick J. Wong <[email protected]>

Eric Sandeen recently found a bug in xfs_repair that flagged extended attribute names containing "/" as corrupt and purged them. There's nothing in the IRIX or Linux manuals that say anything about slashes not being allowed (and Linux certainly allows this) so let's make sure this continues to work.

Signed-off-by Darrick J. Wong <[email protected]>

---
 tests/generic/708     |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/708.out |    6 ++++++
 tests/generic/group   |    1 +
 3 files changed, 55 insertions(+)
 create mode 100755 tests/generic/708
 create mode 100644 tests/generic/708.out

tests/generic/708 new file +48 -0

new file mode 100755
index 00000000..ee63ba79
@@ -0,0 +1,48 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2019 Oracle, Inc.  All Rights Reserved.
+#
+# FS QA Test No. 708
+#
+# Check that xattrs can have slashes in their name.
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+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/attr
+. ./common/filter
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+_require_attrs
+
+rm -f $seqres.full
+
+_scratch_mkfs >> $seqres.full 2>&1
+_scratch_mount
+
+echo "set attr"
+file=$SCRATCH_MNT/moofile
+touch $file
+setfattr -n "user.boo/hoo" -v "woof" $file
+
+echo "check attr"
+getfattr -d --absolute-names $file | _filter_scratch
+
+# success, all done(?)
+status=0
+exit

tests/generic/708.out new file +6 -0

new file mode 100644
index 00000000..6978532e
@@ -0,0 +1,6 @@
+QA output created by 708
+set attr
+check attr
+# file: SCRATCH_MNT/moofile
+user.boo/hoo="woof"
+

tests/generic/group +1 -0

index ea5aa7aa..f88125b4 100644
@@ -525,3 +525,4 @@
 520 auto quick log
 521 soak long_rw
 522 soak long_rw
+708 auto quick attr

Eric Sandeen

2019-01-04 21:27 UTC #1 lore raw

Quoted 23 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>
Eric Sandeen recently found a bug in xfs_repair that flagged extended attribute names containing "/" as corrupt and purged them. There's nothing in the IRIX or Linux manuals that say anything about slashes not being allowed (and Linux certainly allows this) so let's make sure this continues to work.
Signed-off-by Darrick J. Wong <[email protected]>
---
 tests/generic/708     |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/708.out |    6 ++++++
 tests/generic/group   |    1 +
 3 files changed, 55 insertions(+)
 create mode 100755 tests/generic/708
 create mode 100644 tests/generic/708.out
tests/generic/708 new file +0 -0
new file mode 100755
index 00000000..ee63ba79

tests/generic/708 line 7

Quoted 7 lines

tests/generic/708 new file +7 -0

@@ -0,0 +1,48 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2019 Oracle, Inc.  All Rights Reserved.
+#
+# FS QA Test No. 708
+#
+# Check that xattrs can have slashes in their name.

Perhaps a note about what failed before? Because set/get/list has always(tm) worked:

# (xfs_repair previously flagged this as a corruption)

Is it worth also validating that '/' in a dir entry /is/ still flagged? Or maybe that's a separate test.

-Eric

Quoted 63 lines

tests/generic/708 new file +41 -0

+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+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/attr
+. ./common/filter
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+_require_attrs
+
+rm -f $seqres.full
+
+_scratch_mkfs >> $seqres.full 2>&1
+_scratch_mount
+
+echo "set attr"
+file=$SCRATCH_MNT/moofile
+touch $file
+setfattr -n "user.boo/hoo" -v "woof" $file
+
+echo "check attr"
+getfattr -d --absolute-names $file | _filter_scratch
+
+# success, all done(?)
+status=0
+exit

tests/generic/708.out new file +6 -0

new file mode 100644
index 00000000..6978532e
@@ -0,0 +1,6 @@
+QA output created by 708
+set attr
+check attr
+# file: SCRATCH_MNT/moofile
+user.boo/hoo="woof"
+

tests/generic/group +1 -0

index ea5aa7aa..f88125b4 100644
@@ -525,3 +525,4 @@
 520 auto quick log
 521 soak long_rw
 522 soak long_rw
+708 auto quick attr

Darrick J. Wong

author

2019-01-04 22:12 UTC #2 lore raw

Quoted 35 lines · Eric Sandeen

Quoted 23 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
Eric Sandeen recently found a bug in xfs_repair that flagged extended attribute names containing "/" as corrupt and purged them. There's nothing in the IRIX or Linux manuals that say anything about slashes not being allowed (and Linux certainly allows this) so let's make sure this continues to work.
Signed-off-by Darrick J. Wong <[email protected]>
---
 tests/generic/708     |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/708.out |    6 ++++++
 tests/generic/group   |    1 +
 3 files changed, 55 insertions(+)
 create mode 100755 tests/generic/708
 create mode 100644 tests/generic/708.out
tests/generic/708 new file +0 -0
new file mode 100755
index 00000000..ee63ba79
tests/generic/708 line 7
Quoted 7 lines
tests/generic/708 new file +7 -0
@@ -0,0 +1,48 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2019 Oracle, Inc.  All Rights Reserved.
+#
+# FS QA Test No. 708
+#
+# Check that xattrs can have slashes in their name.
Perhaps a note about what failed before? Because set/get/list has always(tm) worked:

Quoted 1 line

# (xfs_repair previously flagged this as a corruption)

Ok.

Quoted 2 lines

Is it worth also validating that '/' in a dir entry /is/ still flagged? Or maybe that's a separate test.

Separate test. The VFS won't let us create a directory entry with a slash in it, which means we have to scribble on the fs with xfs_db to set up the test.

--D

Quoted 65 lines

-Eric

Quoted 63 lines

tests/generic/708 new file +41 -0

+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+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/attr
+. ./common/filter
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+_require_attrs
+
+rm -f $seqres.full
+
+_scratch_mkfs >> $seqres.full 2>&1
+_scratch_mount
+
+echo "set attr"
+file=$SCRATCH_MNT/moofile
+touch $file
+setfattr -n "user.boo/hoo" -v "woof" $file
+
+echo "check attr"
+getfattr -d --absolute-names $file | _filter_scratch
+
+# success, all done(?)
+status=0
+exit

tests/generic/708.out new file +6 -0

new file mode 100644
index 00000000..6978532e
@@ -0,0 +1,6 @@
+QA output created by 708
+set attr
+check attr
+# file: SCRATCH_MNT/moofile
+user.boo/hoo="woof"
+

tests/generic/group +1 -0

index ea5aa7aa..f88125b4 100644
@@ -525,3 +525,4 @@
 520 auto quick log
 521 soak long_rw
 522 soak long_rw
+708 auto quick attr

Eric Sandeen

Reviewed-by

2019-01-04 22:21 UTC #3 lore raw

Quoted 36 lines · Darrick J. Wong

Quoted 35 lines · Eric Sandeen
Quoted 23 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
Eric Sandeen recently found a bug in xfs_repair that flagged extended attribute names containing "/" as corrupt and purged them. There's nothing in the IRIX or Linux manuals that say anything about slashes not being allowed (and Linux certainly allows this) so let's make sure this continues to work.
Signed-off-by Darrick J. Wong <[email protected]>
---
 tests/generic/708     |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/708.out |    6 ++++++
 tests/generic/group   |    1 +
 3 files changed, 55 insertions(+)
 create mode 100755 tests/generic/708
 create mode 100644 tests/generic/708.out
tests/generic/708 new file +0 -0
new file mode 100755
index 00000000..ee63ba79
tests/generic/708 line 7
Quoted 7 lines
tests/generic/708 new file +7 -0
@@ -0,0 +1,48 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2019 Oracle, Inc.  All Rights Reserved.
+#
+# FS QA Test No. 708
+#
+# Check that xattrs can have slashes in their name.
Perhaps a note about what failed before? Because set/get/list has always(tm) worked:

Quoted 3 lines

Quoted 1 line
# (xfs_repair previously flagged this as a corruption)
Ok.

Reviewed-by Eric Sandeen <[email protected]>

(fine by me if that little change goes in on commit, not that important but adds some clarity about what got fixed IMHO)

Darrick J. Wong

author

2019-01-04 23:14 UTC #4 lore raw

PATCH v2 1/1 generic: test that xattrs can have slashes in their names

In reply to Darrick J. Wong

From: Darrick J. Wong <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

Reviewed-by Eric Sandeen <[email protected]>

---
 tests/generic/708     |   50 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/708.out |    6 ++++++
 tests/generic/group   |    1 +
 3 files changed, 57 insertions(+)
 create mode 100755 tests/generic/708
 create mode 100644 tests/generic/708.out

tests/generic/708 new file +50 -0

new file mode 100755
index 00000000..c32f966c
@@ -0,0 +1,50 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2019 Oracle, Inc.  All Rights Reserved.
+#
+# FS QA Test No. 708
+#
+# Check that xattrs can have slashes in their name.
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+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/attr
+. ./common/filter
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+_require_attrs
+
+rm -f $seqres.full
+
+_scratch_mkfs >> $seqres.full 2>&1
+_scratch_mount
+
+echo "set attr"
+file=$SCRATCH_MNT/moofile
+touch $file
+setfattr -n "user.boo/hoo" -v "woof" $file
+
+echo "check attr"
+getfattr -d --absolute-names $file | _filter_scratch
+
+# Now we let the fsck tool check the filesystem, because xfs_repair had a
+# regression where it would flag and erase any xattr with a '/' in it.
+
+status=0
+exit

tests/generic/708.out new file +6 -0

new file mode 100644
index 00000000..6978532e
@@ -0,0 +1,6 @@
+QA output created by 708
+set attr
+check attr
+# file: SCRATCH_MNT/moofile
+user.boo/hoo="woof"
+

tests/generic/group +1 -0

index ea5aa7aa..f88125b4 100644
@@ -525,3 +525,4 @@
 520 auto quick log
 521 soak long_rw
 522 soak long_rw
+708 auto quick attr

Carlos Maiolino

Reviewed-by on v2 1/1

2019-01-07 09:59 UTC #5 lore raw

Quoted 55 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>
Eric Sandeen recently found a bug in xfs_repair that flagged extended attribute names containing "/" as corrupt and purged them. There's nothing in the IRIX or Linux manuals that say anything about slashes not being allowed (and Linux certainly allows this) so let's make sure this continues to work.
Signed-off-by Darrick J. Wong <[email protected]>
Reviewed-by Eric Sandeen <[email protected]>
---
 tests/generic/708     |   50 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/708.out |    6 ++++++
 tests/generic/group   |    1 +
 3 files changed, 57 insertions(+)
 create mode 100755 tests/generic/708
 create mode 100644 tests/generic/708.out
tests/generic/708 new file +31 -0
new file mode 100755
index 00000000..c32f966c
@@ -0,0 +1,50 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2019 Oracle, Inc.  All Rights Reserved.
+#
+# FS QA Test No. 708
+#
+# Check that xattrs can have slashes in their name.
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+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/attr
+. ./common/filter
+
+# real QA test starts here
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+_require_attrs

tests/generic/708 line 41

Quoted 10 lines

tests/generic/708 new file +10 -0

+
+rm -f $seqres.full
+
+_scratch_mkfs >> $seqres.full 2>&1
+_scratch_mount
+
+echo "set attr"
+file=$SCRATCH_MNT/moofile
+touch $file
+setfattr -n "user.boo/hoo" -v "woof" $file

[off-topic] I love your creativity for naming :D

tests/generic/708 line 48

Quoted 7 lines

tests/generic/708 new file +7 -0

+
+echo "check attr"
+getfattr -d --absolute-names $file | _filter_scratch
+
+# Now we let the fsck tool check the filesystem, because xfs_repair had a
+# regression where it would flag and erase any xattr with a '/' in it.
+

I'd prefer this to be in the beginning of the test description instead of 'hidden' in the middle of the code, so, it's easily accessible via 'grep'.

But the test itself looks good, and I don't think this change is worth a V3, so if you want, you could move it to the test description before merging, or ignore my suggestion completely :P
But anyway, you can add:

Reviewed-by Carlos Maiolino <[email protected]>

Cheers

Signature · 1 line

Carlos