On Thu, Feb 16, 2023 at 9:25 AM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
>
> On 02/16, Alexei Starovoitov wrote:
> > On Wed, Feb 15, 2023 at 3:59 PM Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> > wrote:
> > >
> > > Make the code more readable by introducing a symbolic constant
> > > instead of using 0.
> > >
> > > Suggested-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> > > Signed-off-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> > > ---
> > > include/uapi/linux/bpf.h | 4 ++++
> > > kernel/bpf/disasm.c | 2 +-
> > > kernel/bpf/verifier.c | 12 +++++++-----
> > > tools/include/linux/filter.h | 2 +-
> > > tools/include/uapi/linux/bpf.h | 4 ++++
> > > 5 files changed, 17 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index 1503f61336b6..37f7588d5b2f 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -1211,6 +1211,10 @@ enum bpf_link_type {
> > > */
> > > #define BPF_PSEUDO_FUNC 4
> > >
> > > +/* when bpf_call->src_reg == BPF_HELPER_CALL, bpf_call->imm == index
> > of a bpf
> > > + * helper function (see ___BPF_FUNC_MAPPER below for a full list)
> > > + */
> > > +#define BPF_HELPER_CALL 0
>
> > I don't like this "cleanup".
> > The code reads fine as-is.
>
> Even in the context of patch 4? There would be the following switch
> without BPF_HELPER_CALL:
>
> switch (insn->src_reg) {
> case 0:
> ...
> break;
>
> case BPF_PSEUDO_CALL:
> ...
> break;
>
> case BPF_PSEUDO_KFUNC_CALL:
> ...
> break;
> }
>
> That 'case 0' feels like it deserves a name. But up to you, I'm fine
> either way.
It's philosophical.
Some people insist on if (ptr == NULL). I insist on if (!ptr).
That's why canonical bpf progs are written as:
val = bpf_map_lookup();
if (!val) ...
zero is zero. It doesn't need #define.
