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.
