Re: [nft PATCH 1/3] tests/shell: mount all of "/var/run" in "test-wrapper.sh"

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Thomas,

Series LGTM, one question below

On Fri, Oct 06, 2023 at 11:42:18AM +0200, Thomas Haller wrote:
> After reboot, "/var/run/netns" does not exist before we run the first
> `ip netns add` command. Previously, "test-wrapper.sh" would mount a
> tmpfs on that directory, but that fails, if the directory doesn't exist.
> You will notice this, by deleting /var/run/netns (which only root can
> delete or create, and which is wiped on reboot).
> 
> Instead, mount all of "/var/run". Then we can also create /var/run/netns
> directory.

Maybe create a specify mount point for this? This will be created
once, then it will remain there for those that run tests?

> This means, any other content from /var/run is hidden too. That's
> probably desirable, because it means we don't depend on stuff that
> happens to be there. If we would require other content in /var/run, then
> the test runner needs to be aware of the requirement and ensure it's
> present. But best is just to not require anything. It's only iproute2
> which insists on /var/run/netns.
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  tests/shell/helpers/test-wrapper.sh | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/shell/helpers/test-wrapper.sh b/tests/shell/helpers/test-wrapper.sh
> index e10360c9b266..13b918f8b8e1 100755
> --- a/tests/shell/helpers/test-wrapper.sh
> +++ b/tests/shell/helpers/test-wrapper.sh
> @@ -23,11 +23,11 @@ START_TIME="$(cut -d ' ' -f1 /proc/uptime)"
>  
>  export TMPDIR="$NFT_TEST_TESTTMPDIR"
>  
> -CLEANUP_UMOUNT_RUN_NETNS=n
> +CLEANUP_UMOUNT_VAR_RUN=n
>  
>  cleanup() {
> -	if [ "$CLEANUP_UMOUNT_RUN_NETNS" = y ] ; then
> -		umount "/var/run/netns" || :
> +	if [ "$CLEANUP_UMOUNT_VAR_RUN" = y ] ; then
> +		umount "/var/run" &>/dev/null || :
>  	fi
>  }
>  
> @@ -38,16 +38,20 @@ printf '%s\n' "$TEST" > "$NFT_TEST_TESTTMPDIR/name"
>  read tainted_before < /proc/sys/kernel/tainted
>  
>  if [ "$NFT_TEST_HAS_UNSHARED_MOUNT" = y ] ; then
> -	# We have a private mount namespace. We will mount /run/netns as a tmpfs,
> -	# this is useful because `ip netns add` wants to add files there.
> +	# We have a private mount namespace. We will mount /var/run/ as a tmpfs.
>  	#
> -	# When running as rootless, this is necessary to get such tests to
> -	# pass.  When running rootful, it's still useful to not touch the
> -	# "real" /var/run/netns of the system.
> -	mkdir -p /var/run/netns
> -	if mount -t tmpfs --make-private "/var/run/netns" ; then
> -		CLEANUP_UMOUNT_RUN_NETNS=y
> +	# The main purpose is so that we can create /var/run/netns, which is
> +	# required for `ip netns add` to work.  When running as rootless, this
> +	# is necessary to get such tests to pass. When running rootful, it's
> +	# still useful to not touch the "real" /var/run/netns of the system.
> +	#
> +	# Note that this also hides everything that might reside in /var/run.
> +	# That is desirable, as tests should not depend on content there (or if
> +	# they do, we need to explicitly handle it as appropriate).
> +	if mount -t tmpfs --make-private "/var/run" ; then
> +		CLEANUP_UMOUNT_VAR_RUN=y
>  	fi
> +	mkdir -p /var/run/netns
>  fi
>  
>  TEST_TAGS_PARSED=0
> -- 
> 2.41.0
>