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.
