Re: [PATCH v2 07/15] tools/rv: Implement BPF monitor discovery and listing

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3746 bytes --]

> tools/rv: Implement BPF monitor discovery and listing

TL;DR:
- "make static" may now fail to link libbpf (missing -lelf/-lz) now that BPF
  is on by default
- Changelog lists fewer search paths than the code scans, including a cwd
  fallback

>     tools/rv: Implement BPF monitor discovery and listing
>
>     Implement the BPF monitor listing functionality that scans for compiled
>     BPF monitor object files and displays them in the monitor list.
>
>     If enabled, running rv list shows in-kernel and BPF monitors found in
>     /usr/share/rv/bpf_monitors/ or in ./bpf_monitors/ relative to the RV
>     binary's location (used during development).
>
>     Signed-off-by: Gabriele Monaco <gmonaco@redhat.com>

The commit message names /usr/share/rv/bpf_monitors/ and ./bpf_monitors/
relative to the rv binary.  Is that the complete search list?

bpf_list_monitors() scans every entry of bpf_base_paths[] with /bpf_monitors
appended:

static char bpf_base_paths[][MAX_PATH] = {
	".",
	"/etc/rv",
	"/usr/local/share/rv",
	"/usr/share/rv",
	"", /* Marker */
};

so /etc/rv/bpf_monitors and /usr/local/share/rv/bpf_monitors are searched
as well.

Entry 0 is replaced with the binary's directory only when
readlink("/proc/self/exe") succeeds in bpf_fill_base_paths().  Otherwise it
stays ".", and the current working directory's bpf_monitors is scanned
instead.

All of the directories are scanned, not one or the other, so a monitor
present in more than one of them is listed more than once.

Should the changelog describe the actual search list and the working
directory fallback, or should the list be trimmed to what is documented?
Is /etc/rv an appropriate place to look for BPF object files?

> diff --git a/tools/verification/rv/Makefile.config b/tools/verification/rv/Makefile.config
> index 066302230eb2..0600170ac217 100644
> --- a/tools/verification/rv/Makefile.config
> +++ b/tools/verification/rv/Makefile.config

[ ... ]

> @@ -43,6 +43,29 @@ else
>    $(info libtracefs is missing. Please install libtracefs-dev/libtracefs-devel)
>  endif
>  
> +ifndef BUILD_BPF
> +  # BPF monitors are optional but enabled by default
> +  BUILD_BPF := 1
> +endif
> +
> +ifeq ($(BUILD_BPF),0)
> +  $(info BPF monitor support disabled, building without BPF monitor support.)
> +endif
> +
> +$(call feature_check,libbpf)
> +ifeq ($(feature-libbpf), 1)
> +  $(call detected,CONFIG_LIBBPF)
> +else
> +  $(info libbpf is missing, building without BPF monitor support.)
> +  $(info Please install libbpf-dev/libbpf-devel)
> +  BUILD_BPF := 0
> +endif
> +
> +ifeq ($(BUILD_BPF),1)
> +  CFLAGS += -DHAVE_LIBBPF
> +  $(call lib_setup,bpf)
> +endif
> +

Can this break "make static" on systems where it worked before this patch?

BPF support is now on by default whenever the libbpf feature test passes,
and lib_setup only adds the output of pkg-config --libs-only-l libbpf, which
is just -lbpf.  libbpf.pc declares:

Requires.private: libelf zlib

and pkg-config only reports those with --static.  The existing static target
in tools/verification/rv/Makefile links with -static and $(EXTLIBS):

static: $(RV_IN)
	$(eval LDFLAGS += -static)
	$(QUIET_LINK)$(CC) $(LDFLAGS) -o $(RV)-static $(RV_IN) $(EXTLIBS)

Would that link libbpf.a without -lelf and -lz and fail with unresolved
libelf and zlib symbols, so that users now need BUILD_BPF=0 to get a static
build?

Could pkg-config be called with --static for the static target, the private
dependencies be added, or BPF support be disabled for static builds?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/36944413541