| Message ID | 20221020120741.212671-1-akihiko.odaki@daynix.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/6] ftp/ftp01: Use tst_net.sh | expand |
Hi Akihiko,
we do this rewrite in a single commit. It does not make much sense to split it.
I'd squash it, if it were ready, but we're not there yet.
Also, you're supposed to replace GPL verbose text at the top, history etc.
with just:
#!/bin/sh
# SPDX-License-Identifier: GPL-2.0-or-later
# Copyright (c) 2022 Akihiko Odaki <akihiko.odaki@daynix.com>
# Copyright (c) 2003 Manoj Iyer <manjo@mail.utexas.edu>
# Copyright (c) 2001 Robbie Williamson <robbiew@us.ibm.com>
TST_TESTFUNC=do_test
TST_CNT=4
...
You forget TST_CNT=4, thus only single test is being run.
i.e. all the useless and now even outdated comments (even mentioning .rhosts, history, setup)
must go away.
The main problem is, that with improperly installed data files test happily
passes, because compares empty strings:
ftp01 1 TINFO: timeout per run is 0h 5m 0s
Not connected.
Not connected.
Not connected.
Not connected.
ls: cannot access 'ascii.sm': No such file or directory
ls: cannot access '/opt/ltp/testcases/bin/datafiles/ascii.sm': No such file or directory
ftp01 1 TPASS: [ '' = '' ] passed as expected
Not connected.
Not connected.
Not connected.
Not connected.
ls: cannot access 'ascii.med': No such file or directory
ls: cannot access '/opt/ltp/testcases/bin/datafiles/ascii.med': No such file or directory
ftp01 1 TPASS: [ '' = '' ] passed as expected
Not connected.
Not connected.
Not connected.
Not connected.
ls: cannot access 'ascii.lg': No such file or directory
ls: cannot access '/opt/ltp/testcases/bin/datafiles/ascii.lg': No such file or directory
ftp01 1 TPASS: [ '' = '' ] passed as expected
Not connected.
Not connected.
Not connected.
Not connected.
ls: cannot access 'ascii.jmb': No such file or directory
ls: cannot access '/opt/ltp/testcases/bin/datafiles/ascii.jmb': No such file or directory
ftp01 1 TPASS: [ '' = '' ] passed as expected
Summary:
passed 4
failed 0
broken 0
skipped 0
warnings 0
When installing it properly, it does not work:
ftp01 1 TINFO: timeout per run is 0h 5m 0s
Not connected.
Not connected.
Not connected.
Not connected.
ls: cannot access 'ascii.sm': No such file or directory
ftp01 1 TFAIL: [ '' = '220' ] failed unexpectedly
Not connected.
Not connected.
Not connected.
Not connected.
ls: cannot access 'ascii.med': No such file or directory
ftp01 1 TFAIL: [ '' = '4020' ] failed unexpectedly
Not connected.
Not connected.
Not connected.
Not connected.
ls: cannot access 'ascii.lg': No such file or directory
ftp01 1 TFAIL: [ '' = '80020' ] failed unexpectedly
Not connected.
Not connected.
Not connected.
Not connected.
ls: cannot access 'ascii.jmb': No such file or directory
ftp01 1 TFAIL: [ '' = '1600020' ] failed unexpectedly
ftp01 2 TINFO: AppArmor enabled, this may affect test results
ftp01 2 TINFO: it can be disabled with TST_DISABLE_APPARMOR=1 (requires super/root)
ftp01 2 TINFO: loaded AppArmor profiles: none
Summary:
passed 0
failed 4
broken 0
skipped 0
warnings 0
First, there should be some check for $sum1 or $sum2 not being empty,
and probably if the file exists (probably in the setup).
The reason is in your version of setup function:
do_setup()
{
TC=ftp
RUSER=${RUSER:-root}
}
Here you have
* empty lines (remove it)
* TC=ftp is useless (remove it)
* RUSER could be defined above the setup function
(more readable, no need to be in the setup function)
But somehow cd into $TST_NET_DATAROOT in ftp code does not work.
I've tried to copy it instead:
do_setup()
{
local file
for file in $ASCII_FILES $BINARY_FILES; do
ROD cp -v $TST_NET_DATAROOT/$file .
done
}
But that does not work. I need some time to figure out what's wrong
(not an FTP expert).
Other things to fix:
Also file variable in test_get() and test_put() should be also declared with
local.
I don't like list_files() at all, why not just define
at the top of the test and then use them as I suggested before?
ASCII_FILES='bin.sm bin.med bin.lg bin.jmb'
BINARY_FILES='ascii.sm ascii.med ascii.lg ascii.jmb'
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
}
Also, thinking about the tool which gets the size. Looking at
https://stackoverflow.com/questions/1815329/portable-way-to-get-file-size-in-bytes-in-the-shell
https://unix.stackexchange.com/questions/16640/how-can-i-get-the-size-of-a-file-in-a-bash-script
'ls -l | awk '{print $5}' might be really the best tool to get file size as both
ls and awk are probably everywhere. And although we wrote some C code to avoid
external dependencies, here I'd keep it, because other alternatives has
drawbacks:
'du -b' is fast, but might not be everywhere.
'stat -c %s' is reported not to be portable:
https://stackoverflow.com/questions/1815329/portable-way-to-get-file-size-in-bytes-in-the-shell#comment21604978_5258707
'wc -c' is reported to be slow and also might not be everywhere:
https://stackoverflow.com/questions/1815329/portable-way-to-get-file-size-in-bytes-in-the-shell#comment21604978_5258707
'find "$file" -printf "%s"' would also work, but might not be everywhere.
I'll try to fix the problem, but not sure when I get time to fix it.
I've added all fixes into my fork, feel free to use it if you have time to post
new version. But please, test it before whether it works.
https://github.com/pevik/ltp/commits/akihiko_odaki/ftp01.v2.fixes
Kind regards,
Petr
Hi Akihiko, I'm sorry I overlooked your message at v1 https://lore.kernel.org/ltp/9894db50-6319-a818-c995-3ba9ab102c4b@daynix.com/ I guess script expects $PASSWD to be set. Also trying to follow the instruction, none of them (allowing root in /etc/ftpusers or filling /root/.netrc with machine localhost) work for me: https://github.com/linux-test-project/ltp/tree/master/testcases/network#ftp-and-telnet-setup I suppose any ftpd implementation must be run on the remote server (localhost, if tst_net.sh uses netns - the default or on remote host, if ssh is used). Otherwise ftp command "user $RUSER $PASSWD" cannot work, right? Although it should behave the same regardless netns or ssh is used, how do you test v3, with ssh or with netns (the default)? Or both? It'd be nice if there was connection detection in the setup and tst_brk TCONF if login does not work. The best would be to have configuration in the setup() + cleanup in the cleanup()). There are tests, which do vsftpd configuration: ftp-download-stress.sh, ftp-upload-stress.sh. BTW in the past we seriously considered to delete these highlevel smoke tests. LTP is concentrated to test kernel API and internals, thus we didn't see much value with smoke tests like this (that's why they haven't been rewritten to use new API), specially if they require complex setup and get false positives when SUT not configured properly. That's why it'd be nice to at least TCONF (if not doing whole setup). Kind regards, Petr
Hi Petr, On 2022/10/21 15:22, Petr Vorel wrote: > Hi Akihiko, > > I'm sorry I overlooked your message at v1 > https://lore.kernel.org/ltp/9894db50-6319-a818-c995-3ba9ab102c4b@daynix.com/ > > I guess script expects $PASSWD to be set. > Also trying to follow the instruction, none of them (allowing root in > /etc/ftpusers or filling /root/.netrc with machine localhost) work for me: > https://github.com/linux-test-project/ltp/tree/master/testcases/network#ftp-and-telnet-setup > > I suppose any ftpd implementation must be run on the remote server (localhost, > if tst_net.sh uses netns - the default or on remote host, if ssh is used). > Otherwise ftp command "user $RUSER $PASSWD" cannot work, right? > > Although it should behave the same regardless netns or ssh is used, > how do you test v3, with ssh or with netns (the default)? Or both? Thanks for sharing your fork. I tried it with SSH, and I had to make the following modifications to run it correctly: - I had to restore commented out "cd $TST_NET_DATAROOT". - I had to change the arguments of test_get and test_put back to "binary" and "ascii". It is used to set the transfer mode (search for "echo $1"). > > It'd be nice if there was connection detection in the setup and tst_brk TCONF if > login does not work. The best would be to have configuration in the setup() + > cleanup in the cleanup()). > > There are tests, which do vsftpd configuration: ftp-download-stress.sh, > ftp-upload-stress.sh. > > BTW in the past we seriously considered to delete these highlevel smoke tests. > LTP is concentrated to test kernel API and internals, thus we didn't see much > value with smoke tests like this (that's why they haven't been rewritten to use > new API), specially if they require complex setup and get false positives when > SUT not configured properly. That's why it'd be nice to at least TCONF (if not > doing whole setup). I just modified this test because it is annoying to set up rsh just to fix this test so I would rather not put more effort for further improvement. Personally I don't object to remove this test either. Regards, Akihiko Odaki > > Kind regards, > Petr
Hi Akihiko, > Hi Petr, > On 2022/10/21 15:22, Petr Vorel wrote: > > Hi Akihiko, > > I'm sorry I overlooked your message at v1 > > https://lore.kernel.org/ltp/9894db50-6319-a818-c995-3ba9ab102c4b@daynix.com/ > > I guess script expects $PASSWD to be set. > > Also trying to follow the instruction, none of them (allowing root in > > /etc/ftpusers or filling /root/.netrc with machine localhost) work for me: > > https://github.com/linux-test-project/ltp/tree/master/testcases/network#ftp-and-telnet-setup > > I suppose any ftpd implementation must be run on the remote server (localhost, > > if tst_net.sh uses netns - the default or on remote host, if ssh is used). > > Otherwise ftp command "user $RUSER $PASSWD" cannot work, right? > > Although it should behave the same regardless netns or ssh is used, > > how do you test v3, with ssh or with netns (the default)? Or both? > Thanks for sharing your fork. I tried it with SSH, and I had to make the > following modifications to run it correctly: > - I had to restore commented out "cd $TST_NET_DATAROOT". Ack, this is correct. > - I had to change the arguments of test_get and test_put back to "binary" > and "ascii". > It is used to set the transfer mode (search for "echo $1"). Ah, I'm sorry I overlooked this. This could be solved with passing it as a first parameter and shift, but I'll stop to be picky on an implementation detail. > > It'd be nice if there was connection detection in the setup and tst_brk TCONF if > > login does not work. The best would be to have configuration in the setup() + > > cleanup in the cleanup()). > > There are tests, which do vsftpd configuration: ftp-download-stress.sh, > > ftp-upload-stress.sh. > > BTW in the past we seriously considered to delete these highlevel smoke tests. > > LTP is concentrated to test kernel API and internals, thus we didn't see much > > value with smoke tests like this (that's why they haven't been rewritten to use > > new API), specially if they require complex setup and get false positives when > > SUT not configured properly. That's why it'd be nice to at least TCONF (if not > > doing whole setup). > I just modified this test because it is annoying to set up rsh just to fix > this test so I would rather not put more effort for further improvement. Understand, ack. Thanks for your work! > Personally I don't object to remove this test either. The fastest solution is to merge your fixed version. But there should be even more modifications: RHOST is obsolete, instead $(tst_ipaddr rhost) should be used. Therefore we should decide if this smoke test (and other FTP tests in LTP) is worth effort. If yes, I should force myself to create ftp_lib.sh and migrate everything to work similar way as ftp-upload-stress.sh. Kind regards, Petr > Regards, > Akihiko Odaki > > Kind regards, > > Petr
Hi Akihiko, ... > > I just modified this test because it is annoying to set up rsh just to fix > > this test so I would rather not put more effort for further improvement. > Understand, ack. Thanks for your work! > > Personally I don't object to remove this test either. > The fastest solution is to merge your fixed version. > But there should be even more modifications: > RHOST is obsolete, instead $(tst_ipaddr rhost) should be used. > Therefore we should decide if this smoke test (and other FTP tests in LTP) is > worth effort. If yes, I should force myself to create ftp_lib.sh and migrate > everything to work similar way as ftp-upload-stress.sh. Merged as it's some improvement. I'm not sure if I invest time to FTP in the future, maybe we should really delete it. The only significant change I did was to force running over SSH: with RHOST="${RHOST:-localhost}" In my case only first half of the tests is working (suppose just wrong setup), but on netns everything si broken and you also tested it on SSH I dared to do this change. I documented the proper fix above in case anybody cares. Kind regards, Petr
Hi, Thanks for suggestions, improvements, and merging. This test assumes FTP is set up and running and that defeats the purpose of netns. It is certainly possible to say the FTP test functionality is covered with testcases/network/stress/ftp and this test can be removed, but for now, the fixes allows me to execute net.tcp_cmds tests without explicitly excluding this. Regards, Akihiko Odaki On 2022/10/24 19:18, Petr Vorel wrote: > Hi Akihiko, > > ... >>> I just modified this test because it is annoying to set up rsh just to fix >>> this test so I would rather not put more effort for further improvement. >> Understand, ack. Thanks for your work! > >>> Personally I don't object to remove this test either. >> The fastest solution is to merge your fixed version. >> But there should be even more modifications: >> RHOST is obsolete, instead $(tst_ipaddr rhost) should be used. > >> Therefore we should decide if this smoke test (and other FTP tests in LTP) is >> worth effort. If yes, I should force myself to create ftp_lib.sh and migrate >> everything to work similar way as ftp-upload-stress.sh. > > Merged as it's some improvement. I'm not sure if I invest time to FTP in the > future, maybe we should really delete it. > > The only significant change I did was to force running over SSH: > with RHOST="${RHOST:-localhost}" > > In my case only first half of the tests is working (suppose just wrong setup), > but on netns everything si broken and you also tested it on SSH I dared to do > this change. I documented the proper fix above in case anybody cares. > > Kind regards, > Petr
Hi Akihiko, > Hi, > Thanks for suggestions, improvements, and merging. > This test assumes FTP is set up and running and that defeats the purpose of > netns. It is certainly possible to say the FTP test functionality is covered > with testcases/network/stress/ftp and this test can be removed, but for now, > the fixes allows me to execute net.tcp_cmds tests without explicitly > excluding this. Thanks a lot for confirming that it works with my additional changes. IMHO ftp can be tested via, that's actually done in ftp-upload-stress.sh. Kind regards, Petr > Regards, > Akihiko Odaki > On 2022/10/24 19:18, Petr Vorel wrote: > > Hi Akihiko, > > ... > > > > I just modified this test because it is annoying to set up rsh just to fix > > > > this test so I would rather not put more effort for further improvement. > > > Understand, ack. Thanks for your work! > > > > Personally I don't object to remove this test either. > > > The fastest solution is to merge your fixed version. > > > But there should be even more modifications: > > > RHOST is obsolete, instead $(tst_ipaddr rhost) should be used. > > > Therefore we should decide if this smoke test (and other FTP tests in LTP) is > > > worth effort. If yes, I should force myself to create ftp_lib.sh and migrate > > > everything to work similar way as ftp-upload-stress.sh. > > Merged as it's some improvement. I'm not sure if I invest time to FTP in the > > future, maybe we should really delete it. > > The only significant change I did was to force running over SSH: > > with RHOST="${RHOST:-localhost}" > > In my case only first half of the tests is working (suppose just wrong setup), > > but on netns everything si broken and you also tested it on SSH I dared to do > > this change. I documented the proper fix above in case anybody cares. > > Kind regards, > > Petr
diff --git a/testcases/network/tcp_cmds/ftp/ftp01.sh b/testcases/network/tcp_cmds/ftp/ftp01.sh index 8d23abc62..a78d8adc0 100755 --- a/testcases/network/tcp_cmds/ftp/ftp01.sh +++ b/testcases/network/tcp_cmds/ftp/ftp01.sh @@ -40,6 +40,11 @@ # #---------------------------------------------------------------------- +TST_TESTFUNC=do_test +TST_SETUP=do_setup +TST_NEEDS_CMDS='awk ftp' +TST_NEEDS_TMPDIR=1 + #----------------------------------------------------------------------- # # FUNCTION: do_setup @@ -50,25 +55,11 @@ 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" - - 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." } @@ -95,53 +86,35 @@ 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" - else - end_testcase "Test Fail: Wrong sum while performing ftp $a $j $i" - fi + EXPECT_PASS "[ '$SUM1' = '$SUM2' ]" sleep $SLEEPTIME done done done } - -#----------------------------------------------------------------------- -# -# FUNCTION: do_cleanup -# -#----------------------------------------------------------------------- - -do_cleanup() -{ - rsh -n -l root $RHOST rmdir "$TCtmp" - tst_cleanup -} - #---------------------------------------------------------------------- # FUNCTION: MAIN # PURPOSE: To invoke the functions to perform the tasks described in @@ -150,9 +123,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 | 64 +++++++------------------ 1 file changed, 17 insertions(+), 47 deletions(-)