Message ID | 20180517090711.10577-1-asmorodskyi@suse.com |
---|---|
State | Changes Requested |
Delegated to: | Petr Vorel |
Headers | show |
Series | [v3,1/1] ipneigh : Use new API | expand |
Hi Anton, > Besides all obvious changes for moving to new API, > also was done : > 1. more generic variable names > 2. add check for del command failure > 3. add input parameter "-c" which allows to control > which command will be used > --- > runtest/net.ipv6 | 2 +- > runtest/net.tcp_cmds | 3 +- > testcases/network/tcp_cmds/ipneigh/ipneigh01.sh | 82 +++++++++++++++---------- > 3 files changed, 53 insertions(+), 34 deletions(-) > diff --git a/runtest/net.ipv6 b/runtest/net.ipv6 > index d8f85cc31..261a7254c 100644 > --- a/runtest/net.ipv6 > +++ b/runtest/net.ipv6 > @@ -7,4 +7,4 @@ tracepath601 tracepath01.sh -6 > traceroute601 traceroute01.sh -6 > dhcpd6 dhcpd_tests.sh -6 > dnsmasq6 dnsmasq_tests.sh -6 > -ipneigh601 ipneigh01.sh -6 > +ipneigh6_ip ipneigh01.sh -6 -c ip > diff --git a/runtest/net.tcp_cmds b/runtest/net.tcp_cmds ... > -TCID=ipneigh01 > NUMLOOPS=${NUMLOOPS:-50} > -TST_TOTAL=2 > -TST_USE_LEGACY_API=1 do_setup() function is not run, there is TST_SETUP variable to tell API it's the setup function (it's called before testing): TST_SETUP="do_setup" > +TST_TESTFUNC=do_test > +TST_OPTS=":c" This is wrong, it supposed to be: TST_OPTS="c:" TST_OPTS definition working as normal option-string for getopts. ':' as very first character of the option-string silent error message and -c then does not expect parameter, so tests TBROK: ipneigh01 1 TBROK: Unexpected positional arguments 'ip' > +TST_PARSE_ARGS="parse_args" > +TST_USAGE="usage" > . tst_net.sh > +usage() > +{ > + echo "-c Test command (ip, $IF_CMD)" if-lib.sh has slightly different behavior ($IF_CMD is specific to if-lib.sh), use something like: echo "-c [ arp | ip ] Test command" > +} > + > +parse_args() > +{ > + case $1 in > + c) IP_CMD="$2";; It'd be better to use more generic variable, like $CMD. > + esac > +} > + > + > do_setup() > { > tst_require_root This is wrong, I overlook it in v2. New API uses TST_NEEDS_ROOT=1 variable (put before loading tst_net.sh) > - tst_check_cmds arp grep ping$TST_IPV6 > + tst_check_cmds arp grep grep is quite common, we don't check it And, Please, change arp to $CMD. tst_check_cmds arp > } > do_test() > { > - local arp_show_cmd="$1" > - local arp_del_cmd="$2" > + local rhost=$(tst_ipaddr rhost) > + case $IP_CMD in > + ip) > + local show_cmd="ip neigh show" > + local del_cmd="ip neigh del $rhost dev $(tst_iface)" > + ;; > + arp) > + if [ -n "$TST_IPV6" ] then ^ Again missing semicolon, breaks test. You can find these failures yourself if you run tests before posting patch. You can provide your own file with tests cases you want, put that file in /opt/ltp/runtest/ $ cat /opt/ltp/runtest/test ipneigh6_ip ipneigh01.sh -6 -c ip ipneigh01_arp ipneigh01.sh -c arp ipneigh01_ip ipneigh01.sh -c ip $ PASSWD=your-root-password /opt/ltp/testscripts/network.sh -f test > + tst_res TCONF "'arp cmd doesn't support IPv6, skipping test-case" > + return 1 > + fi > + local show_cmd="arp -a" > + local del_cmd="arp -d $rhost" > + ;; > + esac > local entry_name > [ "$TST_IPV6" ] && entry_name="NDISC" || entry_name="ARP" I prefer to use: local entry_name="ARP" [ "$TST_IPV6" ] && entry_name="NDISC" > - tst_resm TINFO "Stress auto-creation of $entry_name cache entry" > - tst_resm TINFO "by pinging '$rhost' and deleting entry again" > - tst_resm TINFO "with '$arp_del_cmd'" > + tst_res TINFO "Stress auto-creation of $entry_name cache entry" > + tst_res TINFO "by pinging '$rhost'" > + tst_res TINFO "and deleting entry again with '$del_cmd'" > + tst_res TINFO "in a loop with $NUMLOOPS iterations" Probably this would be enough: tst_res TINFO "Stress auto-creation of $entry_name cache entry $NUMLOOPS times" > for i in $(seq 1 $NUMLOOPS); do > - ping$TST_IPV6 -q -c1 $rhost > /dev/null > + tst_ping tst_ping is noisy as it reports TPASS, which is not purpose of this test. I'm sorry, can you put back the previous version, with TFAIL? ping$TST_IPV6 -q -c1 $rhost > /dev/null || \ tst_brk TFAIL "cannot ping $rhost" > local k > local ret=1 > - # wait for arp entry at least 3 seconds > for k in $(seq 1 30); do > - $arp_show_cmd | grep -q $rhost > + $show_cmd | grep -q $rhost > if [ $? -eq 0 ]; then > ret=0 > break; > @@ -57,28 +85,18 @@ do_test() > done > [ "$ret" -ne 0 ] && \ > - tst_brkm TFAIL "$entry_name entry '$rhost' not listed" > - > - $arp_del_cmd > - > - $arp_show_cmd | grep -q "${rhost}.*$(tst_hwaddr rhost)" && \ > - tst_brkm TFAIL "'$arp_del_cmd' failed, entry has " \ > + tst_brk TFAIL "$entry_name entry '$rhost' not listed" > + $del_cmd > + if [ $? -ne 0 ]; then > + tst_brk TFAIL "fail to delete entry" > + fi This can be shortened: $del_cmd || tst_brk TFAIL "fail to delete entry" ... Kind regards, Petr
Hi Petr > You can provide your own file with tests cases you want, put that file in /opt/ltp/runtest/ > $ cat /opt/ltp/runtest/test > ipneigh6_ip ipneigh01.sh -6 -c ip > ipneigh01_arp ipneigh01.sh -c arp > ipneigh01_ip ipneigh01.sh -c ip > > $ PASSWD=your-root-password /opt/ltp/testscripts/network.sh -f test thanks for advice , I also thought about something like this because I think that ipneigh term is not the best one and not showing what actually covered ( not only ip neighbour but also arp command ) and both cases it is just tools to delete line from arp table which is just one step in test flow and not sure if it is main one ? But I don't feel confident enough to came up with new name. And I would do what you suggesting here only if I will have better name for ipneigh01.sh script. Do you have some certain ideas how better to name it ? > >> - tst_resm TINFO "Stress auto-creation of $entry_name cache entry" >> - tst_resm TINFO "by pinging '$rhost' and deleting entry again" >> - tst_resm TINFO "with '$arp_del_cmd'" >> + tst_res TINFO "Stress auto-creation of $entry_name cache entry" >> + tst_res TINFO "by pinging '$rhost'" >> + tst_res TINFO "and deleting entry again with '$del_cmd'" >> + tst_res TINFO "in a loop with $NUMLOOPS iterations" > Probably this would be enough: > tst_res TINFO "Stress auto-creation of $entry_name cache entry $NUMLOOPS times" Why you think that 4 lines is too much ? I personally don't see a problem to give user a little more info and don't think that you can call it spaming ............. all other your suggestions are applied . after solving this two topics I will provide v4 ( hope the last one :D )
Hi Anton, > > You can provide your own file with tests cases you want, put that file in /opt/ltp/runtest/ > > $ cat /opt/ltp/runtest/test > > ipneigh6_ip ipneigh01.sh -6 -c ip > > ipneigh01_arp ipneigh01.sh -c arp > > ipneigh01_ip ipneigh01.sh -c ip > > $ PASSWD=your-root-password /opt/ltp/testscripts/network.sh -f test > thanks for advice , I also thought about something like this because I think > that ipneigh term is not the best one and not showing what actually covered > ( not only ip neighbour but also arp command ) and both cases it is just > tools to > delete line from arp table which is just one step in test flow and not sure > if it is main one ? But I don't feel confident enough to came up with new > name. And I would do what you suggesting here only if I will have better > name for ipneigh01.sh script. Do you have some certain ideas how better to > name it ? Command was renamed in a217233ab: network/tcp_cmds/arp01: rename to ipneigh01 'ipneigh' is more appropriate name for the test as it covers IPv4 and IPv6 cache entries with 'ip neigh' command and arp command is obsolete. --- I think that's valid point, I'd keep the name. One day arp part gets deleted. > > > - tst_resm TINFO "Stress auto-creation of $entry_name cache entry" > > > - tst_resm TINFO "by pinging '$rhost' and deleting entry again" > > > - tst_resm TINFO "with '$arp_del_cmd'" > > > + tst_res TINFO "Stress auto-creation of $entry_name cache entry" > > > + tst_res TINFO "by pinging '$rhost'" > > > + tst_res TINFO "and deleting entry again with '$del_cmd'" > > > + tst_res TINFO "in a loop with $NUMLOOPS iterations" > > Probably this would be enough: > > tst_res TINFO "Stress auto-creation of $entry_name cache entry $NUMLOOPS times" > Why you think that 4 lines is too much ? I personally don't see a problem to > give user a little more info and don't think that you can call it spaming I really thing that this is sufficient enough (I forgot to add the command before): tst_res TINFO "Stress auto-creation of $entry_name cache entry with '$del_cmd' $NUMLOOPS times" Why shorter lines: 1) This 4 lines is trying to express what the test does. Curious user should look into the test itself. LTP approach is to print command when they fail, otherwise be informative, but not verbose. 2) It's difficult to grep log message which is split, it's better to avoid it if possible) Kind regards, Petr
diff --git a/runtest/net.ipv6 b/runtest/net.ipv6 index d8f85cc31..261a7254c 100644 --- a/runtest/net.ipv6 +++ b/runtest/net.ipv6 @@ -7,4 +7,4 @@ tracepath601 tracepath01.sh -6 traceroute601 traceroute01.sh -6 dhcpd6 dhcpd_tests.sh -6 dnsmasq6 dnsmasq_tests.sh -6 -ipneigh601 ipneigh01.sh -6 +ipneigh6_ip ipneigh01.sh -6 -c ip diff --git a/runtest/net.tcp_cmds b/runtest/net.tcp_cmds index 859f48127..cfeacee5b 100644 --- a/runtest/net.tcp_cmds +++ b/runtest/net.tcp_cmds @@ -2,7 +2,8 @@ # # PLEASE READ THE README FILE IN /tcp_cmds BEFORE RUNNING THESE. # -ipneigh01 ipneigh01.sh +ipneigh01_arp ipneigh01.sh -c arp +ipneigh01_ip ipneigh01.sh -c ip arping01 arping01.sh clockdiff01 clockdiff01.sh ftp export TCbin=$LTPROOT/testcases/network/tcp_cmds/ftp; ftp01 diff --git a/testcases/network/tcp_cmds/ipneigh/ipneigh01.sh b/testcases/network/tcp_cmds/ipneigh/ipneigh01.sh index 9af3aa31e..52da61125 100755 --- a/testcases/network/tcp_cmds/ipneigh/ipneigh01.sh +++ b/testcases/network/tcp_cmds/ipneigh/ipneigh01.sh @@ -1,4 +1,5 @@ #!/bin/sh +# Copyright (c) 2018 SUSE Linux GmbH # Copyright (c) 2016 Oracle and/or its affiliates. All Rights Reserved. # Copyright (c) International Business Machines Corp., 2000 # This program is free software; you can redistribute it and/or @@ -16,39 +17,66 @@ # # Test basic functionality of 'arp' and 'ip neigh'. -TCID=ipneigh01 NUMLOOPS=${NUMLOOPS:-50} -TST_TOTAL=2 -TST_USE_LEGACY_API=1 +TST_TESTFUNC=do_test +TST_OPTS=":c" +TST_PARSE_ARGS="parse_args" +TST_USAGE="usage" . tst_net.sh +usage() +{ + echo "-c Test command (ip, $IF_CMD)" +} + +parse_args() +{ + case $1 in + c) IP_CMD="$2";; + esac +} + + do_setup() { tst_require_root - tst_check_cmds arp grep ping$TST_IPV6 + tst_check_cmds arp grep } do_test() { - local arp_show_cmd="$1" - local arp_del_cmd="$2" + local rhost=$(tst_ipaddr rhost) + case $IP_CMD in + ip) + local show_cmd="ip neigh show" + local del_cmd="ip neigh del $rhost dev $(tst_iface)" + ;; + arp) + if [ -n "$TST_IPV6" ] then + tst_res TCONF "'arp cmd doesn't support IPv6, skipping test-case" + return 1 + fi + local show_cmd="arp -a" + local del_cmd="arp -d $rhost" + ;; + esac local entry_name [ "$TST_IPV6" ] && entry_name="NDISC" || entry_name="ARP" - tst_resm TINFO "Stress auto-creation of $entry_name cache entry" - tst_resm TINFO "by pinging '$rhost' and deleting entry again" - tst_resm TINFO "with '$arp_del_cmd'" + tst_res TINFO "Stress auto-creation of $entry_name cache entry" + tst_res TINFO "by pinging '$rhost'" + tst_res TINFO "and deleting entry again with '$del_cmd'" + tst_res TINFO "in a loop with $NUMLOOPS iterations" for i in $(seq 1 $NUMLOOPS); do - ping$TST_IPV6 -q -c1 $rhost > /dev/null + tst_ping local k local ret=1 - # wait for arp entry at least 3 seconds for k in $(seq 1 30); do - $arp_show_cmd | grep -q $rhost + $show_cmd | grep -q $rhost if [ $? -eq 0 ]; then ret=0 break; @@ -57,28 +85,18 @@ do_test() done [ "$ret" -ne 0 ] && \ - tst_brkm TFAIL "$entry_name entry '$rhost' not listed" - - $arp_del_cmd - - $arp_show_cmd | grep -q "${rhost}.*$(tst_hwaddr rhost)" && \ - tst_brkm TFAIL "'$arp_del_cmd' failed, entry has " \ + tst_brk TFAIL "$entry_name entry '$rhost' not listed" + $del_cmd + if [ $? -ne 0 ]; then + tst_brk TFAIL "fail to delete entry" + fi + + $show_cmd | grep -q "${rhost}.*$(tst_hwaddr rhost)" && \ + tst_brk TFAIL "'$del_cmd' failed, entry has " \ "$(tst_hwaddr rhost)' $i/$NUMLOOPS" done - tst_resm TPASS "verified adding/removing of $entry_name cache entry" + tst_res TPASS "verified adding/removing of $entry_name cache entry" } -do_setup - -rhost=$(tst_ipaddr rhost) - -if [ -z "$TST_IPV6" ]; then - do_test "arp -a" "arp -d $rhost" -else - tst_resm TCONF "'arp cmd doesn't support IPv6, skipping test-case" -fi - -do_test "ip neigh show" "ip neigh del $rhost dev $(tst_iface)" - -tst_exit +tst_run \ No newline at end of file