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@xxxxxxxxxx>
> ---
> 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.
