| Message ID | 20221013055904.28978-1-akihiko.odaki@daynix.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ftp/ftp01: Use tst_net.sh | expand |
Hi Akihiko,
> This allows to use SSH rather than RSH.
Thanks for your work. First of all, code is not working, because ASCII_FILES and
BIN_FILES are in "$LTPROOT/testcases/bin/datafiles. have look at approach in
testcases/network/nfs/nfs_stress/nfs02.sh:
LTP_DATAFILES="$LTPROOT/testcases/bin/datafiles"
Otherwise it fails:
ftp01 1 TINFO: timeout per run is 0h 5m 0s
Not connected.
Not connected.
Not connected.
Not connected.
ls: cannot access 'bin.sm': No such file or directory
ls: cannot access '/opt/ltp/testcases/bin/datafiles/bin.sm': No such file or directory
ftp01 1 TINFO: Test Successful doing ftp get bin.sm binary
Not connected.
ftp: Can't chdir `/opt/ltp/testcases/bin/datafiles': No such file or directory
Not connected.
Not connected.
Not connected.
ftp01 1 TBROK: 'sum /tmp/LTP_ftp01.e9NUSqkHa3/bin.sm' failed on '': 'sum: /tmp/LTP_ftp01.e9NUSqkHa3/bin.sm: No such file or directory'
ftp01 1 TINFO: AppArmor enabled, this may affect test results
ftp01 1 TINFO: it can be disabled with TST_DISABLE_APPARMOR=1 (requires super/root)
ftp01 1 TINFO: loaded AppArmor profiles: none
...
Or have I miss something?
When we convert file to use LTP new API (shell API in this case:
https://github.com/linux-test-project/ltp/wiki/Shell-Test-API)
we do cleanup the code.
NFS tests or other shell tests which use tst_net.sh and *not* set
TST_USE_LEGACY_API=1 (i.e. rewritten to use LTP new shell API) are good
examples.
ftp01.sh requires at least this cleanup:
* remove useless comments like "FUNCTION: do_setup"
* code cleanup of do_test: having several loops is really crazy,
how about write functions and pass them parameters?
* please use $( ... ) instead of ` ... `.
* use local to declare new variables
* I'd remove sleep option
* replace GPL with:
# SPDX-License-Identifier: GPL-2.0-or-later
* update copyright
Proper API use
* tst_require_cmds should be replaced by TST_NEEDS_CMDS at the top.
Even if you keep to use tst_require_cmds awk ftp
this line is useless (1) tst_brk quits testing 2) we use ssh, why to mention
.rhosts?):
[ $? = 0 ] || tst_brk TBROK "Check .rhosts file on remote machine."
* instead of "for i in binary ascii; do" I'd use:
TST_TESTFUNC=do_test
TST_CNT=4
TST_NEEDS_TMPDIR=1
LTP_DATAFILES="$LTPROOT/testcases/bin/datafiles"
test_ftp()
{
for j in $*; do
done
}
do_test()
{
case $1 in
1) test_get $BIN_FILES;;
2) test_get $ASCII_FILES;;
3) test_put $BIN_FILES;;
4) test_put $ASCII_FILES;;
esac
}
test_get()
{
local sum1 sum2
for file in $*; do
{
echo "user $RUSER $PASSWD"
echo "$i"
echo "cd $TST_NET_DATAROOT"
echo "get $file"
echo "quit"
} | ftp -nv $RHOST
sum1="$(ls -l $file | awk '{print $5}')"
sum2="$(ls -l $TST_NET_DATAROOT/$file | awk '{print $5}')"
rm -f $file
EXPECT_PASS "[ '$sum1' = '$sum2' ]"
done
}
Kind regards,
Petr
Hi Petr, Thanks for reviewing. I have sent the new version with changes accordingly to your comments though I forgot to name it "v2". Please check it. Regarding paths to ASCII_FILES and BIN_FILES, the patch refers to $TST_NET_DATAROOT so it produces correct paths, prefixed with "$LTPROOT/testcases/bin/datafiles". I guess your test failure is related to the error message "Not connected" rather than the path to files. The below is the output I got with v2: [ 320.638778] LTP: starting ftp (ftp01.sh) ftp01 1 TINFO: timeout per run is 0h 5m 0s Connected to 10.0.2.15 (10.0.2.15). 220 (vsFTPd 3.0.5) 331 Please specify the password. 230 Login successful. 200 Switching to Binary mode. 250 Directory successfully changed. local: ascii.sm remote: ascii.sm 227 Entering Passive Mode (10,0,2,15,35,3). 150 Opening BINARY mode data connection for ascii.sm (220 bytes). 226 Transfer complete. 220 bytes received in 4.1e-05 secs (5365.85 Kbytes/sec) 221 Goodbye. ftp01 1 TPASS: [ '220' = '220' ] passed as expected Regards, Akihiko Odaki On 2022/10/13 21:59, Petr Vorel wrote: > Hi Akihiko, > >> This allows to use SSH rather than RSH. > > Thanks for your work. First of all, code is not working, because ASCII_FILES and > BIN_FILES are in "$LTPROOT/testcases/bin/datafiles. have look at approach in > testcases/network/nfs/nfs_stress/nfs02.sh: > LTP_DATAFILES="$LTPROOT/testcases/bin/datafiles" > > Otherwise it fails: > > ftp01 1 TINFO: timeout per run is 0h 5m 0s > Not connected. > Not connected. > Not connected. > Not connected. > ls: cannot access 'bin.sm': No such file or directory > ls: cannot access '/opt/ltp/testcases/bin/datafiles/bin.sm': No such file or directory > ftp01 1 TINFO: Test Successful doing ftp get bin.sm binary > Not connected. > ftp: Can't chdir `/opt/ltp/testcases/bin/datafiles': No such file or directory > Not connected. > Not connected. > Not connected. > ftp01 1 TBROK: 'sum /tmp/LTP_ftp01.e9NUSqkHa3/bin.sm' failed on '': 'sum: /tmp/LTP_ftp01.e9NUSqkHa3/bin.sm: No such file or directory' > ftp01 1 TINFO: AppArmor enabled, this may affect test results > ftp01 1 TINFO: it can be disabled with TST_DISABLE_APPARMOR=1 (requires super/root) > ftp01 1 TINFO: loaded AppArmor profiles: none > > ... > > Or have I miss something? > > When we convert file to use LTP new API (shell API in this case: > https://github.com/linux-test-project/ltp/wiki/Shell-Test-API) > we do cleanup the code. > > NFS tests or other shell tests which use tst_net.sh and *not* set > TST_USE_LEGACY_API=1 (i.e. rewritten to use LTP new shell API) are good > examples. > > ftp01.sh requires at least this cleanup: > * remove useless comments like "FUNCTION: do_setup" > * code cleanup of do_test: having several loops is really crazy, > how about write functions and pass them parameters? > * please use $( ... ) instead of ` ... `. > * use local to declare new variables > * I'd remove sleep option > * replace GPL with: > # SPDX-License-Identifier: GPL-2.0-or-later > * update copyright > > Proper API use > * tst_require_cmds should be replaced by TST_NEEDS_CMDS at the top. > Even if you keep to use tst_require_cmds awk ftp > this line is useless (1) tst_brk quits testing 2) we use ssh, why to mention > .rhosts?): > [ $? = 0 ] || tst_brk TBROK "Check .rhosts file on remote machine." > * instead of "for i in binary ascii; do" I'd use: > > TST_TESTFUNC=do_test > TST_CNT=4 > TST_NEEDS_TMPDIR=1 > LTP_DATAFILES="$LTPROOT/testcases/bin/datafiles" > > test_ftp() > { > for j in $*; do > done > } > > do_test() > { > case $1 in > 1) test_get $BIN_FILES;; > 2) test_get $ASCII_FILES;; > 3) test_put $BIN_FILES;; > 4) test_put $ASCII_FILES;; > esac > } > > test_get() > { > local sum1 sum2 > > for file in $*; do > { > echo "user $RUSER $PASSWD" > echo "$i" > echo "cd $TST_NET_DATAROOT" > echo "get $file" > echo "quit" > } | ftp -nv $RHOST > > sum1="$(ls -l $file | awk '{print $5}')" > sum2="$(ls -l $TST_NET_DATAROOT/$file | awk '{print $5}')" > rm -f $file > EXPECT_PASS "[ '$sum1' = '$sum2' ]" > done > } > > > Kind regards, > Petr
diff --git a/testcases/network/tcp_cmds/ftp/ftp01.sh b/testcases/network/tcp_cmds/ftp/ftp01.sh index 8d23abc62..9900ad491 100755 --- a/testcases/network/tcp_cmds/ftp/ftp01.sh +++ b/testcases/network/tcp_cmds/ftp/ftp01.sh @@ -40,6 +40,10 @@ # #---------------------------------------------------------------------- +TST_TESTFUNC=do_test +TST_SETUP=do_setup +TST_NEEDS_TMPDIR=1 + #----------------------------------------------------------------------- # # FUNCTION: do_setup @@ -50,25 +54,15 @@ do_setup() { TC=ftp - TCtmp=${TCtmp:-$LTPROOT/$TC${EXEC_SUFFIX}$$} - TCdat=${TCdat:-$LTPROOT/datafiles} SLEEPTIME=${SLEEPTIME:-0} ASCII_FILES=${ASCII_FILES:-"ascii.sm ascii.med ascii.lg ascii.jmb"} BIN_FILES=${BIN_FILES:-"bin.sm bin.med bin.lg bin.jmb"} - RHOST=${RHOST:-`hostname`} RUSER=${RUSER:-root} - PASSWD=${PASSWD:-.pasroot} - - tst_setup - - exists awk ftp rsh - cd "$TCtmp" + tst_require_cmds awk ftp - rsh -n -l root $RHOST mkdir -p "$TCtmp" - rsh -n -l root $RHOST chown -R ${RUSER} "$TCtmp" - [ $? = 0 ] || end_testcase "Check .rhosts file on remote machine." + [ $? = 0 ] || tst_brk TBROK "Check .rhosts file on remote machine." } @@ -95,51 +89,39 @@ do_test() if [ $a = "get" ]; then { echo user $RUSER $PASSWD - echo lcd $TCtmp echo $i - echo cd $TCdat + echo cd $TST_NET_DATAROOT echo $a $j echo quit } | ftp -nv $RHOST - SUM1=`ls -l $TCtmp/$j | awk '{print $5}'` - SUM2=`ls -l $TCdat/$j | awk '{print $5}'` - rm -f $TCtmp/$j + SUM1=`ls -l $j | awk '{print $5}'` + SUM2=`ls -l $TST_NET_DATAROOT/$j | awk '{print $5}'` + rm -f $j else { echo user $RUSER $PASSWD - echo lcd $TCdat + echo lcd $TST_NET_DATAROOT echo $i - echo cd $TCtmp + echo cd $TST_TMPDIR echo $a $j echo quit } | ftp -nv $RHOST - SUM1=`rsh -n -l root $RHOST sum $TCtmp/$j | awk '{print $1}'` - SUM2=`sum $TCdat/$j | awk '{print $1}'` - rsh -n -l root $RHOST rm -f $TCtmp/$j + SUM1=`tst_rhost_run -c "sum $TST_TMPDIR/$j" -s | awk '{print $1}'` + SUM2=`sum $TST_NET_DATAROOT/$j | awk '{print $1}'` + tst_rhost_run -c "rm -f $TST_TMPDIR/$j" fi if [ $SUM1 = $SUM2 ]; then - tst_resm TINFO "Test Successful doing ftp $a $j $i" + tst_res TINFO "Test Successful doing ftp $a $j $i" else - end_testcase "Test Fail: Wrong sum while performing ftp $a $j $i" + tst_brk TFAIL "Test Fail: Wrong sum while performing ftp $a $j $i" fi sleep $SLEEPTIME done done done -} - -#----------------------------------------------------------------------- -# -# FUNCTION: do_cleanup -# -#----------------------------------------------------------------------- - -do_cleanup() -{ - rsh -n -l root $RHOST rmdir "$TCtmp" - tst_cleanup + tst_res TPASS "Test Successful" } #---------------------------------------------------------------------- @@ -150,9 +132,6 @@ do_cleanup() # OUTPUT: A testcase run log with the results of the execution of this # test. #---------------------------------------------------------------------- -. net_cmdlib.sh +. tst_net.sh -read_opts $* -do_setup -do_test -end_testcase +tst_run
This allows to use SSH rather than RSH. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- testcases/network/tcp_cmds/ftp/ftp01.sh | 61 ++++++++----------------- 1 file changed, 20 insertions(+), 41 deletions(-)