Re: [PATCH bpf-next] selftests/bpf: remove logic duplication in test_verifier.c

[Krzesimir Nowak <krzesimir@xxxxxxxxxx>]

On Thu, Jul 11, 2019 at 4:43 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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.
>

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



-- 
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