Re: [PATCH net-next v2 4/4] selftests: drv-net: rss_ctx: add tests for RSS configuration and contexts

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 25 Jun 2024 16:43:55 +0200 Petr Machata wrote:
> > There are 4 things to clean up, with doesn't cover all of them
> > naturally and complicates the code.  
> 
> Yeah, you can't use it everywhere, but you can use it for the ethtool
> config here.
> 
> Re complexity, how about this?
> 
> import contextlib
> 
> @contextlib.contextmanager
> def require_contexts(cfg, count):
>     qcnt = len(_get_rx_cnts(cfg))
>     if qcnt >= count:
>         qcnt = None
>     else:
>         try:
>             ksft_pr(f"Increasing queue count {qcnt} -> {count}")
>             ethtool(f"-L {cfg.ifname} combined {count}")
>         except:
>             raise KsftSkipEx("Not enough queues for the test")
> 
>     try:
>         yield
>     finally:
>         if qcnt is not None:
>             ethtool(f"-L {cfg.ifname} combined {qcnt}")
> 
> This is mostly just business logic, hardly any boilerplate, and still
> just uses standard Python. You get the setup and cleanup next to each
> other, which is important for cross-comparing the two.

TBH I don't really understand of how the above works.

> Anyway, if I don't persuade you for The Right Path, something like this
> would at least get rid of the duplication:
> 
>     qcnt = contexts_setup(cfg, 2 + 2 * ctx_cnt)
>     try:
>         ...
>     finally:
>         if qcnt:
>             contexts_teardown(cfg, qcnt)

Are we discussing this exact test script or general guidance?

If the general guidance, my principle is to make the test look like
a list of bash commands as much as possible. Having to wrap
every single command you need to undo with a context manager
will take us pretty far from a linear script.

That's why I'd prefer if we provided a mechanism which makes
it easy to defer execution, rather than focus on particular cases.

> > Once again, I'm thinking about adding some form of deferred execution.
> > 	
> > 	ethtool(f"-L {self._cfg.ifname} combined {self._qcnt}")
> > 	undo(ethtool, f"-L {self._cfg.ifname} combined {old_qcnt}")
> >
> > Where cleanups will be executed in reverse order by ksft_run() after
> > the test, with the option to delete them.
> >
> > 	nid = ethtool_create(cfg, "-N", flow)
> > 	ntuple = undo(ethtool, f"-N {cfg.ifname} delete {nid}")
> > 	# .. code using ntuple ...
> > 	ntuple.exec()
> > 	# .. now ntuple is gone
> >
> > or/and:
> >
> > 	nid = ethtool_create(cfg, "-N", flow)
> > 	with undo(ethtool, f"-N {cfg.ifname} delete {nid}"):
> > 		# .. code using ntuple ...
> > 	# .. now ntuple is gone
> >
> > Thoughts?  
> 
> Sure, this can be done, but you are introducing a new mechanism to solve
> something that the language has had support for for 15 years or so.

Well, I can't make the try: yield work for me :(

#!/bin/python3

import contextlib

@contextlib.contextmanager
def bla():
    try:
        yield
    except:
        print("deferred thing")

bla()
print("done")


Gives me:
$ ./test.py 
done


I don't know enough Python, IDK if we can assume much Python expertise
from others.

What we basically want is a form of atexit:

https://docs.python.org/3/library/atexit.html

The fact atexit module exists makes me wonder whether the problem is
really solved by the language itself. But maybe there's a deeper reason
for atexit.

> Like, it's not terrible. I like it better than the try/finally aprroach,
> because at least the setup and cleanup are localized.
> 
> Call it defer though? It doesn't "undo" there and then, but at some
> later point.

I like "defer". We're enqueuing for deferred exec. So another option
would be "enqueue". But I think "defer" is indeed better.

rm = defer(cmd, "rm example.txt")
rm.exec()   # run now, remove from internal queue
rm.cancel() # remove from queue, don't run

?