On Thu, Feb 8, 2024 at 9:43 AM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
>
>
> 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.
Yes, exactly. if bpf_iter_xxx_new() failed, subsequent
bpf_iter_xxx_next() should return NULL and bpf_iter_xxx_destroy() will
be a no-op as well. So yes, there is no need for extra error checking
here.
>
> >
> > Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> > ---
> > 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 */
> >
