Message-ID: <87jzg288sd.fsf@nvidia.com>
Date: Tue, 27 Aug 2024 10:53:53 +0200
From: Petr Machata <petrm@...dia.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
CC: Petr Machata <petrm@...dia.com>, Shuah Khan <shuah@...nel.org>,
	<linux-kselftest@...r.kernel.org>, <netdev@...r.kernel.org>, Jakub Kicinski
	<kuba@...nel.org>, Ido Schimmel <idosch@...dia.com>, Amit Cohen
	<amcohen@...dia.com>, Benjamin Poirier <bpoirier@...dia.com>, Hangbin Liu
	<liuhangbin@...il.com>, Vladimir Oltean <vladimir.oltean@....com>,
	<mlxsw@...dia.com>
Subject: Re: [RFC PATCH net-next 1/5] selftests: forwarding: Introduce
 deferred commands


Przemek Kitszel <przemyslaw.kitszel@...el.com> writes:

> On 8/26/24 17:20, Petr Machata wrote:
>> Przemek Kitszel <przemyslaw.kitszel@...el.com> 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?

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 :-/

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

Nor should it have to. It just needs to dispose of all responsibilities it
has acquired (read: clean up what it has dirtied, or what others have
dirtied for it). That's done by closing the defer scope.

But let me take a step back. I've been going back and forth on this
basically since yesterday.

In practice, the caller-defined scopes lead to nicer code.

If run_tests creates an implicit scope per test, most of the tests can just
issue their defers without thinking about it too much.

For cases where the implicit scope is not enough, the caller has to know
that a certain function needs to be run in a dedicated scope or else it
will interfere with something else that it's running. That's not great, it
complicates the caller-callee contract in a way that's not captured
anywhere in the syntax. But I suspect it's going to be just fine, these
scripts are not exactly complex, and if there's an interference, I figure
it will be easy to notice.

The major upside is that we avoid the need to pepper the code with
defer_scoped_fn.

So I'll drop defer_scoped_fn and add in_defer_scope:

in_defer_scope()
{
	local ret

	defer_scope_push
	"$@"
	ret=$?
	defer_scope_pop

	return ret
}

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

I'd just grandfather those in, but having this stuff consolidated would
obviously be nice.

I think in practice we just need to add the trap registration to
forwarding.sh, and per bash script do something like:

-trap cleanup EXIT
 setup_prepare
+defer cleanup
 setup_wait

It should be fairly mechanical most of the time. But the defer stuff works
without it as well, so we can take care of that later on.

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

Oh, that's what you mean by relaxing the edits. Yeah, I think I'd want that
to print 2 if at all possible. I think in_ns() is the only helper that
violates this.

Powered by blists - more mailing lists