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

On Tue, Aug 4, 2026 at 10:59=E2=80=AFAM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issu=
e(s) to consider:
> - [Medium] The patch introduces a behavior change in permissive mode that=
 results in noisy build logs and redundant error messages, contradicting th=
e 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 ske=
leton 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 s=
hell
> script (gen_bpf_skel.sh) to reduce Makefile complexity and escaping issue=
s. It
> handles regular, light, signed light, and linked skeletons through script
> flags.
>
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selft=
ests/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=3D.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=3D.linked1.o) $$< &&              =
 \
>
> [ ... ]
>
> > +$(TRUNNER_BPF_SKELS): %.skel.h: %.bpf.o $(BPFTOOL) gen_bpf_skel.sh | $=
(TRUNNER_OUTPUT)
> > +     $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
> > +     $(Q)BPFTOOL=3D$$(BPFTOOL) ./gen_bpf_skel.sh                      =
         \
> > +             --name $$(notdir $$(<:.bpf.o=3D))                        =
         \
> > +             --skel $$@ --subskel $$(@:.skel.h=3D.subskel.h) $$< $$(ca=
ll skip_on_fail,SKEL,$$(@:.skel.h=3D.subskel.h))
>
> [Severity: Medium]
> Does this change in permissive mode behavior contradict the commit messag=
e
> 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, prin=
t
> SKIP-SKEL, and exit early without executing bpftool.
>
> With the missing-file guard removed, the recipe now unconditionally print=
s
> GEN-SKEL and calls gen_bpf_skel.sh. This invokes bpftool on a missing fil=
e,
> causing it to fail loudly to stderr before the skip_on_fail suffix catche=
s
> the exit code. This results in noisy build logs and redundant error messa=
ges.

It does - good catch. Reproduced:

prozak@powertower$ touch progs/fentry_test.c && $MK LLVM=3D BPF_STRICT_BUIL=
D=3D0 \
 "progs/fentry_test.c-CFLAGS=3D--definitely-bogus-flag" \
 $PWD/fentry_test.bpf.o $PWD/fentry_test.lskel.h 2>&1 | tail -15
...
  CLNG-BPF [test_progs] fentry_test.bpf.o
clang: error: unknown argument: '--definitely-bogus-flag'
  SKIP-BPF     fentry_test.bpf.o
  GEN-SKEL [test_progs (signed)] fentry_test.lskel.h
libbpf: failed to open file '.../fentry_test.bpf.o': -ENOENT
Error: failed to link '.../fentry_test.bpf.o': No such file or directory (2=
)
  SKIP-SKEL    fentry_test.lskel.h

The GEN-SKEL line and the two error lines are new - the old recipe's
missing-input guard skipped silently before bpftool ever ran. The
final state is identical (outputs removed, build reports success and
continues):

prozak@powertower$ touch progs/fentry_test.c && $MK LLVM=3D
BPF_STRICT_BUILD=3D0 2>&1
...
  CLNG-BPF [test_progs] fentry_test.bpf.o
  GEN-SKEL [test_progs (signed)] fentry_test.lskel.h
...
  CLNG-BPF [test_progs-no_alu32] fentry_test.bpf.o
  GEN-SKEL [test_progs-no_alu32 (signed)] fentry_test.lskel.h
...
  CLNG-BPF [test_progs-cpuv4] fentry_test.bpf.o
  GEN-SKEL [test_progs-cpuv4 (signed)] fentry_test.lskel.h
...

While the output differs, on reflection the extra printing is
derivative - the compile failure is
already reported in full where it happens - so the next revision will
restore the quiet skip
(and with it, the 'behavior is unchanged' claim becomes accurate).

>
> > diff --git a/tools/testing/selftests/bpf/gen_bpf_skel.sh b/tools/testin=
g/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 PAT=
H).
> > +# 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.

Fair, will fix before re-spin.

>
> > +
> > +set -u
>
> --
> Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260804170156.17=
[email protected]?part=3D6