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

Hi Petr,

Thanks for your review.

> 
> Thanks for handling this!
> 
> LGTM? few notes below.
> Reviewed-by: Petr Vorel <[email protected]>
> ...
> 
>> -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.

How about get_socket_type_bare() ?

> 
>>   {
>>   	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
>>   }


>> +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
+1

> 
>> +
>> +	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.
+1

> 
> +			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.
+1. I will switch to below for consistency.
	[ "$new_version" ] || \
		tst_brk TCONF ...
> 
>> +
>> +	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.
Actually we need to remove the leading space, otherwise we break
nfsstat01 where $VERSION is being used as whole scalar value.
case $VERSION in

I can switch to the solution you suggested here.

> 
> 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

Regards,
Avinesh

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