Re: [PATCH 5/5] common/rc: print scratch and test mount options

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

On Mon, Jun 24, 2024 at 02:02:33PM +0000, Daniel Gomez wrote:
> On Fri, Jun 14, 2024 at 08:55:55AM GMT, Darrick J. Wong wrote:
> > On Fri, Jun 14, 2024 at 06:17:26AM +0000, Daniel Gomez wrote:
> > > Mount options for a SCRATCH device might not be the same in the TEST
> > > device if RECREATE_TEST_DEV is not enabled. So, print mount options for
> > > each device to clarify this.
> > > 
> > > Add mount and mkfs info for TEST devices so we get the same information
> > > being printed for both devices.
> > > 
> > > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > > ---
> > >  check     |  4 ++++
> > >  common/rc | 20 ++++++++++++++++++--
> > >  2 files changed, 22 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/check b/check
> > > index 723a52e30..e02d28b39 100755
> > > --- a/check
> > > +++ b/check
> > > @@ -807,7 +807,11 @@ function run_section()
> > >  	# print out our test configuration
> > >  	echo "FSTYP         -- `_full_fstyp_details`"
> > >  	echo "PLATFORM      -- `_full_platform_details`"
> > > +	echo "TEST device:"
> > > +	echo "MKFS_OPTIONS  -- `_test_mkfs_options`"
> > > +	echo "MOUNT_OPTIONS -- `_test_mount_options`"
> > >  	if [ ! -z "$SCRATCH_DEV" ]; then
> > > +	  echo "SCRATCH device:"
> > >  	  echo "MKFS_OPTIONS  -- `_scratch_mkfs_options`"
> > >  	  echo "MOUNT_OPTIONS -- `_scratch_mount_options`"
> > 
> > Now there are two lines that start with "MKFS_OPTIONS"; will this break
> > anyone's parsing scripts?  Or should these be prefixed:
> > 
> > echo "TEST_MKFS_OPTIONS -- `_test_mkfs_options`"
> > ...
> > echo "SCRATCH_MKFS_OPTIONS -- `_scratch_mkfs_options`"
> > 
> > ?
> 
> This looks like my initial version, but I prefer the 'sub-menu style' because
> we did not have these variables before. However, I think it makes sense to
> introduce them so we can differentiate between them. 

The trouble is, if your parsing script were doing something like
splitting on "-- " then the "TEST device:" string would not split
properly and you'd have to add context retention of some sort to know
which "MKFS_OPTIONS" this was for.  Better just to namespace the new
lines from the start.  I guess for compatibility's sake then we'd have
to have:

TEST_MKFS_OPTIONS -- -o foo=bar
MKFS_OPTIONS      -- -o foo=baz

in the output, along with a comment somewhere in the source that the
non-prefixed scratch mkfs options is that way for hyst[eo]rical output
parsing reasons.

--D

> I guess introducing this change would break anyone's parsing scripts regardless?
> However, I do think is necessary to specify the mount differences.
> > 
> > Also should these four variables be captured explicitly by the reports
> > that are generated by common/report ?
> 
> I guess the report only includes the scratch mount options. I will update it to
> include the new specific test and scratch mount/mkfs options.
> 
> > 
> > --D
> > 
> > >  	fi
> > > diff --git a/common/rc b/common/rc
> > > index a42792c37..b351a82eb 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -235,6 +235,17 @@ _scratch_mount_options()
> > >  					$SCRATCH_DEV $SCRATCH_MNT
> > >  }
> > >  
> > > +_test_mount_options()
> > > +{
> > > +	_test_options mount
> > > +
> > > +	if $RECREATE_TEST_DEV || [ "$OLD_FSTYP" != "$FSTYP" ]; then
> > > +		echo $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > > +	else
> > > +		echo $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > > +	fi
> > > +}
> > > +
> > >  _supports_filetype()
> > >  {
> > >  	local dir=$1
> > > @@ -456,8 +467,7 @@ _test_mount()
> > >          return $?
> > >      fi
> > >  
> > > -    _test_options mount
> > > -    _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > > +    _mount -t $FSTYP$FUSE_SUBTYP `_test_mount_options $*`
> > >      mount_ret=$?
> > >      [ $mount_ret -ne 0 ] && return $mount_ret
> > >      _idmapped_mount $TEST_DEV $TEST_DIR
> > > @@ -571,6 +581,12 @@ _metadump_dev() {
> > >  	esac
> > >  }
> > >  
> > > +_test_mkfs_options()
> > > +{
> > > +	_test_options mkfs
> > > +	echo $TEST_OPTIONS $MKFS_OPTIONS $* $TEST_DEV
> > > +}
> > > +
> > >  _test_mkfs()
> > >  {
> > >      case $FSTYP in
> > > -- 
> > > 2.43.0
> > >