| Message ID | 1466505629-9402-1-git-send-email-eguan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 21, 2016 at 06:40:28PM +0800, Eryu Guan wrote: > Some tests require that there's no certain mount option, so > introduce a new helper _require_no_mount_opts() to do the check on > $MOUNT_OPTIONS. I think this is fine, except for the name. It's more of an exclude rule rather than a "require" rule. i.e. _exclude_mount_option() is closer to it's purpose. The only other question I have is that mount options can be different between test and scratch devices - the test device mount options can be set via TEST_FS_MOUNT_OPTS, as well as via MOUNT_OPTIONS. Does this rule need to handle that? Cheers, Dave.
On Wed, Jun 22, 2016 at 09:42:02AM +1000, Dave Chinner wrote: > On Tue, Jun 21, 2016 at 06:40:28PM +0800, Eryu Guan wrote: > > Some tests require that there's no certain mount option, so > > introduce a new helper _require_no_mount_opts() to do the check on > > $MOUNT_OPTIONS. > > I think this is fine, except for the name. It's more of an exclude > rule rather than a "require" rule. i.e. _exclude_mount_option() is > closer to it's purpose. This does look better to me, thanks! > > The only other question I have is that mount options can be > different between test and scratch devices - the test device mount > options can be set via TEST_FS_MOUNT_OPTS, as well as via > MOUNT_OPTIONS. Does this rule need to handle that? I didn't think about TEST_FS_MOUNT_OPTS. Currently there's no need to handle it, MOUNT_OPTIONS is sufficient I think. How about I rename it to _exclude_scratch_mount_option()? And we can always add another _exclude_test_mount_option() if needed in future. 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 Wed, Jun 22, 2016 at 11:23:51AM +0800, Eryu Guan wrote: > On Wed, Jun 22, 2016 at 09:42:02AM +1000, Dave Chinner wrote: > > On Tue, Jun 21, 2016 at 06:40:28PM +0800, Eryu Guan wrote: > > > Some tests require that there's no certain mount option, so > > > introduce a new helper _require_no_mount_opts() to do the check on > > > $MOUNT_OPTIONS. > > > > I think this is fine, except for the name. It's more of an exclude > > rule rather than a "require" rule. i.e. _exclude_mount_option() is > > closer to it's purpose. > > This does look better to me, thanks! > > > > > The only other question I have is that mount options can be > > different between test and scratch devices - the test device mount > > options can be set via TEST_FS_MOUNT_OPTS, as well as via > > MOUNT_OPTIONS. Does this rule need to handle that? > > I didn't think about TEST_FS_MOUNT_OPTS. Currently there's no need to > handle it, MOUNT_OPTIONS is sufficient I think. > > How about I rename it to _exclude_scratch_mount_option()? And we can > always add another _exclude_test_mount_option() if needed in future. Sounds good. Cheers, Dave.
On Thu, Jun 23, 2016 at 09:06:50AM +1000, Dave Chinner wrote: > On Wed, Jun 22, 2016 at 11:23:51AM +0800, Eryu Guan wrote: > > On Wed, Jun 22, 2016 at 09:42:02AM +1000, Dave Chinner wrote: > > > On Tue, Jun 21, 2016 at 06:40:28PM +0800, Eryu Guan wrote: > > > > Some tests require that there's no certain mount option, so > > > > introduce a new helper _require_no_mount_opts() to do the check on > > > > $MOUNT_OPTIONS. > > > > > > I think this is fine, except for the name. It's more of an exclude > > > rule rather than a "require" rule. i.e. _exclude_mount_option() is > > > closer to it's purpose. > > > > This does look better to me, thanks! > > > > > > > > The only other question I have is that mount options can be > > > different between test and scratch devices - the test device mount > > > options can be set via TEST_FS_MOUNT_OPTS, as well as via > > > MOUNT_OPTIONS. Does this rule need to handle that? > > > > I didn't think about TEST_FS_MOUNT_OPTS. Currently there's no need to > > handle it, MOUNT_OPTIONS is sufficient I think. > > > > How about I rename it to _exclude_scratch_mount_option()? And we can > > always add another _exclude_test_mount_option() if needed in future. > > Sounds good. Hi Dave, My v4 patches simply rename the helper to this "_exclude_scratch_mount_option", no function change. Then I take your reviews as a "Reviewed-by" tag based on "I think this is fine, except for the name." and the above "Sounds good.". So I can queue them for next pull request and start release testing, and don't have to bother you providing an explicit reviewed-by. Please let me know if you have different thoughts. 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 Fri, Jun 24, 2016 at 12:50:19PM +0800, Eryu Guan wrote: > On Thu, Jun 23, 2016 at 09:06:50AM +1000, Dave Chinner wrote: > > On Wed, Jun 22, 2016 at 11:23:51AM +0800, Eryu Guan wrote: > > > On Wed, Jun 22, 2016 at 09:42:02AM +1000, Dave Chinner wrote: > > > > On Tue, Jun 21, 2016 at 06:40:28PM +0800, Eryu Guan wrote: > > > > > Some tests require that there's no certain mount option, so > > > > > introduce a new helper _require_no_mount_opts() to do the check on > > > > > $MOUNT_OPTIONS. > > > > > > > > I think this is fine, except for the name. It's more of an exclude > > > > rule rather than a "require" rule. i.e. _exclude_mount_option() is > > > > closer to it's purpose. > > > > > > This does look better to me, thanks! > > > > > > > > > > > The only other question I have is that mount options can be > > > > different between test and scratch devices - the test device mount > > > > options can be set via TEST_FS_MOUNT_OPTS, as well as via > > > > MOUNT_OPTIONS. Does this rule need to handle that? > > > > > > I didn't think about TEST_FS_MOUNT_OPTS. Currently there's no need to > > > handle it, MOUNT_OPTIONS is sufficient I think. > > > > > > How about I rename it to _exclude_scratch_mount_option()? And we can > > > always add another _exclude_test_mount_option() if needed in future. > > > > Sounds good. > > Hi Dave, > > My v4 patches simply rename the helper to this > "_exclude_scratch_mount_option", no function change. Then I take your > reviews as a "Reviewed-by" tag based on "I think this is fine, except for > the name." and the above "Sounds good.". So I can queue them for next > pull request and start release testing, and don't have to bother you > providing an explicit reviewed-by. Yup - it's better to ask thn assume, though, so: Reviewed-by: Dave Chinner <dchinner@redhat.com> Cheers, Dave.
diff --git a/common/rc b/common/rc index 3a9c4d1..680ea2b 100644 --- a/common/rc +++ b/common/rc @@ -2926,8 +2926,16 @@ _require_cloner() _notrun "cloner binary not present at $CLONER_PROG" } +_require_no_mount_opts() +{ + if echo $MOUNT_OPTIONS | grep -q "$1"; then + _notrun "mount option \"$1\" not allowed in this test" + fi +} + _require_atime() { + _require_no_mount_opts "noatime" if [ "$FSTYP" == "nfs" ]; then _notrun "atime related mount options have no effect on NFS" fi diff --git a/tests/generic/192 b/tests/generic/192 index 6bbc87c..d3ed0ec 100755 --- a/tests/generic/192 +++ b/tests/generic/192 @@ -38,13 +38,6 @@ _access_time() stat -c %X $1 } -is_noatime_set() { - case "$MOUNT_OPTIONS" in - *noatime*) return 0;; - esac - return 1 -} - # get standard environment, filters and checks . ./common/rc . ./common/filter @@ -61,10 +54,6 @@ _require_atime #delay=45 delay=40 -if is_noatime_set; then - _notrun "this test makes no sense with noatime" -fi - testfile=$TEST_DIR/testfile rm -f $testfile rm -f $seqres.full diff --git a/tests/xfs/134 b/tests/xfs/134 index cd47069..a7df9fe 100755 --- a/tests/xfs/134 +++ b/tests/xfs/134 @@ -50,6 +50,9 @@ _supported_os Linux IRIX _require_test _require_xfs_quota +# we can't run with group quotas +_require_no_mount_opts "gquota" +_require_no_mount_opts "grpquota" dir=$SCRATCH_MNT/project @@ -70,11 +73,6 @@ _scratch_mkfs_xfs >/dev/null 2>&1 #if pquota's already in mount options then we dont need to enable -# we can't run with group quotas -if ( `echo $MOUNT_OPTIONS | grep -q gquota` || `echo $MOUNT_OPTIONS | grep -q grpquota` ) -then - _notrun "Can't run with group quotas enabled" -fi EXTRA_MOUNT_OPTIONS="-o pquota" if ! _scratch_mount "$EXTRA_MOUNT_OPTIONS" >$tmp.out 2>&1
Some tests require that there's no certain mount option, so introduce a new helper _require_no_mount_opts() to do the check on $MOUNT_OPTIONS. Also convert generic/192 and xfs/134 to use this helper. Signed-off-by: Eryu Guan <eguan@redhat.com> --- v2: - rebase on top of current master common/rc | 8 ++++++++ tests/generic/192 | 11 ----------- tests/xfs/134 | 8 +++----- 3 files changed, 11 insertions(+), 16 deletions(-)