| Message ID | 20240816132450.346744-1-leitao@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v4] net: netconsole: selftests: Create a new netconsole selftest | expand |
Hi Breno, On 16/08/2024 15:24, Breno Leitao wrote: > Adds a selftest that creates two virtual interfaces, assigns one to a > new namespace, and assigns IP addresses to both. > > It listens on the destination interface using socat and configures a > dynamic target on netconsole, pointing to the destination IP address. > > The test then checks if the message was received properly on the > destination interface. > > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > Changelog: > > v4: > * Avoid sleeping in waiting for sockets and files (Matthieu Baerts) > * Some other improvements (Matthieu Baerts) > * Add configfs as a dependency (Jakub) Thank you for the new version! It looks good to me, but again, my review mainly focused on the Bash-related stuff, not on the netconsole test itself. I just have one question below, but not blocking. (...) > diff --git a/tools/testing/selftests/drivers/net/netcons_basic.sh b/tools/testing/selftests/drivers/net/netcons_basic.sh > new file mode 100755 > index 000000000000..5c3686af1fe8 > --- /dev/null > +++ b/tools/testing/selftests/drivers/net/netcons_basic.sh > @@ -0,0 +1,249 @@ (...) > +check_file_size() { > + local file="$1" > + > + if [[ ! -f "$file" ]]; then > + # File might not exist yet > + return 1 > + fi > + > + # Get file size > + local size=$(stat -c %s "$file" 2>/dev/null) > + # Check if stat command succeeded > + if [[ $? -ne 0 ]]; then > + return 1 > + fi > + > + # Check if size is greater than zero > + if [[ "$size" -gt 0 ]]; then > + return 0 # file size > 0 > + else > + return 1 # file size == 0 > + fi > +} (...) > +# Wait until socat saves the file to disk > +busywait "${BUSYWAIT_TIMEOUT}" check_file_size "${OUTPUT_FILE}" It looks like your 'check_file_size' helper is a reimplementation of 'test -s <FILE>', no? Can you not simply use: busywait "${BUSYWAIT_TIMEOUT}" test -s "${OUTPUT_FILE}" Apart from that, the rest looks good to me! Acked-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> Cheers, Matt
On Fri, Aug 16, 2024 at 04:02:51PM +0200, Matthieu Baerts wrote: > Hi Breno, > > On 16/08/2024 15:24, Breno Leitao wrote: > > Adds a selftest that creates two virtual interfaces, assigns one to a > > new namespace, and assigns IP addresses to both. > > > > It listens on the destination interface using socat and configures a > > dynamic target on netconsole, pointing to the destination IP address. > > > > The test then checks if the message was received properly on the > > destination interface. > > > > Signed-off-by: Breno Leitao <leitao@debian.org> > > --- > > Changelog: > > > > v4: > > * Avoid sleeping in waiting for sockets and files (Matthieu Baerts) > > * Some other improvements (Matthieu Baerts) > > * Add configfs as a dependency (Jakub) > > Thank you for the new version! > > It looks good to me, but again, my review mainly focused on the > Bash-related stuff, not on the netconsole test itself. > > I just have one question below, but not blocking. > > (...) > > > diff --git a/tools/testing/selftests/drivers/net/netcons_basic.sh b/tools/testing/selftests/drivers/net/netcons_basic.sh > > new file mode 100755 > > index 000000000000..5c3686af1fe8 > > --- /dev/null > > +++ b/tools/testing/selftests/drivers/net/netcons_basic.sh > > @@ -0,0 +1,249 @@ > > (...) > > > +check_file_size() { > > + local file="$1" > > + > > + if [[ ! -f "$file" ]]; then > > + # File might not exist yet > > + return 1 > > + fi > > + > > + # Get file size > > + local size=$(stat -c %s "$file" 2>/dev/null) > > + # Check if stat command succeeded > > + if [[ $? -ne 0 ]]; then > > + return 1 > > + fi > > + > > + # Check if size is greater than zero > > + if [[ "$size" -gt 0 ]]; then > > + return 0 # file size > 0 > > + else > > + return 1 # file size == 0 > > + fi > > +} > > (...) > > > +# Wait until socat saves the file to disk > > +busywait "${BUSYWAIT_TIMEOUT}" check_file_size "${OUTPUT_FILE}" > > It looks like your 'check_file_size' helper is a reimplementation of > 'test -s <FILE>', no? Can you not simply use: Why would you like to do it in one line when you can write a 15-lines function that does exactly the same!? :-P I will send a v5 with `test -x`, I will just wait for more reviews. Thanks!
On 16/08/2024 16:50, Breno Leitao wrote: > On Fri, Aug 16, 2024 at 04:02:51PM +0200, Matthieu Baerts wrote: >> Hi Breno, >> >> On 16/08/2024 15:24, Breno Leitao wrote: >>> Adds a selftest that creates two virtual interfaces, assigns one to a >>> new namespace, and assigns IP addresses to both. >>> >>> It listens on the destination interface using socat and configures a >>> dynamic target on netconsole, pointing to the destination IP address. >>> >>> The test then checks if the message was received properly on the >>> destination interface. >>> >>> Signed-off-by: Breno Leitao <leitao@debian.org> >>> --- >>> Changelog: >>> >>> v4: >>> * Avoid sleeping in waiting for sockets and files (Matthieu Baerts) >>> * Some other improvements (Matthieu Baerts) >>> * Add configfs as a dependency (Jakub) >> >> Thank you for the new version! >> >> It looks good to me, but again, my review mainly focused on the >> Bash-related stuff, not on the netconsole test itself. >> >> I just have one question below, but not blocking. >> >> (...) >> >>> diff --git a/tools/testing/selftests/drivers/net/netcons_basic.sh b/tools/testing/selftests/drivers/net/netcons_basic.sh >>> new file mode 100755 >>> index 000000000000..5c3686af1fe8 >>> --- /dev/null >>> +++ b/tools/testing/selftests/drivers/net/netcons_basic.sh >>> @@ -0,0 +1,249 @@ >> >> (...) >> >>> +check_file_size() { >>> + local file="$1" >>> + >>> + if [[ ! -f "$file" ]]; then >>> + # File might not exist yet >>> + return 1 >>> + fi >>> + >>> + # Get file size >>> + local size=$(stat -c %s "$file" 2>/dev/null) >>> + # Check if stat command succeeded >>> + if [[ $? -ne 0 ]]; then >>> + return 1 >>> + fi >>> + >>> + # Check if size is greater than zero >>> + if [[ "$size" -gt 0 ]]; then >>> + return 0 # file size > 0 >>> + else >>> + return 1 # file size == 0 >>> + fi >>> +} >> >> (...) >> >>> +# Wait until socat saves the file to disk >>> +busywait "${BUSYWAIT_TIMEOUT}" check_file_size "${OUTPUT_FILE}" >> >> It looks like your 'check_file_size' helper is a reimplementation of >> 'test -s <FILE>', no? Can you not simply use: > > Why would you like to do it in one line when you can write a 15-lines > function that does exactly the same!? :-P True :-P > I will send a v5 with `test -x`, I will just wait for more reviews. Thanks! Indeed, better to wait at least 24h between submissions. (Also, I guess you meant to write `test -s`, not `-x`.) Cheers, Matt
diff --git a/MAINTAINERS b/MAINTAINERS index 5dbf23cf11c8..9a371ddd8719 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -15772,6 +15772,7 @@ M: Breno Leitao <leitao@debian.org> S: Maintained F: Documentation/networking/netconsole.rst F: drivers/net/netconsole.c +F: tools/testing/selftests/drivers/net/netcons_basic.sh NETDEVSIM M: Jakub Kicinski <kuba@kernel.org> diff --git a/tools/testing/selftests/drivers/net/Makefile b/tools/testing/selftests/drivers/net/Makefile index e54f382bcb02..8bee2e94049b 100644 --- a/tools/testing/selftests/drivers/net/Makefile +++ b/tools/testing/selftests/drivers/net/Makefile @@ -1,8 +1,10 @@ # SPDX-License-Identifier: GPL-2.0 -TEST_INCLUDES := $(wildcard lib/py/*.py) +TEST_INCLUDES := $(wildcard lib/py/*.py) \ + ../../net/lib.sh \ TEST_PROGS := \ + netcons_basic.sh \ ping.py \ queues.py \ stats.py \ diff --git a/tools/testing/selftests/drivers/net/config b/tools/testing/selftests/drivers/net/config index f6a58ce8a230..a2d8af60876d 100644 --- a/tools/testing/selftests/drivers/net/config +++ b/tools/testing/selftests/drivers/net/config @@ -1,2 +1,6 @@ CONFIG_IPV6=y CONFIG_NETDEVSIM=m +CONFIG_CONFIGFS_FS=y +CONFIG_NETCONSOLE=m +CONFIG_NETCONSOLE_DYNAMIC=y +CONFIG_NETCONSOLE_EXTENDED_LOG=y diff --git a/tools/testing/selftests/drivers/net/netcons_basic.sh b/tools/testing/selftests/drivers/net/netcons_basic.sh new file mode 100755 index 000000000000..5c3686af1fe8 --- /dev/null +++ b/tools/testing/selftests/drivers/net/netcons_basic.sh @@ -0,0 +1,249 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: GPL-2.0 + +# This test creates two netdevsim virtual interfaces, assigns one of them (the +# "destination interface") to a new namespace, and assigns IP addresses to both +# interfaces. +# +# It listens on the destination interface using socat and configures a dynamic +# target on netconsole, pointing to the destination IP address. +# +# Finally, it checks whether the message was received properly on the +# destination interface. Note that this test may pollute the kernel log buffer +# (dmesg) and relies on dynamic configuration and namespaces being configured. +# +# Author: Breno Leitao <leitao@debian.org> + +set -euo pipefail + +SCRIPTDIR=$(dirname "$(readlink -e "${BASH_SOURCE[0]}")") + +# Simple script to test dynamic targets in netconsole +SRCIF="" # to be populated later +SRCIP=192.168.1.1 +DSTIF="" # to be populated later +DSTIP=192.168.1.2 + +PORT="6666" +MSG="netconsole selftest" +TARGET=$(mktemp -u netcons_XXXXX) +NETCONS_CONFIGFS="/sys/kernel/config/netconsole" +NETCONS_PATH="${NETCONS_CONFIGFS}"/"${TARGET}" +# NAMESPACE will be populated by setup_ns with a random value +NAMESPACE="" + +# IDs for netdevsim +NSIM_DEV_1_ID=$((256 + RANDOM % 256)) +NSIM_DEV_2_ID=$((512 + RANDOM % 256)) + +# Used to create and delete namespaces +source "${SCRIPTDIR}"/../../net/lib.sh +source "${SCRIPTDIR}"/../../net/net_helper.sh + +# Create netdevsim interfaces +create_ifaces() { + local NSIM_DEV_SYS_NEW=/sys/bus/netdevsim/new_device + + echo "$NSIM_DEV_2_ID" > "$NSIM_DEV_SYS_NEW" + echo "$NSIM_DEV_1_ID" > "$NSIM_DEV_SYS_NEW" + udevadm settle 2> /dev/null || true + + local NSIM1=/sys/bus/netdevsim/devices/netdevsim"$NSIM_DEV_1_ID" + local NSIM2=/sys/bus/netdevsim/devices/netdevsim"$NSIM_DEV_2_ID" + + # These are global variables + SRCIF=$(find "$NSIM1"/net -maxdepth 1 -type d ! \ + -path "$NSIM1"/net -exec basename {} \;) + DSTIF=$(find "$NSIM2"/net -maxdepth 1 -type d ! \ + -path "$NSIM2"/net -exec basename {} \;) +} + +link_ifaces() { + local NSIM_DEV_SYS_LINK="/sys/bus/netdevsim/link_device" + local SRCIF_IFIDX=$(cat /sys/class/net/"$SRCIF"/ifindex) + local DSTIF_IFIDX=$(cat /sys/class/net/"$DSTIF"/ifindex) + + exec {NAMESPACE_FD}</var/run/netns/"${NAMESPACE}" + exec {INITNS_FD}</proc/self/ns/net + + # Bind the dst interface to namespace + ip link set "${DSTIF}" netns "${NAMESPACE}" + + # Linking one device to the other one (on the other namespace} + if ! echo "${INITNS_FD}:$SRCIF_IFIDX $NAMESPACE_FD:$DSTIF_IFIDX" > $NSIM_DEV_SYS_LINK + then + echo "linking netdevsim1 with netdevsim2 should succeed" + cleanup + exit "${ksft_skip}" + fi +} + +function configure_ip() { + # Configure the IPs for both interfaces + ip netns exec "${NAMESPACE}" ip addr add "${DSTIP}"/24 dev "${DSTIF}" + ip netns exec "${NAMESPACE}" ip link set "${DSTIF}" up + + ip addr add "${SRCIP}"/24 dev "${SRCIF}" + ip link set "${SRCIF}" up +} + +function set_network() { + # setup_ns function is coming from lib.sh + setup_ns NAMESPACE + + # Create both interfaces, and assign the destination to a different + # namespace + create_ifaces + + # Link both interfaces back to back + link_ifaces + + configure_ip +} + +function create_dynamic_target() { + DSTMAC=$(ip netns exec "${NAMESPACE}" \ + ip link show "${DSTIF}" | awk '/ether/ {print $2}') + + # Create a dynamic target + mkdir "${NETCONS_PATH}" + + echo "${DSTIP}" > "${NETCONS_PATH}"/remote_ip + echo "${SRCIP}" > "${NETCONS_PATH}"/local_ip + echo "${DSTMAC}" > "${NETCONS_PATH}"/remote_mac + echo "${SRCIF}" > "${NETCONS_PATH}"/dev_name + + echo 1 > "${NETCONS_PATH}"/enabled +} + +function cleanup() { + local NSIM_DEV_SYS_DEL="/sys/bus/netdevsim/del_device" + + # delete netconsole dynamic reconfiguration + echo 0 > "${NETCONS_PATH}"/enabled + # Remove the configfs entry + rmdir "${NETCONS_PATH}" + + # Delete netdevsim devices + echo "$NSIM_DEV_2_ID" > "$NSIM_DEV_SYS_DEL" + echo "$NSIM_DEV_1_ID" > "$NSIM_DEV_SYS_DEL" + + # this is coming from lib.sh + cleanup_all_ns +} + +function listen_port_and_save_to() { + local OUTPUT=${1} + # Just wait for 2 seconds + timeout 2 ip netns exec "${NAMESPACE}" \ + socat UDP-LISTEN:"${PORT}",fork "${OUTPUT}" +} + +function validate_result() { + local TMPFILENAME="$1" + + # Check if the file exists + if [ ! -f "$TMPFILENAME" ]; then + echo "FAIL: File was not generated." >&2 + exit "${ksft_fail}" + fi + + if ! grep -q "${MSG}" "${TMPFILENAME}"; then + echo "FAIL: ${MSG} not found in ${TMPFILENAME}" >&2 + cat "${TMPFILENAME}" >&2 + exit "${ksft_fail}" + fi + + # Delete the file once it is validated, otherwise keep it + # for debugging purposes + rm "${TMPFILENAME}" + exit "${ksft_pass}" +} + +function check_for_dependencies() { + if [ "$(id -u)" -ne 0 ]; then + echo "This script must be run as root" >&2 + exit "${ksft_skip}" + fi + + if ! which socat > /dev/null ; then + echo "SKIP: socat(1) is not available" >&2 + exit "${ksft_skip}" + fi + + if ! which ip > /dev/null ; then + echo "SKIP: ip(1) is not available" >&2 + exit "${ksft_skip}" + fi + + if ! which udevadm > /dev/null ; then + echo "SKIP: udevadm(1) is not available" >&2 + exit "${ksft_skip}" + fi + + if [ ! -d "${NETCONS_CONFIGFS}" ]; then + echo "SKIP: directory ${NETCONS_CONFIGFS} does not exist. Check if NETCONSOLE_DYNAMIC is enabled" >&2 + exit "${ksft_skip}" + fi + + if ip link show "${DSTIF}" 2> /dev/null; then + echo "SKIP: interface ${DSTIF} exists in the system. Not overwriting it." >&2 + exit "${ksft_skip}" + fi +} + +check_file_size() { + local file="$1" + + if [[ ! -f "$file" ]]; then + # File might not exist yet + return 1 + fi + + # Get file size + local size=$(stat -c %s "$file" 2>/dev/null) + # Check if stat command succeeded + if [[ $? -ne 0 ]]; then + return 1 + fi + + # Check if size is greater than zero + if [[ "$size" -gt 0 ]]; then + return 0 # file size > 0 + else + return 1 # file size == 0 + fi +} + + +# ========== # +# Start here # +# ========== # +modprobe netdevsim 2> /dev/null || true +modprobe netconsole 2 > /dev/null || true + +# The content of kmsg will be save to the following file +OUTPUT_FILE="/tmp/${TARGET}" + +# Check for basic system dependency and exit if not found +check_for_dependencies +# Set current loglevel to KERN_INFO(6), and default to KERN_NOTICE(5) +echo "6 5" > /proc/sys/kernel/printk +# Remove the namespace, interfaces and netconsole target on exit +trap cleanup EXIT +# Create one namespace and two interfaces +set_network +# Create a dynamic target for netconsole +create_dynamic_target +# Listed for netconsole port inside the namespace and destination interface +listen_port_and_save_to "${OUTPUT_FILE}" & +# Wait for socat to start and listen to the port. +wait_local_port_listen "${NAMESPACE}" "${PORT}" udp +# Send the message +echo "${MSG}: ${TARGET}" > /dev/kmsg +# Wait until socat saves the file to disk +busywait "${BUSYWAIT_TIMEOUT}" check_file_size "${OUTPUT_FILE}" + +# Make sure the message was received in the dst part +# and exit +validate_result "${OUTPUT_FILE}"
Adds a selftest that creates two virtual interfaces, assigns one to a new namespace, and assigns IP addresses to both. It listens on the destination interface using socat and configures a dynamic target on netconsole, pointing to the destination IP address. The test then checks if the message was received properly on the destination interface. Signed-off-by: Breno Leitao <leitao@debian.org> --- Changelog: v4: * Avoid sleeping in waiting for sockets and files (Matthieu Baerts) * Some other improvements (Matthieu Baerts) * Add configfs as a dependency (Jakub) v3: * Defined CONFIGs in config file (Jakub) * Identention fixes (Petr Machata) * Use setup_ns in a better way (Matthieu Baerts) * Add dependencies in TEST_INCLUDES (Hangbin Liu) * https://lore.kernel.org/all/20240815095157.3064722-1-leitao@debian.org/ v2: * Change the location of the path (Jakub) * Move from veth to netdevsim * Other small changes in dependency checks and cleanup * https://lore.kernel.org/all/20240813183825.837091-1-leitao@debian.org/ v1: * https://lore.kernel.org/all/ZqyUHN770pjSofTC@gmail.com/ MAINTAINERS | 1 + tools/testing/selftests/drivers/net/Makefile | 4 +- tools/testing/selftests/drivers/net/config | 4 + .../selftests/drivers/net/netcons_basic.sh | 249 ++++++++++++++++++ 4 files changed, 257 insertions(+), 1 deletion(-) create mode 100755 tools/testing/selftests/drivers/net/netcons_basic.sh