| Message ID | 1486932224-17075-2-git-send-email-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Feb 12, 2017 at 10:43:36PM +0200, Amir Goldstein wrote: > When $TEST_DEV is mounted at a different location then $TEST_DIR, > _require_test() aborts the test with an error: > TEST_DEV=/dev/sda5 is mounted but not on TEST_DIR=/mnt/test > > There are several problems with current sanity check: > 1. the output of the error is mixed into out.bad and hard to see > 2. the test partition is unmounted at the end of the test regardless > of the fact that it not pass the sanity that we have exclusivity > 3. scratch partition has a similar sanity check in _require_scratch(), > but we may not get to it, because $SCRATCH_DEV is unmounted prior > to running the tests (which could unmount another mount point). > > To solve all these problems, introduce a helper _check_mounted_on(). > It checks if a device is mounted on a given mount point and optionally > checks the mounted fs type. > > The sanity checks in _require_scratch() and _require_test() are > converted to use the helper and gain the check for correct fs type. > > The helper is used in init_rc() to sanity check both test and scratch > partitions, before tests are run and before $SCRATCH_DEV is unmounted. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > common/rc | 83 +++++++++++++++++++++++++++++++++++++-------------------------- > 1 file changed, 49 insertions(+), 34 deletions(-) > > diff --git a/common/rc b/common/rc > index 76515e2..d831b17 100644 > --- a/common/rc > +++ b/common/rc > @@ -1319,6 +1319,43 @@ _supported_os() > _notrun "not suitable for this OS: $HOSTOS" > } > > +# check if a FS on a device is mounted > +# if so, verify that it is mounted on mount point > +# if fstype is given as argument, verify that it is also > +# mounted with correct fs type > +# > +_check_mounted_on() > +{ > + local devname=$1 > + local dev=$2 > + local mntname=$3 > + local mnt=$4 > + local type=$5 > + > + # Note that we use -F here so grep doesn't try to interpret an NFS over > + # IPv6 server as a regular expression > + local mount_rec=`_mount | grep -F "$dev"` > + [ -n "$mount_rec" ] || return 1 > + > + # if it's mounted, make sure its on $mnt > + if ! (echo $mount_rec | grep -q "$mnt") > + then > + echo "$devname=$dev is mounted but not on $mntname=$mnt - aborting" > + echo "Already mounted result:" > + echo $mount_rec > + exit 1 > + fi > + > + if [ -n "$type" -a "`_fs_type $dev`" != "$type" ] > + then > + echo "$devname=$dev is mounted but not a type $type filesystem" > + # raw $DF_PROG cannot handle NFS/CIFS/overlay correctly > + _df_device $dev > + exit 1 > + fi > + return 0 > +} > + > # this test needs a scratch partition - check we're ok & unmount it > # No post-test check of the device is required. e.g. the test intentionally > # finishes the test with the filesystem in a corrupt state > @@ -1373,21 +1410,9 @@ _require_scratch_nocheck() > ;; > esac > > - # mounted? > - # Note that we use -F here so grep doesn't try to interpret an NFS over > - # IPv6 server as a regular expression. > - mount_rec=`_mount | grep -F $SCRATCH_DEV` > - if [ "$mount_rec" ] > + if _check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT > then > - # if it's mounted, make sure its on $SCRATCH_MNT > - if ! echo $mount_rec | grep -q $SCRATCH_MNT > - then > - echo "\$SCRATCH_DEV=$SCRATCH_DEV is mounted but not on \$SCRATCH_MNT=$SCRATCH_MNT - aborting" > - echo "Already mounted result:" > - echo $mount_rec > - exit 1 > - fi > - # and then unmount it > + # if it's mounted, unmount it > if ! _scratch_unmount > then > echo "failed to unmount $SCRATCH_DEV" > @@ -1458,21 +1483,8 @@ _require_test() > ;; > esac > > - # mounted? > - # Note that we use -F here so grep doesn't try to interpret an NFS over > - # IPv6 server as a regular expression. > - mount_rec=`_mount | grep -F $TEST_DEV` > - if [ "$mount_rec" ] > + if ! _check_mounted_on TEST_DEV $TEST_DEV TEST_DIR $TEST_DIR > then > - # if it's mounted, make sure its on $TEST_DIR > - if ! echo $mount_rec | grep -q $TEST_DIR > - then > - echo "\$TEST_DEV=$TEST_DEV is mounted but not on \$TEST_DIR=$TEST_DIR - aborting" > - echo "Already mounted result:" > - echo $mount_rec > - exit 1 > - fi > - else > out=`_mount_or_remount_rw "$MOUNT_OPTIONS" $TEST_DEV $TEST_DIR` > if [ $? -ne 1 ]; then > echo $out > @@ -3075,13 +3087,16 @@ init_rc() > fi > fi > > - if [ "`_fs_type $TEST_DEV`" != "$FSTYP" ] > - then > - echo "common/rc: Error: \$TEST_DEV ($TEST_DEV) is not a MOUNTED $FSTYP filesystem" > - # raw $DF_PROG cannot handle NFS/CIFS/overlay correctly > - _df_device $TEST_DEV > - exit 1 > + # Sanity check that TEST partition is not mounted at another mount point > + # or as another fs type > + _check_mounted_on TEST_DEV $TEST_DEV TEST_DIR $TEST_DIR $FSTYP > + if [ -n "$SCRATCH_DEV" ]; then > + # Sanity check that SCRATCH partition is not mounted at another > + # mount point, because it is about to be unmounted and formatted. > + # Another fs type for scratch is fine (bye bye old fs type). > + _check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT > fi My test configs look like: TEST_DEV=/dev/sda5 SCRATCH_DEV=/dev/sda6 TEST_DIR=/mnt/testarea/test SCRATCH_MNT=/mnt/testarea/scratch and if I mount SCRATCH_DEV at /mnt/xfs (or some other mountpoints rather than SCRATCH_MNT), ./check -overlay overlay/??? isn't able to detect this mis-configuration. [root@dhcp-66-86-11 xfstests]# ./check -overlay overlay/002 FSTYP -- overlay PLATFORM -- Linux/x86_64 dhcp-66-86-11 4.10.0-rc7 MKFS_OPTIONS -- /mnt/testarea/scratch MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 -o lowerdir=/mnt/testarea/scratch/ovl-lower,upperdir=/mnt/testarea/scratch/ovl-upper,workdir=/mnt/testarea/scratch/ovl-work [root@dhcp-66-86-11 xfstests]# And nothing useful was printed. This is because my rootfs has no filetype support, but the _notrun message is redirected to a file in check, as "if ! _scratch_mkfs >$tmp.err 2>&1" Adding _check_mounted_on against OVL_BASE_TEST/SCRATCH_DEV here could fix it for me. Thanks, Eryu > + > # Figure out if we need to add -F ("foreign", deprecated) option to xfs_io > $XFS_IO_PROG -c stat $TEST_DIR 2>&1 | grep -q "is not on an XFS filesystem" && \ > export XFS_IO_PROG="$XFS_IO_PROG -F" > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 13, 2017 at 1:10 PM, Eryu Guan <eguan@redhat.com> wrote: > On Sun, Feb 12, 2017 at 10:43:36PM +0200, Amir Goldstein wrote: >> When $TEST_DEV is mounted at a different location then $TEST_DIR, >> _require_test() aborts the test with an error: >> TEST_DEV=/dev/sda5 is mounted but not on TEST_DIR=/mnt/test >> >> There are several problems with current sanity check: >> 1. the output of the error is mixed into out.bad and hard to see >> 2. the test partition is unmounted at the end of the test regardless >> of the fact that it not pass the sanity that we have exclusivity >> 3. scratch partition has a similar sanity check in _require_scratch(), >> but we may not get to it, because $SCRATCH_DEV is unmounted prior >> to running the tests (which could unmount another mount point). >> >> To solve all these problems, introduce a helper _check_mounted_on(). >> It checks if a device is mounted on a given mount point and optionally >> checks the mounted fs type. >> >> The sanity checks in _require_scratch() and _require_test() are >> converted to use the helper and gain the check for correct fs type. >> >> The helper is used in init_rc() to sanity check both test and scratch >> partitions, before tests are run and before $SCRATCH_DEV is unmounted. >> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >> --- >> common/rc | 83 +++++++++++++++++++++++++++++++++++++-------------------------- >> 1 file changed, 49 insertions(+), 34 deletions(-) ... > My test configs look like: > > TEST_DEV=/dev/sda5 > SCRATCH_DEV=/dev/sda6 > TEST_DIR=/mnt/testarea/test > SCRATCH_MNT=/mnt/testarea/scratch > > and if I mount SCRATCH_DEV at /mnt/xfs (or some other mountpoints rather > than SCRATCH_MNT), ./check -overlay overlay/??? isn't able to detect > this mis-configuration. > > [root@dhcp-66-86-11 xfstests]# ./check -overlay overlay/002 > FSTYP -- overlay > PLATFORM -- Linux/x86_64 dhcp-66-86-11 4.10.0-rc7 > MKFS_OPTIONS -- /mnt/testarea/scratch > MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 -o lowerdir=/mnt/testarea/scratch/ovl-lower,upperdir=/mnt/testarea/scratch/ovl-upper,workdir=/mnt/testarea/scratch/ovl-work > > [root@dhcp-66-86-11 xfstests]# > > And nothing useful was printed. This is because my rootfs has no > filetype support, but the _notrun message is redirected to a file in > check, as > > "if ! _scratch_mkfs >$tmp.err 2>&1" > > Adding _check_mounted_on against OVL_BASE_TEST/SCRATCH_DEV here could > fix it for me. > Actually, there that test already exists in: _scratch_mkfs _scratch_cleanup_files _overlay_base_scratch_mount _check_mounted_on Only I forgot to cleanup an unneeded _overlay_base_scratch_unmount() from _scratch_cleanup_files().. @@ -698,8 +698,6 @@ _scratch_cleanup_files() overlay) # Avoid rm -rf /* if we messed up [ -n "$OVL_BASE_SCRATCH_MNT" ] || return 1 - # overlay 'mkfs' needs to make sure base fs is mounted and clean - _overlay_base_scratch_unmount 2>/dev/null _overlay_base_scratch_mount rm -rf $OVL_BASE_SCRATCH_MNT/* _overlay_mkdirs $OVL_BASE_SCRATCH_MNT With this patch, test does abort for _check_mounted_on() failure, (please verify) but output is still lost inside tmp.err. To fix this I would need to not exit 1 inside _check_mounted_on() but always by callers. Is that what you prefer that I do? -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 13, 2017 at 1:44 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Mon, Feb 13, 2017 at 1:10 PM, Eryu Guan <eguan@redhat.com> wrote: >> On Sun, Feb 12, 2017 at 10:43:36PM +0200, Amir Goldstein wrote: >>> When $TEST_DEV is mounted at a different location then $TEST_DIR, >>> _require_test() aborts the test with an error: >>> TEST_DEV=/dev/sda5 is mounted but not on TEST_DIR=/mnt/test >>> >>> There are several problems with current sanity check: >>> 1. the output of the error is mixed into out.bad and hard to see >>> 2. the test partition is unmounted at the end of the test regardless >>> of the fact that it not pass the sanity that we have exclusivity >>> 3. scratch partition has a similar sanity check in _require_scratch(), >>> but we may not get to it, because $SCRATCH_DEV is unmounted prior >>> to running the tests (which could unmount another mount point). >>> >>> To solve all these problems, introduce a helper _check_mounted_on(). >>> It checks if a device is mounted on a given mount point and optionally >>> checks the mounted fs type. >>> >>> The sanity checks in _require_scratch() and _require_test() are >>> converted to use the helper and gain the check for correct fs type. >>> >>> The helper is used in init_rc() to sanity check both test and scratch >>> partitions, before tests are run and before $SCRATCH_DEV is unmounted. >>> >>> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >>> --- >>> common/rc | 83 +++++++++++++++++++++++++++++++++++++-------------------------- >>> 1 file changed, 49 insertions(+), 34 deletions(-) > > ... > >> My test configs look like: >> >> TEST_DEV=/dev/sda5 >> SCRATCH_DEV=/dev/sda6 >> TEST_DIR=/mnt/testarea/test >> SCRATCH_MNT=/mnt/testarea/scratch >> >> and if I mount SCRATCH_DEV at /mnt/xfs (or some other mountpoints rather >> than SCRATCH_MNT), ./check -overlay overlay/??? isn't able to detect >> this mis-configuration. >> >> [root@dhcp-66-86-11 xfstests]# ./check -overlay overlay/002 >> FSTYP -- overlay >> PLATFORM -- Linux/x86_64 dhcp-66-86-11 4.10.0-rc7 >> MKFS_OPTIONS -- /mnt/testarea/scratch >> MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 -o lowerdir=/mnt/testarea/scratch/ovl-lower,upperdir=/mnt/testarea/scratch/ovl-upper,workdir=/mnt/testarea/scratch/ovl-work >> >> [root@dhcp-66-86-11 xfstests]# >> >> And nothing useful was printed. This is because my rootfs has no >> filetype support, but the _notrun message is redirected to a file in >> check, as >> >> "if ! _scratch_mkfs >$tmp.err 2>&1" >> >> Adding _check_mounted_on against OVL_BASE_TEST/SCRATCH_DEV here could >> fix it for me. >> > > Actually, there that test already exists in: > > _scratch_mkfs > _scratch_cleanup_files > _overlay_base_scratch_mount > _check_mounted_on > > Only I forgot to cleanup an unneeded _overlay_base_scratch_unmount() > from _scratch_cleanup_files().. > > @@ -698,8 +698,6 @@ _scratch_cleanup_files() > overlay) > # Avoid rm -rf /* if we messed up > [ -n "$OVL_BASE_SCRATCH_MNT" ] || return 1 > - # overlay 'mkfs' needs to make sure base fs is mounted and clean > - _overlay_base_scratch_unmount 2>/dev/null > _overlay_base_scratch_mount > rm -rf $OVL_BASE_SCRATCH_MNT/* > _overlay_mkdirs $OVL_BASE_SCRATCH_MNT > > With this patch, test does abort for _check_mounted_on() failure, > (please verify) > but output is still lost inside tmp.err. > To fix this I would need to not exit 1 inside _check_mounted_on() but > always by callers. > Is that what you prefer that I do? How about something more general like this: (tested with your test case and with wipefs -a $SCRATCH_DEV): @@ -376,6 +376,11 @@ _wrapup() seq="check" check="$RESULT_BASE/check" + if [ -n "$check_err" ]; then + echo "check: $check_err" + check_err="" + fi + @@ -466,6 +471,7 @@ _check_filesystems() _prepare_test_list +check_err="" if $OPTIONS_HAVE_SECTIONS; then trap "_summary; exit \$status" 0 1 2 3 15 else @@ -567,10 +576,10 @@ for section in $HOST_OPTIONS_SECTIONS; do # call the overridden mkfs - make sure the FS is built # the same as we'll create it later. - if ! _scratch_mkfs >$tmp.err 2>&1 + check_err=`_scratch_mkfs 2>&1` + if [ $? -ne 0 ] then echo "our local _scratch_mkfs routine ..." - cat $tmp.err echo "check: failed to mkfs \$SCRATCH_DEV using specified options" status=1 exit @@ -578,14 +587,15 @@ for section in $HOST_OPTIONS_SECTIONS; do # call the overridden mount - make sure the FS mounts with # the same options that we'll mount with later. - if ! _scratch_mount >$tmp.err 2>&1 + check_err=`_scratch_mount 2>&1` + if [ $? -ne 0 ] then echo "our local mount routine ..." - cat $tmp.err echo "check: failed to mount \$SCRATCH_DEV using specified options" status=1 exit fi + check_err="" fi -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 13, 2017 at 03:33:23PM +0200, Amir Goldstein wrote: > On Mon, Feb 13, 2017 at 1:44 PM, Amir Goldstein <amir73il@gmail.com> wrote: > > On Mon, Feb 13, 2017 at 1:10 PM, Eryu Guan <eguan@redhat.com> wrote: > >> On Sun, Feb 12, 2017 at 10:43:36PM +0200, Amir Goldstein wrote: > >>> When $TEST_DEV is mounted at a different location then $TEST_DIR, > >>> _require_test() aborts the test with an error: > >>> TEST_DEV=/dev/sda5 is mounted but not on TEST_DIR=/mnt/test > >>> > >>> There are several problems with current sanity check: > >>> 1. the output of the error is mixed into out.bad and hard to see > >>> 2. the test partition is unmounted at the end of the test regardless > >>> of the fact that it not pass the sanity that we have exclusivity > >>> 3. scratch partition has a similar sanity check in _require_scratch(), > >>> but we may not get to it, because $SCRATCH_DEV is unmounted prior > >>> to running the tests (which could unmount another mount point). > >>> > >>> To solve all these problems, introduce a helper _check_mounted_on(). > >>> It checks if a device is mounted on a given mount point and optionally > >>> checks the mounted fs type. > >>> > >>> The sanity checks in _require_scratch() and _require_test() are > >>> converted to use the helper and gain the check for correct fs type. > >>> > >>> The helper is used in init_rc() to sanity check both test and scratch > >>> partitions, before tests are run and before $SCRATCH_DEV is unmounted. > >>> > >>> Signed-off-by: Amir Goldstein <amir73il@gmail.com> > >>> --- > >>> common/rc | 83 +++++++++++++++++++++++++++++++++++++-------------------------- > >>> 1 file changed, 49 insertions(+), 34 deletions(-) > > > > ... > > > >> My test configs look like: > >> > >> TEST_DEV=/dev/sda5 > >> SCRATCH_DEV=/dev/sda6 > >> TEST_DIR=/mnt/testarea/test > >> SCRATCH_MNT=/mnt/testarea/scratch > >> > >> and if I mount SCRATCH_DEV at /mnt/xfs (or some other mountpoints rather > >> than SCRATCH_MNT), ./check -overlay overlay/??? isn't able to detect > >> this mis-configuration. > >> > >> [root@dhcp-66-86-11 xfstests]# ./check -overlay overlay/002 > >> FSTYP -- overlay > >> PLATFORM -- Linux/x86_64 dhcp-66-86-11 4.10.0-rc7 > >> MKFS_OPTIONS -- /mnt/testarea/scratch > >> MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 -o lowerdir=/mnt/testarea/scratch/ovl-lower,upperdir=/mnt/testarea/scratch/ovl-upper,workdir=/mnt/testarea/scratch/ovl-work > >> > >> [root@dhcp-66-86-11 xfstests]# > >> > >> And nothing useful was printed. This is because my rootfs has no > >> filetype support, but the _notrun message is redirected to a file in > >> check, as > >> > >> "if ! _scratch_mkfs >$tmp.err 2>&1" > >> > >> Adding _check_mounted_on against OVL_BASE_TEST/SCRATCH_DEV here could > >> fix it for me. > >> > > > > Actually, there that test already exists in: > > > > _scratch_mkfs > > _scratch_cleanup_files > > _overlay_base_scratch_mount > > _check_mounted_on Hmm, I don't think this kind of basic config sanity check belongs here, this should be done in config and env setup time. (So I think _overlay_mount should be fixed too, that _supports_filetype check doesn't belong there either.) How about adding these checks in init_rc, along with other _check_mounted_on checks against TEST_DEV and SCRATCH_DEV? > > > > Only I forgot to cleanup an unneeded _overlay_base_scratch_unmount() > > from _scratch_cleanup_files().. > > > > @@ -698,8 +698,6 @@ _scratch_cleanup_files() > > overlay) > > # Avoid rm -rf /* if we messed up > > [ -n "$OVL_BASE_SCRATCH_MNT" ] || return 1 > > - # overlay 'mkfs' needs to make sure base fs is mounted and clean > > - _overlay_base_scratch_unmount 2>/dev/null > > _overlay_base_scratch_mount > > rm -rf $OVL_BASE_SCRATCH_MNT/* > > _overlay_mkdirs $OVL_BASE_SCRATCH_MNT > > > > With this patch, test does abort for _check_mounted_on() failure, > > (please verify) Yes, this works. Thanks, Eryu > > but output is still lost inside tmp.err. > > To fix this I would need to not exit 1 inside _check_mounted_on() but > > always by callers. > > Is that what you prefer that I do? > > How about something more general like this: > (tested with your test case and with wipefs -a $SCRATCH_DEV): > > @@ -376,6 +376,11 @@ _wrapup() > seq="check" > check="$RESULT_BASE/check" > > + if [ -n "$check_err" ]; then > + echo "check: $check_err" > + check_err="" > + fi > + > > @@ -466,6 +471,7 @@ _check_filesystems() > > _prepare_test_list > > +check_err="" > if $OPTIONS_HAVE_SECTIONS; then > trap "_summary; exit \$status" 0 1 2 3 15 > else > > @@ -567,10 +576,10 @@ for section in $HOST_OPTIONS_SECTIONS; do > # call the overridden mkfs - make sure the FS is built > # the same as we'll create it later. > > - if ! _scratch_mkfs >$tmp.err 2>&1 > + check_err=`_scratch_mkfs 2>&1` > + if [ $? -ne 0 ] > then > echo "our local _scratch_mkfs routine ..." > - cat $tmp.err > echo "check: failed to mkfs \$SCRATCH_DEV using specified options" > status=1 > exit > @@ -578,14 +587,15 @@ for section in $HOST_OPTIONS_SECTIONS; do > > # call the overridden mount - make sure the FS mounts with > # the same options that we'll mount with later. > - if ! _scratch_mount >$tmp.err 2>&1 > + check_err=`_scratch_mount 2>&1` > + if [ $? -ne 0 ] > then > echo "our local mount routine ..." > - cat $tmp.err > echo "check: failed to mount \$SCRATCH_DEV using > specified options" > status=1 > exit > fi > + check_err="" > fi -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 14, 2017 at 7:51 AM, Eryu Guan <eguan@redhat.com> wrote: > On Mon, Feb 13, 2017 at 03:33:23PM +0200, Amir Goldstein wrote: >> On Mon, Feb 13, 2017 at 1:44 PM, Amir Goldstein <amir73il@gmail.com> wrote: >> > On Mon, Feb 13, 2017 at 1:10 PM, Eryu Guan <eguan@redhat.com> wrote: >> >> On Sun, Feb 12, 2017 at 10:43:36PM +0200, Amir Goldstein wrote: >> >>> When $TEST_DEV is mounted at a different location then $TEST_DIR, >> >>> _require_test() aborts the test with an error: >> >>> TEST_DEV=/dev/sda5 is mounted but not on TEST_DIR=/mnt/test >> >>> >> >>> There are several problems with current sanity check: >> >>> 1. the output of the error is mixed into out.bad and hard to see >> >>> 2. the test partition is unmounted at the end of the test regardless >> >>> of the fact that it not pass the sanity that we have exclusivity >> >>> 3. scratch partition has a similar sanity check in _require_scratch(), >> >>> but we may not get to it, because $SCRATCH_DEV is unmounted prior >> >>> to running the tests (which could unmount another mount point). >> >>> >> >>> To solve all these problems, introduce a helper _check_mounted_on(). >> >>> It checks if a device is mounted on a given mount point and optionally >> >>> checks the mounted fs type. >> >>> >> >>> The sanity checks in _require_scratch() and _require_test() are >> >>> converted to use the helper and gain the check for correct fs type. >> >>> >> >>> The helper is used in init_rc() to sanity check both test and scratch >> >>> partitions, before tests are run and before $SCRATCH_DEV is unmounted. >> >>> >> >>> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >> >>> --- >> >>> common/rc | 83 +++++++++++++++++++++++++++++++++++++-------------------------- >> >>> 1 file changed, 49 insertions(+), 34 deletions(-) >> > >> > ... >> > >> >> My test configs look like: >> >> >> >> TEST_DEV=/dev/sda5 >> >> SCRATCH_DEV=/dev/sda6 >> >> TEST_DIR=/mnt/testarea/test >> >> SCRATCH_MNT=/mnt/testarea/scratch >> >> >> >> and if I mount SCRATCH_DEV at /mnt/xfs (or some other mountpoints rather >> >> than SCRATCH_MNT), ./check -overlay overlay/??? isn't able to detect >> >> this mis-configuration. >> >> >> >> [root@dhcp-66-86-11 xfstests]# ./check -overlay overlay/002 >> >> FSTYP -- overlay >> >> PLATFORM -- Linux/x86_64 dhcp-66-86-11 4.10.0-rc7 >> >> MKFS_OPTIONS -- /mnt/testarea/scratch >> >> MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 -o lowerdir=/mnt/testarea/scratch/ovl-lower,upperdir=/mnt/testarea/scratch/ovl-upper,workdir=/mnt/testarea/scratch/ovl-work >> >> >> >> [root@dhcp-66-86-11 xfstests]# >> >> >> >> And nothing useful was printed. This is because my rootfs has no >> >> filetype support, but the _notrun message is redirected to a file in >> >> check, as >> >> >> >> "if ! _scratch_mkfs >$tmp.err 2>&1" >> >> >> >> Adding _check_mounted_on against OVL_BASE_TEST/SCRATCH_DEV here could >> >> fix it for me. >> >> >> > >> > Actually, there that test already exists in: >> > >> > _scratch_mkfs >> > _scratch_cleanup_files >> > _overlay_base_scratch_mount >> > _check_mounted_on > > Hmm, I don't think this kind of basic config sanity check belongs here, > this should be done in config and env setup time. (So I think > _overlay_mount should be fixed too, that _supports_filetype check > doesn't belong there either.) > > How about adding these checks in init_rc, along with other > _check_mounted_on checks against TEST_DEV and SCRATCH_DEV? > Yes, that makes sense. But I still wonder how "exit 1" from within helpers should be handled from check when stdout/err are redirected to $tmp.err. Trying to catch the config error earlier is a good practice, but it won't ensure against the same type of problem in the future. What did you think about my approach to store mkfs output in $check_err var instead of $tmp.err file and spew $check_err in _wrapup? BTW I tries 'cat $tmp.err' in _wrapup, but output is still redirected to $tmp.err while in trap, so cat says: "cat: input file is output file". -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 14, 2017 at 08:02:27AM +0200, Amir Goldstein wrote: [snip] > >> >> My test configs look like: > >> >> > >> >> TEST_DEV=/dev/sda5 > >> >> SCRATCH_DEV=/dev/sda6 > >> >> TEST_DIR=/mnt/testarea/test > >> >> SCRATCH_MNT=/mnt/testarea/scratch > >> >> > >> >> and if I mount SCRATCH_DEV at /mnt/xfs (or some other mountpoints rather > >> >> than SCRATCH_MNT), ./check -overlay overlay/??? isn't able to detect > >> >> this mis-configuration. > >> >> > >> >> [root@dhcp-66-86-11 xfstests]# ./check -overlay overlay/002 > >> >> FSTYP -- overlay > >> >> PLATFORM -- Linux/x86_64 dhcp-66-86-11 4.10.0-rc7 > >> >> MKFS_OPTIONS -- /mnt/testarea/scratch > >> >> MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 -o lowerdir=/mnt/testarea/scratch/ovl-lower,upperdir=/mnt/testarea/scratch/ovl-upper,workdir=/mnt/testarea/scratch/ovl-work > >> >> > >> >> [root@dhcp-66-86-11 xfstests]# > >> >> > >> >> And nothing useful was printed. This is because my rootfs has no > >> >> filetype support, but the _notrun message is redirected to a file in > >> >> check, as > >> >> > >> >> "if ! _scratch_mkfs >$tmp.err 2>&1" > >> >> > >> >> Adding _check_mounted_on against OVL_BASE_TEST/SCRATCH_DEV here could > >> >> fix it for me. > >> >> > >> > > >> > Actually, there that test already exists in: > >> > > >> > _scratch_mkfs > >> > _scratch_cleanup_files > >> > _overlay_base_scratch_mount > >> > _check_mounted_on > > > > Hmm, I don't think this kind of basic config sanity check belongs here, > > this should be done in config and env setup time. (So I think > > _overlay_mount should be fixed too, that _supports_filetype check > > doesn't belong there either.) > > > > How about adding these checks in init_rc, along with other > > _check_mounted_on checks against TEST_DEV and SCRATCH_DEV? > > > > Yes, that makes sense. > > But I still wonder how "exit 1" from within helpers should be handled > from check when stdout/err are redirected to $tmp.err. > Trying to catch the config error earlier is a good practice, but > it won't ensure against the same type of problem in the future. > > What did you think about my approach to store mkfs output in $check_err > var instead of $tmp.err file and spew $check_err in _wrapup? > BTW I tries 'cat $tmp.err' in _wrapup, but output is still redirected to > $tmp.err while in trap, so cat says: "cat: input file is output file". I don't think we need to worry about that or handle it. IMO, if we redirect a helper that does "exit", we're doing something wrong, so either fix the redirection or fix the helper (just return not exit). I went through helpers in common/rc roughly, except the _overlay_mount issue, seems we don't have other such problems right now :) Thanks, Eryu -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 14, 2017 at 9:23 AM, Eryu Guan <eguan@redhat.com> wrote: > On Tue, Feb 14, 2017 at 08:02:27AM +0200, Amir Goldstein wrote: > [snip] >> >> >> My test configs look like: >> >> >> >> >> >> TEST_DEV=/dev/sda5 >> >> >> SCRATCH_DEV=/dev/sda6 >> >> >> TEST_DIR=/mnt/testarea/test >> >> >> SCRATCH_MNT=/mnt/testarea/scratch >> >> >> >> >> >> and if I mount SCRATCH_DEV at /mnt/xfs (or some other mountpoints rather >> >> >> than SCRATCH_MNT), ./check -overlay overlay/??? isn't able to detect >> >> >> this mis-configuration. >> >> >> >> >> >> [root@dhcp-66-86-11 xfstests]# ./check -overlay overlay/002 >> >> >> FSTYP -- overlay >> >> >> PLATFORM -- Linux/x86_64 dhcp-66-86-11 4.10.0-rc7 >> >> >> MKFS_OPTIONS -- /mnt/testarea/scratch >> >> >> MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 -o lowerdir=/mnt/testarea/scratch/ovl-lower,upperdir=/mnt/testarea/scratch/ovl-upper,workdir=/mnt/testarea/scratch/ovl-work >> >> >> >> >> >> [root@dhcp-66-86-11 xfstests]# >> >> >> >> >> >> And nothing useful was printed. This is because my rootfs has no >> >> >> filetype support, but the _notrun message is redirected to a file in >> >> >> check, as >> >> >> >> >> >> "if ! _scratch_mkfs >$tmp.err 2>&1" >> >> >> >> >> >> Adding _check_mounted_on against OVL_BASE_TEST/SCRATCH_DEV here could >> >> >> fix it for me. >> >> >> >> >> > >> >> > Actually, there that test already exists in: >> >> > >> >> > _scratch_mkfs >> >> > _scratch_cleanup_files >> >> > _overlay_base_scratch_mount >> >> > _check_mounted_on >> > >> > Hmm, I don't think this kind of basic config sanity check belongs here, >> > this should be done in config and env setup time. (So I think >> > _overlay_mount should be fixed too, that _supports_filetype check >> > doesn't belong there either.) >> > >> > How about adding these checks in init_rc, along with other >> > _check_mounted_on checks against TEST_DEV and SCRATCH_DEV? >> > >> >> Yes, that makes sense. >> >> But I still wonder how "exit 1" from within helpers should be handled >> from check when stdout/err are redirected to $tmp.err. >> Trying to catch the config error earlier is a good practice, but >> it won't ensure against the same type of problem in the future. >> >> What did you think about my approach to store mkfs output in $check_err >> var instead of $tmp.err file and spew $check_err in _wrapup? >> BTW I tries 'cat $tmp.err' in _wrapup, but output is still redirected to >> $tmp.err while in trap, so cat says: "cat: input file is output file". > > I don't think we need to worry about that or handle it. IMO, if we > redirect a helper that does "exit", we're doing something wrong, so > either fix the redirection or fix the helper (just return not exit). > > I went through helpers in common/rc roughly, except the _overlay_mount > issue, seems we don't have other such problems right now :) > OK. less work for me :) I'll just fix the helper then. -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 14, 2017 at 7:51 AM, Eryu Guan <eguan@redhat.com> wrote: > On Mon, Feb 13, 2017 at 03:33:23PM +0200, Amir Goldstein wrote: >> On Mon, Feb 13, 2017 at 1:44 PM, Amir Goldstein <amir73il@gmail.com> wrote: >> > On Mon, Feb 13, 2017 at 1:10 PM, Eryu Guan <eguan@redhat.com> wrote: >> >> On Sun, Feb 12, 2017 at 10:43:36PM +0200, Amir Goldstein wrote: >> >>> When $TEST_DEV is mounted at a different location then $TEST_DIR, >> >>> _require_test() aborts the test with an error: >> >>> TEST_DEV=/dev/sda5 is mounted but not on TEST_DIR=/mnt/test >> >>> >> >>> There are several problems with current sanity check: >> >>> 1. the output of the error is mixed into out.bad and hard to see >> >>> 2. the test partition is unmounted at the end of the test regardless >> >>> of the fact that it not pass the sanity that we have exclusivity >> >>> 3. scratch partition has a similar sanity check in _require_scratch(), >> >>> but we may not get to it, because $SCRATCH_DEV is unmounted prior >> >>> to running the tests (which could unmount another mount point). >> >>> >> >>> To solve all these problems, introduce a helper _check_mounted_on(). >> >>> It checks if a device is mounted on a given mount point and optionally >> >>> checks the mounted fs type. >> >>> >> >>> The sanity checks in _require_scratch() and _require_test() are >> >>> converted to use the helper and gain the check for correct fs type. >> >>> >> >>> The helper is used in init_rc() to sanity check both test and scratch >> >>> partitions, before tests are run and before $SCRATCH_DEV is unmounted. >> >>> >> >>> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >> >>> --- >> >>> common/rc | 83 +++++++++++++++++++++++++++++++++++++-------------------------- >> >>> 1 file changed, 49 insertions(+), 34 deletions(-) >> > >> > ... >> > >> >> My test configs look like: >> >> >> >> TEST_DEV=/dev/sda5 >> >> SCRATCH_DEV=/dev/sda6 >> >> TEST_DIR=/mnt/testarea/test >> >> SCRATCH_MNT=/mnt/testarea/scratch >> >> >> >> and if I mount SCRATCH_DEV at /mnt/xfs (or some other mountpoints rather >> >> than SCRATCH_MNT), ./check -overlay overlay/??? isn't able to detect >> >> this mis-configuration. >> >> >> >> [root@dhcp-66-86-11 xfstests]# ./check -overlay overlay/002 >> >> FSTYP -- overlay >> >> PLATFORM -- Linux/x86_64 dhcp-66-86-11 4.10.0-rc7 >> >> MKFS_OPTIONS -- /mnt/testarea/scratch >> >> MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 -o lowerdir=/mnt/testarea/scratch/ovl-lower,upperdir=/mnt/testarea/scratch/ovl-upper,workdir=/mnt/testarea/scratch/ovl-work >> >> >> >> [root@dhcp-66-86-11 xfstests]# >> >> >> >> And nothing useful was printed. This is because my rootfs has no >> >> filetype support, but the _notrun message is redirected to a file in >> >> check, as >> >> >> >> "if ! _scratch_mkfs >$tmp.err 2>&1" >> >> >> >> Adding _check_mounted_on against OVL_BASE_TEST/SCRATCH_DEV here could >> >> fix it for me. >> >> >> > >> > Actually, there that test already exists in: >> > >> > _scratch_mkfs >> > _scratch_cleanup_files >> > _overlay_base_scratch_mount >> > _check_mounted_on > > Hmm, I don't think this kind of basic config sanity check belongs here, > this should be done in config and env setup time. (So I think > _overlay_mount should be fixed too, that _supports_filetype check > doesn't belong there either.) > > How about adding these checks in init_rc, along with other > _check_mounted_on checks against TEST_DEV and SCRATCH_DEV? > Sorry, I tried adding those base fs tests in init_rc, but it got too complicated. So in V4, I left the sanity checks inside _overlay_base_scratch_mount and modified the _check_mounted_on helper to not exit and return error values which callers acts upon. -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/common/rc b/common/rc index 76515e2..d831b17 100644 --- a/common/rc +++ b/common/rc @@ -1319,6 +1319,43 @@ _supported_os() _notrun "not suitable for this OS: $HOSTOS" } +# check if a FS on a device is mounted +# if so, verify that it is mounted on mount point +# if fstype is given as argument, verify that it is also +# mounted with correct fs type +# +_check_mounted_on() +{ + local devname=$1 + local dev=$2 + local mntname=$3 + local mnt=$4 + local type=$5 + + # Note that we use -F here so grep doesn't try to interpret an NFS over + # IPv6 server as a regular expression + local mount_rec=`_mount | grep -F "$dev"` + [ -n "$mount_rec" ] || return 1 + + # if it's mounted, make sure its on $mnt + if ! (echo $mount_rec | grep -q "$mnt") + then + echo "$devname=$dev is mounted but not on $mntname=$mnt - aborting" + echo "Already mounted result:" + echo $mount_rec + exit 1 + fi + + if [ -n "$type" -a "`_fs_type $dev`" != "$type" ] + then + echo "$devname=$dev is mounted but not a type $type filesystem" + # raw $DF_PROG cannot handle NFS/CIFS/overlay correctly + _df_device $dev + exit 1 + fi + return 0 +} + # this test needs a scratch partition - check we're ok & unmount it # No post-test check of the device is required. e.g. the test intentionally # finishes the test with the filesystem in a corrupt state @@ -1373,21 +1410,9 @@ _require_scratch_nocheck() ;; esac - # mounted? - # Note that we use -F here so grep doesn't try to interpret an NFS over - # IPv6 server as a regular expression. - mount_rec=`_mount | grep -F $SCRATCH_DEV` - if [ "$mount_rec" ] + if _check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT then - # if it's mounted, make sure its on $SCRATCH_MNT - if ! echo $mount_rec | grep -q $SCRATCH_MNT - then - echo "\$SCRATCH_DEV=$SCRATCH_DEV is mounted but not on \$SCRATCH_MNT=$SCRATCH_MNT - aborting" - echo "Already mounted result:" - echo $mount_rec - exit 1 - fi - # and then unmount it + # if it's mounted, unmount it if ! _scratch_unmount then echo "failed to unmount $SCRATCH_DEV" @@ -1458,21 +1483,8 @@ _require_test() ;; esac - # mounted? - # Note that we use -F here so grep doesn't try to interpret an NFS over - # IPv6 server as a regular expression. - mount_rec=`_mount | grep -F $TEST_DEV` - if [ "$mount_rec" ] + if ! _check_mounted_on TEST_DEV $TEST_DEV TEST_DIR $TEST_DIR then - # if it's mounted, make sure its on $TEST_DIR - if ! echo $mount_rec | grep -q $TEST_DIR - then - echo "\$TEST_DEV=$TEST_DEV is mounted but not on \$TEST_DIR=$TEST_DIR - aborting" - echo "Already mounted result:" - echo $mount_rec - exit 1 - fi - else out=`_mount_or_remount_rw "$MOUNT_OPTIONS" $TEST_DEV $TEST_DIR` if [ $? -ne 1 ]; then echo $out @@ -3075,13 +3087,16 @@ init_rc() fi fi - if [ "`_fs_type $TEST_DEV`" != "$FSTYP" ] - then - echo "common/rc: Error: \$TEST_DEV ($TEST_DEV) is not a MOUNTED $FSTYP filesystem" - # raw $DF_PROG cannot handle NFS/CIFS/overlay correctly - _df_device $TEST_DEV - exit 1 + # Sanity check that TEST partition is not mounted at another mount point + # or as another fs type + _check_mounted_on TEST_DEV $TEST_DEV TEST_DIR $TEST_DIR $FSTYP + if [ -n "$SCRATCH_DEV" ]; then + # Sanity check that SCRATCH partition is not mounted at another + # mount point, because it is about to be unmounted and formatted. + # Another fs type for scratch is fine (bye bye old fs type). + _check_mounted_on SCRATCH_DEV $SCRATCH_DEV SCRATCH_MNT $SCRATCH_MNT fi + # Figure out if we need to add -F ("foreign", deprecated) option to xfs_io $XFS_IO_PROG -c stat $TEST_DIR 2>&1 | grep -q "is not on an XFS filesystem" && \ export XFS_IO_PROG="$XFS_IO_PROG -F"
When $TEST_DEV is mounted at a different location then $TEST_DIR, _require_test() aborts the test with an error: TEST_DEV=/dev/sda5 is mounted but not on TEST_DIR=/mnt/test There are several problems with current sanity check: 1. the output of the error is mixed into out.bad and hard to see 2. the test partition is unmounted at the end of the test regardless of the fact that it not pass the sanity that we have exclusivity 3. scratch partition has a similar sanity check in _require_scratch(), but we may not get to it, because $SCRATCH_DEV is unmounted prior to running the tests (which could unmount another mount point). To solve all these problems, introduce a helper _check_mounted_on(). It checks if a device is mounted on a given mount point and optionally checks the mounted fs type. The sanity checks in _require_scratch() and _require_test() are converted to use the helper and gain the check for correct fs type. The helper is used in init_rc() to sanity check both test and scratch partitions, before tests are run and before $SCRATCH_DEV is unmounted. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- common/rc | 83 +++++++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 49 insertions(+), 34 deletions(-)