| Message ID | 20230126215401.29101-7-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | tst_net.sh fixes + cleanup | expand |
Hi! What I didn't get here is why do we put the code to detect IPV6 into two different functions and then call it twice nearly from the same place in the library. Is there a reason why can't we put the sysctl check right after the proc check in the current ipv6 detection function?
Hi Cyril, first, thanks for you review! > Hi! > What I didn't get here is why do we put the code to detect IPV6 into two > different functions and then call it twice nearly from the same place in > the library. Is there a reason why can't we put the sysctl check right > after the proc check in the current ipv6 detection function? Well, the old check (cmdline check for ipv6.disable=1) must be run before creating interfaces. But the new sysctl based tests test interfaces (${L,R}HOST_IFACES). Technically it could be run just after ipv6.disable=1 check, because it's needed only for the non-default ssh based two host setup [1]. But ${L,R}HOST_IFACES initialization: export LHOST_IFACES="${LHOST_IFACES:-eth0}" export RHOST_IFACES="${RHOST_IFACES:-eth0}" would have to be moved before this check and run only for ssh based test (the default netns based single host will create interfaces in init_ltp_netspace() and for them ipv6.disable=1 check is good enough). I wanted to keep variables in single place, but I'm ok to add this exception. Or we can keep variables where they are and run this check on ${LHOST_IFACES:-} and ${RHOST_IFACES:-} (i.e. only if these interfaces are set externally - which should be only on ssh two host based setup). I also thought that having to run this new check separately could be useful (setup can change any time). But as nobody required that /complained it's needed, let's ignore this. Kind regards, Petr [1] https://github.com/linux-test-project/ltp/tree/master/testcases/network#single-host-configuration
Hi! > > What I didn't get here is why do we put the code to detect IPV6 into two > > different functions and then call it twice nearly from the same place in > > the library. Is there a reason why can't we put the sysctl check right > > after the proc check in the current ipv6 detection function? > > Well, the old check (cmdline check for ipv6.disable=1) must be run before > creating interfaces. But the new sysctl based tests test interfaces > (${L,R}HOST_IFACES). Technically it could be run just after ipv6.disable=1 > check, because it's needed only for the non-default ssh based two host setup [1]. > But ${L,R}HOST_IFACES initialization: > > export LHOST_IFACES="${LHOST_IFACES:-eth0}" > export RHOST_IFACES="${RHOST_IFACES:-eth0}" > > would have to be moved before this check and run only for ssh based test > (the default netns based single host will create interfaces in init_ltp_netspace() > and for them ipv6.disable=1 check is good enough). I wanted to keep variables in > single place, but I'm ok to add this exception. Or we can keep variables where > they are and run this check on ${LHOST_IFACES:-} and ${RHOST_IFACES:-} (i.e. > only if these interfaces are set externally - which should be only on ssh two > host based setup). > > I also thought that having to run this new check separately could be useful > (setup can change any time). But as nobody required that /complained it's > needed, let's ignore this. Ah, it's more complicated than a single switch, there is actually a master swtich, default settings and per interface settings. i.e. /proc/sys/net/ipv6/conf/default/disable_ipv6 /proc/sys/net/ipv6/conf/all/disable_ipv6 /proc/sys/net/ipv6/conf/${IFACE}/disable_ipv6 Since the deafult value is I suppose inherited by newly added interfaces we have to check if ipv6 is disabled by sysctl in the case of network namespaces as well, right? Technically we could probably check the default/disable_ipv6 before we attempt to create new interfaces, but I guess that we woudl rather want to have the same code both for ssh and namespaces.
> Hi! > > > What I didn't get here is why do we put the code to detect IPV6 into two > > > different functions and then call it twice nearly from the same place in > > > the library. Is there a reason why can't we put the sysctl check right > > > after the proc check in the current ipv6 detection function? > > Well, the old check (cmdline check for ipv6.disable=1) must be run before > > creating interfaces. But the new sysctl based tests test interfaces > > (${L,R}HOST_IFACES). Technically it could be run just after ipv6.disable=1 > > check, because it's needed only for the non-default ssh based two host setup [1]. > > But ${L,R}HOST_IFACES initialization: > > export LHOST_IFACES="${LHOST_IFACES:-eth0}" > > export RHOST_IFACES="${RHOST_IFACES:-eth0}" > > would have to be moved before this check and run only for ssh based test > > (the default netns based single host will create interfaces in init_ltp_netspace() > > and for them ipv6.disable=1 check is good enough). I wanted to keep variables in > > single place, but I'm ok to add this exception. Or we can keep variables where > > they are and run this check on ${LHOST_IFACES:-} and ${RHOST_IFACES:-} (i.e. > > only if these interfaces are set externally - which should be only on ssh two > > host based setup). > > I also thought that having to run this new check separately could be useful > > (setup can change any time). But as nobody required that /complained it's > > needed, let's ignore this. > Ah, it's more complicated than a single switch, there is actually a > master swtich, default settings and per interface settings. > i.e. /proc/sys/net/ipv6/conf/default/disable_ipv6 > /proc/sys/net/ipv6/conf/all/disable_ipv6 > /proc/sys/net/ipv6/conf/${IFACE}/disable_ipv6 > Since the deafult value is I suppose inherited by newly added interfaces > we have to check if ipv6 is disabled by sysctl in the case of network > namespaces as well, right? Technically we could probably check the > default/disable_ipv6 before we attempt to create new interfaces, but I > guess that we would rather want to have the same code both for ssh and > namespaces. Good catch, yes, conf/default/disable_ipv6 is inherited, thus mandatory for check altogether with conf/${IFACE}/disable_ipv6 (for ${LHOST_IFACES:-} and ${RHOST_IFACES:-} - ssh). We can ignore conf/all/disable_ipv6 [1]: Reading this value does not have any particular meaning. It does not say whether IPv6 support is enabled or disabled. Returned value can be 1 also in the case when some interface has disable_ipv6 set to 0 and has configured IPv6 addresses. I'd add this check before calling init_ltp_netspace(). That expects: 1) nobody changes this setup (one day we may want to test this functionality, thus even LTP itself can change this setup) 2) any NIC without IPv6 means no IPv6 testing, right? BTW at least network tests which use virt_lib.sh requires IPv6 for IPv4 tests. ip6_virt_remote="$(TST_IPV6=6 tst_ipaddr_un rhost)" which is then used in virt_setup(): # ./macsec02.sh macsec02 1 TINFO: setup IPsec transport/esp_aead des3_ede macsec02 1 TINFO: setup local macsec with 'icvlen 16 encodingsa 0 replay on window 300 encrypt on protect on' macsec02 1 TINFO: setup rhost macsec with 'icvlen 16 encodingsa 0 replay on window 300 encrypt on protect on' RTNETLINK answers: Operation not supported macsec02 1 TBROK: ip addr add ::2/64 dev ltp_v0 nodad failed # ./vlan03.sh RTNETLINK answers: Operation not supported vlan03 1 TBROK: ip addr add ::2/64 dev ltp_v0 nodad failed These tests would have to be either split to IPv4 (default) and IPv6 (-6) or entirely depend on IPv6 (obviously first is better, but more work). Kind regards, Petr [1] https://docs.kernel.org/networking/ip-sysctl.html
Hi Cyril, ... > Ah, it's more complicated than a single switch, there is actually a > master swtich, default settings and per interface settings. > i.e. /proc/sys/net/ipv6/conf/default/disable_ipv6 > /proc/sys/net/ipv6/conf/all/disable_ipv6 > /proc/sys/net/ipv6/conf/${IFACE}/disable_ipv6 > Since the deafult value is I suppose inherited by newly added interfaces > we have to check if ipv6 is disabled by sysctl in the case of network > namespaces as well, right? Technically we could probably check the > default/disable_ipv6 before we attempt to create new interfaces, but I > guess that we woudl rather want to have the same code both for ssh and > namespaces. In the end I decided to add check for net.ipv6.conf.all.disable_ipv6=1 in tst_init_iface() (calls ip link set ...). This way it should be tested only when needed. That means we need 2 functions. I wonder if it still makes sense to check net.ipv6.conf.$iface.disable_ipv6 (IMHO needed for two host based setup with IPv6 non-friendly configuration). I'd be for this check, but it could be in function which checks cmdline. Kind regards, Petr
diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh index 6cb9f02a5f..3a9c16f070 100644 --- a/testcases/lib/tst_net.sh +++ b/testcases/lib/tst_net.sh @@ -100,7 +100,10 @@ tst_brk_() [ -z "$TST_USE_LEGACY_API" ] && tst_brk $@ || tst_brkm $@ } -tst_net_detect_ipv6() +# detect IPv6 not disabled via ipv6.disable=1 kernel cmdline parameter +# or via CONFIG_IPV6=y kernel configure option +# $TST_NET_IPV6_ENABLED: 1 (enabled), 0 (disabled) +tst_net_detect_ipv6_cmdline() { local type="${1:-lhost}" local cmd='[ -f /proc/net/if_inet6 ]' @@ -970,7 +973,7 @@ tst_default_max_pkt() [ -n "$TST_USE_LEGACY_API" ] && . test.sh || . tst_test.sh # detect IPv6 support on lhost for tests which don't use test links -tst_net_detect_ipv6 +tst_net_detect_ipv6_cmdline [ -n "$TST_NET_SKIP_VARIABLE_INIT" ] && return 0 @@ -1007,7 +1010,7 @@ tst_require_cmds tst_net_iface_prefix tst_net_ip_prefix tst_net_vars eval $(tst_net_ip_prefix $IPV4_LHOST || echo "exit $?") eval $(tst_net_ip_prefix -r $IPV4_RHOST || echo "exit $?") -[ "$TST_NET_IPV6_ENABLED" = 1 ] && tst_net_detect_ipv6 rhost +[ "$TST_NET_IPV6_ENABLED" = 1 ] && tst_net_detect_ipv6_cmdline rhost if [ "$TST_NET_IPV6_ENABLED" = 1 ]; then eval $(tst_net_ip_prefix $IPV6_LHOST || echo "exit $?")
Rename tst_net_detect_ipv6() to tst_net_detect_ipv6_cmdline() because there will be another function to detect IPv6 disabled via sysctl. Signed-off-by: Petr Vorel <pvorel@suse.cz> --- testcases/lib/tst_net.sh | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)