On Fri, Jun 28, 2024 at 11:13:28AM GMT, Zorro Lang wrote:
> On Mon, Jun 24, 2024 at 01:33:55PM +0000, Daniel Gomez wrote:
> > On Mon, Jun 17, 2024 at 03:06:54PM GMT, Zorro Lang wrote:
> > > On Fri, Jun 14, 2024 at 06:17:25AM +0000, Daniel Gomez wrote:
> > > > Add support for test device recreation (RECREATE_TEST_DEV=true) for
> > > > tmpfs. This allows to inherit the tmpfs profile extra mount options
> > > > (TMPFS_MOUNT_OPTIONS) for the test device.
> > > >
> > > > Signed-off-by: Daniel Gomez <da.gomez@xxxxxxxxxxx>
> > > > ---
> > > > common/rc | 5 ++++-
> > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/common/rc b/common/rc
> > > > index 163041fea..7f995b0fa 100644
> > > > --- a/common/rc
> > > > +++ b/common/rc
> > > > @@ -457,7 +457,7 @@ _test_mount()
> > > > fi
> > > >
> > > > _test_options mount
> > > > - _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > > > + _mount -t $FSTYP$FUSE_SUBTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $TMPFS_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> > >
> > > We shouldn't use a FSTYP specific parameter in a common mount line.
> > > Better to set it in a "$FSTYP=tmpfs" branch. I notice that we set:
> > >
> > > tmpfs)
> > > # We need to specify the size at mount, use 1G by default
> > > echo "-o size=1G $TMPFS_MOUNT_OPTIONS"
> > >
> > > in _common_mount_opts(). Then it will be set in MOUNT_OPTIONS and
> > > TEST_FS_MOUNT_OPTS, won't that help?
> >
> > No for the recreation path with specific section variables as we have not yet
> > source '. common/rc'. Dropping changes in _test_mount() and force reading test
> > mount options makes $TMPFS_MOUNT_OPTION available for the test in the recreation
> > path.
> >
> > diff --git a/check b/check
> > index 723a52e30..b3f7d7b14 100755
> > --- a/check
> > +++ b/check
> > @@ -773,6 +773,7 @@ function run_section()
> > status=1
> > exit
> > fi
> > + _test_mount_opts
>
> Actually the get_next_config function (from common/config) should have reloaded
> the mount and mkfs paramters.
>
> run_section()
> {
> ...
> get_next_config $section
> ...
>
> if $RECREATE_TEST_DEV || [ "$OLD_FSTYP" != "$FSTYP" ]; then
> ...
> if ! _test_mkfs >$tmp.err 2>&1
> ...
> if ! _test_mount
> ...
> }
>
> But why it doesn't?
>
> Oh, due to the get_next_config does things as this:
>
> get_next_config() {
> ...
> unset MOUNT_OPTIONS
> unset TEST_FS_MOUNT_OPTS
> unset MKFS_OPTIONS
> unset FSCK_OPTIONS
> ...
> if [ ! -z "$OLD_FSTYP" ] && [ $OLD_FSTYP != $FSTYP ]; then
> [ -z "$MOUNT_OPTIONS" ] && _mount_opts
> [ -z "$TEST_FS_MOUNT_OPTS" ] && _test_mount_opts
> [ -z "$MKFS_OPTIONS" ] && _mkfs_opts
> [ -z "$FSCK_OPTIONS" ] && _fsck_opts
> ...
> else
> [ -z "$MOUNT_OPTIONS" ] && export MOUNT_OPTIONS=$OLD_MOUNT_OPTIONS
> [ -z "$TEST_FS_MOUNT_OPTS" ] && export TEST_FS_MOUNT_OPTS=$OLD_TEST_FS_MOUNT_OPTS
> [ -z "$MKFS_OPTIONS" ] && export MKFS_OPTIONS=$OLD_MKFS_OPTIONS
> [ -z "$FSCK_OPTIONS" ] && export FSCK_OPTIONS=$OLD_FSCK_OPTIONS
> [ -z "$USE_EXTERNAL" ] && export USE_EXTERNAL=$OLD_USE_EXTERNAL
> fi
> ...
> }
>
> Only if the later FSTYP is different with the former one, then get_next_config
> reloads those OPTIONS.
>
> 1) That helps you don't need to set duplicated paramters in each section.
> 2) But that causes you can't reset/reload something likes TEST_FS_MOUNT_OPTS,
> except you set MOUNT_OPTIONS, TEST_FS_MOUNT_OPTS, ... directly.
>
> I'm not sure how many people depends on the 1)st behavior long time. And how
> many people hope to remove the limitation of [ $OLD_FSTYP != $FSTYP ]. Any change
> will be default behavior change.
>
> But we might can bring the $RECREATE_TEST_DEV into get_next_config(), likes:
> if $RECREATE_TEST_DEV || ([ -n "$OLD_FSTYP" ] && [ "$OLD_FSTYP" != "$FSTYP" ])
>
> I didn't give it a test, any better ideas are welcome :)
This looks good to me to allow parsing specific fs mount options in sections.
I've also tested and works for my test case. Below my changes after dropping
the above:
diff --git a/common/config b/common/config
index 43b32b93d..c739c4578 100644
--- a/common/config
+++ b/common/config
@@ -802,7 +802,7 @@ get_next_config() {
fi
parse_config_section $1
- if [ ! -z "$OLD_FSTYP" ] && [ $OLD_FSTYP != $FSTYP ]; then
+ if $RECREATE_TEST_DEV || ([ ! -z "$OLD_FSTYP" ] && [ $OLD_FSTYP != $FSTYP ]); then
[ -z "$MOUNT_OPTIONS" ] && _mount_opts
[ -z "$TEST_FS_MOUNT_OPTS" ] && _test_mount_opts
[ -z "$MKFS_OPTIONS" ] && _mkfs_opts
I think this is also the original intention of the recreation TEST_DEV
(commit [1]).
[1] f8e4f532f18d7517430d9849bfc042305d7f7f4d (check: Allow to recreate
TEST_DEV)
> Thanks,
> Zorro
>
> > if ! _test_mount
> > then
> > echo "check: failed to mount $TEST_DEV on $TEST_DIR"
> >
> > I understand from Darrick's comment that the change above in _test_mount() is
> > wrong. Would this solution be okay? Not sure if this is a bug but I'd guess all
> > specific mount options are now ignored in the recreation path (not only tmpfs).
>
>
>
> >
> > >
> > > Thanks,
> > > Zorro
> > >
> > >
> > > > mount_ret=$?
> > > > [ $mount_ret -ne 0 ] && return $mount_ret
> > > > _idmapped_mount $TEST_DEV $TEST_DIR
> > > > @@ -604,6 +604,9 @@ _test_mkfs()
> > > > pvfs2)
> > > > # do nothing for pvfs2
> > > > ;;
> > > > + tmpfs)
> > > > + # do nothing for tmpfs
> > > > + ;;
> > > > udf)
> > > > $MKFS_UDF_PROG $MKFS_OPTIONS $* $TEST_DEV > /dev/null
> > > > ;;
> > > > --
> > > > 2.43.0
> > > >
> > >
> >
>
