[v3,1/1] ipneigh : Use new API

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(-)

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