| Message ID | 2a743318-585d-9eb1-5430-2b07246348ab@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, May 14, 2018 at 12:09:16PM -0500, Eric Sandeen wrote: > This tests the online label ioctl that btrfs has, which has been > recently proposed for XFS. > > To run, it requires an updated xfs_io with the label command and a > filesystem that supports it > > A slight change here to _require_xfs_io_command as well, so that tests > which simply fail with "Inappropriate ioctl" can be caught in the > common case. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > (urgh send as proper new thread, sorry) > > This passes on btrfs, _notruns on xfs/ext4 of yore, and passes > on xfs w/ my online label patchset (as long as xfs_io has the new > capability) > > V2: Add a max label length helper > Set the proper btrfs max label length o_O oops > Filter trailing whitespace from blkid output > > > diff --git a/common/rc b/common/rc > index 9ffab7fd..88a99cff 100644 > --- a/common/rc > +++ b/common/rc > @@ -2158,6 +2158,9 @@ _require_xfs_io_command() > echo $testio | grep -q "Inappropriate ioctl" && \ > _notrun "xfs_io $command support is missing" > ;; > + "label") > + testio=`$XFS_IO_PROG -c "label" $TEST_DIR 2>&1` > + ;; > "open") > # -c "open $f" is broken in xfs_io <= 4.8. Along with the fix, > # a new -C flag was introduced to execute one shot commands. > @@ -2196,7 +2199,7 @@ _require_xfs_io_command() > rm -f $testfile 2>&1 > /dev/null > echo $testio | grep -q "not found" && \ > _notrun "xfs_io $command support is missing" > - echo $testio | grep -q "Operation not supported" && \ > + echo $testio | grep -q "Operation not supported\|Inappropriate ioctl" && \ > _notrun "xfs_io $command failed (old kernel/wrong fs?)" > echo $testio | grep -q "Invalid" && \ > _notrun "xfs_io $command failed (old kernel/wrong fs/bad args?)" > @@ -3802,6 +3805,31 @@ _require_scratch_feature() > esac > } > > +# The maximum filesystem label length, not including terminating NULL > +_label_get_max() > +{ > + case $FSTYP in > + xfs) > + MAXLEN=12 > + ;; > + btrfs) > + MAXLEN=255 > + ;; > + *) > + MAXLEN=0 Why not just _notrun here? > + ;; > + esac > + > + echo $MAXLEN > +} > + > +_require_label_get_max() > +{ > + if [ $(_label_get_max) -eq 0 ]; then > + _notrun "$FSTYP does not define maximum label length" > + fi And this check can go away? Also, shouldn't it be _require_online_label_change() ? And then maybe you can move the xfs_io label command check inside it? > +# remove previous $seqres.full before test > +rm -f $seqres.full > + > +# real QA test starts here > + > +_supported_fs generic > +_supported_os Linux > +_require_scratch > +_require_xfs_io_command "label" > +_require_label_get_max > + > +_scratch_mkfs > $seqres.full 2>&1 > +_scratch_mount > + > +# Make sure we can set & clear the label > +$XFS_IO_PROG -c "label label.$seq" $SCRATCH_MNT > +$XFS_IO_PROG -c "label" $SCRATCH_MNT > + > +# And that userspace can see it now, while mounted > +# NB: some blkid has trailing whitespace, filter it out here > +blkid -s LABEL $SCRATCH_DEV | _filter_scratch | sed -e "s/ $//g" > + > +# And that the it is still there when it's unmounted > +_scratch_unmount > +blkid -s LABEL $SCRATCH_DEV | _filter_scratch | sed -e "s/ $//g" Ok, so "LABEL" here is a special blkid match token.... > +# And that it persists after a remount > +_scratch_mount > +$XFS_IO_PROG -c "label" $SCRATCH_MNT > + > +# And that a too-long label is rejected, beyond the interface max: > +LABEL=$(perl -e "print 'l' x 257;") And now you use it as a variable. Nasty and confusing. Using lower case for local variables is the norm, right? I thought we were only supposed to use upper case for global test harness variables... But even making it "label" is problematic: > +$XFS_IO_PROG -c "label $LABEL" $SCRATCH_MNT because "label" is an xfs_io command. Perhaps call it "fs_label"? Cheers, Dave.
On 5/14/18 6:11 PM, Dave Chinner wrote: > On Mon, May 14, 2018 at 12:09:16PM -0500, Eric Sandeen wrote: >> This tests the online label ioctl that btrfs has, which has been >> recently proposed for XFS. >> >> To run, it requires an updated xfs_io with the label command and a >> filesystem that supports it >> >> A slight change here to _require_xfs_io_command as well, so that tests >> which simply fail with "Inappropriate ioctl" can be caught in the >> common case. >> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- <snip> >> +# The maximum filesystem label length, not including terminating NULL >> +_label_get_max() >> +{ >> + case $FSTYP in >> + xfs) >> + MAXLEN=12 >> + ;; >> + btrfs) >> + MAXLEN=255 >> + ;; >> + *) >> + MAXLEN=0 > > Why not just _notrun here? do we want to go through the other steps only to get here and notrun halfway through? >> + ;; >> + esac >> + >> + echo $MAXLEN >> +} >> + >> +_require_label_get_max() >> +{ >> + if [ $(_label_get_max) -eq 0 ]; then >> + _notrun "$FSTYP does not define maximum label length" >> + fi > > And this check can go away? We'd like to know if all the validations in this can complete before we get started, right? That's why I did it this way. > Also, shouldn't it be _require_online_label_change() ? And then > maybe you can move the xfs_io label command check inside it? Well, we want to know a lot of things: 1) can the kernel code for this filesystem support online label 2) does xfs_io support the label command 3) does this test know the maximum label length to test for this fs the above stuff is for 3) >> +# remove previous $seqres.full before test >> +rm -f $seqres.full >> + >> +# real QA test starts here >> + >> +_supported_fs generic >> +_supported_os Linux >> +_require_scratch >> +_require_xfs_io_command "label" >> +_require_label_get_max >> + >> +_scratch_mkfs > $seqres.full 2>&1 >> +_scratch_mount >> + >> +# Make sure we can set & clear the label >> +$XFS_IO_PROG -c "label label.$seq" $SCRATCH_MNT >> +$XFS_IO_PROG -c "label" $SCRATCH_MNT >> + >> +# And that userspace can see it now, while mounted >> +# NB: some blkid has trailing whitespace, filter it out here >> +blkid -s LABEL $SCRATCH_DEV | _filter_scratch | sed -e "s/ $//g" >> + >> +# And that the it is still there when it's unmounted >> +_scratch_unmount >> +blkid -s LABEL $SCRATCH_DEV | _filter_scratch | sed -e "s/ $//g" > > Ok, so "LABEL" here is a special blkid match token.... yep >> +# And that it persists after a remount >> +_scratch_mount >> +$XFS_IO_PROG -c "label" $SCRATCH_MNT >> + >> +# And that a too-long label is rejected, beyond the interface max: >> +LABEL=$(perl -e "print 'l' x 257;") > > And now you use it as a variable. Nasty and confusing. Using lower > case for local variables is the norm, right? I thought we were only > supposed to use upper case for global test harness variables... I guess I didn't know about that convention (or forgot) ok, yeah, that's a little confusing I guess. *shrug* I can change it if you prefer. Obviously the "blkid -s LABEL" must remain "LABEL" > But even making it "label" is problematic: > >> +$XFS_IO_PROG -c "label $LABEL" $SCRATCH_MNT > > because "label" is an xfs_io command. Perhaps call it "fs_label"? ok -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 14, 2018 at 06:26:07PM -0500, Eric Sandeen wrote: > On 5/14/18 6:11 PM, Dave Chinner wrote: > > On Mon, May 14, 2018 at 12:09:16PM -0500, Eric Sandeen wrote: > >> This tests the online label ioctl that btrfs has, which has been > >> recently proposed for XFS. > >> > >> To run, it requires an updated xfs_io with the label command and a > >> filesystem that supports it > >> > >> A slight change here to _require_xfs_io_command as well, so that tests > >> which simply fail with "Inappropriate ioctl" can be caught in the > >> common case. > >> > >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> > >> --- > > <snip> > > >> +# The maximum filesystem label length, not including terminating NULL > >> +_label_get_max() > >> +{ > >> + case $FSTYP in > >> + xfs) > >> + MAXLEN=12 > >> + ;; > >> + btrfs) > >> + MAXLEN=255 > >> + ;; > >> + *) > >> + MAXLEN=0 > > > > Why not just _notrun here? > > do we want to go through the other steps only to get here and notrun > halfway through? > > >> + ;; > >> + esac > >> + > >> + echo $MAXLEN > >> +} > >> + > >> +_require_label_get_max() > >> +{ > >> + if [ $(_label_get_max) -eq 0 ]; then > >> + _notrun "$FSTYP does not define maximum label length" > >> + fi > > > > And this check can go away? > > We'd like to know if all the validations in this can complete before we > get started, right? That's why I did it this way. Sure, just trying to be a bit defensive as people often forget _requires directives when writing new tests.... > > Also, shouldn't it be _require_online_label_change() ? And then > > maybe you can move the xfs_io label command check inside it? > > Well, we want to know a lot of things: > > 1) can the kernel code for this filesystem support online label > 2) does xfs_io support the label command > 3) does this test know the maximum label length to test for this fs > > the above stuff is for 3) What I was suggesting was doing all of these in one function similar to _require_xfs_sparse_inodes, _require_meta_uuid, etc: _require_online_label_change() { # need xfs_io support _require_xfs_io_command "label" # need fstests knowledge of label size [ $(_label_get_max) -eq 0 ] && _notrun "$FSTYP does not define maximum label length" # need kernel support $XFS_IO_PROG -c label $TEST_DIR > /dev/null 2>&1 [ $? -ne 0 ] && _notrun "Kernel does not support FS_IOC_GETLABEL" } Which also means that the label_f command in xfs_io needs to set the exitcode to non-zero when the ioctl fails so that xfs_io returns non-zero on failure... Cheers, Dave.
diff --git a/common/rc b/common/rc index 9ffab7fd..88a99cff 100644 --- a/common/rc +++ b/common/rc @@ -2158,6 +2158,9 @@ _require_xfs_io_command() echo $testio | grep -q "Inappropriate ioctl" && \ _notrun "xfs_io $command support is missing" ;; + "label") + testio=`$XFS_IO_PROG -c "label" $TEST_DIR 2>&1` + ;; "open") # -c "open $f" is broken in xfs_io <= 4.8. Along with the fix, # a new -C flag was introduced to execute one shot commands. @@ -2196,7 +2199,7 @@ _require_xfs_io_command() rm -f $testfile 2>&1 > /dev/null echo $testio | grep -q "not found" && \ _notrun "xfs_io $command support is missing" - echo $testio | grep -q "Operation not supported" && \ + echo $testio | grep -q "Operation not supported\|Inappropriate ioctl" && \ _notrun "xfs_io $command failed (old kernel/wrong fs?)" echo $testio | grep -q "Invalid" && \ _notrun "xfs_io $command failed (old kernel/wrong fs/bad args?)" @@ -3802,6 +3805,31 @@ _require_scratch_feature() esac } +# The maximum filesystem label length, not including terminating NULL +_label_get_max() +{ + case $FSTYP in + xfs) + MAXLEN=12 + ;; + btrfs) + MAXLEN=255 + ;; + *) + MAXLEN=0 + ;; + esac + + echo $MAXLEN +} + +_require_label_get_max() +{ + if [ $(_label_get_max) -eq 0 ]; then + _notrun "$FSTYP does not define maximum label length" + fi +} + init_rc ################################################################################ diff --git a/tests/generic/479 b/tests/generic/479 old mode 100644 new mode 100755 diff --git a/tests/generic/485 b/tests/generic/485 new file mode 100755 index 00000000..941afd3d --- /dev/null +++ b/tests/generic/485 @@ -0,0 +1,90 @@ +#! /bin/bash +# FS QA Test 485 +# +# Test the online filesystem label set/get ioctls +# +#----------------------------------------------------------------------- +# Copyright (c) 2018 Red Hat, Inc. All Rights Reserved. +# Author: Eric Sandeen <sandeen@redhat.com> +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- +# + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# remove previous $seqres.full before test +rm -f $seqres.full + +# real QA test starts here + +_supported_fs generic +_supported_os Linux +_require_scratch +_require_xfs_io_command "label" +_require_label_get_max + +_scratch_mkfs > $seqres.full 2>&1 +_scratch_mount + +# Make sure we can set & clear the label +$XFS_IO_PROG -c "label label.$seq" $SCRATCH_MNT +$XFS_IO_PROG -c "label" $SCRATCH_MNT + +# And that userspace can see it now, while mounted +# NB: some blkid has trailing whitespace, filter it out here +blkid -s LABEL $SCRATCH_DEV | _filter_scratch | sed -e "s/ $//g" + +# And that the it is still there when it's unmounted +_scratch_unmount +blkid -s LABEL $SCRATCH_DEV | _filter_scratch | sed -e "s/ $//g" + +# And that it persists after a remount +_scratch_mount +$XFS_IO_PROG -c "label" $SCRATCH_MNT + +# And that a too-long label is rejected, beyond the interface max: +LABEL=$(perl -e "print 'l' x 257;") +$XFS_IO_PROG -c "label $LABEL" $SCRATCH_MNT + +# And it succeeds right at the filesystem max: +MAXLEN=$(_label_get_max) +LABEL=$(perl -e "print 'o' x $MAXLEN;") +$XFS_IO_PROG -c "label $LABEL" $SCRATCH_MNT | sed -e 's/o\+/MAXLABEL/' + +# And that it fails past the filesystem max: +let TOOLONG=MAXLEN+1 +LABEL=$(perl -e "print 'o' x $TOOLONG;") +$XFS_IO_PROG -c "label $LABEL" $SCRATCH_MNT + +# success, all done +status=0 +exit diff --git a/tests/generic/485.out b/tests/generic/485.out new file mode 100644 index 00000000..24efca8c --- /dev/null +++ b/tests/generic/485.out @@ -0,0 +1,9 @@ +QA output created by 485 +label = "label.485" +label = "label.485" +SCRATCH_DEV: LABEL="label.485" +SCRATCH_DEV: LABEL="label.485" +label = "label.485" +label: Invalid argument +label = "MAXLABEL" +label: Invalid argument diff --git a/tests/generic/group b/tests/generic/group index 19be9267..9ac5bbd1 100644 --- a/tests/generic/group +++ b/tests/generic/group @@ -487,3 +487,4 @@ 482 auto metadata replay 483 auto quick log metadata 484 auto quick +485 auto quick
This tests the online label ioctl that btrfs has, which has been recently proposed for XFS. To run, it requires an updated xfs_io with the label command and a filesystem that supports it A slight change here to _require_xfs_io_command as well, so that tests which simply fail with "Inappropriate ioctl" can be caught in the common case. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- (urgh send as proper new thread, sorry) This passes on btrfs, _notruns on xfs/ext4 of yore, and passes on xfs w/ my online label patchset (as long as xfs_io has the new capability) V2: Add a max label length helper Set the proper btrfs max label length o_O oops Filter trailing whitespace from blkid output -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html