[PATCH v2 1/2] common/rc: Check 'tPnE' flags on a directory instead of a regilar file

[PATCH v2 1/2] common/rc: Check 'tPnE' flags on a directory instead of a regilar file

[Xiao Yang]

'tPnE' flags are only valid for a directory so check them on a directory.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 common/rc | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index aa5a7409..6487b9f2 100644
--- a/common/rc
+++ b/common/rc
@@ -2163,14 +2163,23 @@ _require_xfs_io_command()
 	local testio
 	case $command in
 	"chattr")
+		local testdir=$TEST_DIR/$$.attr_dir
+		mkdir $TEST_DIR/$$.attr_dir
 		if [ -z "$param" ]; then
 			param=s
 		fi
 		# Test xfs_io chattr support AND
 		# filesystem FS_IOC_FSSETXATTR support
-		testio=`$XFS_IO_PROG -F -f -c "chattr +$param" $testfile 2>&1`
-		$XFS_IO_PROG -F -f -r -c "chattr -$param" $testfile 2>&1
+		# 'tPnE' flags are only valid for a directory so check them on a directory.
+		if echo "$param" | egrep -q 't|P|n|E'; then
+			testio=`$XFS_IO_PROG -F -c "chattr +$param" $testdir 2>&1`
+			$XFS_IO_PROG -F -r -c "chattr -$param" $testdir 2>&1
+		else
+			testio=`$XFS_IO_PROG -F -f -c "chattr +$param" $testfile 2>&1`
+			$XFS_IO_PROG -F -r -c "chattr -$param" $testfile 2>&1
+		fi
 		param_checked="+$param"
+		rm -rf $testdir 2>&1 > /dev/null
 		;;
 	"chproj")
 		testio=`$XFS_IO_PROG -F -f -c "chproj 0" $testfile 2>&1`
-- 
2.21.0

[PATCH v2 2/2] common/rc: Add extra check for xfs_io -c "chattr" on XFS

[Xiao Yang]

On XFS, ioctl(FSSETXATTR)(called by xfs_io -c "chattr") maskes off unsupported
or invalid flags silently.  For example,
1) With kernel v4.4 which doesn't support dax flag, try to set dax flag on a
file by the lastest xfs_io -c "chattr" command:
--------------------------------------------
# xfs_io -f -c "chattr +x" testfile;echo $?
0
# xfs_io -c "lsattr" testfile
----------------X testfile
--------------------------------------------
2) Realtime inheritance flag is only valid for a directory and try to set
realtime inheritance flag on a file:
--------------------------------------------
# xfs_io -f -c "chattr +t" testfile;echo $?
0
# xfs_io -c "lsattr" testfile
----------------X testfile
--------------------------------------------

In this case, we need to check these flags by extra ioctl(FSGETXATTR)(called
by xfs_io -c "lsattr").

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 common/rc | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/common/rc b/common/rc
index 6487b9f2..807a1f6c 100644
--- a/common/rc
+++ b/common/rc
@@ -2158,6 +2158,7 @@ _require_xfs_io_command()
 	local param="$*"
 	local param_checked=""
 	local opts=""
+	local attr_info=""
 
 	local testfile=$TEST_DIR/$$.xfs_io
 	local testio
@@ -2173,9 +2174,11 @@ _require_xfs_io_command()
 		# 'tPnE' flags are only valid for a directory so check them on a directory.
 		if echo "$param" | egrep -q 't|P|n|E'; then
 			testio=`$XFS_IO_PROG -F -c "chattr +$param" $testdir 2>&1`
+			attr_info=`$XFS_IO_PROG -F -r -c "lsattr" $testdir | awk '{print $1}'`
 			$XFS_IO_PROG -F -r -c "chattr -$param" $testdir 2>&1
 		else
 			testio=`$XFS_IO_PROG -F -f -c "chattr +$param" $testfile 2>&1`
+			attr_info=`$XFS_IO_PROG -F -r -c "lsattr" $testfile | awk '{print $1}'`
 			$XFS_IO_PROG -F -r -c "chattr -$param" $testfile 2>&1
 		fi
 		param_checked="+$param"
@@ -2300,6 +2303,19 @@ _require_xfs_io_command()
 		echo $testio | grep -q "\(invalid option\|not supported\)" && \
 			_notrun "xfs_io $command doesn't support $param"
 	fi
+
+	# On XFS, ioctl(FSSETXATTR)(called by xfs_io -c "chattr") maskes off unsupported
+	# or invalid flags silently so need to check these flags by extra ioctl(FSGETXATTR)
+	# (called by xfs_io -c "lsattr").
+	# The following URL explains why we don't change the behavior of XFS.
+	# https://www.spinics.net/lists/linux-xfs/msg44725.html
+	if [ -n "$attr_info" -a "$FSTYP" = "xfs" ]; then
+		local num=${#param}
+		for i in $(seq 0 $((num-1))); do
+			echo $attr_info | grep -q "${param:$i:1}" || \
+				_notrun "xfs_io $command +${param:$i:1} support is missing (unknown flag in kernel)"
+		done
+	fi
 }
 
 # check that kernel and filesystem support direct I/O
-- 
2.21.0

Re: [PATCH v2 1/2] common/rc: Check 'tPnE' flags on a directory instead of a regilar file

[Christoph Hellwig]

On Mon, Sep 14, 2020 at 01:13:59PM +0800, Xiao Yang wrote:
> 'tPnE' flags are only valid for a directory so check them on a directory.
> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>

The change looks good, but I wonder if we should split the chattr
tests into

_require_chattr_file_flag

and

_require_chattr_dir_flag

to make the whole thing a little less convoluted..

Re: [PATCH v2 1/2] common/rc: Check 'tPnE' flags on a directory instead of a regilar file

[Xiao Yang]

On 2020/9/14 15:26, Christoph Hellwig wrote:
> On Mon, Sep 14, 2020 at 01:13:59PM +0800, Xiao Yang wrote:
>> 'tPnE' flags are only valid for a directory so check them on a directory.
>>
>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> The change looks good, but I wonder if we should split the chattr
> tests into
>
> _require_chattr_file_flag
>
> and
>
> _require_chattr_dir_flag
>
> to make the whole thing a little less convoluted..
Hi Christoph,

Sorry for the late reply.

It seems hard to factor out _require_chattr_file_flag() and 
_require_chattr_dir_flag()
because we need to get attribute after running chattr command and then check
attribute after parsing all stderr of chattr command.
Could you provide me some detailed guide? Thanks a lot :-)

Best Regards,
Xiao Yang
>
>
> .
>