From: Geliang Tang <tanggeliang@kylinos.cn>
This patch includes lib.sh into mptcp_lib.sh, uses setup_ns() helper
defined in lib.sh to set up namespaces in mptcp_lib_ns_init(). Then for
each namespace in NS_LIST, run all sysctl commands. This can drop some
duplicate code.
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
.../testing/selftests/net/mptcp/mptcp_lib.sh | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index ad2ebda5cb64..59eb77e7813d 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -1,6 +1,8 @@
#! /bin/bash
# SPDX-License-Identifier: GPL-2.0
+. "$(dirname "${0}")/../lib.sh"
+
readonly KSFT_PASS=0
readonly KSFT_FAIL=1
readonly KSFT_SKIP=4
@@ -412,20 +414,13 @@ mptcp_lib_check_tools() {
}
mptcp_lib_ns_init() {
- local sec rndh
-
- sec=$(date +%s)
- rndh=$(printf %x "${sec}")-$(mktemp -u XXXXXX)
+ setup_ns "${@}"
local netns
- for netns in "${@}"; do
- eval "${netns}=${netns}-${rndh}"
-
- ip netns add "${!netns}" || exit ${KSFT_SKIP}
- ip -net "${!netns}" link set lo up
- ip netns exec "${!netns}" sysctl -q net.mptcp.enabled=1
- ip netns exec "${!netns}" sysctl -q net.ipv4.conf.all.rp_filter=0
- ip netns exec "${!netns}" sysctl -q net.ipv4.conf.default.rp_filter=0
+ for netns in $NS_LIST; do
+ ip netns exec "${netns}" sysctl -q net.mptcp.enabled=1
+ ip netns exec "${netns}" sysctl -q net.ipv4.conf.all.rp_filter=0
+ ip netns exec "${netns}" sysctl -q net.ipv4.conf.default.rp_filter=0
done
}
--
2.43.0
Hi Geliang, On 23/05/2024 10:08, Geliang Tang wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > This patch includes lib.sh into mptcp_lib.sh, uses setup_ns() helper > defined in lib.sh to set up namespaces in mptcp_lib_ns_init(). Then for > each namespace in NS_LIST, run all sysctl commands. This can drop some > duplicate code. > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> > --- > .../testing/selftests/net/mptcp/mptcp_lib.sh | 19 +++++++------------ > 1 file changed, 7 insertions(+), 12 deletions(-) > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh > index ad2ebda5cb64..59eb77e7813d 100644 > --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh > +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh (...) > @@ -412,20 +414,13 @@ mptcp_lib_check_tools() { > } > > mptcp_lib_ns_init() { > - local sec rndh > - > - sec=$(date +%s) > - rndh=$(printf %x "${sec}")-$(mktemp -u XXXXXX) > + setup_ns "${@}" > > local netns > - for netns in "${@}"; do > - eval "${netns}=${netns}-${rndh}" > - > - ip netns add "${!netns}" || exit ${KSFT_SKIP} > - ip -net "${!netns}" link set lo up > - ip netns exec "${!netns}" sysctl -q net.mptcp.enabled=1 > - ip netns exec "${!netns}" sysctl -q net.ipv4.conf.all.rp_filter=0 > - ip netns exec "${!netns}" sysctl -q net.ipv4.conf.default.rp_filter=0 > + for netns in $NS_LIST; do Could you add {} (${NS_LIST}), to keep the same style (and usually recommended in order to avoid typos, etc.) Also, I wonder if it is a good idea to use '${NS_LIST}': it might contain existing netns. e.g. with your patch 3/4, in mptcp_connect, mptcp_lib_ns_init will be called twice, modifying the first netns twice. What if you keep: for netns in "${@}"; do and use "${!netns}" instead? > + ip netns exec "${netns}" sysctl -q net.mptcp.enabled=1 > + ip netns exec "${netns}" sysctl -q net.ipv4.conf.all.rp_filter=0 > + ip netns exec "${netns}" sysctl -q net.ipv4.conf.default.rp_filter=0 > done > } > Cheers, Matt -- Sponsored by the NGI0 Core fund.
Hi Matt, On Thu, 2024-05-23 at 11:18 +0200, Matthieu Baerts wrote: > Hi Geliang, > > On 23/05/2024 10:08, Geliang Tang wrote: > > From: Geliang Tang <tanggeliang@kylinos.cn> > > > > This patch includes lib.sh into mptcp_lib.sh, uses setup_ns() > > helper > > defined in lib.sh to set up namespaces in mptcp_lib_ns_init(). Then > > for > > each namespace in NS_LIST, run all sysctl commands. This can drop > > some > > duplicate code. > > > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> > > --- > > .../testing/selftests/net/mptcp/mptcp_lib.sh | 19 +++++++-------- > > ---- > > 1 file changed, 7 insertions(+), 12 deletions(-) > > > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh > > b/tools/testing/selftests/net/mptcp/mptcp_lib.sh > > index ad2ebda5cb64..59eb77e7813d 100644 > > --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh > > +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh > > > (...) > > > @@ -412,20 +414,13 @@ mptcp_lib_check_tools() { > > } > > > > mptcp_lib_ns_init() { > > - local sec rndh > > - > > - sec=$(date +%s) > > - rndh=$(printf %x "${sec}")-$(mktemp -u XXXXXX) > > + setup_ns "${@}" > > > > local netns > > - for netns in "${@}"; do > > - eval "${netns}=${netns}-${rndh}" > > - > > - ip netns add "${!netns}" || exit ${KSFT_SKIP} > > - ip -net "${!netns}" link set lo up > > - ip netns exec "${!netns}" sysctl -q > > net.mptcp.enabled=1 > > - ip netns exec "${!netns}" sysctl -q > > net.ipv4.conf.all.rp_filter=0 > > - ip netns exec "${!netns}" sysctl -q > > net.ipv4.conf.default.rp_filter=0 > > + for netns in $NS_LIST; do > > Could you add {} (${NS_LIST}), to keep the same style (and usually > recommended in order to avoid typos, etc.) > > Also, I wonder if it is a good idea to use '${NS_LIST}': it might > contain existing netns. e.g. with your patch 3/4, in mptcp_connect, > mptcp_lib_ns_init will be called twice, modifying the first netns > twice. > > What if you keep: > > for netns in "${@}"; do > > and use "${!netns}" instead? We don't know the name of ns now because "mktemp -u XXXXXX" is executed within setup_ns(). So we have to use $NS_LIST. > > > > + ip netns exec "${netns}" sysctl -q > > net.mptcp.enabled=1 > > + ip netns exec "${netns}" sysctl -q > > net.ipv4.conf.all.rp_filter=0 > > + ip netns exec "${netns}" sysctl -q > > net.ipv4.conf.default.rp_filter=0 > > done > > } > > > > Cheers, > Matt
On 23/05/2024 11:26, Geliang Tang wrote: > Hi Matt, > > On Thu, 2024-05-23 at 11:18 +0200, Matthieu Baerts wrote: >> Hi Geliang, >> >> On 23/05/2024 10:08, Geliang Tang wrote: >>> From: Geliang Tang <tanggeliang@kylinos.cn> >>> >>> This patch includes lib.sh into mptcp_lib.sh, uses setup_ns() >>> helper >>> defined in lib.sh to set up namespaces in mptcp_lib_ns_init(). Then >>> for >>> each namespace in NS_LIST, run all sysctl commands. This can drop >>> some >>> duplicate code. >>> >>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> >>> --- >>> .../testing/selftests/net/mptcp/mptcp_lib.sh | 19 +++++++-------- >>> ---- >>> 1 file changed, 7 insertions(+), 12 deletions(-) >>> >>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh >>> b/tools/testing/selftests/net/mptcp/mptcp_lib.sh >>> index ad2ebda5cb64..59eb77e7813d 100644 >>> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh >>> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh >> >> >> (...) >> >>> @@ -412,20 +414,13 @@ mptcp_lib_check_tools() { >>> } >>> >>> mptcp_lib_ns_init() { >>> - local sec rndh >>> - >>> - sec=$(date +%s) >>> - rndh=$(printf %x "${sec}")-$(mktemp -u XXXXXX) >>> + setup_ns "${@}" >>> >>> local netns >>> - for netns in "${@}"; do >>> - eval "${netns}=${netns}-${rndh}" >>> - >>> - ip netns add "${!netns}" || exit ${KSFT_SKIP} >>> - ip -net "${!netns}" link set lo up >>> - ip netns exec "${!netns}" sysctl -q >>> net.mptcp.enabled=1 >>> - ip netns exec "${!netns}" sysctl -q >>> net.ipv4.conf.all.rp_filter=0 >>> - ip netns exec "${!netns}" sysctl -q >>> net.ipv4.conf.default.rp_filter=0 >>> + for netns in $NS_LIST; do >> >> Could you add {} (${NS_LIST}), to keep the same style (and usually >> recommended in order to avoid typos, etc.) >> >> Also, I wonder if it is a good idea to use '${NS_LIST}': it might >> contain existing netns. e.g. with your patch 3/4, in mptcp_connect, >> mptcp_lib_ns_init will be called twice, modifying the first netns >> twice. >> >> What if you keep: >> >> for netns in "${@}"; do >> >> and use "${!netns}" instead? > > We don't know the name of ns now because "mktemp -u XXXXXX" is executed > within setup_ns(). So we have to use $NS_LIST. Will 'setup_ns' not assign the variable names given in parameter? If we give 'ns1', it will set 'ns1=xxxx'. So we can use '${ns1}' to get the name. If 'netns="ns1"', we can then get the name with '${!netns}', using '!' (${!netns}) to resolve the name, no? Cheers, Matt -- Sponsored by the NGI0 Core fund.
© 2016 - 2024 Red Hat, Inc.