Re: [RFC PATCH 1/4] check: refactor argument parsing with getopt

[Zorro Lang <zlang@kernel.org>]

On Mon, Aug 17, 2026 at 08:37:49PM +0530, Ojaswin Mujoo wrote:
> On Mon, Aug 17, 2026 at 03:56:40PM +0800, Zorro Lang wrote:
> > On Fri, Aug 14, 2026 at 01:50:40PM +0530, Ojaswin Mujoo wrote:
> > > On Tue, May 12, 2026 at 09:25:36PM +0800, Zorro Lang wrote:
> > > > Replace the legacy, hand-written argument parsing loop with getopt.
> > > > Also compatible with old-style options (e.g. -nfs, -afs, -glusterfs,
> > > > -cifs, -9p, -fuse, -virtiofs, -pvfs2, -tmpfs, -ubifs, -overlay,
> > > > -udiff), pre-process them into long options before giving to getopt.
> > > > 
> > > > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > > > ---
> > > >  check | 179 +++++++++++++++++++++++++++++++++++++++++-----------------
> > > >  1 file changed, 126 insertions(+), 53 deletions(-)
> > > > 
> > > > diff --git a/check b/check
> > > > index ad685edc..11bdf81b 100755
> > > > --- a/check
> > > > +++ b/check
> > > > @@ -272,48 +272,97 @@ _prepare_test_list()
> > > >  	rm -f $tmp.list
> > > >  }
> > > >  
> > > > -# Process command arguments first.
> > > > +# Backward compatible with the old options mode. Translate word-style options
> > > > +# that getopt would misinterpret into long options.
> > > > +compat_old_option()
> > > > +{
> > > > +	check_args=()
> > > > +	while [ $# -gt 0 ]; do
> > > > +		case "$1" in
> > > > +		-nfs)		check_args+=("--fs" "nfs") ;;
> > > > +		-afs)		check_args+=("--fs" "afs") ;;
> > > > +		-glusterfs)	check_args+=("--fs" "glusterfs") ;;
> > > > +		-cifs)		check_args+=("--fs" "cifs") ;;
> > > > +		-9p)		check_args+=("--fs" "9p") ;;
> > > > +		-fuse)		check_args+=("--fs" "fuse") ;;
> > > > +		-virtiofs)	check_args+=("--fs" "virtiofs") ;;
> > > > +		-pvfs2)		check_args+=("--fs" "pvfs2") ;;
> > > > +		-tmpfs)		check_args+=("--fs" "tmpfs") ;;
> > > > +		-ubifs)		check_args+=("--fs" "ubifs") ;;
> > > > +		-overlay)	check_args+=("--fs" "overlay") ;;
> > > > +		-udiff)		check_args+=("--udiff") ;;
> > > > +		*)		check_args+=("$1") ;;
> > > > +		esac
> > > > +		shift
> > > > +	done
> > > > +}
> > > > +
> > > > +short_opts="g:x:X:e:E:s:S:lnri:I:TdbR:L:h"
> > > > +long_opts="fs:,exact-order,large-fs,extra-space:,udiff,help"
> > > > +
> > > > +compat_old_option "$@"
> > > > +
> > > > +# Note: The '+' prefix in getopt's option string preserves the existing
> > > > +# behavior that option parsing stops at the first non-option test argument.
> > > > +parsed_opts=$(getopt -n "check" -o +"${short_opts}" -l "$long_opts" -- "${check_args[@]}")
> > > > +test $? -ne 0 && usage
> > > > +
> > > > +eval set -- "$parsed_opts"
> > > > +
> > > >  while [ $# -gt 0 ]; do
> > > >  	case "$1" in
> > > > -	-\? | -h | --help) usage ;;
> > > 
> > > Hi Zorro, mostly looks good but I think we slightly change the behavior
> > > here. Earlier ./check -\? would print help but not:
> > > 
> > >    ./check -\?
> > >   check: invalid option -- '?'
> > >   Warning: need to define parameters for host <host>
> > >          or set variables:
> > >           TEST_DIR TEST_DEV
> > > 
> > > Maybe we should add this to compat_old_option ( although i really doubt
> > > anyone would care about -\? :) )
> > 
> > Thanks for the review! Turns out `-\?` is indeed an option
> 
> Hi Zorro,
> 
> Yeah haha, i doubt anyone uses it tho.
> > 
> >   while [ $# -gt 0 ]; do
> >         case "$1" in
> >         -\? | -h | --help) usage ;;
> > 
> > I've never used it before and wasn't sure if anyone actually does. But given
> > that it's the existing behavior, let's keep it consistent. I'll update and
> > remove the "RFC" flag in next version.

Hi,

I just found out that "?" is not supported as a valid option character by getopt,
and "-?" doesn't seem to be standard usage anyway. Because of this, we can't really
support "-?" natively via getopt.

However, I can handle "-?" inside compat_old_option() by mapping "-\?)" to
check_args+=("-h") and treating it as a deprecated option. Is that good to you and
others?

Thanks,
Zorro

> 
> Yep sounds good.
> 
> Thanks,
> ojaswin
> 
> > 
> > Thanks,
> > Zorro
> > 
> > > 
> > > Other than that, feel free to add:
> > > Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > > > -
> > > > -	-nfs|-afs|-glusterfs|-cifs|-9p|-fuse|-virtiofs|-pvfs2|-tmpfs|-ubifs)
> > > > -		FSTYP="${1:1}"
> > > > +	--fs)
> > > > +		if [ "$2" == "overlay" ];then
> > > > +			[ "$FSTYP" == overlay ] || \
> > > > +				export OVL_BASE_FSTYP="$FSTYP"
> > > > +			FSTYP=overlay
> > > > +			export OVERLAY=true
> > > > +		else
> > > > +			FSTYP="$2"
> > > > +		fi
> > > > +		shift
> > > >  		;;
> > > > -	-overlay)
> > > > -		[ "$FSTYP" == overlay ] || export OVL_BASE_FSTYP="$FSTYP"
> > > > -		FSTYP=overlay
> > > > -		export OVERLAY=true
> > > > +	--udiff)
> > > > +		diff="$diff -u"
> > > >  		;;
> > > > -
> > > > -	-g)	group=$2 ; shift ;
> > > > -		GROUP_LIST="$GROUP_LIST ${group//,/ }"
> > > > +	-g)
> > > > +		GROUP_LIST="$GROUP_LIST ${2//,/ }"
> > > > +		shift
> > > >  		;;
> > > > -
> > > > -	-x)	xgroup=$2 ; shift ;
> > > > -		XGROUP_LIST="$XGROUP_LIST ${xgroup//,/ }"
> > > > +	-x)
> > > > +		XGROUP_LIST="$XGROUP_LIST ${2//,/ }"
> > > > +		shift
> > > >  		;;
> > > > -
> > > > -	-X)	subdir_xfile=$2; shift ;
> > > > +	-X)
> > > > +		subdir_xfile="$2"
> > > > +		shift
> > > >  		;;
> > > >  	-e)
> > > > -		xfile=$2; shift ;
> > > >  		readarray -t -O "${#exclude_tests[@]}" exclude_tests < \
> > > > -			<(echo "$xfile" | tr ', ' '\n\n')
> > > > +			<(echo "$2" | tr ', ' '\n\n')
> > > > +		shift
> > > >  		;;
> > > > -
> > > > -	-E)	xfile=$2; shift ;
> > > > -		if [ -f $xfile ]; then
> > > > +	-E)
> > > > +		if [ -f "$2" ]; then
> > > >  			readarray -t -O ${#exclude_tests[@]} exclude_tests < \
> > > > -				<(sed "s/#.*$//" $xfile)
> > > > +				<(sed "s/#.*$//" "$2")
> > > >  		fi
> > > > +		shift
> > > > +		;;
> > > > +	-s)
> > > > +		RUN_SECTION="$RUN_SECTION $2"
> > > > +		shift
> > > > +		;;
> > > > +	-S)
> > > > +		EXCLUDE_SECTION="$EXCLUDE_SECTION $2"
> > > > +		shift
> > > > +		;;
> > > > +	-l)
> > > > +		diff="diff"
> > > > +		;;
> > > > +	-n)
> > > > +		showme=true
> > > >  		;;
> > > > -	-s)	RUN_SECTION="$RUN_SECTION $2"; shift ;;
> > > > -	-S)	EXCLUDE_SECTION="$EXCLUDE_SECTION $2"; shift ;;
> > > > -	-l)	diff="diff" ;;
> > > > -	-udiff)	diff="$diff -u" ;;
> > > > -
> > > > -	-n)	showme=true ;;
> > > >  	-r)
> > > >  		if $exact_order; then
> > > >  			_fatal "Cannot specify -r and --exact-order."
> > > > @@ -322,40 +371,64 @@ while [ $# -gt 0 ]; do
> > > >  		;;
> > > >  	--exact-order)
> > > >  		if $randomize; then
> > > > -			_fatal "Cannnot specify --exact-order and -r."
> > > > +			_fatal "Cannot specify --exact-order and -r."
> > > >  		fi
> > > >  		exact_order=true
> > > >  		;;
> > > > -	-i)	iterations=$2; shift ;;
> > > > -	-I) 	iterations=$2; istop=true; shift ;;
> > > > -	-T)	timestamp=true ;;
> > > > -	-d)	DUMP_OUTPUT=true ;;
> > > > -	-b)	brief_test_summary=true;;
> > > > -	-R)	report_fmt=$2 ; shift ;
> > > > -		REPORT_LIST="$REPORT_LIST ${report_fmt//,/ }"
> > > > +	-i)
> > > > +		iterations=$2
> > > > +		shift
> > > > +		;;
> > > > +	-I)
> > > > +		iterations=$2
> > > > +		istop=true
> > > > +		shift
> > > > +		;;
> > > > +	-T)
> > > > +		timestamp=true
> > > > +		;;
> > > > +	-d)
> > > > +		DUMP_OUTPUT=true
> > > > +		;;
> > > > +	-b)
> > > > +		brief_test_summary=true
> > > > +		;;
> > > > +	-R)
> > > > +		REPORT_LIST="$REPORT_LIST ${2//,/ }"
> > > >  		do_report=true
> > > > +		shift
> > > >  		;;
> > > > -	--large-fs) export LARGE_SCRATCH_DEV=yes ;;
> > > > -	--extra-space=*) export SCRATCH_DEV_EMPTY_SPACE=${r#*=} ;;
> > > > -	-L)	[[ $2 =~ ^[0-9]+$ ]] || usage
> > > > -		loop_on_fail=$2; shift
> > > > +	--large-fs)
> > > > +		export LARGE_SCRATCH_DEV=yes
> > > > +		;;
> > > > +	--extra-space)
> > > > +		export SCRATCH_DEV_EMPTY_SPACE="$2"
> > > > +		shift
> > > > +		;;
> > > > +	-L)
> > > > +		[[ $2 =~ ^[0-9]+$ ]] || usage
> > > > +		loop_on_fail=$2
> > > > +		shift
> > > > +		;;
> > > > +	-h|--help)
> > > > +		usage
> > > > +		;;
> > > > +	--)
> > > > +		shift
> > > > +		break
> > > > +		;;
> > > > +	*)
> > > > +		usage
> > > >  		;;
> > > > -
> > > > -	-*)	usage ;;
> > > > -	*)	# not an argument, we've got tests now.
> > > > -		have_test_arg=true ;;
> > > >  	esac
> > > > -
> > > > -	# if we've found a test specification, the break out of the processing
> > > > -	# loop before we shift the arguments so that this is the first argument
> > > > -	# that we process in the test arg loop below.
> > > > -	if $have_test_arg; then
> > > > -		break;
> > > > -	fi
> > > > -
> > > >  	shift
> > > >  done
> > > >  
> > > > +# Remaining arguments
> > > > +if [ $# -gt 0 ]; then
> > > > +	have_test_arg=true
> > > > +fi
> > > > +
> > > >  # we need common/rc, that also sources common/config. We need to source it
> > > >  # after processing args, overlay needs FSTYP set before sourcing common/config
> > > >  if ! . ./common/rc; then
> > > > -- 
> > > > 2.54.0
> > > > 
>