selftests: forwarding: Introduce deferred commands

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:

- 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.

- 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.

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.

The following patches will convert several selftests to this new framework.

Signed-off-by Petr Machata <[email protected]>

---
 tools/testing/selftests/net/forwarding/lib.sh | 83 +++++++++++++++++++

Quoted 1 line · Petr Machata

The following patches will convert several selftests to this new framework.

The intention is to make sure new tests are using these helpers?

Quoted 4 lines

Signed-off-by Petr Machata <[email protected]>

---
 tools/testing/selftests/net/forwarding/lib.sh | 83 +++++++++++++++++++

Does it make sense to place these helpers in net/lib.sh?

Quoted 3 lines

Quoted 1 line

 tools/testing/selftests/net/forwarding/lib.sh | 83 +++++++++++++++++++  

Does it make sense to place these helpers in net/lib.sh?

Yeah, it does.

Quoted 3 lines

Quoted 1 line

Does it make sense to place these helpers in net/lib.sh?

Yeah, it does.

Would it further make sense to split them to their own file (net/lib_defer.sh?) and source that in net/lib.sh?

Should be pretty self-contained.

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:

  tools/testing/selftests/net/forwarding/lib.sh | 83 +++++++++++++++++++

- 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.

- 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.

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.

Quoted 9 lines · Petr Machata

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!

Quoted 1 line

  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?

Quoted 4 lines

- 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.

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.

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)

Quoted 5 lines

- 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)

Quoted 10 lines

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().

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.

After the above parts will be discussed out I will look more into the details of the code more deeply.

Quoted 16 lines · Przemek Kitszel

Quoted 9 lines · Petr Machata

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!

Quoted 1 line

   tools/testing/selftests/net/forwarding/lib.sh | 83 +++++++++++++++++++

Quoted 2 lines

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.

Quoted 31 lines

Quoted 5 lines

- 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()
}

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 14 lines

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)

Quoted 5 lines

- 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.

Quoted 5 lines

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.

Quoted 16 lines

Quoted 11 lines

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.

Quoted 5 lines

I think that the name of the new_scope() would be better, still concise, but more precise as:
subscope_defer(),
trapped(), or
sub_trap().

Quoted 5 lines

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.

Quoted 3 lines

After the above parts will be discussed out I will look more into the details of the code more deeply.

Quoted 18 lines · Petr Machata

Quoted 16 lines · Przemek Kitszel

Quoted 9 lines · Petr Machata

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!

Quoted 1 line

   tools/testing/selftests/net/forwarding/lib.sh | 83 +++++++++++++++++++

Quoted 5 lines

Quoted 2 lines

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?

Quoted 36 lines

Quoted 30 lines

Quoted 4 lines

- 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()
}

Quoted 6 lines

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.

Quoted 4 lines

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".

Quoted 18 lines

Quoted 13 lines

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)

Quoted 5 lines

- 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.

Quoted 5 lines

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)

Quoted 6 lines

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.

Quoted 15 lines

Quoted 15 lines

Quoted 10 lines

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.

Quoted 5 lines

Quoted 5 lines

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

Quoted 6 lines

Quoted 5 lines

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?

Quoted 4 lines

Quoted 3 lines

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.

Quoted 1 line

what about separate file, but included from net/lib.sh?

Unfortunately that would be even worse. Then you need to remember to put the file into TEST_INCLUDES despite seemingly not using it.

Like ideally we'd have automation for this. But I don't know how to do that without actually parsing the bash files, and that's just asking for trouble. Maybe after the defer stuff we also need a module system :-/

Quoted 9 lines · On Tue, 27 Aug 2024 10:53:53 +0200 Petr Machata wrote:

Quoted 5 lines

Quoted 3 lines

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?

Unfortunately that would be even worse. Then you need to remember to put the file into TEST_INCLUDES despite seemingly not using it.

Quoted 3 lines

Like ideally we'd have automation for this. But I don't know how to do that without actually parsing the bash files, and that's just asking for trouble. Maybe after the defer stuff we also need a module system :-/

FWIW we could throw it into net/lib, which has a fake target, see:

b86761ff6374 ("selftests: net: add scaffolding for Netlink tests in Python")

Instead of having a suite of dedicated cleanup functions, use the defer framework to schedule cleanups right as their setup functions are run.

This makes a dedicated cleanup() moot, instead fall back to the lib.sh-provided one, which invokes the necessary defer cleanups as well.

Signed-off-by Petr Machata <[email protected]>

Tests interrupted part-way through leave behind a running mausezahn. Use defer to schedule a traffic stop after traffic is started. Mark the functions that run traffic as defer-scoped, such that the traffic is stopped right after the function returns.

Signed-off-by Petr Machata <[email protected]>

Use defer scopes to manage qdisc lifetime.

Signed-off-by Petr Machata <[email protected]>

Tests interrupted part-way through leave behind a running mausezahn. Use defer to schedule a traffic stop after traffic is started.

Signed-off-by Petr Machata <[email protected]>

---
 tools/testing/selftests/net/forwarding/sch_tbf_core.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

tools/testing/selftests/net/forwarding/sch_tbf_core.sh +2 -1

index 9cd884d4a5de..5d58c04e055c 100644
@@ -213,12 +213,12 @@ do_tbf_test()
 	local mbit=$1; shift
 
 	start_traffic $h1.$vlan $(ipaddr 1 $vlan) $(ipaddr 2 $vlan) $h2_mac
+	defer stop_traffic
 	sleep 5 # Wait for the burst to dwindle
 
 	local t2=$(busywait_for_counter 1000 +1 tbf_get_counter $vlan)
 	sleep 10
 	local t3=$(tbf_get_counter $vlan)
-	stop_traffic
 
 	RET=0
 

@@ -231,3 +231,4 @@ do_tbf_test()
 
 	log_test "TC $((vlan - 10)): TBF rate ${mbit}Mbit"
 }
+defer_scoped_fn do_tbf_test

Signature · 1 line

2.45.0

Quoted 2 lines · Petr Machata

Tests interrupted part-way through leave behind a running mausezahn. Use defer to schedule a traffic stop after traffic is started.

Any reason not to convert the setup part to the defer mechanism as well?