[PATCH net-next v2 0/3] selftests: drv-net: add ability to schedule cleanup with defer()

[PATCH net-next v2 0/3] selftests: drv-net: add ability to schedule cleanup with defer()

[Jakub Kicinski]

Introduce a defer / cleanup mechanism for driver selftests.
More detailed info in the second patch.

Jakub Kicinski (3):
  selftests: net: ksft: avoid continue when handling results
  selftests: drv-net: add ability to schedule cleanup with defer()
  selftests: drv-net: rss_ctx: convert to defer()

 .../selftests/drivers/net/hw/rss_ctx.py       | 225 ++++++++----------
 tools/testing/selftests/net/lib/py/ksft.py    |  49 ++--
 tools/testing/selftests/net/lib/py/utils.py   |  34 +++
 3 files changed, 167 insertions(+), 141 deletions(-)

-- 
2.45.2

[PATCH net-next v2 1/3] selftests: net: ksft: avoid continue when handling results

[Jakub Kicinski]

Exception handlers print the result and use continue
to skip the non-exception result printing. This makes
inserting common post-test code hard. Refactor to
avoid the continues and have only one ktap_result() call.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/net/lib/py/ksft.py | 28 +++++++++++-----------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/net/lib/py/ksft.py b/tools/testing/selftests/net/lib/py/ksft.py
index b6ce3f33d41e..789433262dc7 100644
--- a/tools/testing/selftests/net/lib/py/ksft.py
+++ b/tools/testing/selftests/net/lib/py/ksft.py
@@ -130,29 +130,29 @@ KSFT_RESULT_ALL = True
     for case in cases:
         KSFT_RESULT = True
         cnt += 1
+        comment = ""
+        cnt_key = ""
+
         try:
             case(*args)
         except KsftSkipEx as e:
-            ktap_result(True, cnt, case, comment="SKIP " + str(e))
-            totals['skip'] += 1
-            continue
+            comment = "SKIP " + str(e)
+            cnt_key = 'skip'
         except KsftXfailEx as e:
-            ktap_result(True, cnt, case, comment="XFAIL " + str(e))
-            totals['xfail'] += 1
-            continue
+            comment = "XFAIL " + str(e)
+            cnt_key = 'xfail'
         except Exception as e:
             tb = traceback.format_exc()
             for line in tb.strip().split('\n'):
                 ksft_pr("Exception|", line)
-            ktap_result(False, cnt, case)
-            totals['fail'] += 1
-            continue
+            KSFT_RESULT = False
+            cnt_key = 'fail'
 
-        ktap_result(KSFT_RESULT, cnt, case)
-        if KSFT_RESULT:
-            totals['pass'] += 1
-        else:
-            totals['fail'] += 1
+        if not cnt_key:
+            cnt_key = 'pass' if KSFT_RESULT else 'fail'
+
+        ktap_result(KSFT_RESULT, cnt, case, comment=comment)
+        totals[cnt_key] += 1
 
     print(
         f"# Totals: pass:{totals['pass']} fail:{totals['fail']} xfail:{totals['xfail']} xpass:0 skip:{totals['skip']} error:0"
-- 
2.45.2

[PATCH net-next v2 2/3] selftests: drv-net: add ability to schedule cleanup with defer()

[Jakub Kicinski]

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

  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.

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

Link: https://lore.kernel.org/all/877cedb2ki.fsf@nvidia.com/ # [1]
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - split refactor to previous patch
 - user bare except instead of except Exception
 - rename _exec() -> exec_only() and use in flush
 - reorder queue removal vs calling callback
 - add print to indicate ID of failed callback
 - remove the state flags
---
 tools/testing/selftests/net/lib/py/ksft.py  | 21 +++++++++++++
 tools/testing/selftests/net/lib/py/utils.py | 34 +++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/tools/testing/selftests/net/lib/py/ksft.py b/tools/testing/selftests/net/lib/py/ksft.py
index 789433262dc7..3aaa2748a58e 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
+
+    i = 0
+    qlen_start = len(global_defer_queue)
+    while global_defer_queue:
+        i += 1
+        entry = global_defer_queue.pop()
+        try:
+            entry.exec_only()
+        except:
+            ksft_pr(f"Exception while handling defer / cleanup (callback {i} of {qlen_start})!")
+            tb = traceback.format_exc()
+            for line in tb.strip().split('\n'):
+                ksft_pr("Defer Exception|", line)
+            KSFT_RESULT = False
+
+
 def ksft_run(cases=None, globs=None, case_pfx=None, args=()):
     cases = cases or []
 
@@ -148,6 +167,8 @@ KSFT_RESULT_ALL = True
             KSFT_RESULT = False
             cnt_key = 'fail'
 
+        ksft_flush_defer()
+
         if not cnt_key:
             cnt_key = 'pass' if KSFT_RESULT else 'fail'
 
diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py
index 405aa510aaf2..72590c3f90f1 100644
--- a/tools/testing/selftests/net/lib/py/utils.py
+++ b/tools/testing/selftests/net/lib/py/utils.py
@@ -66,6 +66,40 @@ import time
         return self.process(terminate=self.terminate, fail=self.check_fail)
 
 
+global_defer_queue = []
+
+
+class defer:
+    def __init__(self, func, *args, **kwargs):
+        global global_defer_queue
+
+        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._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()
+
+    def exec_only(self):
+        self.func(*self.args, **self.kwargs)
+
+    def cancel(self):
+        self._queue.remove(self)
+
+    def exec(self):
+        self.cancel()
+        self.exec_only()
+
+
 def tool(name, args, json=None, ns=None, host=None):
     cmd_str = name + ' '
     if json:
-- 
2.45.2

[PATCH net-next v2 3/3] selftests: drv-net: rss_ctx: convert to defer()

[Jakub Kicinski]

Use just added defer().

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
v2:
 - drop the use of .queued flag, track removals locally
---
 .../selftests/drivers/net/hw/rss_ctx.py       | 225 ++++++++----------
 1 file changed, 98 insertions(+), 127 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/hw/rss_ctx.py b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
index 68c7d40214eb..3a804e41f7cb 100755
--- a/tools/testing/selftests/drivers/net/hw/rss_ctx.py
+++ b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
@@ -8,7 +8,7 @@ from lib.py import NetDrvEpEnv
 from lib.py import NetdevFamily
 from lib.py import KsftSkipEx
 from lib.py import rand_port
-from lib.py import ethtool, ip, GenerateTraffic, CmdExitFailure
+from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
 
 
 def _rss_key_str(key):
@@ -127,64 +127,56 @@ from lib.py import ethtool, ip, GenerateTraffic, CmdExitFailure
 
     # Try to allocate more queues when necessary
     qcnt = len(_get_rx_cnts(cfg))
-    if qcnt >= 2 + 2 * ctx_cnt:
-        qcnt = None
-    else:
+    if qcnt < 2 + 2 * ctx_cnt:
         try:
             ksft_pr(f"Increasing queue count {qcnt} -> {2 + 2 * ctx_cnt}")
             ethtool(f"-L {cfg.ifname} combined {2 + 2 * ctx_cnt}")
+            defer(ethtool, f"-L {cfg.ifname} combined {qcnt}")
         except:
             raise KsftSkipEx("Not enough queues for the test")
 
-    ntuple = []
-    ctx_id = []
     ports = []
-    try:
-        # Use queues 0 and 1 for normal traffic
-        ethtool(f"-X {cfg.ifname} equal 2")
 
-        for i in range(ctx_cnt):
-            want_cfg = f"start {2 + i * 2} equal 2"
-            create_cfg = want_cfg if create_with_cfg else ""
+    # Use queues 0 and 1 for normal traffic
+    ethtool(f"-X {cfg.ifname} equal 2")
+    defer(ethtool, f"-X {cfg.ifname} default")
 
-            try:
-                ctx_id.append(ethtool_create(cfg, "-X", f"context new {create_cfg}"))
-            except CmdExitFailure:
-                # try to carry on and skip at the end
-                if i == 0:
-                    raise
-                ksft_pr(f"Failed to create context {i + 1}, trying to test what we got")
-                ctx_cnt = i
-                break
+    for i in range(ctx_cnt):
+        want_cfg = f"start {2 + i * 2} equal 2"
+        create_cfg = want_cfg if create_with_cfg else ""
 
-            if not create_with_cfg:
-                ethtool(f"-X {cfg.ifname} context {ctx_id[i]} {want_cfg}")
+        try:
+            ctx_id = ethtool_create(cfg, "-X", f"context new {create_cfg}")
+            defer(ethtool, f"-X {cfg.ifname} context {ctx_id} delete")
+        except CmdExitFailure:
+            # try to carry on and skip at the end
+            if i == 0:
+                raise
+            ksft_pr(f"Failed to create context {i + 1}, trying to test what we got")
+            ctx_cnt = i
+            break
 
-            # Sanity check the context we just created
-            data = get_rss(cfg, ctx_id[i])
-            ksft_eq(min(data['rss-indirection-table']), 2 + i * 2, "Unexpected context cfg: " + str(data))
-            ksft_eq(max(data['rss-indirection-table']), 2 + i * 2 + 1, "Unexpected context cfg: " + str(data))
+        if not create_with_cfg:
+            ethtool(f"-X {cfg.ifname} context {ctx_id} {want_cfg}")
 
-            ports.append(rand_port())
-            flow = f"flow-type tcp{cfg.addr_ipver} dst-port {ports[i]} context {ctx_id[i]}"
-            ntuple.append(ethtool_create(cfg, "-N", flow))
+        # Sanity check the context we just created
+        data = get_rss(cfg, ctx_id)
+        ksft_eq(min(data['rss-indirection-table']), 2 + i * 2, "Unexpected context cfg: " + str(data))
+        ksft_eq(max(data['rss-indirection-table']), 2 + i * 2 + 1, "Unexpected context cfg: " + str(data))
 
-        for i in range(ctx_cnt):
-            cnts = _get_rx_cnts(cfg)
-            GenerateTraffic(cfg, port=ports[i]).wait_pkts_and_stop(20000)
-            cnts = _get_rx_cnts(cfg, prev=cnts)
+        ports.append(rand_port())
+        flow = f"flow-type tcp{cfg.addr_ipver} dst-port {ports[i]} context {ctx_id}"
+        ntuple = ethtool_create(cfg, "-N", flow)
+        defer(ethtool, f"-N {cfg.ifname} delete {ntuple}")
 
-            ksft_lt(sum(cnts[ :2]), 10000, "traffic on main context:" + str(cnts))
-            ksft_ge(sum(cnts[2+i*2:4+i*2]), 20000, f"traffic on context {i}: " + str(cnts))
-            ksft_eq(sum(cnts[2:2+i*2] + cnts[4+i*2:]), 0, "traffic on other contexts: " + str(cnts))
-    finally:
-        for nid in ntuple:
-            ethtool(f"-N {cfg.ifname} delete {nid}")
-        for cid in ctx_id:
-            ethtool(f"-X {cfg.ifname} context {cid} delete")
-        ethtool(f"-X {cfg.ifname} default")
-        if qcnt:
-            ethtool(f"-L {cfg.ifname} combined {qcnt}")
+    for i in range(ctx_cnt):
+        cnts = _get_rx_cnts(cfg)
+        GenerateTraffic(cfg, port=ports[i]).wait_pkts_and_stop(20000)
+        cnts = _get_rx_cnts(cfg, prev=cnts)
+
+        ksft_lt(sum(cnts[ :2]), 10000, "traffic on main context:" + str(cnts))
+        ksft_ge(sum(cnts[2+i*2:4+i*2]), 20000, f"traffic on context {i}: " + str(cnts))
+        ksft_eq(sum(cnts[2:2+i*2] + cnts[4+i*2:]), 0, "traffic on other contexts: " + str(cnts))
 
     if requested_ctx_cnt != ctx_cnt:
         raise KsftSkipEx(f"Tested only {ctx_cnt} contexts, wanted {requested_ctx_cnt}")
@@ -216,24 +208,23 @@ from lib.py import ethtool, ip, GenerateTraffic, CmdExitFailure
 
     # Try to allocate more queues when necessary
     qcnt = len(_get_rx_cnts(cfg))
-    if qcnt >= 2 + 2 * ctx_cnt:
-        qcnt = None
-    else:
+    if qcnt < 2 + 2 * ctx_cnt:
         try:
             ksft_pr(f"Increasing queue count {qcnt} -> {2 + 2 * ctx_cnt}")
             ethtool(f"-L {cfg.ifname} combined {2 + 2 * ctx_cnt}")
+            defer(ethtool, f"-L {cfg.ifname} combined {qcnt}")
         except:
             raise KsftSkipEx("Not enough queues for the test")
 
     ntuple = []
-    ctx_id = []
+    ctx = []
     ports = []
 
     def remove_ctx(idx):
-        ethtool(f"-N {cfg.ifname} delete {ntuple[idx]}")
+        ntuple[idx].exec()
         ntuple[idx] = None
-        ethtool(f"-X {cfg.ifname} context {ctx_id[idx]} delete")
-        ctx_id[idx] = None
+        ctx[idx].exec()
+        ctx[idx] = None
 
     def check_traffic():
         for i in range(ctx_cnt):
@@ -241,7 +232,7 @@ from lib.py import ethtool, ip, GenerateTraffic, CmdExitFailure
             GenerateTraffic(cfg, port=ports[i]).wait_pkts_and_stop(20000)
             cnts = _get_rx_cnts(cfg, prev=cnts)
 
-            if ctx_id[i] is None:
+            if ctx[i]:
                 ksft_lt(sum(cnts[ :2]), 10000, "traffic on main context:" + str(cnts))
                 ksft_ge(sum(cnts[2+i*2:4+i*2]), 20000, f"traffic on context {i}: " + str(cnts))
                 ksft_eq(sum(cnts[2:2+i*2] + cnts[4+i*2:]), 0, "traffic on other contexts: " + str(cnts))
@@ -249,41 +240,32 @@ from lib.py import ethtool, ip, GenerateTraffic, CmdExitFailure
                 ksft_ge(sum(cnts[ :2]), 20000, "traffic on main context:" + str(cnts))
                 ksft_eq(sum(cnts[2: ]),     0, "traffic on other contexts: " + str(cnts))
 
-    try:
-        # Use queues 0 and 1 for normal traffic
-        ethtool(f"-X {cfg.ifname} equal 2")
+    # Use queues 0 and 1 for normal traffic
+    ethtool(f"-X {cfg.ifname} equal 2")
+    defer(ethtool, f"-X {cfg.ifname} default")
 
-        for i in range(ctx_cnt):
-            ctx_id.append(ethtool_create(cfg, "-X", f"context new start {2 + i * 2} equal 2"))
+    for i in range(ctx_cnt):
+        ctx_id = ethtool_create(cfg, "-X", f"context new start {2 + i * 2} equal 2")
+        ctx.append(defer(ethtool, f"-X {cfg.ifname} context {ctx_id} delete"))
 
-            ports.append(rand_port())
-            flow = f"flow-type tcp{cfg.addr_ipver} dst-port {ports[i]} context {ctx_id[i]}"
-            ntuple.append(ethtool_create(cfg, "-N", flow))
+        ports.append(rand_port())
+        flow = f"flow-type tcp{cfg.addr_ipver} dst-port {ports[i]} context {ctx_id}"
+        ntuple_id = ethtool_create(cfg, "-N", flow)
+        ntuple.append(defer(ethtool, f"-N {cfg.ifname} delete {ntuple_id}"))
 
-        check_traffic()
+    check_traffic()
 
-        # Remove middle context
-        remove_ctx(ctx_cnt // 2)
-        check_traffic()
+    # Remove middle context
+    remove_ctx(ctx_cnt // 2)
+    check_traffic()
 
-        # Remove first context
-        remove_ctx(0)
-        check_traffic()
+    # Remove first context
+    remove_ctx(0)
+    check_traffic()
 
-        # Remove last context
-        remove_ctx(-1)
-        check_traffic()
-
-    finally:
-        for nid in ntuple:
-            if nid is not None:
-                ethtool(f"-N {cfg.ifname} delete {nid}")
-        for cid in ctx_id:
-            if cid is not None:
-                ethtool(f"-X {cfg.ifname} context {cid} delete")
-        ethtool(f"-X {cfg.ifname} default")
-        if qcnt:
-            ethtool(f"-L {cfg.ifname} combined {qcnt}")
+    # Remove last context
+    remove_ctx(-1)
+    check_traffic()
 
     if requested_ctx_cnt != ctx_cnt:
         raise KsftSkipEx(f"Tested only {ctx_cnt} contexts, wanted {requested_ctx_cnt}")
@@ -298,69 +280,58 @@ from lib.py import ethtool, ip, GenerateTraffic, CmdExitFailure
     require_ntuple(cfg)
 
     queue_cnt = len(_get_rx_cnts(cfg))
-    if queue_cnt >= 4:
-        queue_cnt = None
-    else:
+    if queue_cnt < 4:
         try:
             ksft_pr(f"Increasing queue count {queue_cnt} -> 4")
             ethtool(f"-L {cfg.ifname} combined 4")
+            defer(ethtool, f"-L {cfg.ifname} combined {queue_cnt}")
         except:
             raise KsftSkipEx("Not enough queues for the test")
 
-    ctx_id = None
-    ntuple = None
     if other_ctx == 0:
         ethtool(f"-X {cfg.ifname} equal 4")
+        defer(ethtool, f"-X {cfg.ifname} default")
     else:
         other_ctx = ethtool_create(cfg, "-X", "context new")
         ethtool(f"-X {cfg.ifname} context {other_ctx} equal 4")
+        defer(ethtool, f"-X {cfg.ifname} context {other_ctx} delete")
 
-    try:
-        ctx_id = ethtool_create(cfg, "-X", "context new")
-        ethtool(f"-X {cfg.ifname} context {ctx_id} start 2 equal 2")
+    ctx_id = ethtool_create(cfg, "-X", "context new")
+    ethtool(f"-X {cfg.ifname} context {ctx_id} start 2 equal 2")
+    defer(ethtool, f"-X {cfg.ifname} context {ctx_id} delete")
 
-        port = rand_port()
-        if other_ctx:
-            flow = f"flow-type tcp{cfg.addr_ipver} dst-port {port} context {other_ctx}"
-            ntuple = ethtool_create(cfg, "-N", flow)
+    port = rand_port()
+    if other_ctx:
+        flow = f"flow-type tcp{cfg.addr_ipver} dst-port {port} context {other_ctx}"
+        ntuple_id = ethtool_create(cfg, "-N", flow)
+        ntuple = defer(ethtool, f"-N {cfg.ifname} delete {ntuple_id}")
 
-        # Test the main context
-        cnts = _get_rx_cnts(cfg)
-        GenerateTraffic(cfg, port=port).wait_pkts_and_stop(20000)
-        cnts = _get_rx_cnts(cfg, prev=cnts)
+    # Test the main context
+    cnts = _get_rx_cnts(cfg)
+    GenerateTraffic(cfg, port=port).wait_pkts_and_stop(20000)
+    cnts = _get_rx_cnts(cfg, prev=cnts)
 
-        ksft_ge(sum(cnts[ :4]), 20000, "traffic on main context: " + str(cnts))
-        ksft_ge(sum(cnts[ :2]),  7000, "traffic on main context (1/2): " + str(cnts))
-        ksft_ge(sum(cnts[2:4]),  7000, "traffic on main context (2/2): " + str(cnts))
-        if other_ctx == 0:
-            ksft_eq(sum(cnts[4: ]),     0, "traffic on other queues: " + str(cnts))
+    ksft_ge(sum(cnts[ :4]), 20000, "traffic on main context: " + str(cnts))
+    ksft_ge(sum(cnts[ :2]),  7000, "traffic on main context (1/2): " + str(cnts))
+    ksft_ge(sum(cnts[2:4]),  7000, "traffic on main context (2/2): " + str(cnts))
+    if other_ctx == 0:
+        ksft_eq(sum(cnts[4: ]),     0, "traffic on other queues: " + str(cnts))
 
-        # Now create a rule for context 1 and make sure traffic goes to a subset
-        if other_ctx:
-            ethtool(f"-N {cfg.ifname} delete {ntuple}")
-            ntuple = None
-        flow = f"flow-type tcp{cfg.addr_ipver} dst-port {port} context {ctx_id}"
-        ntuple = ethtool_create(cfg, "-N", flow)
+    # Now create a rule for context 1 and make sure traffic goes to a subset
+    if other_ctx:
+        ntuple.exec()
+    flow = f"flow-type tcp{cfg.addr_ipver} dst-port {port} context {ctx_id}"
+    ntuple_id = ethtool_create(cfg, "-N", flow)
+    defer(ethtool, f"-N {cfg.ifname} delete {ntuple_id}")
 
-        cnts = _get_rx_cnts(cfg)
-        GenerateTraffic(cfg, port=port).wait_pkts_and_stop(20000)
-        cnts = _get_rx_cnts(cfg, prev=cnts)
+    cnts = _get_rx_cnts(cfg)
+    GenerateTraffic(cfg, port=port).wait_pkts_and_stop(20000)
+    cnts = _get_rx_cnts(cfg, prev=cnts)
 
-        ksft_lt(sum(cnts[ :2]),  7000, "traffic on main context: " + str(cnts))
-        ksft_ge(sum(cnts[2:4]), 20000, "traffic on extra context: " + str(cnts))
-        if other_ctx == 0:
-            ksft_eq(sum(cnts[4: ]),     0, "traffic on other queues: " + str(cnts))
-    finally:
-        if ntuple is not None:
-            ethtool(f"-N {cfg.ifname} delete {ntuple}")
-        if ctx_id:
-            ethtool(f"-X {cfg.ifname} context {ctx_id} delete")
-        if other_ctx == 0:
-            ethtool(f"-X {cfg.ifname} default")
-        else:
-            ethtool(f"-X {cfg.ifname} context {other_ctx} delete")
-        if queue_cnt:
-            ethtool(f"-L {cfg.ifname} combined {queue_cnt}")
+    ksft_lt(sum(cnts[ :2]),  7000, "traffic on main context: " + str(cnts))
+    ksft_ge(sum(cnts[2:4]), 20000, "traffic on extra context: " + str(cnts))
+    if other_ctx == 0:
+        ksft_eq(sum(cnts[4: ]),     0, "traffic on other queues: " + str(cnts))
 
 
 def test_rss_context_overlap2(cfg):
-- 
2.45.2

Re: [PATCH net-next v2 2/3] selftests: drv-net: add ability to schedule cleanup with defer()

[Przemek Kitszel]

On 6/27/24 20:55, 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. It's also possible to capture them
> and execute earlier (in which case they get automatically de-queued).
> 
>    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.
> 
> 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..
> 
> Link: https://lore.kernel.org/all/877cedb2ki.fsf@nvidia.com/ # [1]
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> v2:
>   - split refactor to previous patch
>   - user bare except instead of except Exception
>   - rename _exec() -> exec_only() and use in flush
>   - reorder queue removal vs calling callback
>   - add print to indicate ID of failed callback
>   - remove the state flags
> ---
>   tools/testing/selftests/net/lib/py/ksft.py  | 21 +++++++++++++
>   tools/testing/selftests/net/lib/py/utils.py | 34 +++++++++++++++++++++
>   2 files changed, 55 insertions(+)
> 

nice improvement!
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Re: [PATCH net-next v2 1/3] selftests: net: ksft: avoid continue when handling results

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> Exception handlers print the result and use continue
> to skip the non-exception result printing. This makes
> inserting common post-test code hard. Refactor to
> avoid the continues and have only one ktap_result() call.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Petr Machata <petrm@nvidia.com>

Re: [PATCH net-next v2 2/3] selftests: drv-net: add ability to schedule cleanup with defer()

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> 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. It's also possible to capture them
> and execute earlier (in which case they get automatically de-queued).
>
>   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.
>
> 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..
>
> Link: https://lore.kernel.org/all/877cedb2ki.fsf@nvidia.com/ # [1]
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Petr Machata <petrm@nvidia.com>

Re: [PATCH net-next v2 3/3] selftests: drv-net: rss_ctx: convert to defer()

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> Use just added defer().
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

All the whitespace changes make it look messy, but it's actually fairly
contained with diff -w.

Reviewed-by: Petr Machata <petrm@nvidia.com>

Re: [PATCH net-next v2 0/3] selftests: drv-net: add ability to schedule cleanup with defer()

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 27 Jun 2024 11:54:59 -0700 you wrote:
> Introduce a defer / cleanup mechanism for driver selftests.
> More detailed info in the second patch.
> 
> Jakub Kicinski (3):
>   selftests: net: ksft: avoid continue when handling results
>   selftests: drv-net: add ability to schedule cleanup with defer()
>   selftests: drv-net: rss_ctx: convert to defer()
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/3] selftests: net: ksft: avoid continue when handling results
    https://git.kernel.org/netdev/net-next/c/147997afaad0
  - [net-next,v2,2/3] selftests: drv-net: add ability to schedule cleanup with defer()
    https://git.kernel.org/netdev/net-next/c/8510801a9dbd
  - [net-next,v2,3/3] selftests: drv-net: rss_ctx: convert to defer()
    https://git.kernel.org/netdev/net-next/c/0759356bf5fa

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html