On Fri, Apr 13, 2018 at 09:00:35AM +1000, Dave Chinner wrote:
> On Thu, Apr 12, 2018 at 09:00:38PM +0800, Eryu Guan wrote:
> > On Thu, Apr 12, 2018 at 10:09:52PM +1000, Dave Chinner wrote:
> > > On Thu, Apr 12, 2018 at 06:00:30PM +0800, yang xu wrote:
> > > > ---
> > > > tests/xfs/444 | 20 ++++++++++++++++++++
> > > > 1 file changed, 20 insertions(+)
> > > >
> > > > diff --git a/tests/xfs/444 b/tests/xfs/444
> > > > index 141be52..fe80502 100755
> > > > --- a/tests/xfs/444
> > > > +++ b/tests/xfs/444
> > > > @@ -58,6 +58,22 @@ _require_test_program "punch-alternating"
> > > > # This is only a v5 filesystem problem
> > > > _require_scratch_xfs_crc
> > > >
> > > > +check_xfs_db_write_bno_array() {
> > >
> > > _require_xfs_db_write_array(), and move to common/xfs.
> > >
> > > > + _scratch_xfs_set_metadata_field "bno[32]" "78" "agfl 0" >> $seqres.full
> > > > +
> > > > + # Before xfsprogs commit 0ebbf1d58898 ("db: limit AGFL bno
> > > > + # arrayi printing), When asking for a single agfl entry,
> > > > + # the result outputs the entire remainder of the array
> > > > + # starting at the given index.
> > > > + # It is difficult to extract single entry values.
> > > > + # So filter them.
> > > > + bno2=$(_scratch_xfs_get_metadata_field "bno[32]" \
> > > > + "agfl 0" |sed -e 's/ .*$//g' | sed -e 's/^.*://g')
> > > > + echo "bno[32] set 78 get $bno2" >> $seqres.full
> > > > +
> > > > + [ "${bno2}" != "78" ] && _fail "xfs_db write can't support bno array"
> > >
> > > test should _notrun is db doesn't have therequired support, not
> > > _fail.
> >
> > If xfsprogs commit 4222d000ed ("db: write via array indexing doesn't
> > work") introduced a new feature, I agreed that we need the _require rule
> > and _notrun the test.
>
> _require* rules are not for "new features" - they are for defining
> the support needed to run the test correctly. This is no different
Agreed, I was not clear previously, by "new features" I mean changes
like new features that change the behavior.
> to detecting the mkfs binary version we are running to determine
> if a certain fix is present or not:
>
> # Skip the test if all calls passed - mkfs accepts invalid input
> _require_xfs_mkfs_validation()
> {
> _xfs_mkfs_validation_check
> if [ "$?" -eq 0 ]; then
> _notrun "Requires newer mkfs with stricter input checks: the oldest supported version of xfsprogs is 4.7."
> fi
> }
Just like this _require rule, it's more like a behavior change that
added stricter input checks.
>
> This is exactly the same sort of situation, because....
>
> > But this issue looks like a bug in xfsprogs to me, not a missing feature
> > in xfsprogs on RHEL7, so I tend to fail the test instead of adding a new
> > _require rule & _notrun the test. And in this case, IMHO, I don't think
> > it's necessary to do any update to the test, just leave the test as it
> > is and file a new bug in Red Hat bugzilla.
>
> ... this isn't a RHEL specific issue - it's an xfsprogs version
> issue. i.e. any older distro that has a binary with a broken
> "write array" command will fail this test. None of them are going to
> get updated xfsprogs packages, so like having an old mkfs.xfs
> binary, this test should run conditionally on having a version of
> xfs_db that actually works correctly....
But I still think it's a pure bug in xfsprogs, not xfsprogs version
issue nor a behavior change in xfsprogs, as we did support "write via
array indexing", just that it was broken in a certain case, and commit
4222d000ed3b fixed that bug. We should expose bugs by letting the test
fail, not paper over it by _notrun the test.
Thanks,
Eryu
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
