[v2,2/3] broken_ip-nexthdr.sh: Check IPv6 support before forcing it

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/network/stress/broken_ip/broken_ip-nexthdr.sh | 1 +
 1 file changed, 1 insertion(+)

Hi!
> diff --git a/testcases/network/stress/broken_ip/broken_ip-nexthdr.sh b/testcases/network/stress/broken_ip/broken_ip-nexthdr.sh
> index ec6643af66..cb4a3dd399 100755
> --- a/testcases/network/stress/broken_ip/broken_ip-nexthdr.sh
> +++ b/testcases/network/stress/broken_ip/broken_ip-nexthdr.sh
> @@ -11,6 +11,7 @@ TST_TESTFUNC="do_test"
>  do_test()
>  {
>  	# not supported on IPv4
> +	tst_net_require_ipv6
>  	TST_IPV6=6
>  	TST_IPVER=6

I was looking at the code if we can simply instead do:

[ "$TST_IPVER" == 6 ] && tst_net_require_ipv6

in the test library, but it looks like the parameters are parsed in the
tst_test.sh in the tst_run() function. Which means that the TST_IPVER is
not actually set until the test starts and the library has no way of
knowing the variable value beforehand.

I guess that we can actually move the option parsing code in the
tst_test.sh so that it happens just right after the script is sourced,
which would make things much easier as the TST_IPVER would end up being
defined in the tst_network.sh and we coud simply use the statement above
without any further hacks like this patch adds.

As a side effect we could clean up the test option parsing code since we
do actually have two different getopts loop in the tst_test.sh library
and as far as I can tell we can do just with one.

Hi Cyril,

first, thanks a lot for a review of network shell tests.

> Hi!
> > diff --git a/testcases/network/stress/broken_ip/broken_ip-nexthdr.sh b/testcases/network/stress/broken_ip/broken_ip-nexthdr.sh
> > index ec6643af66..cb4a3dd399 100755
> > --- a/testcases/network/stress/broken_ip/broken_ip-nexthdr.sh
> > +++ b/testcases/network/stress/broken_ip/broken_ip-nexthdr.sh
> > @@ -11,6 +11,7 @@ TST_TESTFUNC="do_test"
> >  do_test()
> >  {
> >  	# not supported on IPv4
> > +	tst_net_require_ipv6
> >  	TST_IPV6=6
> >  	TST_IPVER=6

> I was looking at the code if we can simply instead do:

> [ "$TST_IPVER" == 6 ] && tst_net_require_ipv6
BTW this code was requested by Alexey, originally I suggested different way
to declare TST_NET_IPV{4,6}_ONLY=1 in the test:

https://lore.kernel.org/ltp/20210714140716.1568-3-pvorel@suse.cz/

> in the test library, but it looks like the parameters are parsed in the
> tst_test.sh in the tst_run() function. Which means that the TST_IPVER is
> not actually set until the test starts and the library has no way of
> knowing the variable value beforehand.
Yes.

> I guess that we can actually move the option parsing code in the
> tst_test.sh so that it happens just right after the script is sourced,
> which would make things much easier as the TST_IPVER would end up being
> defined in the tst_network.sh and we coud simply use the statement above
> without any further hacks like this patch adds.
Interesting. Originally I thought I'd need IPv6 check for netns_*.sh tests, but
in the end I realized that better will be if they use tst_net.sh with forcing
netns only.

Sure, I can have look into putting -6 handling into tst_test.sh. It's just a bit
strange to me that tst_test.sh parses it, but it's actually used elsewhere.

> As a side effect we could clean up the test option parsing code since we
> do actually have two different getopts loop in the tst_test.sh library
> and as far as I can tell we can do just with one.
When I looked last time into this and it looked to me that both are needed.
The second one (which is actually run the first - ":hi:$TST_OPTS" is for
ignoring errors when -h is set. But maybe these warnings ("Invalid number of
positional parameters:" and "Unexpected positional arguments" might be really
possible to handle inside of tst_run().

Kind regards,
Petr

Hi Cyril,

> Hi!
> > diff --git a/testcases/network/stress/broken_ip/broken_ip-nexthdr.sh b/testcases/network/stress/broken_ip/broken_ip-nexthdr.sh
> > index ec6643af66..cb4a3dd399 100755
> > --- a/testcases/network/stress/broken_ip/broken_ip-nexthdr.sh
> > +++ b/testcases/network/stress/broken_ip/broken_ip-nexthdr.sh
> > @@ -11,6 +11,7 @@ TST_TESTFUNC="do_test"
> >  do_test()
> >  {
> >  	# not supported on IPv4
> > +	tst_net_require_ipv6
> >  	TST_IPV6=6
> >  	TST_IPVER=6

> I was looking at the code if we can simply instead do:

> [ "$TST_IPVER" == 6 ] && tst_net_require_ipv6

> in the test library, but it looks like the parameters are parsed in the
> tst_test.sh in the tst_run() function. Which means that the TST_IPVER is
> not actually set until the test starts and the library has no way of
> knowing the variable value beforehand.

> I guess that we can actually move the option parsing code in the
> tst_test.sh so that it happens just right after the script is sourced,
> which would make things much easier as the TST_IPVER would end up being
> defined in the tst_network.sh and we coud simply use the statement above
> without any further hacks like this patch adds.

> As a side effect we could clean up the test option parsing code since we
> do actually have two different getopts loop in the tst_test.sh library
> and as far as I can tell we can do just with one.

I tried to implement this [1] (or [2] if I force push), and using getopts the
end of the script only would require also to move loading '. tst_test.sh' at the
end of *all* the test scripts because setup functions needs to be loaded before
sourcing tst_test.sh. Do we want this?

Kind regards,
Petr

[1] https://github.com/pevik/ltp/commit/9d77e5d964751ea3f0c8d22c4265c0041e2ebbc9
[2] https://github.com/pevik/ltp/commits/tst_test.sh/cleanup-getopts

Hi!
> I tried to implement this [1] (or [2] if I force push), and using getopts the
> end of the script only would require also to move loading '. tst_test.sh' at the
> end of *all* the test scripts because setup functions needs to be loaded before
> sourcing tst_test.sh. Do we want this?

Sounds reasonable. I would go even further and put the line that sources
the tst_test.sh to be last line in the test.

> Hi!
> > I tried to implement this [1] (or [2] if I force push), and using getopts the
> > end of the script only would require also to move loading '. tst_test.sh' at the
> > end of *all* the test scripts because setup functions needs to be loaded before
> > sourcing tst_test.sh. Do we want this?

> Sounds reasonable. I would go even further and put the line that sources
> the tst_test.sh to be last line in the test.
+1, I'll also update docs examples and mention it.

Kind regards,
Petr