netdev - Re: [PATCH net-next v3 1/3] net: filter: simplify label names from jump-table

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <536286E5.1080309@redhat.com>
Date:	Thu, 01 May 2014 19:39:49 +0200
From:	Daniel Borkmann <dborkman@...hat.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	davem@...emloft.net, ast@...mgrid.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v3 1/3] net: filter: simplify label names from
 jump-table

On 05/01/2014 07:20 PM, Eric Dumazet wrote:
> On Thu, 2014-05-01 at 18:16 +0200, Daniel Borkmann wrote:
>> This patch simplifies label naming for the BPF jump-table.
>> When we define labels via DL(), we just concatenate/textify
>> the combination of instruction opcode which consists of the
>> class, subclass, word size, target register and so on. Each
>> time we leave BPF_ prefix intact, so that e.g. the preprocessor
>> generates a label BPF_ALU_BPF_ADD_BPF_X for DL(BPF_ALU, BPF_ADD,
>> BPF_X) whereas a label name of ALU_ADD_X is much more easy
>> to grasp. Pure cleanup only.
>>
>> Signed-off-by: Daniel Borkmann <dborkman@...hat.com>
>> Acked-by: Alexei Starovoitov <ast@...mgrid.com>
>> ---
>
> This exactly makes harder to use grep and future code reviews.
>
> # git grep -n BPF_XADD
> include/linux/filter.h:21:#define BPF_XADD      0xc0    /* exclusive add */
> net/core/filter.c:224:          DL(BPF_STX, BPF_XADD, BPF_W),
> net/core/filter.c:225:          DL(BPF_STX, BPF_XADD, BPF_DW),
> net/core/filter.c:481:  BPF_STX_BPF_XADD_BPF_W: /* lock xadd *(u32 *)(A + insn->off) += X */
> net/core/filter.c:485:  BPF_STX_BPF_XADD_BPF_DW: /* lock xadd *(u64 *)(A + insn->off) += X */
>
> Compare now with :
>
> # git grep -n XADD
>
> So I am quite against this patch.

Well, if you review the code itself in filter.c it makes it hard
to read and review, with this patch, you'll immediately get the
label name and what it actually does, so I think it's quite convenient
and way more readable by itself. For grepping, you can always add
subdirectories you're searching for, besides that I don't think
that everything in the kernel needs to have unique names only so
that one can grep for it among the whole tree.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html