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

[Logan Gunthorpe <logang@deltatee.com>]

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