[PATCH 0/2] fstests: fix test issue of xfs/157

[PATCH 0/2] fstests: fix test issue of xfs/157

[Zorro Lang]

xfs/157 started to fail since 2f7e1b8a6f09 ("xfs/157,xfs/547,xfs/548: switch to
using _scratch_mkfs_sized") was merged.

  FSTYP         -- xfs (non-debug)
  PLATFORM      -- Linux/x86_64
  MKFS_OPTIONS  -- -f -m rmapbt=1 /dev/sda3
  MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/sda3 /mnt/scratch

  xfs/157 7s ... - output mismatch (see /root/git/xfstests/results//xfs/157.out.bad)
      --- tests/xfs/157.out       2024-11-01 01:05:03.664543576 +0800  
      +++ /root/git/xfstests/results//xfs/157.out.bad     2024-11-01 02:56:47.994007900 +0800
      @@ -6,10 +6,10 @@
       label = "oldlabel"
       label = "newlabel"
       S3: Check that setting with rtdev works
      -label = "oldlabel"
      +label = ""
       label = "newlabel"
       S4: Check that setting with rtdev + logdev works
      ...
      (Run 'diff -u /root/git/xfstests/tests/xfs/157.out /root/git/xfstests/results//xfs/157.out.bad'  to see the entire diff)
  Ran: xfs/157
  Failures: xfs/157
  Failed 1 of 1 tests

Before that change, the _scratch_mkfs can drop "rmapbt=1" option from $MKFS_OPTIONS,
only keep the "-L label" option. That's why this test never failed before. To fix
this failure, this patchset does:

The [PATCH 1/2] helps _scratch_mkfs_sized to support extra argument, then the
extra argument can be sent to _scratch_mkfs, which will try to "protect"
the extra argument from being dropped.

The [PATCH 2/2] invokes the new _scratch_mkfs_sized in xfs/157 to fix the
test failure of it.

Thanks,
Zorro

[PATCH 1/2] common/rc: _scratch_mkfs_sized supports extra arguments

[Zorro Lang]

To give more arguments to _scratch_mkfs_sized, we generally do as:

  MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size

to give "-L oldlabel" to it. But if _scratch_mkfs_sized fails, it
will get rid of the whole MKFS_OPTIONS and try to mkfs again.
Likes:

  ** mkfs failed with extra mkfs options added to "-L oldlabel -m rmapbt=1" by test 157 **
  ** attempting to mkfs using only test 157 options: -d size=524288000 -b size=4096 **

But that's not the fault of "-L oldlabel". So for keeping the mkfs
options ("-L oldlabel") we need, we'd better to let the
scratch_mkfs_sized to support extra arguments, rather than using
global MKFS_OPTIONS.

Signed-off-by: Zorro Lang <zlang@kernel.org>
---
 common/rc | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/common/rc b/common/rc
index 2af26f23f..ce8602383 100644
--- a/common/rc
+++ b/common/rc
@@ -1023,11 +1023,13 @@ _small_fs_size_mb()
 }
 
 # Create fs of certain size on scratch device
-# _try_scratch_mkfs_sized <size in bytes> [optional blocksize]
+# _try_scratch_mkfs_sized <size in bytes> [optional blocksize] [other options]
 _try_scratch_mkfs_sized()
 {
 	local fssize=$1
-	local blocksize=$2
+	shift
+	local blocksize=$1
+	shift
 	local def_blksz
 	local blocksize_opt
 	local rt_ops
@@ -1091,10 +1093,10 @@ _try_scratch_mkfs_sized()
 		# don't override MKFS_OPTIONS that set a block size.
 		echo $MKFS_OPTIONS |grep -E -q "b\s*size="
 		if [ $? -eq 0 ]; then
-			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops
+			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops "$@"
 		else
 			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops \
-				-b size=$blocksize
+				-b size=$blocksize "$@"
 		fi
 		;;
 	ext2|ext3|ext4)
@@ -1105,7 +1107,7 @@ _try_scratch_mkfs_sized()
 				_notrun "Could not make scratch logdev"
 			MKFS_OPTIONS="$MKFS_OPTIONS -J device=$SCRATCH_LOGDEV"
 		fi
-		${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
+		${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
 		;;
 	gfs2)
 		# mkfs.gfs2 doesn't automatically shrink journal files on small
@@ -1120,13 +1122,13 @@ _try_scratch_mkfs_sized()
 			(( journal_size >= min_journal_size )) || journal_size=$min_journal_size
 			MKFS_OPTIONS="-J $journal_size $MKFS_OPTIONS"
 		fi
-		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -O -b $blocksize $SCRATCH_DEV $blocks
+		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -O -b $blocksize "$@" $SCRATCH_DEV $blocks
 		;;
 	ocfs2)
-		yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
+		yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
 		;;
 	udf)
-		$MKFS_UDF_PROG $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
+		$MKFS_UDF_PROG $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
 		;;
 	btrfs)
 		local mixed_opt=
@@ -1134,33 +1136,33 @@ _try_scratch_mkfs_sized()
 		# the device is not zoned. Ref: btrfs-progs: btrfs_min_dev_size()
 		(( fssize < $((256 * 1024 * 1024)) )) &&
 			! _scratch_btrfs_is_zoned && mixed_opt='--mixed'
-		$MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV
+		$MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize "$@" $SCRATCH_DEV
 		;;
 	jfs)
-		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS $SCRATCH_DEV $blocks
+		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS "$@" $SCRATCH_DEV $blocks
 		;;
 	reiserfs)
-		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
+		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
 		;;
 	reiser4)
 		# mkfs.resier4 requires size in KB as input for creating filesystem
-		$MKFS_REISER4_PROG $MKFS_OPTIONS -y -b $blocksize $SCRATCH_DEV \
+		$MKFS_REISER4_PROG $MKFS_OPTIONS -y -b $blocksize "$@" $SCRATCH_DEV \
 				   `expr $fssize / 1024`
 		;;
 	f2fs)
 		# mkfs.f2fs requires # of sectors as an input for the size
 		local sector_size=`blockdev --getss $SCRATCH_DEV`
-		$MKFS_F2FS_PROG $MKFS_OPTIONS $SCRATCH_DEV `expr $fssize / $sector_size`
+		$MKFS_F2FS_PROG $MKFS_OPTIONS "$@" $SCRATCH_DEV `expr $fssize / $sector_size`
 		;;
 	tmpfs)
 		local free_mem=`_free_memory_bytes`
 		if [ "$free_mem" -lt "$fssize" ] ; then
 		   _notrun "Not enough memory ($free_mem) for tmpfs with $fssize bytes"
 		fi
-		export MOUNT_OPTIONS="-o size=$fssize $TMPFS_MOUNT_OPTIONS"
+		export MOUNT_OPTIONS="-o size=$fssize "$@" $TMPFS_MOUNT_OPTIONS"
 		;;
 	bcachefs)
-		$MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize $blocksize_opt $SCRATCH_DEV
+		$MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize $blocksize_opt "$@" $SCRATCH_DEV
 		;;
 	*)
 		_notrun "Filesystem $FSTYP not supported in _scratch_mkfs_sized"
@@ -1170,7 +1172,7 @@ _try_scratch_mkfs_sized()
 
 _scratch_mkfs_sized()
 {
-	_try_scratch_mkfs_sized $* || _notrun "_scratch_mkfs_sized failed with ($*)"
+	_try_scratch_mkfs_sized "$@" || _notrun "_scratch_mkfs_sized failed with ($@)"
 }
 
 # Emulate an N-data-disk stripe w/ various stripe units
-- 
2.45.2

[PATCH 2/2] xfs/157: do not drop necessary mkfs options

[Zorro Lang]

To give the test option "-L oldlabel" to _scratch_mkfs_sized, xfs/157
does:

  MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size

but the _scratch_mkfs_sized trys to keep the $fs_size, when mkfs
fails with incompatible $MKFS_OPTIONS options, likes this:

  ** mkfs failed with extra mkfs options added to "-L oldlabel -m rmapbt=1" by test 157 **
  ** attempting to mkfs using only test 157 options: -d size=524288000 -b size=4096 **

but the "-L oldlabel" is necessary, we shouldn't drop it. To avoid
that, we give the "-L oldlabel" to _scratch_mkfs_sized through
function parameters, not through global MKFS_OPTIONS.

Signed-off-by: Zorro Lang <zlang@kernel.org>
---
 tests/xfs/157 | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/xfs/157 b/tests/xfs/157
index 9b5badbae..f8f102d78 100755
--- a/tests/xfs/157
+++ b/tests/xfs/157
@@ -66,8 +66,7 @@ scenario() {
 }
 
 check_label() {
-	MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size \
-		>> $seqres.full
+	_scratch_mkfs_sized "$fs_size" "" "-L oldlabel" >> $seqres.full 2>&1
 	_scratch_xfs_db -c label
 	_scratch_xfs_admin -L newlabel "$@" >> $seqres.full
 	_scratch_xfs_db -c label
-- 
2.45.2

Re: [PATCH 1/2] common/rc: _scratch_mkfs_sized supports extra arguments

[Darrick J. Wong]

On Sun, Nov 17, 2024 at 03:07:59AM +0800, Zorro Lang wrote:
> To give more arguments to _scratch_mkfs_sized, we generally do as:
> 
>   MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size
> 
> to give "-L oldlabel" to it. But if _scratch_mkfs_sized fails, it
> will get rid of the whole MKFS_OPTIONS and try to mkfs again.
> Likes:
> 
>   ** mkfs failed with extra mkfs options added to "-L oldlabel -m rmapbt=1" by test 157 **
>   ** attempting to mkfs using only test 157 options: -d size=524288000 -b size=4096 **
> 
> But that's not the fault of "-L oldlabel". So for keeping the mkfs
> options ("-L oldlabel") we need, we'd better to let the
> scratch_mkfs_sized to support extra arguments, rather than using
> global MKFS_OPTIONS.
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---
>  common/rc | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 2af26f23f..ce8602383 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1023,11 +1023,13 @@ _small_fs_size_mb()
>  }
>  
>  # Create fs of certain size on scratch device
> -# _try_scratch_mkfs_sized <size in bytes> [optional blocksize]
> +# _try_scratch_mkfs_sized <size in bytes> [optional blocksize] [other options]
>  _try_scratch_mkfs_sized()
>  {
>  	local fssize=$1
> -	local blocksize=$2
> +	shift
> +	local blocksize=$1
> +	shift
>  	local def_blksz
>  	local blocksize_opt
>  	local rt_ops
> @@ -1091,10 +1093,10 @@ _try_scratch_mkfs_sized()
>  		# don't override MKFS_OPTIONS that set a block size.
>  		echo $MKFS_OPTIONS |grep -E -q "b\s*size="
>  		if [ $? -eq 0 ]; then
> -			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops
> +			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops "$@"
>  		else
>  			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops \
> -				-b size=$blocksize
> +				-b size=$blocksize "$@"
>  		fi
>  		;;
>  	ext2|ext3|ext4)
> @@ -1105,7 +1107,7 @@ _try_scratch_mkfs_sized()
>  				_notrun "Could not make scratch logdev"
>  			MKFS_OPTIONS="$MKFS_OPTIONS -J device=$SCRATCH_LOGDEV"
>  		fi
> -		${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
> +		${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
>  		;;
>  	gfs2)
>  		# mkfs.gfs2 doesn't automatically shrink journal files on small
> @@ -1120,13 +1122,13 @@ _try_scratch_mkfs_sized()
>  			(( journal_size >= min_journal_size )) || journal_size=$min_journal_size
>  			MKFS_OPTIONS="-J $journal_size $MKFS_OPTIONS"
>  		fi
> -		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -O -b $blocksize $SCRATCH_DEV $blocks
> +		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -O -b $blocksize "$@" $SCRATCH_DEV $blocks
>  		;;
>  	ocfs2)
> -		yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
> +		yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
>  		;;
>  	udf)
> -		$MKFS_UDF_PROG $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
> +		$MKFS_UDF_PROG $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
>  		;;
>  	btrfs)
>  		local mixed_opt=
> @@ -1134,33 +1136,33 @@ _try_scratch_mkfs_sized()
>  		# the device is not zoned. Ref: btrfs-progs: btrfs_min_dev_size()
>  		(( fssize < $((256 * 1024 * 1024)) )) &&
>  			! _scratch_btrfs_is_zoned && mixed_opt='--mixed'
> -		$MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV
> +		$MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize "$@" $SCRATCH_DEV
>  		;;
>  	jfs)
> -		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS $SCRATCH_DEV $blocks
> +		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS "$@" $SCRATCH_DEV $blocks
>  		;;
>  	reiserfs)
> -		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
> +		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
>  		;;
>  	reiser4)
>  		# mkfs.resier4 requires size in KB as input for creating filesystem
> -		$MKFS_REISER4_PROG $MKFS_OPTIONS -y -b $blocksize $SCRATCH_DEV \
> +		$MKFS_REISER4_PROG $MKFS_OPTIONS -y -b $blocksize "$@" $SCRATCH_DEV \
>  				   `expr $fssize / 1024`
>  		;;
>  	f2fs)
>  		# mkfs.f2fs requires # of sectors as an input for the size
>  		local sector_size=`blockdev --getss $SCRATCH_DEV`
> -		$MKFS_F2FS_PROG $MKFS_OPTIONS $SCRATCH_DEV `expr $fssize / $sector_size`
> +		$MKFS_F2FS_PROG $MKFS_OPTIONS "$@" $SCRATCH_DEV `expr $fssize / $sector_size`
>  		;;
>  	tmpfs)
>  		local free_mem=`_free_memory_bytes`
>  		if [ "$free_mem" -lt "$fssize" ] ; then
>  		   _notrun "Not enough memory ($free_mem) for tmpfs with $fssize bytes"
>  		fi
> -		export MOUNT_OPTIONS="-o size=$fssize $TMPFS_MOUNT_OPTIONS"
> +		export MOUNT_OPTIONS="-o size=$fssize "$@" $TMPFS_MOUNT_OPTIONS"
>  		;;
>  	bcachefs)
> -		$MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize $blocksize_opt $SCRATCH_DEV
> +		$MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize $blocksize_opt "$@" $SCRATCH_DEV
>  		;;
>  	*)
>  		_notrun "Filesystem $FSTYP not supported in _scratch_mkfs_sized"
> @@ -1170,7 +1172,7 @@ _try_scratch_mkfs_sized()
>  
>  _scratch_mkfs_sized()
>  {
> -	_try_scratch_mkfs_sized $* || _notrun "_scratch_mkfs_sized failed with ($*)"
> +	_try_scratch_mkfs_sized "$@" || _notrun "_scratch_mkfs_sized failed with ($@)"

Nit: Don't use '$@' within a longer string -- either it's "$@" so that
each element in the arg array is rendered individually as a separate
string parameter to the program being called, or "foo $*" so that you
end up with a single string.

shellcheck will complain about that, though bash itself doesn't seem to
care.

--D

>  }
>  
>  # Emulate an N-data-disk stripe w/ various stripe units
> -- 
> 2.45.2
> 
> 

Re: [PATCH 2/2] xfs/157: do not drop necessary mkfs options

[Darrick J. Wong]

On Sun, Nov 17, 2024 at 03:08:00AM +0800, Zorro Lang wrote:
> To give the test option "-L oldlabel" to _scratch_mkfs_sized, xfs/157
> does:
> 
>   MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size
> 
> but the _scratch_mkfs_sized trys to keep the $fs_size, when mkfs
> fails with incompatible $MKFS_OPTIONS options, likes this:
> 
>   ** mkfs failed with extra mkfs options added to "-L oldlabel -m rmapbt=1" by test 157 **
>   ** attempting to mkfs using only test 157 options: -d size=524288000 -b size=4096 **
> 
> but the "-L oldlabel" is necessary, we shouldn't drop it. To avoid
> that, we give the "-L oldlabel" to _scratch_mkfs_sized through
> function parameters, not through global MKFS_OPTIONS.
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---
>  tests/xfs/157 | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tests/xfs/157 b/tests/xfs/157
> index 9b5badbae..f8f102d78 100755
> --- a/tests/xfs/157
> +++ b/tests/xfs/157
> @@ -66,8 +66,7 @@ scenario() {
>  }
>  
>  check_label() {
> -	MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size \
> -		>> $seqres.full
> +	_scratch_mkfs_sized "$fs_size" "" "-L oldlabel" >> $seqres.full 2>&1

Don't quote the "-L" and "oldlabel" within the same string unless you
want them passed as a single string to _scratch_mkfs.  Right now that
works because although you have _scratch_mkfs_sized using "$@"
(doublequote-dollarsign-atsign-doublequote) to pass its arguments intact
to _scratch_mkfs, it turns out that _scratch_mkfs just brazely passes $*
(with no quoting) to the actual MKFS_PROG which results in any space in
any single argument being treated as an argument separator and the
string is broken into multiple arguments.

This is why you *can't* do _scratch_mkfs -L "moo cow".

This is also part of why everyone hates bash.

--D

>  	_scratch_xfs_db -c label
>  	_scratch_xfs_admin -L newlabel "$@" >> $seqres.full
>  	_scratch_xfs_db -c label
> -- 
> 2.45.2
> 
> 

Re: [PATCH 1/2] common/rc: _scratch_mkfs_sized supports extra arguments

[Darrick J. Wong]

On Mon, Nov 18, 2024 at 02:21:36PM -0800, Darrick J. Wong wrote:
> On Sun, Nov 17, 2024 at 03:07:59AM +0800, Zorro Lang wrote:
> > To give more arguments to _scratch_mkfs_sized, we generally do as:
> > 
> >   MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size
> > 
> > to give "-L oldlabel" to it. But if _scratch_mkfs_sized fails, it
> > will get rid of the whole MKFS_OPTIONS and try to mkfs again.
> > Likes:
> > 
> >   ** mkfs failed with extra mkfs options added to "-L oldlabel -m rmapbt=1" by test 157 **
> >   ** attempting to mkfs using only test 157 options: -d size=524288000 -b size=4096 **
> > 
> > But that's not the fault of "-L oldlabel". So for keeping the mkfs
> > options ("-L oldlabel") we need, we'd better to let the
> > scratch_mkfs_sized to support extra arguments, rather than using
> > global MKFS_OPTIONS.
> > 
> > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > ---
> >  common/rc | 34 ++++++++++++++++++----------------
> >  1 file changed, 18 insertions(+), 16 deletions(-)
> > 
> > diff --git a/common/rc b/common/rc
> > index 2af26f23f..ce8602383 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -1023,11 +1023,13 @@ _small_fs_size_mb()
> >  }
> >  
> >  # Create fs of certain size on scratch device
> > -# _try_scratch_mkfs_sized <size in bytes> [optional blocksize]
> > +# _try_scratch_mkfs_sized <size in bytes> [optional blocksize] [other options]
> >  _try_scratch_mkfs_sized()
> >  {
> >  	local fssize=$1
> > -	local blocksize=$2
> > +	shift
> > +	local blocksize=$1
> > +	shift
> >  	local def_blksz
> >  	local blocksize_opt
> >  	local rt_ops
> > @@ -1091,10 +1093,10 @@ _try_scratch_mkfs_sized()
> >  		# don't override MKFS_OPTIONS that set a block size.
> >  		echo $MKFS_OPTIONS |grep -E -q "b\s*size="
> >  		if [ $? -eq 0 ]; then
> > -			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops
> > +			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops "$@"
> >  		else
> >  			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops \
> > -				-b size=$blocksize
> > +				-b size=$blocksize "$@"
> >  		fi
> >  		;;
> >  	ext2|ext3|ext4)
> > @@ -1105,7 +1107,7 @@ _try_scratch_mkfs_sized()
> >  				_notrun "Could not make scratch logdev"
> >  			MKFS_OPTIONS="$MKFS_OPTIONS -J device=$SCRATCH_LOGDEV"
> >  		fi
> > -		${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
> > +		${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
> >  		;;
> >  	gfs2)
> >  		# mkfs.gfs2 doesn't automatically shrink journal files on small
> > @@ -1120,13 +1122,13 @@ _try_scratch_mkfs_sized()
> >  			(( journal_size >= min_journal_size )) || journal_size=$min_journal_size
> >  			MKFS_OPTIONS="-J $journal_size $MKFS_OPTIONS"
> >  		fi
> > -		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -O -b $blocksize $SCRATCH_DEV $blocks
> > +		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -O -b $blocksize "$@" $SCRATCH_DEV $blocks
> >  		;;
> >  	ocfs2)
> > -		yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
> > +		yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
> >  		;;
> >  	udf)
> > -		$MKFS_UDF_PROG $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
> > +		$MKFS_UDF_PROG $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
> >  		;;
> >  	btrfs)
> >  		local mixed_opt=
> > @@ -1134,33 +1136,33 @@ _try_scratch_mkfs_sized()
> >  		# the device is not zoned. Ref: btrfs-progs: btrfs_min_dev_size()
> >  		(( fssize < $((256 * 1024 * 1024)) )) &&
> >  			! _scratch_btrfs_is_zoned && mixed_opt='--mixed'
> > -		$MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV
> > +		$MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize "$@" $SCRATCH_DEV
> >  		;;
> >  	jfs)
> > -		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS $SCRATCH_DEV $blocks
> > +		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS "$@" $SCRATCH_DEV $blocks
> >  		;;
> >  	reiserfs)
> > -		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
> > +		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
> >  		;;
> >  	reiser4)
> >  		# mkfs.resier4 requires size in KB as input for creating filesystem
> > -		$MKFS_REISER4_PROG $MKFS_OPTIONS -y -b $blocksize $SCRATCH_DEV \
> > +		$MKFS_REISER4_PROG $MKFS_OPTIONS -y -b $blocksize "$@" $SCRATCH_DEV \
> >  				   `expr $fssize / 1024`
> >  		;;
> >  	f2fs)
> >  		# mkfs.f2fs requires # of sectors as an input for the size
> >  		local sector_size=`blockdev --getss $SCRATCH_DEV`
> > -		$MKFS_F2FS_PROG $MKFS_OPTIONS $SCRATCH_DEV `expr $fssize / $sector_size`
> > +		$MKFS_F2FS_PROG $MKFS_OPTIONS "$@" $SCRATCH_DEV `expr $fssize / $sector_size`
> >  		;;
> >  	tmpfs)
> >  		local free_mem=`_free_memory_bytes`
> >  		if [ "$free_mem" -lt "$fssize" ] ; then
> >  		   _notrun "Not enough memory ($free_mem) for tmpfs with $fssize bytes"
> >  		fi
> > -		export MOUNT_OPTIONS="-o size=$fssize $TMPFS_MOUNT_OPTIONS"
> > +		export MOUNT_OPTIONS="-o size=$fssize "$@" $TMPFS_MOUNT_OPTIONS"
> >  		;;
> >  	bcachefs)
> > -		$MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize $blocksize_opt $SCRATCH_DEV
> > +		$MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize $blocksize_opt "$@" $SCRATCH_DEV
> >  		;;
> >  	*)
> >  		_notrun "Filesystem $FSTYP not supported in _scratch_mkfs_sized"
> > @@ -1170,7 +1172,7 @@ _try_scratch_mkfs_sized()
> >  
> >  _scratch_mkfs_sized()
> >  {
> > -	_try_scratch_mkfs_sized $* || _notrun "_scratch_mkfs_sized failed with ($*)"
> > +	_try_scratch_mkfs_sized "$@" || _notrun "_scratch_mkfs_sized failed with ($@)"
> 
> Nit: Don't use '$@' within a longer string -- either it's "$@" so that
> each element in the arg array is rendered individually as a separate
> string parameter to the program being called, or "foo $*" so that you
> end up with a single string.
> 
> shellcheck will complain about that, though bash itself doesn't seem to
> care.

Oh, also, as I was rebasing my branch atop yours, I wondered, should
this patch convert these MKFS_OPTIONS overrides?

ext4/059
ext4/060
xfs/107
xfs/547

--D

> --D
> 
> >  }
> >  
> >  # Emulate an N-data-disk stripe w/ various stripe units
> > -- 
> > 2.45.2
> > 
> > 
> 

Re: [PATCH 1/2] common/rc: _scratch_mkfs_sized supports extra arguments

[Zorro Lang]

On Mon, Nov 18, 2024 at 02:21:36PM -0800, Darrick J. Wong wrote:
> On Sun, Nov 17, 2024 at 03:07:59AM +0800, Zorro Lang wrote:
> > To give more arguments to _scratch_mkfs_sized, we generally do as:
> > 
> >   MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size
> > 
> > to give "-L oldlabel" to it. But if _scratch_mkfs_sized fails, it
> > will get rid of the whole MKFS_OPTIONS and try to mkfs again.
> > Likes:
> > 
> >   ** mkfs failed with extra mkfs options added to "-L oldlabel -m rmapbt=1" by test 157 **
> >   ** attempting to mkfs using only test 157 options: -d size=524288000 -b size=4096 **
> > 
> > But that's not the fault of "-L oldlabel". So for keeping the mkfs
> > options ("-L oldlabel") we need, we'd better to let the
> > scratch_mkfs_sized to support extra arguments, rather than using
> > global MKFS_OPTIONS.
> > 
> > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > ---
> >  common/rc | 34 ++++++++++++++++++----------------
> >  1 file changed, 18 insertions(+), 16 deletions(-)
> > 
> > diff --git a/common/rc b/common/rc
> > index 2af26f23f..ce8602383 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -1023,11 +1023,13 @@ _small_fs_size_mb()
> >  }
> >  
> >  # Create fs of certain size on scratch device
> > -# _try_scratch_mkfs_sized <size in bytes> [optional blocksize]
> > +# _try_scratch_mkfs_sized <size in bytes> [optional blocksize] [other options]
> >  _try_scratch_mkfs_sized()
> >  {
> >  	local fssize=$1
> > -	local blocksize=$2
> > +	shift
> > +	local blocksize=$1
> > +	shift
> >  	local def_blksz
> >  	local blocksize_opt
> >  	local rt_ops
> > @@ -1091,10 +1093,10 @@ _try_scratch_mkfs_sized()
> >  		# don't override MKFS_OPTIONS that set a block size.
> >  		echo $MKFS_OPTIONS |grep -E -q "b\s*size="
> >  		if [ $? -eq 0 ]; then
> > -			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops
> > +			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops "$@"
> >  		else
> >  			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops \
> > -				-b size=$blocksize
> > +				-b size=$blocksize "$@"
> >  		fi
> >  		;;
> >  	ext2|ext3|ext4)
> > @@ -1105,7 +1107,7 @@ _try_scratch_mkfs_sized()
> >  				_notrun "Could not make scratch logdev"
> >  			MKFS_OPTIONS="$MKFS_OPTIONS -J device=$SCRATCH_LOGDEV"
> >  		fi
> > -		${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
> > +		${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
> >  		;;
> >  	gfs2)
> >  		# mkfs.gfs2 doesn't automatically shrink journal files on small
> > @@ -1120,13 +1122,13 @@ _try_scratch_mkfs_sized()
> >  			(( journal_size >= min_journal_size )) || journal_size=$min_journal_size
> >  			MKFS_OPTIONS="-J $journal_size $MKFS_OPTIONS"
> >  		fi
> > -		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -O -b $blocksize $SCRATCH_DEV $blocks
> > +		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -O -b $blocksize "$@" $SCRATCH_DEV $blocks
> >  		;;
> >  	ocfs2)
> > -		yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
> > +		yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
> >  		;;
> >  	udf)
> > -		$MKFS_UDF_PROG $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
> > +		$MKFS_UDF_PROG $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
> >  		;;
> >  	btrfs)
> >  		local mixed_opt=
> > @@ -1134,33 +1136,33 @@ _try_scratch_mkfs_sized()
> >  		# the device is not zoned. Ref: btrfs-progs: btrfs_min_dev_size()
> >  		(( fssize < $((256 * 1024 * 1024)) )) &&
> >  			! _scratch_btrfs_is_zoned && mixed_opt='--mixed'
> > -		$MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV
> > +		$MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize "$@" $SCRATCH_DEV
> >  		;;
> >  	jfs)
> > -		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS $SCRATCH_DEV $blocks
> > +		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS "$@" $SCRATCH_DEV $blocks
> >  		;;
> >  	reiserfs)
> > -		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
> > +		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
> >  		;;
> >  	reiser4)
> >  		# mkfs.resier4 requires size in KB as input for creating filesystem
> > -		$MKFS_REISER4_PROG $MKFS_OPTIONS -y -b $blocksize $SCRATCH_DEV \
> > +		$MKFS_REISER4_PROG $MKFS_OPTIONS -y -b $blocksize "$@" $SCRATCH_DEV \
> >  				   `expr $fssize / 1024`
> >  		;;
> >  	f2fs)
> >  		# mkfs.f2fs requires # of sectors as an input for the size
> >  		local sector_size=`blockdev --getss $SCRATCH_DEV`
> > -		$MKFS_F2FS_PROG $MKFS_OPTIONS $SCRATCH_DEV `expr $fssize / $sector_size`
> > +		$MKFS_F2FS_PROG $MKFS_OPTIONS "$@" $SCRATCH_DEV `expr $fssize / $sector_size`
> >  		;;
> >  	tmpfs)
> >  		local free_mem=`_free_memory_bytes`
> >  		if [ "$free_mem" -lt "$fssize" ] ; then
> >  		   _notrun "Not enough memory ($free_mem) for tmpfs with $fssize bytes"
> >  		fi
> > -		export MOUNT_OPTIONS="-o size=$fssize $TMPFS_MOUNT_OPTIONS"
> > +		export MOUNT_OPTIONS="-o size=$fssize "$@" $TMPFS_MOUNT_OPTIONS"
> >  		;;
> >  	bcachefs)
> > -		$MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize $blocksize_opt $SCRATCH_DEV
> > +		$MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize $blocksize_opt "$@" $SCRATCH_DEV
> >  		;;
> >  	*)
> >  		_notrun "Filesystem $FSTYP not supported in _scratch_mkfs_sized"
> > @@ -1170,7 +1172,7 @@ _try_scratch_mkfs_sized()
> >  
> >  _scratch_mkfs_sized()
> >  {
> > -	_try_scratch_mkfs_sized $* || _notrun "_scratch_mkfs_sized failed with ($*)"
> > +	_try_scratch_mkfs_sized "$@" || _notrun "_scratch_mkfs_sized failed with ($@)"
> 
> Nit: Don't use '$@' within a longer string -- either it's "$@" so that
> each element in the arg array is rendered individually as a separate
> string parameter to the program being called, or "foo $*" so that you
> end up with a single string.

Hi Darrick, do you mean I should keep the later $*? Likes:

  _try_scratch_mkfs_sized "$@" || _notrun "_scratch_mkfs_sized failed with ($*)"

> 
> shellcheck will complain about that, though bash itself doesn't seem to
> care.
> 
> --D
> 
> >  }
> >  
> >  # Emulate an N-data-disk stripe w/ various stripe units
> > -- 
> > 2.45.2
> > 
> > 
> 

Re: [PATCH 2/2] xfs/157: do not drop necessary mkfs options

[Zorro Lang]

On Mon, Nov 18, 2024 at 02:26:14PM -0800, Darrick J. Wong wrote:
> On Sun, Nov 17, 2024 at 03:08:00AM +0800, Zorro Lang wrote:
> > To give the test option "-L oldlabel" to _scratch_mkfs_sized, xfs/157
> > does:
> > 
> >   MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size
> > 
> > but the _scratch_mkfs_sized trys to keep the $fs_size, when mkfs
> > fails with incompatible $MKFS_OPTIONS options, likes this:
> > 
> >   ** mkfs failed with extra mkfs options added to "-L oldlabel -m rmapbt=1" by test 157 **
> >   ** attempting to mkfs using only test 157 options: -d size=524288000 -b size=4096 **
> > 
> > but the "-L oldlabel" is necessary, we shouldn't drop it. To avoid
> > that, we give the "-L oldlabel" to _scratch_mkfs_sized through
> > function parameters, not through global MKFS_OPTIONS.
> > 
> > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > ---
> >  tests/xfs/157 | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/tests/xfs/157 b/tests/xfs/157
> > index 9b5badbae..f8f102d78 100755
> > --- a/tests/xfs/157
> > +++ b/tests/xfs/157
> > @@ -66,8 +66,7 @@ scenario() {
> >  }
> >  
> >  check_label() {
> > -	MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size \
> > -		>> $seqres.full
> > +	_scratch_mkfs_sized "$fs_size" "" "-L oldlabel" >> $seqres.full 2>&1
> 
> Don't quote the "-L" and "oldlabel" within the same string unless you
> want them passed as a single string to _scratch_mkfs.  Right now that
> works because although you have _scratch_mkfs_sized using "$@"

I use "$@" just for _scratch_mkfs_sized can give an empty argument to
_try_scratch_mkfs_sized to be its second argument.

how about:
_scratch_mkfs_sized "$fs_size" "" -L oldlabel

> (doublequote-dollarsign-atsign-doublequote) to pass its arguments intact
> to _scratch_mkfs, it turns out that _scratch_mkfs just brazely passes $*
> (with no quoting) to the actual MKFS_PROG which results in any space in
> any single argument being treated as an argument separator and the
> string is broken into multiple arguments.
> 
> This is why you *can't* do _scratch_mkfs -L "moo cow".
> 
> This is also part of why everyone hates bash.

Hmm... do you need to change the $* of _scratch_mkfs to $@ too?

> 
> --D
> 
> >  	_scratch_xfs_db -c label
> >  	_scratch_xfs_admin -L newlabel "$@" >> $seqres.full
> >  	_scratch_xfs_db -c label
> > -- 
> > 2.45.2
> > 
> > 
> 

Re: [PATCH 1/2] common/rc: _scratch_mkfs_sized supports extra arguments

[Darrick J. Wong]

On Thu, Nov 21, 2024 at 05:17:33PM +0800, Zorro Lang wrote:
> On Mon, Nov 18, 2024 at 02:21:36PM -0800, Darrick J. Wong wrote:
> > On Sun, Nov 17, 2024 at 03:07:59AM +0800, Zorro Lang wrote:
> > > To give more arguments to _scratch_mkfs_sized, we generally do as:
> > > 
> > >   MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size
> > > 
> > > to give "-L oldlabel" to it. But if _scratch_mkfs_sized fails, it
> > > will get rid of the whole MKFS_OPTIONS and try to mkfs again.
> > > Likes:
> > > 
> > >   ** mkfs failed with extra mkfs options added to "-L oldlabel -m rmapbt=1" by test 157 **
> > >   ** attempting to mkfs using only test 157 options: -d size=524288000 -b size=4096 **
> > > 
> > > But that's not the fault of "-L oldlabel". So for keeping the mkfs
> > > options ("-L oldlabel") we need, we'd better to let the
> > > scratch_mkfs_sized to support extra arguments, rather than using
> > > global MKFS_OPTIONS.
> > > 
> > > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > > ---
> > >  common/rc | 34 ++++++++++++++++++----------------
> > >  1 file changed, 18 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index 2af26f23f..ce8602383 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -1023,11 +1023,13 @@ _small_fs_size_mb()
> > >  }
> > >  
> > >  # Create fs of certain size on scratch device
> > > -# _try_scratch_mkfs_sized <size in bytes> [optional blocksize]
> > > +# _try_scratch_mkfs_sized <size in bytes> [optional blocksize] [other options]
> > >  _try_scratch_mkfs_sized()
> > >  {
> > >  	local fssize=$1
> > > -	local blocksize=$2
> > > +	shift
> > > +	local blocksize=$1
> > > +	shift
> > >  	local def_blksz
> > >  	local blocksize_opt
> > >  	local rt_ops
> > > @@ -1091,10 +1093,10 @@ _try_scratch_mkfs_sized()
> > >  		# don't override MKFS_OPTIONS that set a block size.
> > >  		echo $MKFS_OPTIONS |grep -E -q "b\s*size="
> > >  		if [ $? -eq 0 ]; then
> > > -			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops
> > > +			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops "$@"
> > >  		else
> > >  			_try_scratch_mkfs_xfs -d size=$fssize $rt_ops \
> > > -				-b size=$blocksize
> > > +				-b size=$blocksize "$@"
> > >  		fi
> > >  		;;
> > >  	ext2|ext3|ext4)
> > > @@ -1105,7 +1107,7 @@ _try_scratch_mkfs_sized()
> > >  				_notrun "Could not make scratch logdev"
> > >  			MKFS_OPTIONS="$MKFS_OPTIONS -J device=$SCRATCH_LOGDEV"
> > >  		fi
> > > -		${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
> > > +		${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
> > >  		;;
> > >  	gfs2)
> > >  		# mkfs.gfs2 doesn't automatically shrink journal files on small
> > > @@ -1120,13 +1122,13 @@ _try_scratch_mkfs_sized()
> > >  			(( journal_size >= min_journal_size )) || journal_size=$min_journal_size
> > >  			MKFS_OPTIONS="-J $journal_size $MKFS_OPTIONS"
> > >  		fi
> > > -		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -O -b $blocksize $SCRATCH_DEV $blocks
> > > +		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -O -b $blocksize "$@" $SCRATCH_DEV $blocks
> > >  		;;
> > >  	ocfs2)
> > > -		yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
> > > +		yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
> > >  		;;
> > >  	udf)
> > > -		$MKFS_UDF_PROG $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
> > > +		$MKFS_UDF_PROG $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
> > >  		;;
> > >  	btrfs)
> > >  		local mixed_opt=
> > > @@ -1134,33 +1136,33 @@ _try_scratch_mkfs_sized()
> > >  		# the device is not zoned. Ref: btrfs-progs: btrfs_min_dev_size()
> > >  		(( fssize < $((256 * 1024 * 1024)) )) &&
> > >  			! _scratch_btrfs_is_zoned && mixed_opt='--mixed'
> > > -		$MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV
> > > +		$MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize "$@" $SCRATCH_DEV
> > >  		;;
> > >  	jfs)
> > > -		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS $SCRATCH_DEV $blocks
> > > +		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS "$@" $SCRATCH_DEV $blocks
> > >  		;;
> > >  	reiserfs)
> > > -		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks
> > > +		${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -b $blocksize "$@" $SCRATCH_DEV $blocks
> > >  		;;
> > >  	reiser4)
> > >  		# mkfs.resier4 requires size in KB as input for creating filesystem
> > > -		$MKFS_REISER4_PROG $MKFS_OPTIONS -y -b $blocksize $SCRATCH_DEV \
> > > +		$MKFS_REISER4_PROG $MKFS_OPTIONS -y -b $blocksize "$@" $SCRATCH_DEV \
> > >  				   `expr $fssize / 1024`
> > >  		;;
> > >  	f2fs)
> > >  		# mkfs.f2fs requires # of sectors as an input for the size
> > >  		local sector_size=`blockdev --getss $SCRATCH_DEV`
> > > -		$MKFS_F2FS_PROG $MKFS_OPTIONS $SCRATCH_DEV `expr $fssize / $sector_size`
> > > +		$MKFS_F2FS_PROG $MKFS_OPTIONS "$@" $SCRATCH_DEV `expr $fssize / $sector_size`
> > >  		;;
> > >  	tmpfs)
> > >  		local free_mem=`_free_memory_bytes`
> > >  		if [ "$free_mem" -lt "$fssize" ] ; then
> > >  		   _notrun "Not enough memory ($free_mem) for tmpfs with $fssize bytes"
> > >  		fi
> > > -		export MOUNT_OPTIONS="-o size=$fssize $TMPFS_MOUNT_OPTIONS"
> > > +		export MOUNT_OPTIONS="-o size=$fssize "$@" $TMPFS_MOUNT_OPTIONS"
> > >  		;;
> > >  	bcachefs)
> > > -		$MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize $blocksize_opt $SCRATCH_DEV
> > > +		$MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize $blocksize_opt "$@" $SCRATCH_DEV
> > >  		;;
> > >  	*)
> > >  		_notrun "Filesystem $FSTYP not supported in _scratch_mkfs_sized"
> > > @@ -1170,7 +1172,7 @@ _try_scratch_mkfs_sized()
> > >  
> > >  _scratch_mkfs_sized()
> > >  {
> > > -	_try_scratch_mkfs_sized $* || _notrun "_scratch_mkfs_sized failed with ($*)"
> > > +	_try_scratch_mkfs_sized "$@" || _notrun "_scratch_mkfs_sized failed with ($@)"
> > 
> > Nit: Don't use '$@' within a longer string -- either it's "$@" so that
> > each element in the arg array is rendered individually as a separate
> > string parameter to the program being called, or "foo $*" so that you
> > end up with a single string.
> 
> Hi Darrick, do you mean I should keep the later $*? Likes:
> 
>   _try_scratch_mkfs_sized "$@" || _notrun "_scratch_mkfs_sized failed with ($*)"

Yes, as you saw in my more recent reposting of this. :)

--D

> > 
> > shellcheck will complain about that, though bash itself doesn't seem to
> > care.
> > 
> > --D
> > 
> > >  }
> > >  
> > >  # Emulate an N-data-disk stripe w/ various stripe units
> > > -- 
> > > 2.45.2
> > > 
> > > 
> > 
> 
> 

Re: [PATCH 2/2] xfs/157: do not drop necessary mkfs options

[Darrick J. Wong]

On Thu, Nov 21, 2024 at 05:35:37PM +0800, Zorro Lang wrote:
> On Mon, Nov 18, 2024 at 02:26:14PM -0800, Darrick J. Wong wrote:
> > On Sun, Nov 17, 2024 at 03:08:00AM +0800, Zorro Lang wrote:
> > > To give the test option "-L oldlabel" to _scratch_mkfs_sized, xfs/157
> > > does:
> > > 
> > >   MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size
> > > 
> > > but the _scratch_mkfs_sized trys to keep the $fs_size, when mkfs
> > > fails with incompatible $MKFS_OPTIONS options, likes this:
> > > 
> > >   ** mkfs failed with extra mkfs options added to "-L oldlabel -m rmapbt=1" by test 157 **
> > >   ** attempting to mkfs using only test 157 options: -d size=524288000 -b size=4096 **
> > > 
> > > but the "-L oldlabel" is necessary, we shouldn't drop it. To avoid
> > > that, we give the "-L oldlabel" to _scratch_mkfs_sized through
> > > function parameters, not through global MKFS_OPTIONS.
> > > 
> > > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > > ---
> > >  tests/xfs/157 | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/tests/xfs/157 b/tests/xfs/157
> > > index 9b5badbae..f8f102d78 100755
> > > --- a/tests/xfs/157
> > > +++ b/tests/xfs/157
> > > @@ -66,8 +66,7 @@ scenario() {
> > >  }
> > >  
> > >  check_label() {
> > > -	MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size \
> > > -		>> $seqres.full
> > > +	_scratch_mkfs_sized "$fs_size" "" "-L oldlabel" >> $seqres.full 2>&1
> > 
> > Don't quote the "-L" and "oldlabel" within the same string unless you
> > want them passed as a single string to _scratch_mkfs.  Right now that
> > works because although you have _scratch_mkfs_sized using "$@"
> 
> I use "$@" just for _scratch_mkfs_sized can give an empty argument to
> _try_scratch_mkfs_sized to be its second argument.
> 
> how about:
> _scratch_mkfs_sized "$fs_size" "" -L oldlabel
> 
> > (doublequote-dollarsign-atsign-doublequote) to pass its arguments intact
> > to _scratch_mkfs, it turns out that _scratch_mkfs just brazely passes $*
> > (with no quoting) to the actual MKFS_PROG which results in any space in
> > any single argument being treated as an argument separator and the
> > string is broken into multiple arguments.
> > 
> > This is why you *can't* do _scratch_mkfs -L "moo cow".
> > 
> > This is also part of why everyone hates bash.
> 
> Hmm... do you need to change the $* of _scratch_mkfs to $@ too?

Yeah, that's something that needs to be done treewide.  The trouble is,
I bet there's other tests out there that have come to rely on the bad
splitting behavior and do things like

_scratch_fubar "-x 555"

becoming

execve("foobar", "-x" "555");

and will break if we try to fix it now.

--D

> > 
> > --D
> > 
> > >  	_scratch_xfs_db -c label
> > >  	_scratch_xfs_admin -L newlabel "$@" >> $seqres.full
> > >  	_scratch_xfs_db -c label
> > > -- 
> > > 2.45.2
> > > 
> > > 
> > 
> 
>