Re: [net-next,v9,09/10] test: Add ext_hdr.py in networking selftests

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html

Simon says: pylint also has quite a lot to say about this patch. I'm
assuming it is all minor stuff. But could you look into addressing the
issues it flags?

---
test: Add ext_hdr.py in networking selftests

This commit adds a new Python helper file that contains Extension Header
format definitions and related functions for building IPv6 extension
header chains in networking selftests.

> diff --git a/tools/testing/selftests/net/ext_hdr.py b/tools/testing/selftests/net/ext_hdr.py
> new file mode 100755
> index 000000000000..bfb7da4a7c88
> --- /dev/null
> +++ b/tools/testing/selftests/net/ext_hdr.py
> @@ -0,0 +1,385 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# Helper functions for creating extension headers using scapy
> +
> +import ctypes
> +import shlex
> +import socket
> +import sys
> +import subprocess
> +import scapy
> +import proto_nums
> +
> +
> +# Read a sysctl
> +def sysctl_read(name):
> +    try:
> +        # shlex.split helps handle arguments correctly
> +        command = shlex.split(f"sysctl -n {name}")
> +        # Use check=True to raise an exception if the command fails
> +        result = subprocess.run(command, check=True,
> +                    capture_output=True, text=True)
> +        value = result.stdout.strip()
> +    except subprocess.CalledProcessError as ex:
> +        print(f"Error reading sysctl: {ex.stderr}")
> +    except FileNotFoundError:
> +        print("The 'sysctl' command was not found. "
> +              "Check your system's PATH.")
> +
> +    return int(value)

What happens when an exception is caught here? When either
subprocess.CalledProcessError or FileNotFoundError occurs, the exception
handlers print an error message but don't set 'value'. The function then
reaches the 'return int(value)' statement, which will raise
UnboundLocalError.

This will crash the test in several places:

tools/testing/selftests/net/ext_hdr.py:check_eh_order() uses the return:
    do_check = sysctl_read("net.ipv6.enforce_ext_hdr_order")

tools/testing/selftests/net/ext_hdr.py:make_eh_chain() uses the return:
    pair = make_full_hbh_dst_options_list(nnext_hdr, args,
        sysctl_read("net.ipv6.max_hbh_opts_number"),
        sysctl_read("net.ipv6.max_hbh_length"))

and:
    pair = make_full_hbh_dst_options_list(nnext_hdr, args,
        sysctl_read("net.ipv6.max_dst_opts_number"),
        sysctl_read("net.ipv6.max_dst_opts_length"))

Should the exception handlers return a default value or re-raise to let
callers handle it?

> +
> +# Common definitions for Destination and Hop-by-Hop options

[ ... ]

Should ext_hdr.py be added to TEST_FILES in the Makefile? The selftests
build system documentation requires that any file imported by test scripts
must be added to TEST_FILES. Without this, tests will work in the source
tree but fail after 'make install' with ModuleNotFoundError.

The file is imported at line 12 ('import proto_nums'), and appears to be
a library meant for import rather than direct execution. Both ext_hdr.py
and proto_nums.py need to be added to TEST_FILES in
tools/testing/selftests/net/Makefile.