Refactor for_each_td() to catch inappropriate td ptr reuse
authorHorshack <horshack@live.com>
Thu, 2 Mar 2023 20:12:54 +0000 (15:12 -0500)
committerHorshack <horshack@live.com>
Fri, 3 Mar 2023 13:24:06 +0000 (08:24 -0500)
commitda8f124f8cf55eedab9cd2d3bd8afc0cd4971295
tree8dda0be4c417c598d8f4bfc600963c0c89aaab0f
parent5a37211238f995657c50e5d0ea6e5e22ff3ca69e
Refactor for_each_td() to catch inappropriate td ptr reuse

I recently introduced a bug caused by reusing a struct thread_data *td
after the end of a for_each_td() loop construct.

Link: https://github.com/axboe/fio/pull/1521#issuecomment-1448591102
To prevent others from making this same mistake, this commit refactors
for_each_td() so that both the struct thread_data * and the loop index
variable are placed inside their own scope for the loop. This will cause
any reference to those variables outside the for_each_td() to produce an
undeclared identifier error, provided the outer scope doesn't already
reuse those same variable names for other code within the routine (which
is fine because the scopes are separate).

Because C/C++ doesn't let you declare two different variable types
within the scope of a for() loop initializer, creating a scope for both
struct thread_data * and the loop index required explicitly declaring a
scope with a curly brace. This means for_each_td() includes an opening
curly brace to create the scope, which means all uses of for_each_td()
must now end with an invocation of a new macro named end_for_each()
to emit an ending curly brace to match the scope brace created by
for_each_td():

for_each_td(td) {
while (td->runstate < TD_EXITED)
sleep(1);
} end_for_each();

The alternative is to end every for_each_td() construct with an inline
curly brace, which is off-putting since the implementation of an extra
opening curly brace is abstracted in for_each_td():

for_each_td(td) {
while (td->runstate < TD_EXITED)
sleep(1);
}}

Most fio logic only declares "struct thread_data *td" and "int i" for use in
for_each_td(), which means those declarations will now cause -Wunused-variable
warnings since they're not used outside the scope of the refactored
for_each_td(). Those declarations have been removed.

Implementing this change caught a latent bug in eta.c::calc_thread_status()
that accesses the ending value of struct thread_data *td after the end
of for_each_td(), now manifesting as a compile error, so working as
designed :)

Signed-off-by: Adam Horshack (horshack@live.com)
13 files changed:
backend.c
dedupe.c
engines/libblkio.c
eta.c
fio.h
init.c
iolog.c
libfio.c
rate-submit.c
stat.c
steadystate.c
verify.c
zbd.c