[v3] common: Check for fiemap range argument support
diff mbox

Message ID 1509610421-16716-1-git-send-email-nborisov@suse.com
State New
Headers show

Commit Message

Nikolay Borisov Nov. 2, 2017, 8:13 a.m. UTC
From: Nikolay Borsiov <nborisov@suse.com>

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
v3:
 * Changed the way we detect ranged args. Now use a regexp which checks 
 explicitly for the ranged args
 common/rc | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Dave Chinner Nov. 2, 2017, 9:12 p.m. UTC | #1
On Thu, Nov 02, 2017 at 10:13:41AM +0200, Nikolay Borisov wrote:
> From: Nikolay Borsiov <nborisov@suse.com>
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> v3:
>  * Changed the way we detect ranged args. Now use a regexp which checks 
>  explicitly for the ranged args
>  common/rc | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index e2a8229..f7a5fe9 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2053,8 +2053,15 @@ _require_xfs_io_command()
>  			-c "$command 4k 8k" $testfile 2>&1`
>  		;;
>  	"fiemap")
> +		if echo "$param" | egrep -q "[[:digit:]]+[bskmgtpe]? [[:digit:]]+[bskmgtpe]?$"

What is that for?

If you're going to dump crazy complex regexes to match something,
you need comments to explain it in both the commit message and the
code.

And, really, if we need to add complex or tricky code to check a
command exists, we've done somethign wrong. Don't make the "fiemap
range" command by triggered/detected by an optional parameter, add a
specific option instead e.g. "-r" for "range query":

$ xfs_io -c "fiemap -r 0 10k" /mnt/foo

Then you can test it like the falloc command tests it's different
parameters....

-Dave.
Nikolay Borisov Nov. 3, 2017, 2:05 p.m. UTC | #2
On  2.11.2017 23:12, Dave Chinner wrote:
> On Thu, Nov 02, 2017 at 10:13:41AM +0200, Nikolay Borisov wrote:
>> From: Nikolay Borsiov <nborisov@suse.com>
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>> v3:
>>  * Changed the way we detect ranged args. Now use a regexp which checks 
>>  explicitly for the ranged args
>>  common/rc | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/common/rc b/common/rc
>> index e2a8229..f7a5fe9 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -2053,8 +2053,15 @@ _require_xfs_io_command()
>>  			-c "$command 4k 8k" $testfile 2>&1`
>>  		;;
>>  	"fiemap")
>> +		if echo "$param" | egrep -q "[[:digit:]]+[bskmgtpe]? [[:digit:]]+[bskmgtpe]?$"
> 
> What is that for?
> 
> If you're going to dump crazy complex regexes to match something,
> you need comments to explain it in both the commit message and the
> code.
> 
> And, really, if we need to add complex or tricky code to check a
> command exists, we've done somethign wrong. Don't make the "fiemap
> range" command by triggered/detected by an optional parameter, add a
> specific option instead e.g. "-r" for "range query":
> 
> $ xfs_io -c "fiemap -r 0 10k" /mnt/foo

Do you mean having the range params as arguments to the -r option (which
would be a bitch to parse since getopt doesn't support multiple args to
an options). Or having the -r serve as a boolean indicating we'd need to
parse the final 2 args (as it's done currently)?

> 
> Then you can test it like the falloc command tests it's different
> parameters....
> 
> -Dave.
> 
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong Nov. 3, 2017, 4:28 p.m. UTC | #3
On Fri, Nov 03, 2017 at 04:05:37PM +0200, Nikolay Borisov wrote:
> 
> 
> On  2.11.2017 23:12, Dave Chinner wrote:
> > On Thu, Nov 02, 2017 at 10:13:41AM +0200, Nikolay Borisov wrote:
> >> From: Nikolay Borsiov <nborisov@suse.com>
> >>
> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> >> ---
> >> v3:
> >>  * Changed the way we detect ranged args. Now use a regexp which checks 
> >>  explicitly for the ranged args
> >>  common/rc | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/common/rc b/common/rc
> >> index e2a8229..f7a5fe9 100644
> >> --- a/common/rc
> >> +++ b/common/rc
> >> @@ -2053,8 +2053,15 @@ _require_xfs_io_command()
> >>  			-c "$command 4k 8k" $testfile 2>&1`
> >>  		;;
> >>  	"fiemap")
> >> +		if echo "$param" | egrep -q "[[:digit:]]+[bskmgtpe]? [[:digit:]]+[bskmgtpe]?$"
> > 
> > What is that for?
> > 
> > If you're going to dump crazy complex regexes to match something,
> > you need comments to explain it in both the commit message and the
> > code.
> > 
> > And, really, if we need to add complex or tricky code to check a
> > command exists, we've done somethign wrong. Don't make the "fiemap
> > range" command by triggered/detected by an optional parameter, add a
> > specific option instead e.g. "-r" for "range query":
> > 
> > $ xfs_io -c "fiemap -r 0 10k" /mnt/foo
> 
> Do you mean having the range params as arguments to the -r option (which
> would be a bitch to parse since getopt doesn't support multiple args to
> an options). Or having the -r serve as a boolean indicating we'd need to
> parse the final 2 args (as it's done currently)?

That wouldn't be a bad way to proceed -- -r tells fiemap to look for two
range arguments; lack of -r tells it to expect zero arguments.  Apparently
we don't really validate the freeform arguments...

$ xfs_io -c 'fiemap mugga wugga fribba gribba ding dong' VERSION
VERSION:
        0: [0..7]: 287903056..287903063

Heh.

--D

> 
> > 
> > Then you can test it like the falloc command tests it's different
> > parameters....
> > 
> > -Dave.
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Sandeen Nov. 14, 2017, 3:09 p.m. UTC | #4
On 11/2/17 3:13 AM, Nikolay Borisov wrote:
> From: Nikolay Borsiov <nborisov@suse.com>
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> v3:
>  * Changed the way we detect ranged args. Now use a regexp which checks 
>  explicitly for the ranged args
>  common/rc | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index e2a8229..f7a5fe9 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2053,8 +2053,15 @@ _require_xfs_io_command()
>  			-c "$command 4k 8k" $testfile 2>&1`
>  		;;
>  	"fiemap")
> +		if echo "$param" | egrep -q "[[:digit:]]+[bskmgtpe]? [[:digit:]]+[bskmgtpe]?$"
> +		then
> +			$XFS_IO_PROG -c "help fiemap" | head -n 1 | grep -q "[offset [len]]" || \
> +				_notrun "xfs_io $command range param support is missing"
> +		fi
> +

What if, rather than difficult to read regexps, we actually checked the functionality?
The check already writes a testfile; you could try fiemapping past EOF and see what
you get, i.e. for a file in a single 4k block:

$ io/xfs_io -c "fiemap" short
short:
	0: [0..7]: 2788960..2788967

Mapping past EOF will give you no extents (if the range values are honored by the
command):

$ io/xfs_io -c "fiemap 4k 4k" short
short:

>  		testio=`$XFS_IO_PROG -F -f -c "pwrite 0 20k" -c "fsync" \
>  			-c "fiemap -v $param" $testfile 2>&1`

so I think you could switch based on [ -n $param ] about whether we were asked
to check a range and if so, map past EOF, and return success or fail based
on what you get back.  The only slight weirdness is that you're asking to check
a specific range (_require_xfs_io_command "fiemap" "0 4k") but possibly testing
a different one ("20k 4k") but not sure if that matters.

It'd be a bit of a departure but with enough comments, maybe ok to do it as:

_require_xfs_io_command "fiemap" "ranged"

and use the "ranged" param to DTRT in the test.

-Eric

> +
>  		param_checked=1
>  		;;
>  	"flink" )
> 
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eryu Guan Nov. 15, 2017, 7:41 a.m. UTC | #5
On Tue, Nov 14, 2017 at 09:09:21AM -0600, Eric Sandeen wrote:
> 
> 
> On 11/2/17 3:13 AM, Nikolay Borisov wrote:
> > From: Nikolay Borsiov <nborisov@suse.com>
> > 
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > ---
> > v3:
> >  * Changed the way we detect ranged args. Now use a regexp which checks 
> >  explicitly for the ranged args
> >  common/rc | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/common/rc b/common/rc
> > index e2a8229..f7a5fe9 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -2053,8 +2053,15 @@ _require_xfs_io_command()
> >  			-c "$command 4k 8k" $testfile 2>&1`
> >  		;;
> >  	"fiemap")
> > +		if echo "$param" | egrep -q "[[:digit:]]+[bskmgtpe]? [[:digit:]]+[bskmgtpe]?$"
> > +		then
> > +			$XFS_IO_PROG -c "help fiemap" | head -n 1 | grep -q "[offset [len]]" || \
> > +				_notrun "xfs_io $command range param support is missing"
> > +		fi
> > +
> 
> What if, rather than difficult to read regexps, we actually checked the functionality?
> The check already writes a testfile; you could try fiemapping past EOF and see what
> you get, i.e. for a file in a single 4k block:
> 
> $ io/xfs_io -c "fiemap" short
> short:
> 	0: [0..7]: 2788960..2788967
> 
> Mapping past EOF will give you no extents (if the range values are honored by the
> command):
> 
> $ io/xfs_io -c "fiemap 4k 4k" short
> short:

Yeah, I'm OK with it, looks better than the complicated regexp and could
keep the fiemap command interface simpler.

> 
> >  		testio=`$XFS_IO_PROG -F -f -c "pwrite 0 20k" -c "fsync" \
> >  			-c "fiemap -v $param" $testfile 2>&1`
> 
> so I think you could switch based on [ -n $param ] about whether we were asked
> to check a range and if so, map past EOF, and return success or fail based
> on what you get back.  The only slight weirdness is that you're asking to check
> a specific range (_require_xfs_io_command "fiemap" "0 4k") but possibly testing
> a different one ("20k 4k") but not sure if that matters.
> 
> It'd be a bit of a departure but with enough comments, maybe ok to do it as:
> 
> _require_xfs_io_command "fiemap" "ranged"
> 
> and use the "ranged" param to DTRT in the test.

This looks fine too to me.

Thanks,
Eryu

> 
> -Eric
> 
> > +
> >  		param_checked=1
> >  		;;
> >  	"flink" )
> > 
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/common/rc b/common/rc
index e2a8229..f7a5fe9 100644
--- a/common/rc
+++ b/common/rc
@@ -2053,8 +2053,15 @@  _require_xfs_io_command()
 			-c "$command 4k 8k" $testfile 2>&1`
 		;;
 	"fiemap")
+		if echo "$param" | egrep -q "[[:digit:]]+[bskmgtpe]? [[:digit:]]+[bskmgtpe]?$"
+		then
+			$XFS_IO_PROG -c "help fiemap" | head -n 1 | grep -q "[offset [len]]" || \
+				_notrun "xfs_io $command range param support is missing"
+		fi
+
 		testio=`$XFS_IO_PROG -F -f -c "pwrite 0 20k" -c "fsync" \
 			-c "fiemap -v $param" $testfile 2>&1`
+
 		param_checked=1
 		;;
 	"flink" )