Message-ID: <CAHP3+4CWSbZdNjSdoQhsB9Cvc9vp7V3qzJ=EVMO5+By9ZeAr3A@mail.gmail.com>
Date: Fri, 31 Oct 2025 14:37:05 +0800
From: Jianyun Gao <jianyungao89@...il.com>
To: bot+bpf-ci@...nel.org
Cc: bpf@...r.kernel.org, andrii@...nel.org, eddyz87@...il.com, ast@...nel.org, 
	daniel@...earbox.net, martin.lau@...ux.dev, song@...nel.org, 
	yonghong.song@...ux.dev, john.fastabend@...il.com, kpsingh@...nel.org, 
	sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org, 
	linux-kernel@...r.kernel.org, martin.lau@...nel.org, clm@...a.com, 
	ihor.solodrai@...ux.dev
Subject: Re: [PATCH 1/5] libbpf: Add doxygen documentation for bpf_map_* APIs
 in bpf.h

I will fix it in the next version.

On Fri, Oct 31, 2025 at 12:01 PM <bot+bpf-ci@...nel.org> wrote:
>
>
> ```
> commit 4da02b18e2168885350f3935b5ce2f1f52c850ba
> Author: Jianyun Gao <jianyungao89@...il.com>
>
> libbpf: Add doxygen documentation for bpf_map_* APIs in bpf.h
>
> This commit adds Doxygen documentation for 13 bpf_map_* API functions in
> the libbpf user-space library header.
>
> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > index e983a3e40..bcb303d95 100644
> > --- a/tools/lib/bpf/bpf.h
> > +++ b/tools/lib/bpf/bpf.h
>
> [ ... ]
>
> > @@ -151,19 +202,452 @@ struct bpf_btf_load_opts {
> >  LIBBPF_API int bpf_btf_load(const void *btf_data, size_t btf_size,
> >                           struct bpf_btf_load_opts *opts);
> >
> > +/**
> > + * @brief Update or insert an element in a BPF map.
> > + *
> > + * Attempts to store the value referenced by @p value into the BPF map
> > + * identified by @p fd under the key referenced by @p key. The semantics
> > + * of the operation are controlled by @p flags:
>
> [ ... ]
>
> > + * @return 0 on success; -1 on failure with errno set appropriately.
> > + *
> > + * Possible errno values include (not exhaustive):
> > + *   - E2BIG:      Key or value size exceeds map definition.
> > + *   - EINVAL:     Invalid map fd, flags, or unsupported operation for map type.
> > + *   - EBADF:      @p fd is not a valid BPF map descriptor.
> > + *   - ENOENT:     Key does not exist (with BPF_EXIST).
> > + *   - EEXIST:     Key already exists (with BPF_NOEXIST).
> > + *   - ENOMEM:     Kernel memory allocation failure.
> > + *   - EPERM/EACCES: Insufficient privileges or rejected by security policy.
> > + *   - ENOSPC:     Map at capacity (for maps with a max entries limit).
> > + *
> > + */
> >  LIBBPF_API int bpf_map_update_elem(int fd, const void *key, const void *value,
> >                                  __u64 flags);
>
> Is the documented return value correct? The implementation uses
> libbpf_err_errno() which returns negative errno values directly on
> failure (e.g., -EINVAL, -ENOENT), not -1 with errno set. Looking at
> bpf_map_update_elem() in bpf.c:
>
>   int bpf_map_update_elem(int fd, const void *key, const void *value,
>                           __u64 flags)
>   {
>       ...
>       ret = sys_bpf(BPF_MAP_UPDATE_ELEM, &attr, attr_sz);
>       return libbpf_err_errno(ret);
>   }
>
> And libbpf_err_errno() in libbpf_internal.h:
>
>   static inline int libbpf_err_errno(int ret)
>   {
>       return ret < 0 ? -errno : ret;
>   }
>
> So when syscall() returns -1, libbpf_err_errno() converts this to
> -errno. Should the documentation state "negative error code on failure"
> instead of "-1 on failure with errno set"?
>
> For comparison, the existing batch operation docs in the same file
> correctly document: "@return 0, on success; negative error code,
> otherwise (errno is also set to..."
>
> > +/**
> > + * @brief Look up an element in a BPF map by key.
>
> [ ... ]
>
> > + * @return 0 on success (value populated),
> > + *         -1 on failure with errno set appropriately:
> > + *         - ENOENT: The key does not exist in the map.
>
> The same return value documentation issue appears in
> bpf_map_lookup_elem(). The implementation uses libbpf_err_errno() so it
> returns negative errno, not -1.
>
> > +/**
> > + * @brief Look up (read) a value stored in a BPF map.
>
> [ ... ]
>
> > + * @return 0 on success; -1 on failure with errno set appropriately:
> > + *         - ENOENT: No element with the specified key exists.
>
> Same issue in bpf_map_lookup_elem_flags() - should document negative
> errno return, not -1.
>
> > +/**
> > + * @brief Atomically look up and delete a single element from a BPF map.
>
> [ ... ]
>
> > + * @return 0 on success (value copied and element deleted);
> > + *         -1 on failure with errno set:
> > + *         - ENOENT: Key not found in the map.
>
> Same issue in bpf_map_lookup_and_delete_elem() - returns negative errno.
>
> > +/**
> > + * @brief Atomically look up and delete an element from a BPF map with extra flags.
>
> [ ... ]
>
> > + * @return 0 on success; -1 on failure with errno set:
> > + *         - ENOENT: Key not found.
>
> Same issue in bpf_map_lookup_and_delete_elem_flags().
>
> > +/**
> > + * @brief Delete (remove) a single element from a BPF map.
>
> [ ... ]
>
> > + * @return 0 on success; -1 on failure with errno set appropriately.
> > + *
> > + */
> >  LIBBPF_API int bpf_map_delete_elem(int fd, const void *key);
>
> Same issue in bpf_map_delete_elem().
>
> > +/**
> > + * @brief Delete an element from a BPF map with optional flags.
>
> [ ... ]
>
> > + * @return 0 on success; -1 on failure with errno set:
> > + *         - ENOENT: Key not found.
>
> Same issue in bpf_map_delete_elem_flags().
>
> > +/**
> > + * @brief Iterate over keys in a BPF map by retrieving the key that follows a given key.
>
> [ ... ]
>
> > + * @return 0 on success (next key stored in @p next_key);
> > + *         -1 on failure with errno set:
> > + *           - ENOENT: No further keys (end of iteration) or map is empty (when @p key is NULL).
>
> Same issue in bpf_map_get_next_key().
>
> All eight of these functions consistently return negative errno values
> via libbpf_err_errno(), but the documentation states they return -1 with
> errno set (the traditional C API pattern). This creates inconsistency
> with the existing batch operation documentation and could mislead users
> about proper error handling.
>
>
> ```
>
> ---
> 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
>
> In-Reply-To-Subject: `libbpf: Add doxygen documentation for bpf_map_* APIs in bpf.h`
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/18961981609

Powered by blists - more mailing lists