Re: [PATCH blktests v2] Fix unintentional skipping of tests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux