[LTP] [PATCH] netns_helper: Make iproute version check work correctly

[LTP] [PATCH] netns_helper: Make iproute version check work correctly

[Feiyu Zhu]

Since the iproute2 patch[1]("replace SNAPSHOT with auto-generated version string"),
the output format of "ip -V" has changed form "ip utility, iproute2-ss*" to
"ip utility, iproute2-*", which leads to an exception when checking the version
of iproute. Use return to avoid unexpected TCONF.

[1]https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=fbef655568

Signed-off-by: Feiyu Zhu <zhufy.jy@cn.fujitsu.com>
---
 testcases/kernel/containers/netns/netns_helper.h  | 10 ++++++++--
 testcases/kernel/containers/netns/netns_helper.sh |  9 +++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/containers/netns/netns_helper.h b/testcases/kernel/containers/netns/netns_helper.h
index 8b87645..8337051 100644
--- a/testcases/kernel/containers/netns/netns_helper.h
+++ b/testcases/kernel/containers/netns/netns_helper.h
@@ -37,6 +37,7 @@ static void check_iproute(unsigned int spe_ipver)
 	FILE *ipf;
 	int n;
 	unsigned int ipver = 0;
+	char ver;
 
 	ipf = popen("ip -V", "r");
 	if (ipf == NULL)
@@ -44,7 +45,14 @@ static void check_iproute(unsigned int spe_ipver)
 				"Failed while opening pipe for iproute check");
 
 	n = fscanf(ipf, "ip utility, iproute2-ss%u", &ipver);
+	pclose(ipf);
 	if (n < 1) {
+		ipf = popen("ip -V", "r");
+		n = fscanf(ipf, "ip utility, iproute2-%s", &ver);
+		if (n >= 1) {
+			pclose(ipf);
+			return;
+		}
 		tst_brkm(TCONF, NULL,
 			"Failed while obtaining version for iproute check");
 	}
@@ -52,8 +60,6 @@ static void check_iproute(unsigned int spe_ipver)
 		tst_brkm(TCONF, NULL, "The commands in iproute tools do "
 			"not support required objects");
 	}
-
-	pclose(ipf);
 }
 
 static int dummy(void *arg)
diff --git a/testcases/kernel/containers/netns/netns_helper.sh b/testcases/kernel/containers/netns/netns_helper.sh
index a5b77a0..bec43ac 100755
--- a/testcases/kernel/containers/netns/netns_helper.sh
+++ b/testcases/kernel/containers/netns/netns_helper.sh
@@ -50,6 +50,15 @@ tst_check_iproute()
 	local cur_ipver="$(ip -V)"
 	local spe_ipver="$1"
 
+	echo $cur_ipver | grep "ip utility, iproute2-ss" > /dev/null
+	ret1=$?
+	echo $cur_ipver | grep "ip utility, iproute2-" > /dev/null
+	ret2=$?
+
+	if [ $ret1 -ne 0 -a $ret2 -eq 0 ]; then
+		return
+	fi
+
 	cur_ipver=${cur_ipver##*s}
 
 	if [ -z "$cur_ipver" -o -z "$spe_ipver" ]; then
-- 
1.8.3.1

[LTP] [PATCH] netns_helper: Make iproute version check work correctly

[Cyril Hrubis]

Hi!
> diff --git a/testcases/kernel/containers/netns/netns_helper.h b/testcases/kernel/containers/netns/netns_helper.h
> index 8b87645..8337051 100644
> --- a/testcases/kernel/containers/netns/netns_helper.h
> +++ b/testcases/kernel/containers/netns/netns_helper.h
> @@ -37,6 +37,7 @@ static void check_iproute(unsigned int spe_ipver)
>  	FILE *ipf;
>  	int n;
>  	unsigned int ipver = 0;
> +	char ver;
>  
>  	ipf = popen("ip -V", "r");
>  	if (ipf == NULL)
> @@ -44,7 +45,14 @@ static void check_iproute(unsigned int spe_ipver)
>  				"Failed while opening pipe for iproute check");
>  
>  	n = fscanf(ipf, "ip utility, iproute2-ss%u", &ipver);
> +	pclose(ipf);
>  	if (n < 1) {
> +		ipf = popen("ip -V", "r");
> +		n = fscanf(ipf, "ip utility, iproute2-%s", &ver);
> +		if (n >= 1) {
> +			pclose(ipf);
> +			return;
> +		}

Can we please instead read the whole string after the dash (-), skip
the 'ss' prefix when present and then convert the string into an
itneger?

There is absolutely no reason to run the command twice.

>  static int dummy(void *arg)
> diff --git a/testcases/kernel/containers/netns/netns_helper.sh b/testcases/kernel/containers/netns/netns_helper.sh
> index a5b77a0..bec43ac 100755
> --- a/testcases/kernel/containers/netns/netns_helper.sh
> +++ b/testcases/kernel/containers/netns/netns_helper.sh
> @@ -50,6 +50,15 @@ tst_check_iproute()
>  	local cur_ipver="$(ip -V)"
>  	local spe_ipver="$1"
>  
> +	echo $cur_ipver | grep "ip utility, iproute2-ss" > /dev/null
> +	ret1=$?
> +	echo $cur_ipver | grep "ip utility, iproute2-" > /dev/null
> +	ret2=$?
> +
> +	if [ $ret1 -ne 0 -a $ret2 -eq 0 ]; then
> +		return
> +	fi

How is this supposed to fix the problem?

This just skips the test if we haven't found one of the strings in the
version, right? Which is wrong anyway.

>  	cur_ipver=${cur_ipver##*s}

But the version here is still wrong if the original string haven't
contained the ss. We would end up with cur_ipver with iproute2-123456,
which will be used in the numerical comparsion.

>  	if [ -z "$cur_ipver" -o -z "$spe_ipver" ]; then
> -- 
> 1.8.3.1
> 
> 
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] netns_helper: Make iproute version check work correctly

[Cyril Hrubis]

Hi!
> >  static int dummy(void *arg)
> > diff --git a/testcases/kernel/containers/netns/netns_helper.sh b/testcases/kernel/containers/netns/netns_helper.sh
> > index a5b77a0..bec43ac 100755
> > --- a/testcases/kernel/containers/netns/netns_helper.sh
> > +++ b/testcases/kernel/containers/netns/netns_helper.sh
> > @@ -50,6 +50,15 @@ tst_check_iproute()
> >  	local cur_ipver="$(ip -V)"
> >  	local spe_ipver="$1"
> >  
> > +	echo $cur_ipver | grep "ip utility, iproute2-ss" > /dev/null
> > +	ret1=$?
> > +	echo $cur_ipver | grep "ip utility, iproute2-" > /dev/null
> > +	ret2=$?
> > +
> > +	if [ $ret1 -ne 0 -a $ret2 -eq 0 ]; then
> > +		return
> > +	fi
> 
> How is this supposed to fix the problem?
> 
> This just skips the test if we haven't found one of the strings in the
                      ^
		      I mean version check, the test iself continues

If nothing else this should TBROK.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] netns_helper: Make iproute version check work correctly

[Petr Vorel]

Hi Zhu,

thank you for working on this. I also started to work on this before LTP
release, trying to fix it in the API, because there are more tests affected by
this (there is also mc_cmds.sh).

My WIP is to create tst_iproute_version.c, which would be used by shell and C tests.
I actually wanted to go little bit further to have general C helper for getting
version (but that'd have to wait after this is fixed). But as you were faster
I'll let you to finish it.

...
> diff --git a/testcases/kernel/containers/netns/netns_helper.sh b/testcases/kernel/containers/netns/netns_helper.sh
> index a5b77a0..bec43ac 100755
> --- a/testcases/kernel/containers/netns/netns_helper.sh
> +++ b/testcases/kernel/containers/netns/netns_helper.sh
> @@ -50,6 +50,15 @@ tst_check_iproute()
>  	local cur_ipver="$(ip -V)"
>  	local spe_ipver="$1"

> +	echo $cur_ipver | grep "ip utility, iproute2-ss" > /dev/null
> +	ret1=$?
> +	echo $cur_ipver | grep "ip utility, iproute2-" > /dev/null
> +	ret2=$?
> +
> +	if [ $ret1 -ne 0 -a $ret2 -eq 0 ]; then
> +		return
> +	fi
> +

FYI my shell version.

Kind regards,
Petr

--- testcases/kernel/containers/netns/netns_helper.sh
+++ testcases/kernel/containers/netns/netns_helper.sh
@@ -47,16 +47,21 @@ IFCONF_IN6_ARG=
 
 tst_check_iproute()
 {
-	local cur_ipver="$(ip -V)"
-	local spe_ipver="$1"
+	local current_ver="$(ip -V)"
+	local expected_ver="111010"
 
-	cur_ipver=${cur_ipver##*s}
+	current_ver=${current_ver##*s}
 
-	if [ -z "$cur_ipver" -o -z "$spe_ipver" ]; then
+	if [ -z "$current_ver" -o -z "$expected_ver" ]; then
 		tst_brk TBROK "failed to obtain valid iproute version"
 	fi
 
-	if [ $cur_ipver -lt $spe_ipver ]; then
+	# new version scheme since v5.7.0-77-gb687d1067169
+	if echo "$current_ver" | grep -q 'iproute2-v\?[0-9]\+\.'; then
+		return
+	fi
+
+	if [ $current_ver -lt $expected_ver ]; then
 		tst_brk TCONF "too old iproute version"
 	fi
 }

[LTP] [PATCH] netns_helper: Make iproute version check work correctly

[Petr Vorel]

Hi Zhu,

...
> FYI my shell version.

> Kind regards,
> Petr

> --- testcases/kernel/containers/netns/netns_helper.sh
> +++ testcases/kernel/containers/netns/netns_helper.sh
> @@ -47,16 +47,21 @@ IFCONF_IN6_ARG=

>  tst_check_iproute()
>  {
> -	local cur_ipver="$(ip -V)"
> -	local spe_ipver="$1"
> +	local current_ver="$(ip -V)"
> +	local expected_ver="111010"

> -	cur_ipver=${cur_ipver##*s}
> +	current_ver=${current_ver##*s}

> -	if [ -z "$cur_ipver" -o -z "$spe_ipver" ]; then
> +	if [ -z "$current_ver" -o -z "$expected_ver" ]; then
I'm sorry, this was supposed to be only this:
    if [ -z "$current_ver" ]; then
($expected_ver is set few lines above).

Kind regards,
Petr

>  		tst_brk TBROK "failed to obtain valid iproute version"
>  	fi

> -	if [ $cur_ipver -lt $spe_ipver ]; then
> +	# new version scheme since v5.7.0-77-gb687d1067169
> +	if echo "$current_ver" | grep -q 'iproute2-v\?[0-9]\+\.'; then
> +		return
> +	fi
> +
> +	if [ $current_ver -lt $expected_ver ]; then
>  		tst_brk TCONF "too old iproute version"
>  	fi
>  }

Kind regards,
Petr