[PATCH] common/rc: Check more options for filefrag command

[PATCH] common/rc: Check more options for filefrag command

[Xiao Yang]

In generic/519, filefrag command use FIBMAP ioctl(-B option) to print
output in extent format(-e option) on purpose and sync file(-s option),
so check if the command supports all of these options by _require_filefrag_options().

Rename _require_fibmap() to _require_filefrag_options()
Also remove duplicate _require_command "$FILEFRAG_PROG" filefrag

References:
1) filefrag supports -e option by commit 2508eaa since v1.42.7.
2) filefrag supports -B option by commit 5d5e01d since v1.41.9.
3) filefrag supports -s option by commit e62847c since v1.41.6.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 common/rc         | 12 +++++++-----
 tests/generic/519 |  3 +--
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/common/rc b/common/rc
index ecb1738..5a11359 100644
--- a/common/rc
+++ b/common/rc
@@ -3843,17 +3843,19 @@ _require_scratch_btime()
 	_scratch_unmount
 }
 
-_require_fibmap()
+_require_filefrag_options()
 {
 	_require_command "$FILEFRAG_PROG" filefrag
 
-	local file="$TEST_DIR/fibmap_testfile"
+	local options=$1
+	local file="$TEST_DIR/testfile"
 
 	echo "XX" > $file
-	${FILEFRAG_PROG} -B $file 2>&1 | grep -q "FIBMAP[[:space:]]*unsupported"
-	if [ $? -eq 0 ]; then
+	local output=`${FILEFRAG_PROG} $options $file 2>&1`
+	echo $output | grep -q "FIBMAP[[:space:]]*unsupported" && \
 		_notrun "FIBMAP not supported by this filesystem"
-	fi
+	echo $output | grep -q "invalid option" && \
+		_notrun "filefrag doesn't support $options option"
 	rm -f $file
 }
 
diff --git a/tests/generic/519 b/tests/generic/519
index 229c5b4..ccc15d4 100755
--- a/tests/generic/519
+++ b/tests/generic/519
@@ -33,8 +33,7 @@ rm -f $seqres.full
 _supported_fs generic
 _supported_os Linux
 _require_scratch
-_require_command "$FILEFRAG_PROG" filefrag
-_require_fibmap
+_require_filefrag_options "-Bes"
 
 testfile="$SCRATCH_MNT/$seq-testfile"
 
-- 
1.8.3.1

Re: [PATCH] common/rc: Check more options for filefrag command

[Eryu Guan]

On Tue, Nov 20, 2018 at 04:41:08PM +0800, Xiao Yang wrote:
> In generic/519, filefrag command use FIBMAP ioctl(-B option) to print
> output in extent format(-e option) on purpose and sync file(-s option),
> so check if the command supports all of these options by _require_filefrag_options().
> 
> Rename _require_fibmap() to _require_filefrag_options()
> Also remove duplicate _require_command "$FILEFRAG_PROG" filefrag
> 
> References:
> 1) filefrag supports -e option by commit 2508eaa since v1.42.7.
> 2) filefrag supports -B option by commit 5d5e01d since v1.41.9.
> 3) filefrag supports -s option by commit e62847c since v1.41.6.
> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
>  common/rc         | 12 +++++++-----
>  tests/generic/519 |  3 +--
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index ecb1738..5a11359 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3843,17 +3843,19 @@ _require_scratch_btime()
>  	_scratch_unmount
>  }
>  
> -_require_fibmap()
> +_require_filefrag_options()
>  {
>  	_require_command "$FILEFRAG_PROG" filefrag
>  
> -	local file="$TEST_DIR/fibmap_testfile"
> +	local options=$1
> +	local file="$TEST_DIR/testfile"
>  
>  	echo "XX" > $file
> -	${FILEFRAG_PROG} -B $file 2>&1 | grep -q "FIBMAP[[:space:]]*unsupported"
> -	if [ $? -eq 0 ]; then
> +	local output=`${FILEFRAG_PROG} $options $file 2>&1`
> +	echo $output | grep -q "FIBMAP[[:space:]]*unsupported" && \
>  		_notrun "FIBMAP not supported by this filesystem"

I think we'd better to split the FIBMAP check from the other options
check. e.g. keep the _require_fibmap() function, but require needed
options first there.

_require_fibmap()
{
	_require_filefrag_options "B"
	<other FIBMAP checks>
}

> -	fi
> +	echo $output | grep -q "invalid option" && \
> +		_notrun "filefrag doesn't support $options option"
>  	rm -f $file
>  }
>  
> diff --git a/tests/generic/519 b/tests/generic/519
> index 229c5b4..ccc15d4 100755
> --- a/tests/generic/519
> +++ b/tests/generic/519
> @@ -33,8 +33,7 @@ rm -f $seqres.full
>  _supported_fs generic
>  _supported_os Linux
>  _require_scratch
> -_require_command "$FILEFRAG_PROG" filefrag
> -_require_fibmap
> +_require_filefrag_options "-Bes"

And we require fibmap and other needed options in the test, e.g.

_require_fibmap
_require_filefrag_options "es"

Note that no "-" in the required options.

Thanks,
Eryu

>  
>  testfile="$SCRATCH_MNT/$seq-testfile"
>  
> -- 
> 1.8.3.1
> 
> 
> 

Re: [PATCH] common/rc: Check more options for filefrag command

[Xiao Yang]

Hi Eryu,

Thanks for your comment.
Your idea seems better to me, and i will send a v2 patch.  :-)

Best Regards,
Xiao Yang
On 2018/11/23 13:12, Eryu Guan wrote:

> On Tue, Nov 20, 2018 at 04:41:08PM +0800, Xiao Yang wrote:
>> In generic/519, filefrag command use FIBMAP ioctl(-B option) to print
>> output in extent format(-e option) on purpose and sync file(-s option),
>> so check if the command supports all of these options by _require_filefrag_options().
>>
>> Rename _require_fibmap() to _require_filefrag_options()
>> Also remove duplicate _require_command "$FILEFRAG_PROG" filefrag
>>
>> References:
>> 1) filefrag supports -e option by commit 2508eaa since v1.42.7.
>> 2) filefrag supports -B option by commit 5d5e01d since v1.41.9.
>> 3) filefrag supports -s option by commit e62847c since v1.41.6.
>>
>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>> ---
>>   common/rc         | 12 +++++++-----
>>   tests/generic/519 |  3 +--
>>   2 files changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index ecb1738..5a11359 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -3843,17 +3843,19 @@ _require_scratch_btime()
>>   	_scratch_unmount
>>   }
>>
>> -_require_fibmap()
>> +_require_filefrag_options()
>>   {
>>   	_require_command "$FILEFRAG_PROG" filefrag
>>
>> -	local file="$TEST_DIR/fibmap_testfile"
>> +	local options=$1
>> +	local file="$TEST_DIR/testfile"
>>
>>   	echo "XX">  $file
>> -	${FILEFRAG_PROG} -B $file 2>&1 | grep -q "FIBMAP[[:space:]]*unsupported"
>> -	if [ $? -eq 0 ]; then
>> +	local output=`${FILEFRAG_PROG} $options $file 2>&1`
>> +	echo $output | grep -q "FIBMAP[[:space:]]*unsupported"&&  \
>>   		_notrun "FIBMAP not supported by this filesystem"
> I think we'd better to split the FIBMAP check from the other options
> check. e.g. keep the _require_fibmap() function, but require needed
> options first there.
>
> _require_fibmap()
> {
> 	_require_filefrag_options "B"
> 	<other FIBMAP checks>
> }
>
>> -	fi
>> +	echo $output | grep -q "invalid option"&&  \
>> +		_notrun "filefrag doesn't support $options option"
>>   	rm -f $file
>>   }
>>
>> diff --git a/tests/generic/519 b/tests/generic/519
>> index 229c5b4..ccc15d4 100755
>> --- a/tests/generic/519
>> +++ b/tests/generic/519
>> @@ -33,8 +33,7 @@ rm -f $seqres.full
>>   _supported_fs generic
>>   _supported_os Linux
>>   _require_scratch
>> -_require_command "$FILEFRAG_PROG" filefrag
>> -_require_fibmap
>> +_require_filefrag_options "-Bes"
> And we require fibmap and other needed options in the test, e.g.
>
> _require_fibmap
> _require_filefrag_options "es"
>
> Note that no "-" in the required options.
>
> Thanks,
> Eryu
>
>>
>>   testfile="$SCRATCH_MNT/$seq-testfile"
>>
>> -- 
>> 1.8.3.1
>>
>>
>>
>
>

[PATCH v2] common/rc: Add _require_filefrag_options() to check options for filefrag

[Xiao Yang]

In generic/519, filefrag command use FIBMAP ioctl(-B option) to print
output in extent format(-e option) on purpose and sync file(-s option),
so add _require_filefrag_options() to check if the command supports
all of these options.

References:
1) filefrag supports -e option by commit 2508eaa since e2fsprogs v1.42.7.
2) filefrag supports -B option by commit 5d5e01d since e2fsprogs v1.41.9.
3) filefrag supports -s option by commit e62847c since e2fsprogs v1.41.6.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 common/rc         | 15 ++++++++++++++-
 tests/generic/519 |  2 +-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index ecb1738..e5da648 100644
--- a/common/rc
+++ b/common/rc
@@ -3843,10 +3843,23 @@ _require_scratch_btime()
 	_scratch_unmount
 }
 
-_require_fibmap()
+_require_filefrag_options()
 {
 	_require_command "$FILEFRAG_PROG" filefrag
 
+	local options=$1
+	local file="$TEST_DIR/options_testfile"
+
+	echo "XX" > $file
+	${FILEFRAG_PROG} -$options $file 2>&1 | grep -q "invalid option" && \
+		_notrun "filefrag doesn't support $options option"
+	rm -f $file
+}
+
+_require_fibmap()
+{
+	_require_filefrag_options "B"
+
 	local file="$TEST_DIR/fibmap_testfile"
 
 	echo "XX" > $file
diff --git a/tests/generic/519 b/tests/generic/519
index 229c5b4..d1a3c17 100755
--- a/tests/generic/519
+++ b/tests/generic/519
@@ -33,8 +33,8 @@ rm -f $seqres.full
 _supported_fs generic
 _supported_os Linux
 _require_scratch
-_require_command "$FILEFRAG_PROG" filefrag
 _require_fibmap
+_require_filefrag_options "es"
 
 testfile="$SCRATCH_MNT/$seq-testfile"
 
-- 
1.8.3.1

Re: [PATCH v2] common/rc: Add _require_filefrag_options() to check options for filefrag

[Dave Chinner]

On Fri, Nov 23, 2018 at 01:24:23PM +0800, Xiao Yang wrote:
> In generic/519, filefrag command use FIBMAP ioctl(-B option) to print
> output in extent format(-e option) on purpose and sync file(-s option),
> so add _require_filefrag_options() to check if the command supports
> all of these options.
> 
> References:
> 1) filefrag supports -e option by commit 2508eaa since e2fsprogs v1.42.7.
> 2) filefrag supports -B option by commit 5d5e01d since e2fsprogs v1.41.9.
> 3) filefrag supports -s option by commit e62847c since e2fsprogs v1.41.6.
> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
>  common/rc         | 15 ++++++++++++++-
>  tests/generic/519 |  2 +-
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index ecb1738..e5da648 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3843,10 +3843,23 @@ _require_scratch_btime()
>  	_scratch_unmount
>  }
>  
> -_require_fibmap()
> +_require_filefrag_options()
>  {
>  	_require_command "$FILEFRAG_PROG" filefrag
>  
> +	local options=$1
> +	local file="$TEST_DIR/options_testfile"
> +
> +	echo "XX" > $file
> +	${FILEFRAG_PROG} -$options $file 2>&1 | grep -q "invalid option" && \
> +		_notrun "filefrag doesn't support $options option"
> +	rm -f $file
> +}

Can we just get rid of filefrag and use xfs_io's fiemap commands
instead?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] common/rc: Add _require_filefrag_options() to check options for filefrag

[Xiao Yang]

On 2018/11/27 5:50, Dave Chinner wrote:
> On Fri, Nov 23, 2018 at 01:24:23PM +0800, Xiao Yang wrote:
>> In generic/519, filefrag command use FIBMAP ioctl(-B option) to print
>> output in extent format(-e option) on purpose and sync file(-s option),
>> so add _require_filefrag_options() to check if the command supports
>> all of these options.
>>
>> References:
>> 1) filefrag supports -e option by commit 2508eaa since e2fsprogs v1.42.7.
>> 2) filefrag supports -B option by commit 5d5e01d since e2fsprogs v1.41.9.
>> 3) filefrag supports -s option by commit e62847c since e2fsprogs v1.41.6.
>>
>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>> ---
>>   common/rc         | 15 ++++++++++++++-
>>   tests/generic/519 |  2 +-
>>   2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index ecb1738..e5da648 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -3843,10 +3843,23 @@ _require_scratch_btime()
>>   	_scratch_unmount
>>   }
>>
>> -_require_fibmap()
>> +_require_filefrag_options()
>>   {
>>   	_require_command "$FILEFRAG_PROG" filefrag
>>
>> +	local options=$1
>> +	local file="$TEST_DIR/options_testfile"
>> +
>> +	echo "XX">  $file
>> +	${FILEFRAG_PROG} -$options $file 2>&1 | grep -q "invalid option"&&  \
>> +		_notrun "filefrag doesn't support $options option"
>> +	rm -f $file
>> +}
> Can we just get rid of filefrag and use xfs_io's fiemap commands
> instead?
Hi Dave,

According to the comment in test,  we have to reproduce the bug by 
specifying FIBMAP ioctl instead of FIEMAP ioctl.
It seems that we can just use FIEMAP ioctl by xfs_io's fiemap command, 
so we use FIBMAP ioctl  forcely by filefrag
with -B option.

Best Regards,
Xiao Yang
> Cheers,
>
> Dave.

Re: [PATCH v2] common/rc: Add _require_filefrag_options() to check options for filefrag

[Dave Chinner]

On Tue, Nov 27, 2018 at 10:47:58AM +0800, Xiao Yang wrote:
> On 2018/11/27 5:50, Dave Chinner wrote:
> >On Fri, Nov 23, 2018 at 01:24:23PM +0800, Xiao Yang wrote:
> >>In generic/519, filefrag command use FIBMAP ioctl(-B option) to print
> >>output in extent format(-e option) on purpose and sync file(-s option),
> >>so add _require_filefrag_options() to check if the command supports
> >>all of these options.
> >>
> >>References:
> >>1) filefrag supports -e option by commit 2508eaa since e2fsprogs v1.42.7.
> >>2) filefrag supports -B option by commit 5d5e01d since e2fsprogs v1.41.9.
> >>3) filefrag supports -s option by commit e62847c since e2fsprogs v1.41.6.
> >>
> >>Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> >>---
> >>  common/rc         | 15 ++++++++++++++-
> >>  tests/generic/519 |  2 +-
> >>  2 files changed, 15 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/common/rc b/common/rc
> >>index ecb1738..e5da648 100644
> >>--- a/common/rc
> >>+++ b/common/rc
> >>@@ -3843,10 +3843,23 @@ _require_scratch_btime()
> >>  	_scratch_unmount
> >>  }
> >>
> >>-_require_fibmap()
> >>+_require_filefrag_options()
> >>  {
> >>  	_require_command "$FILEFRAG_PROG" filefrag
> >>
> >>+	local options=$1
> >>+	local file="$TEST_DIR/options_testfile"
> >>+
> >>+	echo "XX">  $file
> >>+	${FILEFRAG_PROG} -$options $file 2>&1 | grep -q "invalid option"&&  \
> >>+		_notrun "filefrag doesn't support $options option"
> >>+	rm -f $file
> >>+}
> >Can we just get rid of filefrag and use xfs_io's fiemap commands
> >instead?
> Hi Dave,
> 
> According to the comment in test,  we have to reproduce the bug by
> specifying FIBMAP ioctl instead of FIEMAP ioctl.
> It seems that we can just use FIEMAP ioctl by xfs_io's fiemap
> command, so we use FIBMAP ioctl  forcely by filefrag
> with -B option.

Add an fibmap command to xfs_io so we are not dependent on filefrag
for testing fibmap.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com