Hi, > -----Original Message----- > From: Lukáš Czerner [mailto:lczerner@xxxxxxxxxx] > Sent: Thursday, February 26, 2015 9:36 PM > To: Zhao Lei > Cc: fstests@xxxxxxxxxxxxxxx > Subject: RE: [PATCH v3 2/2] Fix warning of "Usage: _is_block_dev dev" > > On Thu, 26 Feb 2015, Zhao Lei wrote: > > > Date: Thu, 26 Feb 2015 20:26:42 +0800 > > From: Zhao Lei <zhaolei@xxxxxxxxxxxxxx> > > To: 'Lukáš Czerner' <lczerner@xxxxxxxxxx> > > Cc: fstests@xxxxxxxxxxxxxxx > > Subject: RE: [PATCH v3 2/2] Fix warning of "Usage: _is_block_dev dev" > > > > Hi, Lukas > > > > Thanks for review. > > > > * From: Lukáš Czerner [mailto:lczerner@xxxxxxxxxx] > > > Sent: Thursday, February 26, 2015 7:30 PM > > > To: Zhaolei > > > Cc: fstests@xxxxxxxxxxxxxxx > > > Subject: Re: [PATCH v3 2/2] Fix warning of "Usage: _is_block_dev dev" > > > > > > On Thu, 26 Feb 2015, Zhaolei wrote: > > > > > > > Date: Thu, 26 Feb 2015 18:38:59 +0800 > > > > From: Zhaolei <zhaolei@xxxxxxxxxxxxxx> > > > > To: fstests@xxxxxxxxxxxxxxx > > > > Cc: Zhao Lei <zhaolei@xxxxxxxxxxxxxx> > > > > Subject: [PATCH v3 2/2] Fix warning of "Usage: _is_block_dev dev" > > > > > > > > From: Zhao Lei <zhaolei@xxxxxxxxxxxxxx> > > > > > > > > _is_block_dev() will show above warning when "$dev" is not exist. > > > > It happened when the program check $TEST_DEV with blank > > > > $SCRATCH_DEV which is optional. > > > > > > > > Changelog v2->v3: > > > > Separate __same_block_dev() from _is_block_dev() to make code > > > > self documenting. > > > > Suggested by: Dave Chinner <david@xxxxxxxxxxxxx> > > > > > > > > Changelog v1->v2: > > > > Rewrite _is_block_dev() to make caller code simple. > > > > Suggested by: Dave Chinner <david@xxxxxxxxxxxxx> > > > > > > This is all very confusing. Sometimes you consider links, sometimes you do > not. > > > > I always consider symbol links. > > > > [-b ...] in _is_block_dev() can work well even if "$1" is link, so we > > need not call use readlink before. > > > > But "stat" command in __same_block_dev() will not return block id if > > "$1" is link, so we need convert it using readlink. > > > > > Error messages are missing completely _is_block_dev() does not > > > return device numbers anymore which might be useful. > > > > > In current code, device number is only used to compare is same device, > > and function named _is_same_device() seems more suitable. > > > > And if we really need to get dev_id in future, maybe add a function > > named _get_dev_id() that time can make code more readable. > > > > > What about just using the original _is_block_dev() as it was > > > intended ? Like > > > this: > > > > > > if [ "`_is_block_dev "$SCRATCH_DEV"`" = "`_is_block_dev > > > "$TEST_DEV"`" ] > > > > > > Simply use quotes around the function argument so that the function > > > always receives an argument even though it might be empty, but in > > > this case you'll not get the error message and the _is_block_dev() > > > will just return nothing - problem solved ? > > > > > This way can solve titled problem, this patch changed more code > > because most of them are restructure around discussion with patch v1. > > But it still seems to me that using proper quoting fixes the problem without > rewriting bunch of code unnecessarily. I've read the discussion and I think > you're talking about: > > "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." > > But the function already works this way, you just need to make sure that the > argument is passed to the function even if that argument is empty - so you > need to use quotes like _is_block_dev "$TEST_DEV". > Its the unfortunate way bash works. > Another discussion is: | From Dave Chinner | > 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 in discussion in v2, Dave Chinner's sample is using return value: | # Returns: | # -1 if no argument passed | # 0 if filename is not a block device | # 1 if filename is a block device. | _is_block_dev() | { | if [ $# -ne 1 ]; then | return -1; | fi | | if [ -b $1 ]; then | return 1; | fi | return 0; | } | It is reason why I changed to use return value in current version. > Simply said > > _is_block_dev "$TEST_DEV" > > will return empty string if $TEST_DEV either does not exist or $TEST_DEV itself > is empty string. However if someone calls > > _is_block_dev > > he'll get an error so he knows that he needs to fix his code. > These 2 way have the respective advantages. For me, both way is acceptable as long as it can fix titled bug. I'll send v4 based on your suggestion, to provide Dave Chinner a selection. Thanks Zhaolei > -Lukas > Thanks! > > > > > Thanks > > Zhaolei > > > > > -Lukas > > > > > > > > > > > > > > Signed-off-by: Zhao Lei <zhaolei@xxxxxxxxxxxxxx> > > > > --- > > > > common/rc | 101 > > > > +++++++++++++++++++++++++++++++++++++------------------------- > > > > 1 file changed, 61 insertions(+), 40 deletions(-) > > > > > > > > diff --git a/common/rc b/common/rc index 5a8c74d..f528c25 100644 > > > > --- a/common/rc > > > > +++ b/common/rc > > > > @@ -945,24 +945,51 @@ _fs_options() > > > > ' </proc/mounts > > > > } > > > > > > > > -# returns device number if a file is a block device > > > > +# Returns: > > > > +# 0: if filename is a block device # 1: if filename is not a > > > > +block device # -1 if argument wrong > > > > # > > > > _is_block_dev() > > > > { > > > > - if [ $# -ne 1 ] > > > > - then > > > > - echo "Usage: _is_block_dev dev" 1>&2 > > > > - exit 1 > > > > - fi > > > > + if [ $# -ne 1 ]; then > > > > + return -1 > > > > + fi > > > > + if [ -b "$1" ]; then > > > > + return 0 > > > > + fi > > > > + return 1; > > > > +} > > > > > > > > - _dev=$1 > > > > - if [ -L "${_dev}" ]; then > > > > - _dev=`readlink -f ${_dev}` > > > > - fi > > > > +# Returns: > > > > +# 0: if the devices are same > > > > +# 1: if the devices are not same > > > > +# > > > > +__same_block_dev() > > > > +{ > > > > + if [ $# -ne 2 ]; then > > > > + return 1 > > > > + fi > > > > > > > > - if [ -b "${_dev}" ]; then > > > > - src/lstat64 ${_dev} | $AWK_PROG '/Device type:/ { print $9 }' > > > > - fi > > > > + _dev1="$1" > > > > + _dev2="$2" > > > > + > > > > + if [ ! -b "$_dev1" -o ! -b "$_dev2" ]; then > > > > + return 1 > > > > + fi > > > > + > > > > + if [ -L "${_dev1}" ]; then > > > > + _dev1=`readlink -f "$_dev1"` > > > > + fi > > > > + if [ -L "${_dev2}" ]; then > > > > + _dev2=`readlink -f "$_dev2"` > > > > + fi > > > > + > > > > + if [ `stat -c %t,%T "$_dev1"` != `stat -c %t,%T "$_dev2"` ]; then > > > > + return 1 > > > > + fi > > > > + > > > > + return 0; > > > > } > > > > > > > > # Do a command, log it to $seqres.full, optionally test return > > > > status @@ -1095,19 +1122,16 @@ _require_scratch_nocheck() > > > > fi > > > > ;; > > > > *) > > > > - if [ -z "$SCRATCH_DEV" -o "`_is_block_dev $SCRATCH_DEV`" = > "" ] > > > > - then > > > > - _notrun "this test requires a valid \$SCRATCH_DEV" > > > > - fi > > > > - if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev > > > $TEST_DEV`" ] > > > > - then > > > > - _notrun "this test requires a valid \$SCRATCH_DEV" > > > > - fi > > > > - if [ ! -d "$SCRATCH_MNT" ] > > > > - then > > > > - _notrun "this test requires a valid \$SCRATCH_MNT" > > > > + if ! _is_block_dev "$SCRATCH_DEV"; then > > > > + _notrun "this test requires a valid \$SCRATCH_DEV" > > > > + fi > > > > + if __same_block_dev "$SCRATCH_DEV" "$TEST_DEV"; then > > > > + _notrun "\$SCRATCH_DEV and \$TEST_DEV are same > device" > > > > + fi > > > > + if [ ! -d "$SCRATCH_MNT" ]; then > > > > + _notrun "this test requires a valid \$SCRATCH_MNT" > > > > fi > > > > - ;; > > > > + ;; > > > > esac > > > > > > > > # mounted? > > > > @@ -1167,19 +1191,16 @@ _require_test() > > > > fi > > > > ;; > > > > *) > > > > - if [ -z "$TEST_DEV" -o "`_is_block_dev $TEST_DEV`" = "" ] > > > > - then > > > > - _notrun "this test requires a valid \$TEST_DEV" > > > > - fi > > > > - if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev > > > $TEST_DEV`" ] > > > > - then > > > > - _notrun "this test requires a valid \$TEST_DEV" > > > > - fi > > > > - if [ ! -d "$TEST_DIR" ] > > > > - then > > > > - _notrun "this test requires a valid \$TEST_DIR" > > > > + if ! _is_block_dev "$TEST_DEV"; then > > > > + _notrun "this test requires a valid \$TEST_DEV" > > > > + fi > > > > + if __same_block_dev "$TEST_DEV" "$SCRATCH_DEV"; then > > > > + _notrun "\$TEST_DEV and \$SCRATCH_DEV are same > device" > > > > fi > > > > - ;; > > > > + if [ ! -d "$TEST_DIR" ]; then > > > > + _notrun "this test requires a valid \$TEST_DIR" > > > > + fi > > > > + ;; > > > > esac > > > > > > > > # mounted? > > > > @@ -1288,7 +1309,7 @@ _require_block_device() > > > > echo "Usage: _require_block_device <dev>" 1>&2 > > > > exit 1 > > > > fi > > > > - if [ "`_is_block_dev $1`" == "" ]; then > > > > + if ! _is_block_dev "$1"; then > > > > _notrun "require $1 to be valid block disk" > > > > fi > > > > } > > > > @@ -2236,10 +2257,10 @@ _require_scratch_dev_pool() > > > > esac > > > > > > > > for i in $SCRATCH_DEV_POOL; do > > > > - if [ "`_is_block_dev $i`" = "" ]; then > > > > + if ! _is_block_dev "$i"; then > > > > _notrun "this test requires valid block disk $i" > > > > fi > > > > - if [ "`_is_block_dev $i`" = "`_is_block_dev $TEST_DEV`" ]; then > > > > + if __same_block_dev "$i" "$TEST_DEV"; then > > > > _notrun "$i is part of TEST_DEV, this test requires unique > disks" > > > > fi > > > > if _mount | grep -q $i; then > > > > > > > > > > -- 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
