Re: [PATCH bpf-next 3/3] libbpf: Check the return value of bpf_iter_<type>_new()

[Yonghong Song <yonghong.song@linux.dev>]

On 2/8/24 1:09 AM, Yafang Shao wrote:
> On success, bpf_iter_<type>_new() return 0. On failure, it returns ERR.
> We'd better check the return value of it.

Not sure whether this patch is necessary or not.

I checked:
   bpf_iter_num_{new,next}
   bpf_iter_task_vma_{new,next}
   bpf_iter_css_task_{new,next}

It looks like the convention is for *_next() return NULL or not
instead of relying on return value of _new() to decide whether to
proceed or not. Maybe Andrii can clarify.

>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>   tools/lib/bpf/bpf_helpers.h | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index 79eaa581be98..2cd2428e3bc6 100644
> --- a/tools/lib/bpf/bpf_helpers.h
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -133,6 +133,15 @@
>   # define __bpf_unreachable()	__builtin_trap()
>   #endif
>   
> +#ifndef __must_check
> +#define __must_check __attribute__((warn_unused_result))
> +#endif
> +
> +static inline void * __must_check ERR_PTR(long error)
> +{
> +	return (void *) error;
> +}
> +
>   /*
>    * Helper function to perform a tail call with a constant/immediate map slot.
>    */
> @@ -340,14 +349,13 @@ extern void bpf_iter_num_destroy(struct bpf_iter_num *it) __weak __ksym;
>   	/* initialize and define destructor */							\
>   	struct bpf_iter_##type ___it __attribute__((aligned(8), /* enforce, just in case */,	\
>   						    cleanup(bpf_iter_##type##_destroy))),	\
> -	/* ___p pointer is just to call bpf_iter_##type##_new() *once* to init ___it */		\
>   			       *___p __attribute__((unused)) = (				\
> -					bpf_iter_##type##_new(&___it, ##args),			\
>   	/* this is a workaround for Clang bug: it currently doesn't emit BTF */			\
>   	/* for bpf_iter_##type##_destroy() when used from cleanup() attribute */		\
> -					(void)bpf_iter_##type##_destroy, (void *)0);		\
> +					(void)bpf_iter_##type##_destroy,			\
> +					ERR_PTR(bpf_iter_##type##_new(&___it, ##args)));	\
>   	/* iteration and termination check */							\
> -	(((cur) = bpf_iter_##type##_next(&___it)));						\
> +	((!___p) && ((cur) = bpf_iter_##type##_next(&___it)));					\
>   )
>   #endif /* bpf_for_each */
>