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@xxxxxxxxxxx>
> > > ---
> > > 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
> > >
