On Mon, Sep 14, 2020 at 04:23:32PM -0600, Logan Gunthorpe wrote:
>
>
> On 2020-09-14 4:09 p.m., Omar Sandoval wrote:
> >> 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.
>
> > 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.
>
> In my opinion, this creates a bit of confusion when reading the code.
> The &&s make it look like the return value is important when, in fact,
> it is not.
>
> It is also easy to make the assumption that adding any bash command to
> the && list will skip the test -- however that is not the case and
> people may introduce subtle bugs that look correct, but go unnoticed.
>
> Logan
Fair enough. Sagi, don't worry about changing this, I'll queue this up
once I try it out.
