Re: [PATCH net-next v3] net: netconsole: selftests: Create a new netconsole selftest

Hi Breno,

On 15/08/2024 11:51, 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:

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)

Thank you for the v3!

I only looked here at how 'setup_ns' was used, (and a few other Bash-related stuff), but not at the test itself.

I have a few comments, but I don't consider them as blocking if you prefer to continue with the current version.

(...)

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..929f27a0fd9c --- /dev/null +++ b/tools/testing/selftests/drivers/net/netcons_basic.sh @@ -0,0 +1,226 @@

(...)

+# This will have some tmp values appended to it in set_network() +NAMESPACE="netconsns_dst"

nit: the comment is no longer correct: if this variable is set before being used with setup_ns, the netns will not have a unique name, but it will use the one defined here. Maybe not what you want?

See this code from lib.sh where "ns_name" is "NAMESPACE":

# Some test may setup/remove same netns multi times if [ -z "${!ns_name}" ]; then eval "${ns_name}=${ns_name,,}-$(mktemp -u XXXXXX)" else cleanup_ns "${!ns_name}" fi

So it will not set a new value, but it will try to clean any netns with this "netconsns_dst" name. I guess that's fine, but maybe you prefer to do like the others and simply define "NAMESPACE" to an empty string?

(...)

+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}
• echo "${INITNS_FD}:$SRCIF_IFIDX $NAMESPACE_FD:$DSTIF_IFIDX" \

• > $NSIM_DEV_SYS_LINK
  

• if [ $? -ne 0 ]; then

Because of the 'set -e' defined above, I guess the script will stop just before in case of error, no? Maybe better with:

if ! echo "(...)" > $NSIM_DEV_SYS_LINK; then

(note that shellcheck should help to spot such issues I think)

• echo "linking netdevsim1 with netdevsim2 should succeed"
  

• cleanup
  

• exit ${ksft_skip}
  

• fi

+}

(...)

+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
  

• return ${ksft_fail}
  

• fi
• if ! grep -q "${MSG}" "${TMPFILENAME}"; then

• echo "FAIL: ${MSG} not found in ${TMPFILENAME}" >&2
  

• cat "${TMPFILENAME}" >&2
  

• return ${ksft_fail}

nit: a tab is missing here.

• fi
• # Delete the file once it is validated, otherwise keep it
• # for debugging purposes
• rm "${TMPFILENAME}"
• return ${ksft_pass}

+}

(...)

+# ========== # +# Start here # +# ========== # +modprobe netdevsim || true +modprobe netconsole || true

If errors can be expected, maybe clearer to mute stderr, not to confuse the people reading the logs?

Same above with 'udevadm settle || 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 +# 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. +sleep 1

I guess that's fine as it is, but it is often better to avoid a sleep with a "random" value: CI can be very slow, e.g. when running without KVM and/or with a debug kernel config. Here, wait_local_port_listen() from net_helper.sh could probably be used. The script will then probably wait less than 1 second.

+# Send the message +echo "${MSG}: ${TARGET}" > /dev/kmsg +# Wait until socat saves the file to disk +sleep 1

For here, I'm not sure, but 'busywait()' could be used, waiting for the OUTPUT_FILE to have a non 0 size?

If you do that, you can maybe increase the timeout you used above, to support very slow environments.

But if you prefer, I guess you can also leave things like they are and see if CIs are complaining (but these errors might not be easy to debug).

+# Make sure the message was received in the dst part +validate_result "${OUTPUT_FILE}" +ret=$?

Here as well, because of 'set -e', this line is probably useless.

validate_result "${OUTPUT_FILE}" || ret=$?

(or exit directly from validate_result() )

+exit ${ret}

Cheers, Matt