On Wed, 23 Jan 2019 15:32:05 -0800, Alexei Starovoitov wrote:
> On Mon, Jan 21, 2019 at 10:43:38AM -0800, Jakub Kicinski wrote:
> > Break up the first 10 kLoC of test verifier test cases
> > out into smaller files. Looks like git line counting
> > gets a little flismy above 16 bit integers, so we need
> > two commits to break up test_verifier.
> >
> > Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
> > Acked-by: Jiong Wang <jiong.w...@netronome.com>
> > ---
> > tools/testing/selftests/bpf/test_verifier.c | 10100 ----------------
> > tools/testing/selftests/bpf/verifier/and.c | 53 +
> > .../selftests/bpf/verifier/array_access.c | 238 +
> ...
> > - {
> > - "DIV64 by 0, zero check",
> > - .insns = {
> > - BPF_MOV32_IMM(BPF_REG_0, 42),
> > - BPF_MOV32_IMM(BPF_REG_1, 0),
> > - BPF_MOV32_IMM(BPF_REG_2, 1),
> > - BPF_ALU64_REG(BPF_DIV, BPF_REG_2, BPF_REG_1),
> > - BPF_EXIT_INSN(),
> > - },
> > - .result = ACCEPT,
> > - .retval = 42,
> > - },
> ...
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/verifier/and.c
> > @@ -0,0 +1,53 @@
> > +{
> > + "invalid and of negative number",
> > + .insns = {
> > + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> > + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> > + BPF_LD_MAP_FD(BPF_REG_1, 0),
> > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> > + BPF_FUNC_map_lookup_elem),
> > + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
> > + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
> > + BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
> > + BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
> > + BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
> > + BPF_ST_MEM(BPF_DW, BPF_REG_0, 0,
> > + offsetof(struct test_val, foo)),
> > + BPF_EXIT_INSN(),
> > + },
> > + .fixup_map_hash_48b = { 3 },
> > + .errstr = "R0 max value is outside of the array range",
> > + .result = REJECT,
> > + .flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
> > +},
>
> I like the removal of one tab, but since we're refactoring the whole thing
> can we remove another tab from instructions?
> In many cases we wrap the lines which makes tests a bit harder to read/write
> since jmp offsets don't easily add from current line number.
> In addition I would propose to represet LD_MAP_FD as two lines too,
> but that's optional. I'm thinking to try that for new tests.
> So how about the following indent:
> {
> "invalid and of negative number",
> .insns = {
> BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> BPF_LD_MAP_FD(BPF_REG_1,
> 0),
> BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
> BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
> BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
> BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
> BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
> BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, offsetof(struct test_val, foo)),
> BPF_EXIT_INSN(),
> },
> .fixup_map_hash_48b = { 3 },
> .errstr = "R0 max value is outside of the array range",
> .result = REJECT,
> .flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
> },
>
> Notice how calls and offsetof() fits into 80 char.
> Another alternative is to switch to two spaces instead of tab:
> {
> "invalid and of negative number",
> .insns = {
> BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> BPF_LD_MAP_FD(BPF_REG_1,
> 0),
> BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
> BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0),
> BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4),
> BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2),
> BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1),
> BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, offsetof(struct test_val, foo)),
> BPF_EXIT_INSN(),
> },
> .fixup_map_hash_48b = { 3 },
> .errstr = "R0 max value is outside of the array range",
> .result = REJECT,
> .flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
> },
>
> Thoughts?
I'd vote for option (1). The two spaces don't seem to provide good
enough visual separation, IMHO we'd either have to go to 4, or put the
opening bracket on it's own line GNU style.
