Re: [LTP] [PATCH] nfs_lib: Skip NFS versions disabled on server

[Petr Vorel <pvorel@suse.cz>]

Hi Avinesh,

> nfs06.sh mount several NFS versions in a single run
> (e.g. "3,4.0,4.1,4.2"). If the server has disabled one of
> the requested versions (reported via /proc/fs/nfsd/versions),
> the whole test currently aborts with TBROK on the failed mount
> instead of continuing with the versions that are supported.

> nfs06 1 TINFO: setup NFSv4.0, socket type tcp
> nfs06 1 TINFO: Mounting /var/tmp/LTP_nfs06.GHDqCwPYES/4.0/1
> nfs06 1 TINFO: Mounting NFS: mount -v -t nfs -o proto=tcp,vers=4.0 10.0.0.2:/var/tmp/LTP_nfs06.GHDqCwPYES/mntpoint/4.0/tcp /var/tmp/LTP_nfs06.GHDqCwPYES/4.0/1
> mount.nfs: mount(2): Invalid argument
> mount.nfs: an incorrect mount option was specified for /var/tmp/LTP_nfs06.GHDqCwPYES/4.0/1
> mount.nfs: timeout set for Wed Aug  5 08:06:38 2026
> mount.nfs: trying text-based options 'proto=tcp,vers=4.0,addr=10.0.0.2,clientaddr=10.0.0.1'
> nfs06 1 TBROK: mount command failed

> Add nfs_server_vers_enabled() to check /proc/fs/nfsd/versions on the
> server, and nfs_filter_versions() to drop disabled versions from
> $VERSION (keeping $SOCKET_TYPE aligned by position) before mounting.
> If none of the requested versions are enabled, TCONF as before.

Thanks for handling this!

LGTM? few notes below.
Reviewed-by: Petr Vorel <pvorel@suse.cz>
...

> -get_socket_type()
> +get_socket_type_raw()
very nit: slightly confusing type, because there is a "raw" socket SOCK_RAW.
I guess any network programmer seeing this will think of that socket, e.g.:
socket(PF_INET, SOCK_RAW, ...);
socket(PF_NETLINK, SOCK_RAW, NETLINK_ROUTE);

But of course it can stay.

>  {
>  	local t
>  	local k=0
>  	for t in $SOCKET_TYPE; do
>  		if [ "$k" -eq "$1" ]; then
> -			echo "${t}${TST_IPV6}"
> +			echo "$t"
>  			return
>  		fi
>  		k=$(( k + 1 ))
>  	done
>  }

> +get_socket_type()
> +{
> +	echo "$(get_socket_type_raw "$1")${TST_IPV6}"
> +}
> +
>  # directory mounted by NFS client
>  get_local_dir()
>  {
> @@ -109,6 +114,51 @@ nfs_server_udp_enabled()
>  	tst_rhost_run -c "grep -q \"^[# ]*udp *= *y\" /etc/nfs.conf $config"
>  }

> +# nfs_server_vers_enabled VERS
> +# Only returns false (1) when the server explicitly lists "-VERS" in
> +# /proc/fs/nfsd/versions. If that file is unavailable or doesn't mention
> +# VERS at all, defaults to true so the mount is still attempted.
> +nfs_server_vers_enabled()
> +{
> +	local vers="$1"
> +	local versions=" $(tst_rhost_run -c 'cat /proc/fs/nfsd/versions 2>/dev/null') "
> +
> +	case "$versions" in
> +	*" -$vers "*) return 1;;
> +	esac

very nit: I was thinking if having the space in case would be slightly more
readable, but probably not. I consider spaces in both versions as unnecessary
(i.e. formatting error) but of course they are necessary.

	local versions="$(tst_rhost_run -c 'cat /proc/fs/nfsd/versions 2>/dev/null')"

	case " $versions " in

> +
> +	return 0
> +}
> +
> +# Drops NFS versions the server explicitly disabled from $VERSION, keeping
> +# $SOCKET_TYPE entries aligned by position with what remains.
> +nfs_filter_versions()
> +{
> +	local v type
> +	local n=0
> +	local new_version=
> +	local new_socket_type=
nit: it should be safe to use it without '=', right?
local v type new_version new_socket_type

> +
> +	for v in $VERSION; do
> +		type=$(get_socket_type_raw $n)
> +
> +		if nfs_server_vers_enabled "$v"; then
> +			new_version="$new_version $v"
> +			new_socket_type="$new_socket_type $type"
> +		else
> +			tst_res TINFO "NFSv$v disabled on server, skipping"

Could this be TCONF so that results summary at the end shows some TCONF?
That indicates something was skipped.

+			tst_res TINFO "NFSv$v disabled on server, skipping"
+			tst_res TCONF "NFSv$v disabled on server, skipping"

> +		fi
> +
> +		n=$(( n + 1 ))
> +	done
> +
> +	[ -z "$new_version" ] && \
> +		tst_brk TCONF "none of the requested NFS versions ($VERSION) are enabled on server"
nit: I would expect this would quit the test on system with
set -o errexit (equivalent of set -e), but magically it works.
FYI normally it's better if any test line exit with 0 => use || (or if ...; then
... fi) instead &&  i.e.

[ ... ] || tst_brk TCONF

But because it works it can stay.

> +
> +	VERSION="${new_version# }"
> +	SOCKET_TYPE="${new_socket_type# }"

Fortunately removing leading space works also on dash, although at least some
string operations aren't part of POSIX [1].

If this is ever problematic, we can fix it with:

	[ "$new_version" ] && new_version="$new_version $v" || new_version="$v"
	[ "$new_socket_type" ] && new_socket_type="$new_socket_type $type" || new_socket_type="$type"

But because removing leading space is not needed, because later code for t in
$SOCKET_TYPE; do will handle that, I'd remove this part entirely.

FYI: (no leading/trailing space in parameters, no $t having just empty space:
SOCKET_TYPE=' udp tcp  '; for t in $SOCKET_TYPE; do echo "'$t'"; done
'udp'
'tcp'

[1] https://mywiki.wooledge.org/Bashism#Parameter_Expansions

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp