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. 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
