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@xxxxxxxxxx>
> >> ---
>
> <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.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
