On Mon, Feb 16, 2015 at 10:27:33AM +0800, Zhao Lei wrote: > Hi, > > > -----Original Message----- > > From: Dave Chinner [mailto:david@xxxxxxxxxxxxx] > > Sent: Monday, February 16, 2015 7:05 AM > > To: Zhaolei > > Cc: fstests@xxxxxxxxxxxxxxx > > Subject: Re: [PATCH] Fix warning of "Usage: _is_block_dev dev" > > > > On Thu, Feb 12, 2015 at 08:48:14PM +0800, Zhaolei wrote: > > > From: Zhao Lei <zhaolei@xxxxxxxxxxxxxx> > > > > > > _is_block_dev() will show above warning when "$dev" is not exist. > > > It happened when user hadn't set $SCRATCH_DEV(optional) and check > > > $TEST_DEV. > > > > _is_block_dev() is used in many places to check whether the block device exists. > > i.e. I'd suggest that _is_block_dev() should return an empty string to indicate > > it's not a block device rather than exit if a null. That means we don't have to > > execute _is_block_dev() in a subshell (i.e. via `_is_block_dev ...`) to prevent it > > from killing the script that runs it if the block device passed to it is null. > > > > That means we don't have to add checks everywhere it is called, and we can > > simplify the calling convention at the same time.... > > > Thanks for your suggestion. > > Are you mean this? > > diff --git a/common/rc b/common/rc > index 7449a1d..12861b8 100644 > --- a/common/rc > +++ b/common/rc > @@ -951,8 +951,7 @@ _is_block_dev() > { > if [ $# -ne 1 ] > then > - echo "Usage: _is_block_dev dev" 1>&2 > - exit 1 > + return > fi > > _dev=$1 > @@ -1095,7 +1094,7 @@ _require_scratch_nocheck() > fi > ;; > *) > - if [ -z "$SCRATCH_DEV" -o "`_is_block_dev $SCRATCH_DEV`" = "" ] > + if [ "`_is_block_dev $SCRATCH_DEV`" = "" ] > then > _notrun "this test requires a valid \$SCRATCH_DEV" > fi > @@ -1167,7 +1166,7 @@ _require_test() > fi > ;; > *) > - if [ -z "$TEST_DEV" -o "`_is_block_dev $TEST_DEV`" = "" ] > + if [ "`_is_block_dev $TEST_DEV`" = "" ] > then > _notrun "this test requires a valid \$TEST_DEV" > Fi Yes, and there are a couple of other places where the same thing can be done. FWIW, should convert to "if [...]; then" format at the same time. > If we want to avoid calling _is_block_dev in a subshell, we can do following change: > > _is_block_dev() > { > return 1 if "$1" is not block dev > } > _same_dev() > { > return 1 if "$1" and "$2" are not same dev > } yes, that's a good idea, too. > And caller code will be: > > if [ ! _is_block_dev "$SCRATCH_DEV" -o _same_dev "$SCRATCH_DEV" "$TEST_DEV" ] > then > _notrun "this test requires a valid \$SCRATCH_DEV" > fi Well, I'd say that if $TEST_DEV exists and $SCRATCH_DEV doesn't, then clearly they are not the same device. Hence the test for _same_dev() should handle those cases correctly internally. 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
