On Apr 22, 2020 / 07:29, Klaus Birkelund Jensen wrote:
> On Apr 22 07:13, Klaus Birkelund Jensen wrote:
> > On Apr 22 01:12, Shinichiro Kawasaki wrote:
> > > On Apr 21, 2020 / 09:33, Klaus Jensen wrote:
> > > >
> > > > -device_requires() {
> > > > - ! _test_dev_is_zoned || _have_fio_zbd_zonemode
> > > > -}
> > > > -
> > > > test_device() {
> > > > echo "Running ${TEST_NAME}"
> > > >
> > > > @@ -25,6 +21,10 @@ test_device() {
> > > > local zbdmode=""
> > > >
> > > > if _test_dev_is_zoned; then
> > > > + if ! _have_fio_zbd_zonemode; then
> > > > + return
> > > > + fi
> > > > +
> > >
> > > This check is equivalent to device_requires() you removed, isn't it?
> > > If the skip check in test_device() is the last resort, it would be the
> > > better to check in device_requires(), I think.
> > >
> >
> > I did fix something... just not the real problem I think.
> >
> > Negations doesnt really work well in device_requires. If changed to
> >
> > ! _require_test_dev_is_zoned || _have_fio_zbd_zonemode
> >
> > then, for non-zoned devices, even though device_requires returns 0,
> > SKIP_REASON ends up being set to "is not a zoned block device" and skips
> > the test in _call_test due to this.
> >
> > There are two fixes; either we add a _require_test_dev_is_not_zoned
> > again or put the negated check in an arithmetic context, that is
> >
> > (( !_require_test_dev_is_zoned )) || _have_fio_zbd_zonemode
> >
> > I think the second option is a hack, so we'd better go with the first
> > choice.
>
> Doh.
>
> The _is_not_zoned version would of course just cause the test to be
> skipped for zoned devices instead.
>
> So I actually think my original fix is the best option here.
Thank you for sharing your thoghts. I agree not to use _is_not_zoned version.
I think _test_dev_is_zoned in device_requires() does not need to be replaced
with _require_test_dev_is_zoned. It does not check requirement. It just checks
if _have_fio_zbd_zonemode check is required or not. Then, how about the code
below? (_test_dev_is_zoned does not touch SKIP_REASON.)
device_requires() {
if _test_dev_is_zoned; then
_have_fio_zbd_zonemode
fi
}
The above code is equivalent to below (less readable though).
device_requires() {
! _test_dev_is_zoned || _have_fio_zbd_zonemode
}
My suggestion is one of the two above. Your original fix also works good, then
this suggestion is not so strong.
--
Best Regards,
Shin'ichiro Kawasaki
