Re: [PATCH] selftests/bpf: convert lirc_mode2 to prog_tests and extend coverage

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing `globfree()` when `glob()` returns an error in `find_loopback_devices`.
- [Low] Orphaned test file `test_lirc_mode2_user.c` left in the source tree.
- [Low] Multi-line comment formatting violates BPF subsystem guidelines.
--

--- Patch [1]: [PATCH] selftests/bpf: convert lirc_mode2 to prog_tests and extend coverage ---
commit 932c1322ffc2d27251b53c4449db5c4ec36061e6
Author: Sean Young <[email protected]>

selftests/bpf: convert lirc_mode2 to prog_tests and extend coverage

While porting, extend coverage of the BPF_LIRC_MODE2 attach/detach/
query API:

[ ... ]

> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 2b2f93dec474b..f8ec0f574559a 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
[ ... ]
> @@ -141,7 +140,6 @@ TEST_GEN_PROGS_EXTENDED = \
>  	bench \
>  	flow_dissector_load \
>  	test_cpp \
> -	test_lirc_mode2_user \
>  	veristat \

[Severity: Low]
This isn't a bug, but it looks like tools/testing/selftests/bpf/test_lirc_mode2_user.c
was removed from the Makefile here but not deleted from the source tree. Should
that file be deleted as part of this patch?

[ ... ]
> diff --git a/tools/testing/selftests/bpf/prog_tests/lirc_mode2.c b/tools/testing/selftests/bpf/prog_tests/lirc_mode2.c
> new file mode 100644
> index 0000000000000..17a38cd03845c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/lirc_mode2.c
> @@ -0,0 +1,328 @@
[ ... ]
> +/* Read the DEVNAME= line out of the first uevent file that matches
> + * pattern, and turn it into a /dev/<name> path.
> + */
> +static bool find_devname(const char *pattern, char *path, size_t path_sz)

[Severity: Low]
This isn't a bug, but does this violate the BPF subsystem multi-line comment
style guidelines? The opening /* should probably be on its own line:

/*
 * Read the DEVNAME= line out of the first uevent file that matches
 * pattern, and turn it into a /dev/<name> path.
 */

[ ... ]
> +/* Load rc-loopback and find the lirc and input chardevs it created. */
> +static bool find_loopback_devices(char *lirc_path, char *input_path,
> +				  size_t path_sz)
> +{
> +	glob_t gl = {};
> +	bool found = false;
> +
> +	/* Ignore failure, we check for the resulting devices below. */
> +	system("modprobe rc-loopback > /dev/null 2>&1");
> +
> +	if (glob("/sys/class/rc/rc*", 0, NULL, &gl)) {
> +		fprintf(stderr, "No rc devices found, is CONFIG_RC_LOOPBACK enabled?\n");
> +		return false;
> +	}

[Severity: Medium]
Could this leak memory if glob() returns an error like GLOB_NOMATCH? Some
implementations of glob() may allocate memory for gl_pathv even on error paths.
Should there be a call to globfree(&gl) before returning false?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1