| Message ID | 1509610421-16716-1-git-send-email-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
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
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
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" )