Message-ID: <127af9ef-2d69-4765-a8f0-0dece3b6dd1d@kernel.org>
Date: Thu, 15 Aug 2024 12:47:22 +0200
From: Matthieu Baerts <matttbe@...nel.org>
To: Breno Leitao <leitao@...ian.org>, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
 liuhangbin@...il.com, petrm@...dia.com, Shuah Khan <shuah@...nel.org>
Cc: netdev@...r.kernel.org, David Wei <dw@...idwei.uk>,
 Willem de Bruijn <willemb@...gle.com>,
 open list <linux-kernel@...r.kernel.org>,
 "open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@...r.kernel.org>
Subject: 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@...ian.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
-- 
Sponsored by the NGI0 Core fund.

Powered by blists - more mailing lists