Re: [PATCH 2/2] generic/500: skip this test if formatting fails

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Feb 02, 2023 at 06:00:57PM +0800, Zorro Lang wrote:
> On Tue, Jan 31, 2023 at 04:51:40PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > This testcase exercises what happens when we race a filesystem
> > perforing discard operations against a thin provisioning device that has
> > run out of space.  To constrain runtime, it creates a 128M thinp volume
> > and formats it.
> > 
> > However, if that initial format fails because (say) the 128M volume is
> > too small, then the test fails.  This is really a case of test
> > preconditions not being satisfied, so let's make the test _notrun when
> > this happens.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  common/dmthin     |    7 ++++-
> >  common/rc         |   80 +++++++++++++++++++++++++++--------------------------
> >  tests/generic/500 |    3 +-
> >  3 files changed, 48 insertions(+), 42 deletions(-)
> > 
> > 
> > diff --git a/common/dmthin b/common/dmthin
> > index 91147e47ac..7107d50804 100644
> > --- a/common/dmthin
> > +++ b/common/dmthin
> > @@ -234,5 +234,10 @@ _dmthin_mount()
> >  _dmthin_mkfs()
> >  {
> >  	_scratch_options mkfs
> > -	_mkfs_dev $SCRATCH_OPTIONS $@ $DMTHIN_VOL_DEV
> > +	_mkfs_dev $SCRATCH_OPTIONS "$@" $DMTHIN_VOL_DEV
> 
> This patch adds quote marks to $@, that an extra change. So I hope to know if
> this change fix something wrong or you just felt it's better to have?

Putting double quotes around "$@" (and any other array access) tells
bash that it shouldn't word split any array elements that contain
spaces[1].  Observe:

$ cat /tmp/a.sh
#!/bin/bash

strace -s99 -e execve echo $@
$ /tmp/a.sh 'how now' moo cow
execve("/usr/bin/echo", ["echo", "how", "now", "moo", "cow"], 0x7ffcfb44f218 /* 68 vars */) = 0
how now moo cow
+++ exited with 0 +++

Oops, 'how now' was supposed to be a single argument, but now it is two.

$ cat /tmp/a.sh
#!/bin/bash

strace -s99 -e execve echo "$@"
$ /tmp/a.sh 'how now' moo cow
execve("/usr/bin/echo", ["echo", "how now", "moo", "cow"], 0x7ffccb72d810 /* 68 vars */) = 0
how now moo cow
+++ exited with 0 +++

With the quoted usage, strings with spaces are copied through correctly.
At the moment I don't think this has any practical effect on fstests
because most kernel developers "know" they shouldn't be putting spaces
in anything they feed to fstests, but I've decided that I'll at least
try to fix some of the more egregious problems as I go.

On that topic, does anyone use shellcheck to static-analyze their
fstests code?

--D

[1] https://www.shellcheck.net/wiki/SC2068

> If this part has a good explanation, others good to me, then I'd like to give
> it:
> 
> Reviewed-by: Zorro Lang <zlang@redhat.com>
> 
> Thanks,
> Zorro
> 
> > +}
> > +_dmthin_try_mkfs()
> > +{
> > +	_scratch_options mkfs
> > +	_try_mkfs_dev $SCRATCH_OPTIONS "$@" $DMTHIN_VOL_DEV
> >  }
> > diff --git a/common/rc b/common/rc
> > index 36eb90e1f1..376a0138b4 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -604,49 +604,49 @@ _test_mkfs()
> >      esac
> >  }
> >  
> > +_try_mkfs_dev()
> > +{
> > +    case $FSTYP in
> > +    nfs*)
> > +	# do nothing for nfs
> > +	;;
> > +    9p)
> > +	# do nothing for 9p
> > +	;;
> > +    fuse)
> > +	# do nothing for fuse
> > +	;;
> > +    virtiofs)
> > +	# do nothing for virtiofs
> > +	;;
> > +    overlay)
> > +	# do nothing for overlay
> > +	;;
> > +    pvfs2)
> > +	# do nothing for pvfs2
> > +	;;
> > +    udf)
> > +        $MKFS_UDF_PROG $MKFS_OPTIONS $*
> > +	;;
> > +    btrfs)
> > +        $MKFS_BTRFS_PROG $MKFS_OPTIONS $*
> > +	;;
> > +    ext2|ext3|ext4)
> > +	$MKFS_PROG -t $FSTYP -- -F $MKFS_OPTIONS $*
> > +	;;
> > +    xfs)
> > +	$MKFS_PROG -t $FSTYP -- -f $MKFS_OPTIONS $*
> > +	;;
> > +    *)
> > +	yes | $MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $*
> > +	;;
> > +    esac
> > +}
> > +
> >  _mkfs_dev()
> >  {
> >      local tmp=`mktemp -u`
> > -    case $FSTYP in
> > -    nfs*)
> > -	# do nothing for nfs
> > -	;;
> > -    9p)
> > -	# do nothing for 9p
> > -	;;
> > -    fuse)
> > -	# do nothing for fuse
> > -	;;
> > -    virtiofs)
> > -	# do nothing for virtiofs
> > -	;;
> > -    overlay)
> > -	# do nothing for overlay
> > -	;;
> > -    pvfs2)
> > -	# do nothing for pvfs2
> > -	;;
> > -    udf)
> > -        $MKFS_UDF_PROG $MKFS_OPTIONS $* 2>$tmp.mkfserr 1>$tmp.mkfsstd
> > -	;;
> > -    btrfs)
> > -        $MKFS_BTRFS_PROG $MKFS_OPTIONS $* 2>$tmp.mkfserr 1>$tmp.mkfsstd
> > -	;;
> > -    ext2|ext3|ext4)
> > -	$MKFS_PROG -t $FSTYP -- -F $MKFS_OPTIONS $* \
> > -		2>$tmp.mkfserr 1>$tmp.mkfsstd
> > -	;;
> > -    xfs)
> > -	$MKFS_PROG -t $FSTYP -- -f $MKFS_OPTIONS $* \
> > -		2>$tmp.mkfserr 1>$tmp.mkfsstd
> > -	;;
> > -    *)
> > -	yes | $MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $* \
> > -		2>$tmp.mkfserr 1>$tmp.mkfsstd
> > -	;;
> > -    esac
> > -
> > -    if [ $? -ne 0 ]; then
> > +    if ! _try_mkfs_dev "$@" 2>$tmp.mkfserr 1>$tmp.mkfsstd; then
> >  	# output stored mkfs output
> >  	cat $tmp.mkfserr >&2
> >  	cat $tmp.mkfsstd
> > diff --git a/tests/generic/500 b/tests/generic/500
> > index bc84d219fa..1151c8f234 100755
> > --- a/tests/generic/500
> > +++ b/tests/generic/500
> > @@ -58,7 +58,8 @@ CLUSTER_SIZE=$((64 * 1024 / 512))		# 64K
> >  
> >  _dmthin_init $BACKING_SIZE $VIRTUAL_SIZE $CLUSTER_SIZE 0
> >  _dmthin_set_fail
> > -_dmthin_mkfs
> > +_dmthin_try_mkfs >> $seqres.full 2>&1 || \
> > +	_notrun "Could not format small thinp filesystem for test"
> >  _dmthin_mount
> >  
> >  # There're two bugs at here, one is dm-thin bug, the other is filesystem
> > 
>