Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] In tools/testing/selftests/net/gtp.py, _setup() catches every
CmdExitFailure from `ip link add gtp0 type gtp role ggsn` and raises…
- [Low] _setup() in tools/testing/selftests/net/gtp.py creates a new
GtpFamily handle inside the test netns for each test case (`with…
--
selftests: net: add a test for the gtp netlink family
This adds a Python selftest that uses the gtp generic netlink family
through its ynl spec. It creates a gtp device, then adds, dumps and
deletes PDP contexts by TEI and by MS address. It also checks that
deleting an unknown context returns ENOENT, and it adds a GtpFamily ynl
wrapper.
> diff --git a/tools/testing/selftests/net/gtp.py
> b/tools/testing/selftests/net/gtp.py
> new file mode 100755
> index 0000000000000..b307699b3c1ed
> --- /dev/null
> +++ b/tools/testing/selftests/net/gtp.py
> @@ -0,0 +1,115 @@
[ ... ]
> +def _setup(ns):
> + """Return a gtp family handle, and the ifindex of a new gtp device.
> +
> + Create the device first: that loads gtp.ko through its rtnl link
> + alias. ynl resolves the family ids once per process, with a
> + CTRL_CMD_GETFAMILY dump which does not autoload modules, so the
> + family must be registered before the first family handle is created.
> + """
> + try:
> + ip("link add gtp0 type gtp role ggsn", ns=str(ns))
> + except CmdExitFailure as e:
> + raise KsftSkipEx("no gtp support (CONFIG_GTP, iproute2)") from e
[Severity: Low]
Is this skip broader than intended? CmdExitFailure is raised for any
non-zero exit from ip. That covers old iproute2 and a missing gtp.ko,
but it also covers a real failure in the gtp newlink path. Examples are
gtp_newlink() failing to create or bind its UDP sockets, an -EINVAL
regression, or -EEXIST if an earlier case left a gtp0 behind.
add_del_pdp(), add_two_pdp() and del_unknown_pdp() all go through
_setup(). If gtp link creation regresses, every case would be reported
as SKIP instead of FAIL.
The patch also adds CONFIG_GTP=m to tools/testing/selftests/net/config,
so a correctly configured run should not need this skip. Would it make
sense to check e.cmd.stderr for the specific "not supported" messages
and re-raise anything else? drivers/net/lib/py/env.py already uses that
pattern:
except CmdExitFailure as e:
if "Operation not supported" not in e.cmd.stderr:
raise
> + defer(ip, "link del gtp0", ns=str(ns))
> + ip("link set gtp0 up", ns=str(ns))
> + link = ip("-d link show gtp0", ns=str(ns), json=True)[0]["ifindex"]
> +
> + with NetNSEnter(str(ns)):
> + gtp = GtpFamily()
> +
> + return gtp, link
[Severity: Low]
This isn't a functional bug, but should the GtpFamily handle be closed
when each case ends?
YnlFamily.__init__() opens a netlink socket. It also stores a
functools.partial(self._op, op_name) on self for every op, which makes a
reference cycle:
for op_name, op in self.ops.items():
bound_f = functools.partial(self._op, op_name)
setattr(self, op.ident_name, bound_f)
Because of that cycle, reference counting alone never frees the socket.
It is freed only by the cyclic GC or at process exit. Each case creates
a new handle, so up to three netlink sockets in the test netns stay
open. Each one keeps the namespace alive after NetNS.__exit__() deletes
it.
YnlFamily already has close() and __enter__()/__exit__(). Would adding
defer(gtp.close) in _setup() be cleaner?
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010105740.223992-1-anilkaushikwireless%40gmail.com
