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

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 <[email protected]>
> > > ---
> > >  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.

Yep sounds good.

Thanks,
ojaswin

> 
> Thanks,
> Zorro
> 
> > 
> > Other than that, feel free to add:
> > Reviewed-by: Ojaswin Mujoo <[email protected]>
> > > -
> > > -	-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
> > >