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

["Darrick J. Wong" <darrick.wong@oracle.com>]

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