On 8/26/24 17:20, Petr Machata wrote:
Przemek Kitszel <przemyslaw.kitszel@xxxxxxxxx> writes:
On 8/22/24 15:49, Petr Machata wrote:
In commit 8510801a9dbd ("selftests: drv-net: add ability to schedule
cleanup with defer()"), a defer helper was added to Python selftests.
The idea is to keep cleanup commands close to their dirtying counterparts,
thereby making it more transparent what is cleaning up what, making it
harder to miss a cleanup, and make the whole cleanup business exception
safe. All these benefits are applicable to bash as well, exception safety
can be interpreted in terms of safety vs. a SIGINT.
This patch therefore introduces a framework of several helpers that serve
to schedule cleanups in bash selftests:
Thank you for working on that, it would be great to have such
improvement for bash scripts in general, not limited to kselftests!
tools/testing/selftests/net/forwarding/lib.sh | 83 +++++++++++++++++++
Make it a new file in more generic location, add a comment section with
some examples and write down any assumptions there, perhaps defer.sh?
I can do it, but it's gonna be more pain in setting up those
TEST_INCLUDES. People will forget. It will be a nuisance.
I'm thinking of just moving it to net/lib.sh, from forwarding.
what about separate file, but included from net/lib.sh?
- defer_scope_push(), defer_scope_pop(): Deferred statements can be batched > together in scopes.
When a scope is popped, the deferred commands
schoduled in that scope are executed in the order opposite to order of
their scheduling.
tldr of this sub-comment at the end
such API could be used in two variants:
1)
function test_executor1() {
for t in tests; do
defer_scope_push()
exec_test1 $t
defer_scope_pop()
done
}
2)
function test_executor2() {
for t in tests; do
exec_test2 $t
done
}
function exec_test2() {
defer_scope_push()
do_stuff "$@"
defer_scope_pop()
}
That fractals down in the same way for "subtests", or some special stuff
like "make a zip" sub/task that will be used. And it could be misused as
a mix of the two variants.
I believe that the 1) is the better way, rationale: you write normal
code that does what needs to be done, using defer(), and caller (that
knows better) decides whether to sub-scope.
But the caller does not know better. The cleanups can't be done
"sometime", but at a predictable place, so that they don't end up
interfering with other work. The callee knows where it needs the
cleanups to happen. The caller shouldn't have to know.
The caller should not have to know what will be cleaned, but knows that
they are done with callee.
OTOH, callee has no idea about the "other work".
As this defer is very similar to golang's in intention, I would give
yet another analogy from golang's world. It's similar to concurrency, you write normal code that
could be parallelized via "go" keyword,
instead of writing async code that needs to be awaited for.
Notice how in go, defer also runs at function exit. Similarly with C++
destructors, run on scope exit. There's no caller-defined "collection
point".
Putting off until "sometime" works for memory. Things like garbage
collection, obstacks, autorelease pools, etc. work, because there's
plenty of memory and we don't mind keeping stuff around until later. But
that doesn't work for the sort of cleanups that selftests typically need
to do.
That's true. But I still believe that it's the caller (or better, "glue
code") responsibility to take care of cleanup schedule.
Going back to the use case variants, there is no much sense to have
push() and pop() dispersed by much from each other, thus I would like
to introduce an API that just combines the two instead:
new_scope exec_test1 $t
(name discussion below)
- defer(): Schedules a defer to the most recently pushed scope (or the
default scope if none was pushed. >
- defer_scopes_cleanup(): Pops any unpopped scopes, including the default
one. The selftests that use defer should run this in their cleanup
function. This is important to get cleanups of interrupted scripts.
this should be *the* trap(1)
with that said, it should be internal to the defer.sh script and it
should be obvious that developers must not introduce their own trap
(as of now we have ~330 in kselftests, ~270 of which in networking)
Yeah, we have 100+ tests that use their own traps in forwarding alone.
That ship has sailed.
I agree that the defer module probably has the "right" to own the exit
trap. Any other cleanups can be expressed in terms of defer, and I don't
know if there are legitimate uses of exit trap with that taken out. But
that's for sometime.
There could be multiple traps for ERR/EXIT/etc conditions, but for
simplicity it's best to rely on just EXIT trap.
So we should convert current scripts one by one to use your new API.
Consistent use of defers however obviates the need for a separate cleanup
function -- everything is just taken care of in defers. So this patch
actually introduces a cleanup() helper in the forwarding lib.sh, which
calls just pre_cleanup() and defer_scopes_cleanup(). Selftests are
obviously still free to override the function.
- defer_scoped_fn(): Sometimes a function would like to introduce a new
defer scope, then run whatever it is that it wants to run, and then pop
the scope to run the deferred cleanups. The helper defer_scoped_fn() can
be used to derive from one function its wrapper that pushes a defer scope
before the function is called, and pops it after it returns.
It is basically a helper I would like to see as new_scope() mentioned
above, but it takes it upside down - it should really be the caller that
sub-scopes.
I think that the name of the new_scope() would be better, still concise,
but more precise as:
subscope_defer(),
trapped(), or
sub_trap().
here I mean that "scope" is too broad without the word "trap" or "defer"
in name
I have no idea how to make a sub-trapped, SIGSEGV isolated scope of bash
execution that has ability to still edit outer scope variables. Perhaps
we could relax the need for edit to have easier implementation? It is
"all ok or failure/rollback" mode of operation anyway most of the time.
I'm not sure what you have in mind.
foo=1
function bumpfoo {
maybe-crash
foo=2
}
new-defer-scope bumpfoo
echo $foo
do you want this to print 2 or 1?
After the above parts will be discussed out I will look more into the
details of the code more deeply.
