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

[Leon Hwang]

On 2026/10/25 00:13, bot+bpf-ci@xxxxxxxxxx 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