| Message ID | 20190711010844.1285018-1-andriin@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] selftests/bpf: remove logic duplication in test_verifier.c | expand |
On Thu, Jul 11, 2019 at 3:08 AM Andrii Nakryiko <andriin@fb.com> wrote: > > test_verifier tests can specify single- and multi-runs tests. Internally > logic of handling them is duplicated. Get rid of it by making single run > retval specification to be a first retvals spec. > > Cc: Krzesimir Nowak <krzesimir@kinvolk.io> > Signed-off-by: Andrii Nakryiko <andriin@fb.com> Looks good, one nit below. Acked-by: Krzesimir Nowak <krzesimir@kinvolk.io> > --- > tools/testing/selftests/bpf/test_verifier.c | 37 ++++++++++----------- > 1 file changed, 18 insertions(+), 19 deletions(-) > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c > index b0773291012a..120ecdf4a7db 100644 > --- a/tools/testing/selftests/bpf/test_verifier.c > +++ b/tools/testing/selftests/bpf/test_verifier.c > @@ -86,7 +86,7 @@ struct bpf_test { > int fixup_sk_storage_map[MAX_FIXUPS]; > const char *errstr; > const char *errstr_unpriv; > - uint32_t retval, retval_unpriv, insn_processed; > + uint32_t insn_processed; > int prog_len; > enum { > UNDEF, > @@ -95,16 +95,24 @@ struct bpf_test { > } result, result_unpriv; > enum bpf_prog_type prog_type; > uint8_t flags; > - __u8 data[TEST_DATA_LEN]; > void (*fill_helper)(struct bpf_test *self); > uint8_t runs; > - struct { > - uint32_t retval, retval_unpriv; > - union { > - __u8 data[TEST_DATA_LEN]; > - __u64 data64[TEST_DATA_LEN / 8]; > + union { > + struct { Maybe consider moving the struct definition outside to further the removal of the duplication? > + uint32_t retval, retval_unpriv; > + union { > + __u8 data[TEST_DATA_LEN]; > + __u64 data64[TEST_DATA_LEN / 8]; > + }; > }; > - } retvals[MAX_TEST_RUNS]; > + struct { > + uint32_t retval, retval_unpriv; > + union { > + __u8 data[TEST_DATA_LEN]; > + __u64 data64[TEST_DATA_LEN / 8]; > + }; > + } retvals[MAX_TEST_RUNS]; > + }; > enum bpf_attach_type expected_attach_type; > }; > > @@ -949,17 +957,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv, > uint32_t expected_val; > int i; > > - if (!test->runs) { > - expected_val = unpriv && test->retval_unpriv ? > - test->retval_unpriv : test->retval; > - > - err = do_prog_test_run(fd_prog, unpriv, expected_val, > - test->data, sizeof(test->data)); > - if (err) > - run_errs++; > - else > - run_successes++; > - } > + if (!test->runs) > + test->runs = 1; > > for (i = 0; i < test->runs; i++) { > if (unpriv && test->retvals[i].retval_unpriv) > -- > 2.17.1 >
On Thu, Jul 11, 2019 at 5:13 AM Krzesimir Nowak <krzesimir@kinvolk.io> wrote: > > On Thu, Jul 11, 2019 at 3:08 AM Andrii Nakryiko <andriin@fb.com> wrote: > > > > test_verifier tests can specify single- and multi-runs tests. Internally > > logic of handling them is duplicated. Get rid of it by making single run > > retval specification to be a first retvals spec. > > > > Cc: Krzesimir Nowak <krzesimir@kinvolk.io> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > Looks good, one nit below. > > Acked-by: Krzesimir Nowak <krzesimir@kinvolk.io> > > > --- > > tools/testing/selftests/bpf/test_verifier.c | 37 ++++++++++----------- > > 1 file changed, 18 insertions(+), 19 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c > > index b0773291012a..120ecdf4a7db 100644 > > --- a/tools/testing/selftests/bpf/test_verifier.c > > +++ b/tools/testing/selftests/bpf/test_verifier.c > > @@ -86,7 +86,7 @@ struct bpf_test { > > int fixup_sk_storage_map[MAX_FIXUPS]; > > const char *errstr; > > const char *errstr_unpriv; > > - uint32_t retval, retval_unpriv, insn_processed; > > + uint32_t insn_processed; > > int prog_len; > > enum { > > UNDEF, > > @@ -95,16 +95,24 @@ struct bpf_test { > > } result, result_unpriv; > > enum bpf_prog_type prog_type; > > uint8_t flags; > > - __u8 data[TEST_DATA_LEN]; > > void (*fill_helper)(struct bpf_test *self); > > uint8_t runs; > > - struct { > > - uint32_t retval, retval_unpriv; > > - union { > > - __u8 data[TEST_DATA_LEN]; > > - __u64 data64[TEST_DATA_LEN / 8]; > > + union { > > + struct { > > Maybe consider moving the struct definition outside to further the > removal of the duplication? Can't do that because then retval/retval_unpriv/data won't be accessible as a normal field of struct bpf_test. It has to be in anonymous structs/unions, unfortunately. I tried the following, but that also didn't work: union { struct bpf_test_retval { uint32_t retval, retval_unpriv; union { __u8 data[TEST_DATA_LEN]; __u64 data64[TEST_DATA_LEN / 8]; }; }; struct bpf_test_retval retvals[MAX_TEST_RUNS]; }; This also made retval/retval_unpriv to not behave as normal fields of struct bpf_test. > > > + uint32_t retval, retval_unpriv; > > + union { > > + __u8 data[TEST_DATA_LEN]; > > + __u64 data64[TEST_DATA_LEN / 8]; > > + }; > > }; > > - } retvals[MAX_TEST_RUNS]; > > + struct { > > + uint32_t retval, retval_unpriv; > > + union { > > + __u8 data[TEST_DATA_LEN]; > > + __u64 data64[TEST_DATA_LEN / 8]; > > + }; > > + } retvals[MAX_TEST_RUNS]; > > + }; > > enum bpf_attach_type expected_attach_type; > > }; > > > > @@ -949,17 +957,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv, > > uint32_t expected_val; > > int i; > > > > - if (!test->runs) { > > - expected_val = unpriv && test->retval_unpriv ? > > - test->retval_unpriv : test->retval; > > - > > - err = do_prog_test_run(fd_prog, unpriv, expected_val, > > - test->data, sizeof(test->data)); > > - if (err) > > - run_errs++; > > - else > > - run_successes++; > > - } > > + if (!test->runs) > > + test->runs = 1; > > > > for (i = 0; i < test->runs; i++) { > > if (unpriv && test->retvals[i].retval_unpriv) > > -- > > 2.17.1 > > > > > -- > Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364 > Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras > Registergericht/Court of registration: Amtsgericht Charlottenburg > Registernummer/Registration number: HRB 171414 B > Ust-ID-Nummer/VAT ID number: DE302207000
On Thu, Jul 11, 2019 at 4:43 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Jul 11, 2019 at 5:13 AM Krzesimir Nowak <krzesimir@kinvolk.io> wrote: > > > > On Thu, Jul 11, 2019 at 3:08 AM Andrii Nakryiko <andriin@fb.com> wrote: > > > > > > test_verifier tests can specify single- and multi-runs tests. Internally > > > logic of handling them is duplicated. Get rid of it by making single run > > > retval specification to be a first retvals spec. > > > > > > Cc: Krzesimir Nowak <krzesimir@kinvolk.io> > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > > > Looks good, one nit below. > > > > Acked-by: Krzesimir Nowak <krzesimir@kinvolk.io> > > > > > --- > > > tools/testing/selftests/bpf/test_verifier.c | 37 ++++++++++----------- > > > 1 file changed, 18 insertions(+), 19 deletions(-) > > > > > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c > > > index b0773291012a..120ecdf4a7db 100644 > > > --- a/tools/testing/selftests/bpf/test_verifier.c > > > +++ b/tools/testing/selftests/bpf/test_verifier.c > > > @@ -86,7 +86,7 @@ struct bpf_test { > > > int fixup_sk_storage_map[MAX_FIXUPS]; > > > const char *errstr; > > > const char *errstr_unpriv; > > > - uint32_t retval, retval_unpriv, insn_processed; > > > + uint32_t insn_processed; > > > int prog_len; > > > enum { > > > UNDEF, > > > @@ -95,16 +95,24 @@ struct bpf_test { > > > } result, result_unpriv; > > > enum bpf_prog_type prog_type; > > > uint8_t flags; > > > - __u8 data[TEST_DATA_LEN]; > > > void (*fill_helper)(struct bpf_test *self); > > > uint8_t runs; > > > - struct { > > > - uint32_t retval, retval_unpriv; > > > - union { > > > - __u8 data[TEST_DATA_LEN]; > > > - __u64 data64[TEST_DATA_LEN / 8]; > > > + union { > > > + struct { > > > > Maybe consider moving the struct definition outside to further the > > removal of the duplication? > > Can't do that because then retval/retval_unpriv/data won't be > accessible as a normal field of struct bpf_test. It has to be in > anonymous structs/unions, unfortunately. > Ah, right. Meh. I tried something like this: #define BPF_DATA_STRUCT \ struct { \ uint32_t retval, retval_unpriv; \ union { \ __u8 data[TEST_DATA_LEN]; \ __u64 data64[TEST_DATA_LEN / 8]; \ }; \ } and then: union { BPF_DATA_STRUCT; BPF_DATA_STRUCT retvals[MAX_TEST_RUNS]; }; And that seems to compile at least. But question is: is this acceptably ugly or unacceptably ugly? :) > I tried the following, but that also didn't work: > > union { > struct bpf_test_retval { > uint32_t retval, retval_unpriv; > union { > __u8 data[TEST_DATA_LEN]; > __u64 data64[TEST_DATA_LEN / 8]; > }; > }; > struct bpf_test_retval retvals[MAX_TEST_RUNS]; > }; > > This also made retval/retval_unpriv to not behave as normal fields of > struct bpf_test. > > > > > > > + uint32_t retval, retval_unpriv; > > > + union { > > > + __u8 data[TEST_DATA_LEN]; > > > + __u64 data64[TEST_DATA_LEN / 8]; > > > + }; > > > }; > > > - } retvals[MAX_TEST_RUNS]; > > > + struct { > > > + uint32_t retval, retval_unpriv; > > > + union { > > > + __u8 data[TEST_DATA_LEN]; > > > + __u64 data64[TEST_DATA_LEN / 8]; > > > + }; > > > + } retvals[MAX_TEST_RUNS]; > > > + }; > > > enum bpf_attach_type expected_attach_type; > > > }; > > > > > > @@ -949,17 +957,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv, > > > uint32_t expected_val; > > > int i; > > > > > > - if (!test->runs) { > > > - expected_val = unpriv && test->retval_unpriv ? > > > - test->retval_unpriv : test->retval; > > > - > > > - err = do_prog_test_run(fd_prog, unpriv, expected_val, > > > - test->data, sizeof(test->data)); > > > - if (err) > > > - run_errs++; > > > - else > > > - run_successes++; > > > - } > > > + if (!test->runs) > > > + test->runs = 1; > > > > > > for (i = 0; i < test->runs; i++) { > > > if (unpriv && test->retvals[i].retval_unpriv) > > > -- > > > 2.17.1 > > > > > > > > > -- > > Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364 > > Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras > > Registergericht/Court of registration: Amtsgericht Charlottenburg > > Registernummer/Registration number: HRB 171414 B > > Ust-ID-Nummer/VAT ID number: DE302207000
On 07/12/2019 09:53 AM, Krzesimir Nowak wrote: > On Thu, Jul 11, 2019 at 4:43 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: >> >> On Thu, Jul 11, 2019 at 5:13 AM Krzesimir Nowak <krzesimir@kinvolk.io> wrote: >>> >>> On Thu, Jul 11, 2019 at 3:08 AM Andrii Nakryiko <andriin@fb.com> wrote: >>>> >>>> test_verifier tests can specify single- and multi-runs tests. Internally >>>> logic of handling them is duplicated. Get rid of it by making single run >>>> retval specification to be a first retvals spec. >>>> >>>> Cc: Krzesimir Nowak <krzesimir@kinvolk.io> >>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com> >>> >>> Looks good, one nit below. >>> >>> Acked-by: Krzesimir Nowak <krzesimir@kinvolk.io> >>> >>>> --- >>>> tools/testing/selftests/bpf/test_verifier.c | 37 ++++++++++----------- >>>> 1 file changed, 18 insertions(+), 19 deletions(-) >>>> >>>> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c >>>> index b0773291012a..120ecdf4a7db 100644 >>>> --- a/tools/testing/selftests/bpf/test_verifier.c >>>> +++ b/tools/testing/selftests/bpf/test_verifier.c >>>> @@ -86,7 +86,7 @@ struct bpf_test { >>>> int fixup_sk_storage_map[MAX_FIXUPS]; >>>> const char *errstr; >>>> const char *errstr_unpriv; >>>> - uint32_t retval, retval_unpriv, insn_processed; >>>> + uint32_t insn_processed; >>>> int prog_len; >>>> enum { >>>> UNDEF, >>>> @@ -95,16 +95,24 @@ struct bpf_test { >>>> } result, result_unpriv; >>>> enum bpf_prog_type prog_type; >>>> uint8_t flags; >>>> - __u8 data[TEST_DATA_LEN]; >>>> void (*fill_helper)(struct bpf_test *self); >>>> uint8_t runs; >>>> - struct { >>>> - uint32_t retval, retval_unpriv; >>>> - union { >>>> - __u8 data[TEST_DATA_LEN]; >>>> - __u64 data64[TEST_DATA_LEN / 8]; >>>> + union { >>>> + struct { >>> >>> Maybe consider moving the struct definition outside to further the >>> removal of the duplication? >> >> Can't do that because then retval/retval_unpriv/data won't be >> accessible as a normal field of struct bpf_test. It has to be in >> anonymous structs/unions, unfortunately. >> > > Ah, right. > > Meh. > > I tried something like this: > > #define BPF_DATA_STRUCT \ > struct { \ > uint32_t retval, retval_unpriv; \ > union { \ > __u8 data[TEST_DATA_LEN]; \ > __u64 data64[TEST_DATA_LEN / 8]; \ > }; \ > } > > and then: > > union { > BPF_DATA_STRUCT; > BPF_DATA_STRUCT retvals[MAX_TEST_RUNS]; > }; > > And that seems to compile at least. But question is: is this > acceptably ugly or unacceptably ugly? :) Both a bit ugly, but I'd have a slight preference towards the above, perhaps a bit more readable like: #define bpf_testdata_struct_t \ struct { \ uint32_t retval, retval_unpriv; \ union { \ __u8 data[TEST_DATA_LEN]; \ __u64 data64[TEST_DATA_LEN / 8]; \ }; \ } union { bpf_testdata_struct_t; bpf_testdata_struct_t retvals[MAX_TEST_RUNS]; }; Thanks, Daniel
On Fri, Jul 12, 2019 at 6:57 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 07/12/2019 09:53 AM, Krzesimir Nowak wrote: > > On Thu, Jul 11, 2019 at 4:43 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > >> > >> On Thu, Jul 11, 2019 at 5:13 AM Krzesimir Nowak <krzesimir@kinvolk.io> wrote: > >>> > >>> On Thu, Jul 11, 2019 at 3:08 AM Andrii Nakryiko <andriin@fb.com> wrote: > >>>> > >>>> test_verifier tests can specify single- and multi-runs tests. Internally > >>>> logic of handling them is duplicated. Get rid of it by making single run > >>>> retval specification to be a first retvals spec. > >>>> > >>>> Cc: Krzesimir Nowak <krzesimir@kinvolk.io> > >>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com> > >>> > >>> Looks good, one nit below. > >>> > >>> Acked-by: Krzesimir Nowak <krzesimir@kinvolk.io> > >>> > >>>> --- > >>>> tools/testing/selftests/bpf/test_verifier.c | 37 ++++++++++----------- > >>>> 1 file changed, 18 insertions(+), 19 deletions(-) > >>>> > >>>> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c > >>>> index b0773291012a..120ecdf4a7db 100644 > >>>> --- a/tools/testing/selftests/bpf/test_verifier.c > >>>> +++ b/tools/testing/selftests/bpf/test_verifier.c > >>>> @@ -86,7 +86,7 @@ struct bpf_test { > >>>> int fixup_sk_storage_map[MAX_FIXUPS]; > >>>> const char *errstr; > >>>> const char *errstr_unpriv; > >>>> - uint32_t retval, retval_unpriv, insn_processed; > >>>> + uint32_t insn_processed; > >>>> int prog_len; > >>>> enum { > >>>> UNDEF, > >>>> @@ -95,16 +95,24 @@ struct bpf_test { > >>>> } result, result_unpriv; > >>>> enum bpf_prog_type prog_type; > >>>> uint8_t flags; > >>>> - __u8 data[TEST_DATA_LEN]; > >>>> void (*fill_helper)(struct bpf_test *self); > >>>> uint8_t runs; > >>>> - struct { > >>>> - uint32_t retval, retval_unpriv; > >>>> - union { > >>>> - __u8 data[TEST_DATA_LEN]; > >>>> - __u64 data64[TEST_DATA_LEN / 8]; > >>>> + union { > >>>> + struct { > >>> > >>> Maybe consider moving the struct definition outside to further the > >>> removal of the duplication? > >> > >> Can't do that because then retval/retval_unpriv/data won't be > >> accessible as a normal field of struct bpf_test. It has to be in > >> anonymous structs/unions, unfortunately. > >> > > > > Ah, right. > > > > Meh. > > > > I tried something like this: > > > > #define BPF_DATA_STRUCT \ > > struct { \ > > uint32_t retval, retval_unpriv; \ > > union { \ > > __u8 data[TEST_DATA_LEN]; \ > > __u64 data64[TEST_DATA_LEN / 8]; \ > > }; \ > > } > > > > and then: > > > > union { > > BPF_DATA_STRUCT; > > BPF_DATA_STRUCT retvals[MAX_TEST_RUNS]; > > }; > > > > And that seems to compile at least. But question is: is this > > acceptably ugly or unacceptably ugly? :) > > Both a bit ugly, but I'd have a slight preference towards the above, > perhaps a bit more readable like: Heh, I had slight preference the other way :) I'll update diff with macro, though. > > #define bpf_testdata_struct_t \ > struct { \ > uint32_t retval, retval_unpriv; \ > union { \ > __u8 data[TEST_DATA_LEN]; \ > __u64 data64[TEST_DATA_LEN / 8]; \ > }; \ > } > union { > bpf_testdata_struct_t; > bpf_testdata_struct_t retvals[MAX_TEST_RUNS]; > }; > > Thanks, > Daniel
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c index b0773291012a..120ecdf4a7db 100644 --- a/tools/testing/selftests/bpf/test_verifier.c +++ b/tools/testing/selftests/bpf/test_verifier.c @@ -86,7 +86,7 @@ struct bpf_test { int fixup_sk_storage_map[MAX_FIXUPS]; const char *errstr; const char *errstr_unpriv; - uint32_t retval, retval_unpriv, insn_processed; + uint32_t insn_processed; int prog_len; enum { UNDEF, @@ -95,16 +95,24 @@ struct bpf_test { } result, result_unpriv; enum bpf_prog_type prog_type; uint8_t flags; - __u8 data[TEST_DATA_LEN]; void (*fill_helper)(struct bpf_test *self); uint8_t runs; - struct { - uint32_t retval, retval_unpriv; - union { - __u8 data[TEST_DATA_LEN]; - __u64 data64[TEST_DATA_LEN / 8]; + union { + struct { + uint32_t retval, retval_unpriv; + union { + __u8 data[TEST_DATA_LEN]; + __u64 data64[TEST_DATA_LEN / 8]; + }; }; - } retvals[MAX_TEST_RUNS]; + struct { + uint32_t retval, retval_unpriv; + union { + __u8 data[TEST_DATA_LEN]; + __u64 data64[TEST_DATA_LEN / 8]; + }; + } retvals[MAX_TEST_RUNS]; + }; enum bpf_attach_type expected_attach_type; }; @@ -949,17 +957,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv, uint32_t expected_val; int i; - if (!test->runs) { - expected_val = unpriv && test->retval_unpriv ? - test->retval_unpriv : test->retval; - - err = do_prog_test_run(fd_prog, unpriv, expected_val, - test->data, sizeof(test->data)); - if (err) - run_errs++; - else - run_successes++; - } + if (!test->runs) + test->runs = 1; for (i = 0; i < test->runs; i++) { if (unpriv && test->retvals[i].retval_unpriv)
test_verifier tests can specify single- and multi-runs tests. Internally logic of handling them is duplicated. Get rid of it by making single run retval specification to be a first retvals spec. Cc: Krzesimir Nowak <krzesimir@kinvolk.io> Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- tools/testing/selftests/bpf/test_verifier.c | 37 ++++++++++----------- 1 file changed, 18 insertions(+), 19 deletions(-)