Hi Dave Chinner > -----Original Message----- > From: Dave Chinner [mailto:david@xxxxxxxxxxxxx] > Sent: Wednesday, April 15, 2015 9:00 PM > To: Zhao Lei > Cc: fstests@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxx] > > > Sent: Wednesday, April 15, 2015 5:45 AM > > > To: Zhaolei > > > Cc: fstests@xxxxxxxxxxxxxxx > > > 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@xxxxxxxxxxxxxx> > > > > > > > > _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@xxxxxxxxxx> > > > > 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@xxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
