Re: [PATCH v3] common: Check for fiemap range argument support

[Nikolay Borisov <nborisov@suse.com>]

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.
>