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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Petr, Breno,

On 14/08/2024 12:24, Petr Machata wrote:
> 
> Breno Leitao <leitao@debian.org> writes:
> 
>> 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.

(...)

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

(...)

> +NAMESPACE="netconsns_dst"

(...)

>> +function set_network() {
>> +	# This is coming from lib.sh. And it does unbound variable access
>> +	set +u
>> +	setup_ns "${NAMESPACE}"
>> +	set -u
> 
> It would make sense to fix lib.sh. I think this is what is needed?
> 
> modified   tools/testing/selftests/net/lib.sh
> @@ -178,7 +178,7 @@ setup_ns()
>  		fi
>  
>  		# Some test may setup/remove same netns multi times
> -		if [ -z "${!ns_name}" ]; then
> +		if ! declare -p "$ns_name" &> /dev/null; then
>  			eval "${ns_name}=${ns_name,,}-$(mktemp -u XXXXXX)"
>  		else
>  			cleanup_ns "${!ns_name}"
> 
> CC'd Geliang Tang <geliang@kernel.org>, Hangbin Liu <liuhangbin@gmail.com>,
> Matthieu Baerts (NGI0) <matttbe@kernel.org> who were in the vicinity
> in the past.
Thank you for having CCed me.

I don't know if lib.sh needs to be modified: setup_ns() is supposed to
be called with the name of an existing variable. Can you not define this
variable before?

I mean: the modification from Petr looks good to me to support 'set -u',
but it sounds safer to define the variable before in the script, just in
case it is defined by in the environment, before starting the test, and
not taking the expected path.

Note that in all the other selftests, setup_ns() is called with the name
of the variable, not a variable like you did, e.g.

  NAMESPACE=
  setup_ns NAMESPACE

instead of:

  NAMESPACE="netconsns_dst"
  setup_ns "${NAMESPACE}"
  NAMESPACE=${NS_LIST[0]}

Maybe better to do like the others?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.