Message-ID: <d7a8b57d-0dea-4160-8aa3-24e18cf2490e@intel.com>
Date: Wed, 26 Jun 2024 09:43:54 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <netdev@...r.kernel.org>, <edumazet@...gle.com>, <pabeni@...hat.com>,
	<willemdebruijn.kernel@...il.com>, <leitao@...ian.org>, <petrm@...dia.com>,
	<davem@...emloft.net>
Subject: Re: [RFC net-next 1/2] selftests: drv-net: add ability to schedule
 cleanup with defer()

On 6/26/24 03:36, Jakub Kicinski wrote:
> This implements what I was describing in [1]. When writing a test
> author can schedule cleanup / undo actions right after the creation
> completes, eg:
> 
>    cmd("touch /tmp/file")
>    defer(cmd, "rm /tmp/file")
> 
> defer() takes the function name as first argument, and the rest are
> arguments for that function. defer()red functions are called in
> inverse order after test exits. 

I like the defer name more than undo, and that makes immediate
connection to golang's defer.

For reference golang's defer is called at function exit, the choice was
made that way (instead C++'s destructor at the end of the scope) to make
it more performant. It's a language construct, so could not be easily 
translated into python.

I see why you expect discussion to be potentially long here ;), I just
showed it to my python speaking friends and all the alternatives came
up: context manager, yield, pytest "built in" fixtures and so on.
I like your approach more through, it is much nicer at actual-test code
side.


> It's also possible to capture them
> and execute earlier (in which case they get automatically de-queued).

not sure if this is good, either test developer should be able to easily
guard call to defer, or could call it with a lambda capture so actual
execution will be no-op-ish.
OTOH, why not, both .exec() and .cancel() are nice to use

so, I'm fine both ways

> 
>    undo = defer(cmd, "rm /tmp/file")
>    # ... some unsafe code ...
>    undo.exec()
> 
> As a nice safety all exceptions from defer()ed calls are captured,
> printed, and ignored (they do make the test fail, however).
> This addresses the common problem of exceptions in cleanup paths
> often being unhandled, leading to potential leaks.

Nice! Please only make it so that cleanup-failure does not overwrite
happy-test-path-failure (IOW "ret = ret ? ret : cleanup_ret")

> 
> There is a global action queue, flushed by ksft_run(). We could support
> function level defers too, I guess, but there's no immediate need..

That would be a must have for general solution, would it require some
boilerplate code at function level?

> 
> Link: https://lore.kernel.org/all/877cedb2ki.fsf@nvidia.com/ # [1]
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>   tools/testing/selftests/net/lib/py/ksft.py  | 49 +++++++++++++++------
>   tools/testing/selftests/net/lib/py/utils.py | 41 +++++++++++++++++
>   2 files changed, 76 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/lib/py/ksft.py b/tools/testing/selftests/net/lib/py/ksft.py
> index 45ffe277d94a..4a72b9cbb27d 100644
> --- a/tools/testing/selftests/net/lib/py/ksft.py
> +++ b/tools/testing/selftests/net/lib/py/ksft.py
> @@ -6,6 +6,7 @@ import sys
>   import time
>   import traceback
>   from .consts import KSFT_MAIN_NAME
> +from .utils import global_defer_queue
>   
>   KSFT_RESULT = None
>   KSFT_RESULT_ALL = True
> @@ -108,6 +109,24 @@ KSFT_RESULT_ALL = True
>       print(res)
>   
>   
> +def ksft_flush_defer():
> +    global KSFT_RESULT
> +
> +    while global_defer_queue:
> +        entry = global_defer_queue[-1]
> +        try:
> +            entry.exec()
> +        except Exception:
> +            if global_defer_queue and global_defer_queue[-1] == entry:
> +                global_defer_queue.pop()
> +
> +            ksft_pr("Exception while handling defer / cleanup!")

please print current queue size, if only for convenience of test
developer to be able tell if they are moving forward in 
fix-rerun-observe cycle

> +            tb = traceback.format_exc()
> +            for line in tb.strip().split('\n'):
> +                ksft_pr("Defer Exception|", line)
> +            KSFT_RESULT = False

I have no idea if this could be other error than just False, if so,
don't overwrite

[...]

>   
> +global_defer_queue = []
> +
> +
> +class defer:
> +    def __init__(self, func, *args, **kwargs):
> +        global global_defer_queue
> +        if global_defer_queue is None:
> +            raise Exception("defer environment has not been set up")
> +
> +        if not callable(func):
> +            raise Exception("defer created with un-callable object, did you call the function instead of passing its name?")
> +
> +        self.func = func
> +        self.args = args
> +        self.kwargs = kwargs
> +
> +        self.queued = True
> +        self.executed = False
> +
> +        self._queue =  global_defer_queue
> +        self._queue.append(self)
> +
> +    def __enter__(self):
> +        return self
> +
> +    def __exit__(self, ex_type, ex_value, ex_tb):
> +        return self.exec()

why do you need __enter__ and __exit__ if this is not a context
manager / to-be-used-via-with?

> +
> +    def _exec(self):
> +        self.func(*self.args, **self.kwargs)
> +
> +    def cancel(self):
> +        self._queue.remove(self)
> +        self.queued = False
> +
> +    def exec(self):
> +        self._exec()
> +        self.cancel()
> +        self.executed = True
> +
> +
>   def tool(name, args, json=None, ns=None, host=None):
>       cmd_str = name + ' '
>       if json:

Powered by blists - more mailing lists