Re: [PATCH v7 1/7] nvme: consolidate nvme requirements based on transport type

[Omar Sandoval <osandov@osandov.com>]

On Mon, Sep 14, 2020 at 04:04:59PM -0600, Logan Gunthorpe wrote:
> 
> 
> On 2020-09-14 3:51 p.m., Omar Sandoval wrote:
> >> diff --git a/tests/nvme/002 b/tests/nvme/002
> >> index 07b7fdae2d39..aaa5ec4d729a 100755
> >> --- a/tests/nvme/002
> >> +++ b/tests/nvme/002
> >> @@ -10,7 +10,8 @@
> >>  DESCRIPTION="create many subsystems and test discovery"
> >>  
> >>  requires() {
> >> -	_have_program nvme && _have_modules loop nvme-loop nvmet && _have_configfs
> >> +	_nvme_requires
> >> +	_have_modules loop
> > 
> > Bash functions return the status of the last executed command, and the
> > requires function needs to return 0 on success and 1 on failure. So,
> > this is losing the return value of _nvme_requires. Just chain multiple
> > checks with && to fix this (here and the other places _nvme_requires was
> > added with other checks):
> > 
> > requires() {
> > 	_nvme_requires && _have_modules loop
> > }
> 
> I noticed this too during my review, but based on my read of the current
> blktest code, the return value of the requires() function is not
> actually used. It seems to only check if SKIP_REASON is set.
> 
> If we want to ensure the return value of requires() is correct, perhaps
> we should check it after we call it and then consult SKIP_REASON? And
> WARN or fail if SKIP_REASON is set when requires() didn't return 1.
> 
> Also, we need to do more than adding &&s... _nvme_requires() will need
> to be fixed up as it calls other _have_x() functions and ignores their
> return value.
> 
> Logan

Oops, you're right, I actually changed this a few months ago in
4824ac3f5c4a ("Skip tests based on SKIP_REASON, not return value").
Totally forgot about that :) IMO it's still cleaner to chain them
together.