On 2026/10/25 00:13, [email protected] wrote: >> 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()? > I'd like to move cmd as the first argument, and keep name __sys_bpf_fd() for the helper.
Thanks, Leon
