Re: [PATCH bpf-next v4 4/6] libbpf: Factor out __sys_bpf_fd helper

[bot+bpf-ci@kernel.org]

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

> libbpf: Factor out __sys_bpf_fd helper

TL;DR: Nit only, not a bug: the new libbpf helper __sys_bpf_fd() has a
misleading name and a different argument order (cmd not first) than
sys_bpf_fd(); a name like sys_bpf_fd_log() is suggested.

> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index b49822d212ae..9a2107d4f6d8 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -203,6 +203,30 @@ int bump_rlimit_memlock(void)
>  	return 0;
>  }
>
> +static int __sys_bpf_fd(union bpf_attr *attr, const size_t attr_sz, enum bpf_cmd cmd,
> +			struct bpf_log_opts *log_opts)
> +{
> +	const size_t attr_common_sz = sizeof(struct bpf_common_attr);
> +	struct bpf_common_attr attr_common;
> +	int fd;
> +
> +	if (!OPTS_VALID(log_opts, bpf_log_opts))
> +		return libbpf_err(-EINVAL);
> +
> +	if (log_opts && feat_supported(NULL, FEAT_BPF_SYSCALL_COMMON_ATTRS)) {
> +		memset(&attr_common, 0, attr_common_sz);
> +		attr_common.log_buf = ptr_to_u64(OPTS_GET(log_opts, buf, NULL));
> +		attr_common.log_size = OPTS_GET(log_opts, size, 0);
> +		attr_common.log_level = OPTS_GET(log_opts, level, 0);
> +		fd = sys_bpf_ext_fd(cmd, attr, attr_sz, &attr_common, attr_common_sz);
> +		OPTS_SET(log_opts, true_size, attr_common.log_true_size);
> +	} else {
> +		fd = sys_bpf_fd(cmd, attr, attr_sz);
> +		OPTS_SET(log_opts, true_size, 0);
> +	}
> +	return fd;
> +}
> +

[ ... ]

> @@ -248,20 +270,7 @@ int bpf_map_create(enum bpf_map_type map_type,
>  	attr.excl_prog_hash_size = OPTS_GET(opts, excl_prog_hash_size, 0);
>
>  	log_opts = OPTS_GET(opts, log_opts, NULL);
> -	if (!OPTS_VALID(log_opts, bpf_log_opts))
> -		return libbpf_err(-EINVAL);
> -
> -	if (log_opts && feat_supported(NULL, FEAT_BPF_SYSCALL_COMMON_ATTRS)) {
> -		memset(&attr_common, 0, attr_common_sz);
> -		attr_common.log_buf = ptr_to_u64(OPTS_GET(log_opts, buf, NULL));
> -		attr_common.log_size = OPTS_GET(log_opts, size, 0);
> -		attr_common.log_level = OPTS_GET(log_opts, level, 0);
> -		fd = sys_bpf_ext_fd(BPF_MAP_CREATE, &attr, attr_sz, &attr_common, attr_common_sz);
> -		OPTS_SET(log_opts, true_size, attr_common.log_true_size);
> -	} else {
> -		fd = sys_bpf_fd(BPF_MAP_CREATE, &attr, attr_sz);
> -		OPTS_SET(log_opts, true_size, 0);
> -	}
> +	fd = __sys_bpf_fd(&attr, attr_sz, BPF_MAP_CREATE, log_opts);
>  	return libbpf_err_errno(fd);
>  }

This isn't a bug, but __sys_bpf_fd() is a higher-level wrapper that
dispatches to sys_bpf_fd() or sys_bpf_ext_fd() after validating log_opts and
probing FEAT_BPF_SYSCALL_COMMON_ATTRS, while a leading double underscore
usually marks the lower-level variant of a function.

The helper also takes (attr, attr_sz, cmd, log_opts), whereas sys_bpf(),
sys_bpf_fd(), sys_bpf_ext() and sys_bpf_ext_fd() all take cmd first, so the
new call reads differently from the nearby calls:

    fd = __sys_bpf_fd(&attr, attr_sz, BPF_MAP_CREATE, log_opts);

Would a name that describes what the helper adds (for example something like
sys_bpf_fd_log()), with cmd as the first argument like the other sys_bpf*()
wrappers, read more naturally next to sys_bpf_fd() and sys_bpf_ext_fd()?


---
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/40108748084