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

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

[Zhaolei]

From: Zhao Lei <zhaolei@cn.fujitsu.com>

_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 <lczerner@redhat.com>
2: Add missed Reported-by.
3: Make $XFSDUMP_PROG always be a pure command, to avoid special
   handling for it.

Reported-by: Filipe David Manana <fdmanana@gmail.com>
Suggested-by: Lukáš Czerner <lczerner@redhat.com>
Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 common/defrag | 13 +++++++++----
 common/rc     |  3 ---
 tests/xfs/195 |  2 +-
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/common/defrag b/common/defrag
index f923dc0..b44ef98 100644
--- a/common/defrag
+++ b/common/defrag
@@ -22,22 +22,27 @@
 
 _require_defrag()
 {
+    local defrag_cmd
+
     case "$FSTYP" in
     xfs)
-        DEFRAG_PROG="$XFS_FSR_PROG"
+        defrag_cmd="$XFS_FSR_PROG"
+        DEFRAG_PROG="$defrag_cmd"
 	;;
     ext4|ext4dev)
-        DEFRAG_PROG="$E4DEFRAG_PROG"
+        defrag_cmd="$E4DEFRAG_PROG"
+        DEFRAG_PROG="$defrag_cmd"
 	;;
     btrfs)
-	DEFRAG_PROG="$BTRFS_UTIL_PROG filesystem defragment"
+        defrag_cmd="$BTRFS_UTIL_PROG"
+        DEFRAG_PROG="defrag_cmd filesystem defragment"
 	;;
     *)
         _notrun "defragmentation not supported for fstype \"$FSTYP\""
 	;;
     esac
 
-    _require_command "$DEFRAG_PROG" defragment
+    _require_command "$defrag_cmd" defragment
     _require_xfs_io_command "fiemap"
 }
 
diff --git a/common/rc b/common/rc
index c1a50f2..02ac02a 100644
--- a/common/rc
+++ b/common/rc
@@ -2923,9 +2923,6 @@ init_rc()
 		$DF_PROG $TEST_DEV
 		exit 1
 	fi
-	# Figure out if we need to add -F ("foreign", deprecated) option to xfs_io
-	xfs_io -c stat $TEST_DIR 2>&1 | grep -q "is not on an XFS filesystem" && \
-	export XFS_IO_PROG="$XFS_IO_PROG -F"
 }
 
 # get real device path name by following link
diff --git a/tests/xfs/195 b/tests/xfs/195
index 21fcb00..075022d 100755
--- a/tests/xfs/195
+++ b/tests/xfs/195
@@ -65,7 +65,7 @@ _supported_os Linux
 
 _require_test
 _require_user
-_require_command "$XFSDUMP_PROG" xfsdump
+[ "$XFSDUMP_PROG" = "" ] && _notrun "xfsdump not found"
 
 echo "Preparing subtree"
 mkdir $TEST_DIR/d
-- 
1.8.5.1

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

[Eryu Guan]

On Mon, Apr 13, 2015 at 12:32:43PM +0800, Zhaolei wrote:
> From: Zhao Lei <zhaolei@cn.fujitsu.com>
> 
> _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 <lczerner@redhat.com>
> 2: Add missed Reported-by.
> 3: Make $XFSDUMP_PROG always be a pure command, to avoid special
>    handling for it.
> 
> Reported-by: Filipe David Manana <fdmanana@gmail.com>
> Suggested-by: Lukáš Czerner <lczerner@redhat.com>
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> ---
>  common/defrag | 13 +++++++++----
>  common/rc     |  3 ---
>  tests/xfs/195 |  2 +-
>  3 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/common/defrag b/common/defrag
> index f923dc0..b44ef98 100644
> --- a/common/defrag
> +++ b/common/defrag
> @@ -22,22 +22,27 @@
>  
>  _require_defrag()
>  {
> +    local defrag_cmd
> +
>      case "$FSTYP" in
>      xfs)
> -        DEFRAG_PROG="$XFS_FSR_PROG"
> +        defrag_cmd="$XFS_FSR_PROG"
> +        DEFRAG_PROG="$defrag_cmd"
>  	;;
>      ext4|ext4dev)
> -        DEFRAG_PROG="$E4DEFRAG_PROG"
> +        defrag_cmd="$E4DEFRAG_PROG"
> +        DEFRAG_PROG="$defrag_cmd"
>  	;;
>      btrfs)
> -	DEFRAG_PROG="$BTRFS_UTIL_PROG filesystem defragment"
> +        defrag_cmd="$BTRFS_UTIL_PROG"
> +        DEFRAG_PROG="defrag_cmd filesystem defragment"
>  	;;
>      *)
>          _notrun "defragmentation not supported for fstype \"$FSTYP\""
>  	;;
>      esac
>  
> -    _require_command "$DEFRAG_PROG" defragment
> +    _require_command "$defrag_cmd" defragment
>      _require_xfs_io_command "fiemap"
>  }
>  
> diff --git a/common/rc b/common/rc
> index c1a50f2..02ac02a 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2923,9 +2923,6 @@ init_rc()
>  		$DF_PROG $TEST_DEV
>  		exit 1
>  	fi
> -	# Figure out if we need to add -F ("foreign", deprecated) option to xfs_io
> -	xfs_io -c stat $TEST_DIR 2>&1 | grep -q "is not on an XFS filesystem" && \
> -	export XFS_IO_PROG="$XFS_IO_PROG -F"

I think we should keep the "-F" option, as xfs_io comes with
distrobutions like RHEL6 still needs "-F" to proceed on non-xfs fs.

[root@dhcp-66-86-3 xfstests]# xfs_io -f -c "pwrite 0 1k" testfile
xfs_io: specified file ["testfile"] is not on an XFS filesystem

Thanks,
Eryu
>  }
>  
>  # get real device path name by following link
> diff --git a/tests/xfs/195 b/tests/xfs/195
> index 21fcb00..075022d 100755
> --- a/tests/xfs/195
> +++ b/tests/xfs/195
> @@ -65,7 +65,7 @@ _supported_os Linux
>  
>  _require_test
>  _require_user
> -_require_command "$XFSDUMP_PROG" xfsdump
> +[ "$XFSDUMP_PROG" = "" ] && _notrun "xfsdump not found"
>  
>  echo "Preparing subtree"
>  mkdir $TEST_DIR/d
> -- 
> 1.8.5.1
> 
> --
> 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

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

[Zhao Lei]

Hi, Eryu

Thanks for review.

> -----Original Message-----
> From: Eryu Guan [mailto:eguan@redhat.com]
> Sent: Monday, April 13, 2015 1:57 PM
> To: Zhaolei
> Cc: fstests@vger.kernel.org
> 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 <zhaolei@cn.fujitsu.com>
> >
> > _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 <lczerner@redhat.com>
> > 2: Add missed Reported-by.
> > 3: Make $XFSDUMP_PROG always be a pure command, to avoid special
> >    handling for it.
> >
> > Reported-by: Filipe David Manana <fdmanana@gmail.com>
> > Suggested-by: Lukáš Czerner <lczerner@redhat.com>
> > Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> > ---
> >  common/defrag | 13 +++++++++----
> >  common/rc     |  3 ---
> >  tests/xfs/195 |  2 +-
> >  3 files changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/common/defrag b/common/defrag index f923dc0..b44ef98
> > 100644
> > --- a/common/defrag
> > +++ b/common/defrag
> > @@ -22,22 +22,27 @@
> >
> >  _require_defrag()
> >  {
> > +    local defrag_cmd
> > +
> >      case "$FSTYP" in
> >      xfs)
> > -        DEFRAG_PROG="$XFS_FSR_PROG"
> > +        defrag_cmd="$XFS_FSR_PROG"
> > +        DEFRAG_PROG="$defrag_cmd"
> >  	;;
> >      ext4|ext4dev)
> > -        DEFRAG_PROG="$E4DEFRAG_PROG"
> > +        defrag_cmd="$E4DEFRAG_PROG"
> > +        DEFRAG_PROG="$defrag_cmd"
> >  	;;
> >      btrfs)
> > -	DEFRAG_PROG="$BTRFS_UTIL_PROG filesystem defragment"
> > +        defrag_cmd="$BTRFS_UTIL_PROG"
> > +        DEFRAG_PROG="defrag_cmd filesystem defragment"
> >  	;;
> >      *)
> >          _notrun "defragmentation not supported for fstype \"$FSTYP\""
> >  	;;
> >      esac
> >
> > -    _require_command "$DEFRAG_PROG" defragment
> > +    _require_command "$defrag_cmd" defragment
> >      _require_xfs_io_command "fiemap"
> >  }
> >
> > diff --git a/common/rc b/common/rc
> > index c1a50f2..02ac02a 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -2923,9 +2923,6 @@ init_rc()
> >  		$DF_PROG $TEST_DEV
> >  		exit 1
> >  	fi
> > -	# Figure out if we need to add -F ("foreign", deprecated) option to xfs_io
> > -	xfs_io -c stat $TEST_DIR 2>&1 | grep -q "is not on an XFS filesystem" && \
> > -	export XFS_IO_PROG="$XFS_IO_PROG -F"
> 
> I think we should keep the "-F" option, as xfs_io comes with distrobutions like
> RHEL6 still needs "-F" to proceed on non-xfs fs.
> 
> [root@dhcp-66-86-3 xfstests]# xfs_io -f -c "pwrite 0 1k" testfile
> xfs_io: specified file ["testfile"] is not on an XFS filesystem
> 
I keep -F in v1, and v2 deleted above -F support by suggestion of
Lukáš Czerner <lczerner@redhat.com>, who is also author of
these code block.

CC: Lukáš Czerner <lczerner@redhat.com>

If we suppose xfstests always runs in, and test newest kernel and user tools,
we can remove obsoleted commands.
It not, we should keep them to make xfstests' compatibility.

What is your opinion?

Thanks
Zhaolei

> Thanks,
> Eryu
> >  }
> >
> >  # get real device path name by following link diff --git
> > a/tests/xfs/195 b/tests/xfs/195 index 21fcb00..075022d 100755
> > --- a/tests/xfs/195
> > +++ b/tests/xfs/195
> > @@ -65,7 +65,7 @@ _supported_os Linux
> >
> >  _require_test
> >  _require_user
> > -_require_command "$XFSDUMP_PROG" xfsdump
> > +[ "$XFSDUMP_PROG" = "" ] && _notrun "xfsdump not found"
> >
> >  echo "Preparing subtree"
> >  mkdir $TEST_DIR/d
> > --
> > 1.8.5.1
> >
> > --
> > 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

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

[Dave Chinner]

On Mon, Apr 13, 2015 at 02:39:48PM +0800, Zhao Lei wrote:
> > From: Eryu Guan [mailto:eguan@redhat.com]
> > Sent: Monday, April 13, 2015 1:57 PM
> > To: Zhaolei
> > Cc: fstests@vger.kernel.org
> > 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 <zhaolei@cn.fujitsu.com>
> > >
> > > _require_command() only accept 2 arguments, first one is pure command,
> > > and second one is name for error message.
> > >
.....
> > > diff --git a/common/rc b/common/rc
> > > index c1a50f2..02ac02a 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -2923,9 +2923,6 @@ init_rc()
> > >  		$DF_PROG $TEST_DEV
> > >  		exit 1
> > >  	fi
> > > -	# Figure out if we need to add -F ("foreign", deprecated) option to xfs_io
> > > -	xfs_io -c stat $TEST_DIR 2>&1 | grep -q "is not on an XFS filesystem" && \
> > > -	export XFS_IO_PROG="$XFS_IO_PROG -F"
> > 
> > I think we should keep the "-F" option, as xfs_io comes with distrobutions like
> > RHEL6 still needs "-F" to proceed on non-xfs fs.
> > 
> > [root@dhcp-66-86-3 xfstests]# xfs_io -f -c "pwrite 0 1k" testfile
> > xfs_io: specified file ["testfile"] is not on an XFS filesystem
> > 
> I keep -F in v1, and v2 deleted above -F support by suggestion of
> Lukáš Czerner <lczerner@redhat.com>, who is also author of
> these code block.
> 
> CC: Lukáš Czerner <lczerner@redhat.com>
> 
> If we suppose xfstests always runs in, and test newest kernel and user tools,
> we can remove obsoleted commands.

No, absolutely not. xfstests needs to run on all sorts of different
kernels and systems, including old vendor kernels. That means
dropping compatibility support for them is not an option until their
QA departments are no longer testing those distros. IOWs, it's going
to be many years before we can drop the "-F" option....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Dave Chinner]

On Mon, Apr 13, 2015 at 12:32:43PM +0800, Zhaolei wrote:
> From: Zhao Lei <zhaolei@cn.fujitsu.com>
> 
> _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 <lczerner@redhat.com>
> 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.
-- 
Dave Chinner
david@fromorbit.com

common: _require_command needs to handle parameters

From: Dave Chinner <dchinner@redhat.com>

_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 <dchinner@redhat.com>
---
 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

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

[Zhao Lei]

Hi, Dave Chinner

> -----Original Message-----
> From: Dave Chinner [mailto:david@fromorbit.com]
> Sent: Wednesday, April 15, 2015 5:45 AM
> To: Zhaolei
> Cc: fstests@vger.kernel.org
> 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 <zhaolei@cn.fujitsu.com>
> >
> > _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 <lczerner@redhat.com>
> > 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
> david@fromorbit.com
> 
> common: _require_command needs to handle parameters
> 
> From: Dave Chinner <dchinner@redhat.com>
> 
> _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 <dchinner@redhat.com>
> ---
>  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

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

[Dave Chinner]

On Wed, Apr 15, 2015 at 08:28:21PM +0800, Zhao Lei wrote:
> Hi, Dave Chinner
> 
> > -----Original Message-----
> > From: Dave Chinner [mailto:david@fromorbit.com]
> > Sent: Wednesday, April 15, 2015 5:45 AM
> > To: Zhaolei
> > Cc: fstests@vger.kernel.org
> > 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 <zhaolei@cn.fujitsu.com>
> > >
> > > _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 <lczerner@redhat.com>
> > > 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.
-- 
Dave Chinner
david@fromorbit.com

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

[Zhao Lei]

Hi Dave Chinner

> -----Original Message-----
> From: Dave Chinner [mailto:david@fromorbit.com]
> Sent: Wednesday, April 15, 2015 9:00 PM
> To: Zhao Lei
> Cc: fstests@vger.kernel.org
> 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:david@fromorbit.com]
> > > Sent: Wednesday, April 15, 2015 5:45 AM
> > > To: Zhaolei
> > > Cc: fstests@vger.kernel.org
> > > 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 <zhaolei@cn.fujitsu.com>
> > > >
> > > > _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 <lczerner@redhat.com>
> > > > 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
> david@fromorbit.com