Re: [RFC PATCH bpf-next v5 6/8] selftests/bpf: extract BPF skeleton generation into a helper script

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch introduces a behavior change in permissive mode that results in noisy build logs and redundant error messages, contradicting the commit message claim that 'behavior is unchanged'. Additionally, a script comment references a non-existent file.
--

--- Patch [6]: [RFC PATCH bpf-next v5 6/8] selftests/bpf: extract BPF skeleton generation into a helper script ---
commit e4a593d3d0bfe3675d723364cb65f81a2812b866
Author: Mykola Lysenko <[email protected]>

selftests/bpf: extract BPF skeleton generation into a helper script

This commit extracts the BPF skeleton generation steps into a dedicated shell
script (gen_bpf_skel.sh) to reduce Makefile complexity and escaping issues. It
handles regular, light, signed light, and linked skeletons through script
flags.

> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 0ee02eeac6bca..dbde2f6bd044d 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile

[ ... ]

> -$(TRUNNER_BPF_SKELS): %.skel.h: %.bpf.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
> -	$(Q)$(if $(PERMISSIVE),if [ ! -f $$< ]; then			\
> -		$$(RM) $$@ $$(@:.skel.h=.subskel.h);			\
> -		printf '  %-12s %s\n' 'SKIP-SKEL' '$$(notdir $$@)' 1>&2; \
> -		exit 0;							\
> -	fi;)								\
> -	printf '  %-12s %s\n' 'GEN-SKEL' '[$(TRUNNER_BINARY)] $$(notdir $$@)' 1>&2; \
> -	$$(BPFTOOL) gen object $$(<:.o=.linked1.o) $$< &&		\

[ ... ]

> +$(TRUNNER_BPF_SKELS): %.skel.h: %.bpf.o $(BPFTOOL) gen_bpf_skel.sh | $(TRUNNER_OUTPUT)
> +	$$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
> +	$(Q)BPFTOOL=$$(BPFTOOL) ./gen_bpf_skel.sh				\
> +		--name $$(notdir $$(<:.bpf.o=))					\
> +		--skel $$@ --subskel $$(@:.skel.h=.subskel.h) $$< $$(call skip_on_fail,SKEL,$$(@:.skel.h=.subskel.h))

[Severity: Medium]
Does this change in permissive mode behavior contradict the commit message
which states "behavior is unchanged"?

Previously, if a .bpf.o file failed to compile in permissive mode, the
skeleton generation recipe would quietly check for the missing file, print
SKIP-SKEL, and exit early without executing bpftool.

With the missing-file guard removed, the recipe now unconditionally prints
GEN-SKEL and calls gen_bpf_skel.sh. This invokes bpftool on a missing file,
causing it to fail loudly to stderr before the skip_on_fail suffix catches
the exit code. This results in noisy build logs and redundant error messages.

> diff --git a/tools/testing/selftests/bpf/gen_bpf_skel.sh b/tools/testing/selftests/bpf/gen_bpf_skel.sh
> new file mode 100755
> index 0000000000000..e234f3a57d46b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/gen_bpf_skel.sh
> @@ -0,0 +1,93 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0

[ ... ]

> +# The bpftool binary is taken from $BPFTOOL (default: bpftool from PATH).
> +# On failure all outputs and intermediates are removed and the script
> +# exits non-zero; permissive-mode skipping is the caller's business
> +# (see skip_on_fail in Makefile.buildvars).

[Severity: Medium]
Is the reference to Makefile.buildvars correct here?

The file Makefile.buildvars does not exist. It looks like skip_on_fail is
actually defined directly in tools/testing/selftests/bpf/Makefile.

> +
> +set -u

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