[v2] Fix caller's argument for _require_command()

Commit Message

Dave Chinner April 14, 2015, 9:45 p.m. UTC

On Mon, Apr 13, 2015 at 12:32:43PM +0800, Zhaolei wrote:
> From: Zhao Lei <[e-mail]>
> 
> _require_command() only accept 2 arguments, first one is pure command,
> and second one is name for error message.
> 
> But some caller misused this function, for example,
>   DEFRAG_PROG="$BTRFS_UTIL_PROG filesystem defragment"
>   _require_command $DEFRAG_PROG defragment
> In above code, _require_command get 4 arguments, the caller's
> second argument is never used, and the second field of first
> argument is treat as "error message" in _require_command.
> 
> We fixed above case by adding quotes to _require_command()'s
> arguments, it fixed most test case, but introduced a new bug,
> in above case, "btrfs filesystem defragment" is not a command,
> and always failed in _require_command(), it caused some testcase
> not work, as btrfs/005.
> 
> This patch fixed above caller.
> 
> Changelog v1->v2:
> 1: Add detail description, suggested by:
>    Lukáš Czerner <[e-mail]>
> 2: Add missed Reported-by.
> 3: Make $XFSDUMP_PROG always be a pure command, to avoid special
>    handling for it.

Having to change xfsdump checks means this is still the wrong fix.

_require_command should simply handle multi-part command strings.

Does the following patch fix your problems?

-Dave.

Comments

Zhaolei April 15, 2015, 12:28 p.m. UTC | #1

Hi, Dave Chinner

> -----Original Message-----
> From: Dave Chinner [mailto:[e-mail]]
> Sent: Wednesday, April 15, 2015 5:45 AM
> To: Zhaolei
> Cc: [e-mail]
> Subject: Re: [PATCH v2] Fix caller's argument for _require_command()
> 
> On Mon, Apr 13, 2015 at 12:32:43PM +0800, Zhaolei wrote:
> > From: Zhao Lei <[e-mail]>
> >
> > _require_command() only accept 2 arguments, first one is pure command,
> > and second one is name for error message.
> >
> > But some caller misused this function, for example,
> >   DEFRAG_PROG="$BTRFS_UTIL_PROG filesystem defragment"
> >   _require_command $DEFRAG_PROG defragment In above code,
> > _require_command get 4 arguments, the caller's second argument is
> > never used, and the second field of first argument is treat as "error
> > message" in _require_command.
> >
> > We fixed above case by adding quotes to _require_command()'s
> > arguments, it fixed most test case, but introduced a new bug, in above
> > case, "btrfs filesystem defragment" is not a command, and always
> > failed in _require_command(), it caused some testcase not work, as
> > btrfs/005.
> >
> > This patch fixed above caller.
> >
> > Changelog v1->v2:
> > 1: Add detail description, suggested by:
> >    Lukáš Czerner <[e-mail]>
> > 2: Add missed Reported-by.
> > 3: Make $XFSDUMP_PROG always be a pure command, to avoid special
> >    handling for it.
> 
> Having to change xfsdump checks means this is still the wrong fix.
> 
> _require_command should simply handle multi-part command strings.
> 
> Does the following patch fix your problems?
> 
This patch can deal with current code, only can't deal with program with blank in filename.
But this is rarely happened, so we need not care about it.
 
Thanks
Zhaolei


> -Dave.
> --
> Dave Chinner
> [e-mail]
> 
> common: _require_command needs to handle parameters
> 
> From: Dave Chinner <[e-mail]>
> 
> _require_command fails when a parameter based command is passed to it,
> such as "xfs_io -F" or "btrfs filesystem defrag" as the command string does not
> point at a binary.  Rather than hacking at all the callers and limiting what we
> can do with $*_PROGS variables, just make _require_command handle this
> case sanely.
> 
> Change _require_command to check for one or two variables passed to it and
> to fail if none or more than 2 parameters are passed. This will catch most cases
> where unquoted parameter-based commands are passed. Further, for the
> command variable, the executable we need to check for is always going to be
> the first token in the variable.
> Hence we can simply ignore everything after the first token for the purposes of
> existence and executable checks on the command.
> 
> Signed-off-by: Dave Chinner <[e-mail]>
> ---
>  common/rc | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index ca8da7f..6ea107e 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1286,10 +1286,22 @@ _require_realtime()  # this test requires that a
> specified command (executable) exists  # $1 - command, $2 - name for error
> message  #
> +# Note: the command string might have parameters, so strip them before
> +checking # whether it is executable.
>  _require_command()
>  {
> -    [ -n "$1" ] && _cmd="$1" || _cmd="$2"
> -    [ -n "$1" -a -x "$1" ] || _notrun "$_cmd utility required, skipped this test"
> +	if [ $# -eq 2 ]; then
> +		_name="$2"
> +	elif [ $# -eq 1 ]; then
> +		_name="$1"
> +	else
> +		_fail "usage: _require_command <command> [<name>]"
> +	fi
> +
> +	_command=`echo "$1" | awk '{ print $1 }'`
> +	if [ ! -x $command ]; then
> +		_notrun "$_name utility required, skipped this test"
> +	fi
>  }
> 
>  # this test requires the device to be valid block device


--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Chinner April 15, 2015, 1 p.m. UTC | #2

On Wed, Apr 15, 2015 at 08:28:21PM +0800, Zhao Lei wrote:
> Hi, Dave Chinner
> 
> > -----Original Message-----
> > From: Dave Chinner [mailto:[e-mail]]
> > Sent: Wednesday, April 15, 2015 5:45 AM
> > To: Zhaolei
> > Cc: [e-mail]
> > Subject: Re: [PATCH v2] Fix caller's argument for _require_command()
> > 
> > On Mon, Apr 13, 2015 at 12:32:43PM +0800, Zhaolei wrote:
> > > From: Zhao Lei <[e-mail]>
> > >
> > > _require_command() only accept 2 arguments, first one is pure command,
> > > and second one is name for error message.
> > >
> > > But some caller misused this function, for example,
> > >   DEFRAG_PROG="$BTRFS_UTIL_PROG filesystem defragment"
> > >   _require_command $DEFRAG_PROG defragment In above code,
> > > _require_command get 4 arguments, the caller's second argument is
> > > never used, and the second field of first argument is treat as "error
> > > message" in _require_command.
> > >
> > > We fixed above case by adding quotes to _require_command()'s
> > > arguments, it fixed most test case, but introduced a new bug, in above
> > > case, "btrfs filesystem defragment" is not a command, and always
> > > failed in _require_command(), it caused some testcase not work, as
> > > btrfs/005.
> > >
> > > This patch fixed above caller.
> > >
> > > Changelog v1->v2:
> > > 1: Add detail description, suggested by:
> > >    Lukáš Czerner <[e-mail]>
> > > 2: Add missed Reported-by.
> > > 3: Make $XFSDUMP_PROG always be a pure command, to avoid special
> > >    handling for it.
> > 
> > Having to change xfsdump checks means this is still the wrong fix.
> > 
> > _require_command should simply handle multi-part command strings.
> > 
> > Does the following patch fix your problems?
> > 
> This patch can deal with current code, only can't deal with program with blank in filename.
> But this is rarely happened, so we need not care about it.

It will work just fine with a minor tweak:

$ [ -x /bin/true ] && echo foo
foo
$ [ -x "" ] && echo foo
$

i.e. the -x check fails as expected when passed an empty command.

> > +	if [ ! -x $command ]; then

i.e this needs changing to [ ! -x "$command" ]....

Cheers,

Dave.

Zhaolei April 15, 2015, 1:15 p.m. UTC | #3

Hi Dave Chinner

> -----Original Message-----
> From: Dave Chinner [mailto:[e-mail]]
> Sent: Wednesday, April 15, 2015 9:00 PM
> To: Zhao Lei
> Cc: [e-mail]
> Subject: Re: [PATCH v2] Fix caller's argument for _require_command()
> 
> On Wed, Apr 15, 2015 at 08:28:21PM +0800, Zhao Lei wrote:
> > Hi, Dave Chinner
> >
> > > -----Original Message-----
> > > From: Dave Chinner [mailto:[e-mail]]
> > > Sent: Wednesday, April 15, 2015 5:45 AM
> > > To: Zhaolei
> > > Cc: [e-mail]
> > > Subject: Re: [PATCH v2] Fix caller's argument for _require_command()
> > >
> > > On Mon, Apr 13, 2015 at 12:32:43PM +0800, Zhaolei wrote:
> > > > From: Zhao Lei <[e-mail]>
> > > >
> > > > _require_command() only accept 2 arguments, first one is pure
> > > > command, and second one is name for error message.
> > > >
> > > > But some caller misused this function, for example,
> > > >   DEFRAG_PROG="$BTRFS_UTIL_PROG filesystem defragment"
> > > >   _require_command $DEFRAG_PROG defragment In above code,
> > > > _require_command get 4 arguments, the caller's second argument is
> > > > never used, and the second field of first argument is treat as
> > > > "error message" in _require_command.
> > > >
> > > > We fixed above case by adding quotes to _require_command()'s
> > > > arguments, it fixed most test case, but introduced a new bug, in
> > > > above case, "btrfs filesystem defragment" is not a command, and
> > > > always failed in _require_command(), it caused some testcase not
> > > > work, as btrfs/005.
> > > >
> > > > This patch fixed above caller.
> > > >
> > > > Changelog v1->v2:
> > > > 1: Add detail description, suggested by:
> > > >    Lukáš Czerner <[e-mail]>
> > > > 2: Add missed Reported-by.
> > > > 3: Make $XFSDUMP_PROG always be a pure command, to avoid special
> > > >    handling for it.
> > >
> > > Having to change xfsdump checks means this is still the wrong fix.
> > >
> > > _require_command should simply handle multi-part command strings.
> > >
> > > Does the following patch fix your problems?
> > >
> > This patch can deal with current code, only can't deal with program with
> blank in filename.
> > But this is rarely happened, so we need not care about it.
> 
> It will work just fine with a minor tweak:
> 
> $ [ -x /bin/true ] && echo foo
> foo
> $ [ -x "" ] && echo foo
> $
> 
> i.e. the -x check fails as expected when passed an empty command.
> 
> > > +	if [ ! -x $command ]; then
> 
> i.e this needs changing to [ ! -x "$command" ]....
> 
I means this command:
[root@ZLLINUX ~]# cp /bin/cp ./"c    p"
[root@ZLLINUX ~]# ./"c    p" --verion
cp (GNU coreutils) 8.4
...

Above file of "c    p" is a command, but can not work with
_require_command.
Not real problem, please forgot it and apply your patch:)

Thanks
Zhaolei

> Cheers,
> 
> Dave.
> --
> Dave Chinner
> [e-mail]


--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/common/rc b/common/rc
index ca8da7f..6ea107e 100644
--- a/common/rc
+++ b/common/rc
@@ -1286,10 +1286,22 @@  _require_realtime()
 # this test requires that a specified command (executable) exists
 # $1 - command, $2 - name for error message
 #
+# Note: the command string might have parameters, so strip them before checking
+# whether it is executable.
 _require_command()
 {
-    [ -n "$1" ] && _cmd="$1" || _cmd="$2"
-    [ -n "$1" -a -x "$1" ] || _notrun "$_cmd utility required, skipped this test"
+	if [ $# -eq 2 ]; then
+		_name="$2"
+	elif [ $# -eq 1 ]; then
+		_name="$1"
+	else
+		_fail "usage: _require_command <command> [<name>]"
+	fi
+
+	_command=`echo "$1" | awk '{ print $1 }'`
+	if [ ! -x $command ]; then
+		_notrun "$_name utility required, skipped this test"
+	fi
 }
 
 # this test requires the device to be valid block device