On Thu, Jul 11, 2019 at 5:13 AM Krzesimir Nowak <krzesimir@xxxxxxxxxx> wrote:
>
> On Thu, Jul 11, 2019 at 3:08 AM Andrii Nakryiko <andriin@xxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Andrii Nakryiko <andriin@xxxxxx>
>
> Looks good, one nit below.
>
> Acked-by: Krzesimir Nowak <krzesimir@xxxxxxxxxx>
>
> > ---
> > 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
