On May 27, 2025 / 08:53, Bart Van Assche wrote:
> On 5/26/25 1:25 AM, Shinichiro Kawasaki wrote:
> > On May 23, 2025 / 09:49, Bart Van Assche wrote:
> > > [ ... ]
> > > +. common/null_blk
> >
> > Nit: this line can be removed since tests/zbd/rc sources common/null_blk.
> >
> > > +requires() {
> > > + _have_driver dm-crypt
> > > + _have_fio
> > > + _have_module null_blk
> >
> > Nit: the line above can be removed since group_requires() zbd/rc checks it.
>
> I prefer to make such dependencies explicit. Relying on indirect
> dependencies is considered a bad practice in multiple coding style
> guides since it makes it almost impossible to remove dependencies
> from dependencies, e.g. removing "_have_module null_blk" from
> "zbd/rc".
I see, that makes sense.
>
> > > +test() {
> > > + set -e
> >
> > Is this required? When I comment out this line and the "set +e" below,
> > still I was able to recreate the deadlock.
>
> I want the entire test to fail if a single command fails. Debugging
> shell scripts is much harder if a script continues after having
> encountered an unexpected error.
I see your intent. However, "set -e" and "set +e" does not work in the test
case. As the bash man page explains, even when commands fail, "set -e" does
not work under some conditions. One of the conditions is "if the command's
return value is being inverted with !". And the blktests "check" script calls
this test() function in the context of "inverted with !". To make the "set -e"
feature work, some changes are required in the "check" script to avoid the "!"
statements.
You may remember, once Hannes had requested this "set -e" or "set -o errexit"
enablement [1]. I had tried to support it and faced a couple of obstacles. Also,
in LSF blktests session a few years ago, Omar noted that he did not enable
"set -e" with intention at the beginning of blktests bring up. Then I stop the
effort for it.
[1] https://github.com/linux-blktests/blktests/issues/89
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.
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.
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.
Based on these approach, I will cook trial patches for the check script.
