On May 28, 2025 / 09:09, Bart Van Assche wrote:
> On 5/27/25 9:15 PM, Shinichiro Kawasaki wrote:
> > Said that, "set -e" is a good practice in general. If it will help blktests
> > contributors including you, I would like to revisit this topic.
> >
> > When I had tried to support "set -e", I faced were two obstacles:
> >
> > 1) There are certain amount of places which trigger sudden exit under "set -e"
> > condition. To fix this, dirty code changes are required, and this code change
> > will need rather large amount of effort.
>
> If we would have to make a choice between set -e and code readability
> and/or maintainability, I prefer the latter.
I think that preference depends on each contributor. My conclusion _was_
"do not do 'set -e'" for whole blktests. What I suggest now is to decide the
preference per test case.
>
> > 2) When a test case exits by "set -e", it may not clean up the test environment.
> > This may leave unexpected test conditions and affect following test cases.
> >
> > Now I can think of two solutions respectively.
> >
> > 1) Apply "set -e" practice only for the limited test cases. The new test case
> > zbd/013 will be the first one. With this approach, we can keep exisiting
> > scripts as they are, and don't need to spend time to modify them.
>
> Does this mean no "set +e" statements anywhere and hence that statements
> that may fail should be surrounded with something that makes bash ignore
> their exit status, e.g. if ... then ... fi?
What I'm thinking of is different. What's in my mind is below: (I have not yet
implemented, so I'm not yet sure if it is really feasible, though)
- If a contributor thinks a new test case should be error free, declare it
by adding the "ERREXIT=1" flag at the beginning of the test case.
- When "check" script finds the "ERREXIT=1" flag in a test case, it does
"set -e" before calling test() or test_device() for the test case. After
the call, do "set +e".
With this approach, the existing test cases are not affected. And we can do the
"set -e" error check in the selected new test cases.
Will this approach fit your needs?
> In some of my own shell
> scripts I define the following function to make it easy to ignore the
> exit status of shell commands that may fail:
>
> ignore_failure() {
> if "$@"; then :; fi
> }
This could be a useful helper function to add, if we introduce the "set -e"
support into blktests.
>
> > 2) Use ERR trap to detect if each test case exited by "set -e". If so, force
> > stop the "check" script run to avoid influence to following test cases.
>
> As you probably know there should only be one trap ERR or trap EXIT
> statement. So that statement should probably occur in the ./check source
> instead of in each test script. To make that trap statement perform test
> specific cleanup it would have to call a cleanup function defined in the
> test/*/* source files.
I was not aware of that restriction. Thanks. I will do some prototype and see
if the existing cleanup function handling can work together with the "set -e"
exit check.
>
> Another solution is to concatenate all statements that shouldn't fail with
> "&&" but that doesn't make the test code look pretty.
I don't think all contributors are okay with this approach.
