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

Hi Avinesh,

> 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() ?
+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 ...

Thank you!

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

Thanks for info.

> I can switch to the solution you suggested here.

I checked LTP sources and we use in old scripts often ${...#...}. I was probably
just too careful about portability.

Kind regards,
Petr

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