* [PATCH bpf-next] selftests/bpf: remove logic duplication in test_verifier.c
@ 2019-07-11 1:08 Andrii Nakryiko
2019-07-11 12:13 ` Krzesimir Nowak
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2019-07-11 1:08 UTC (permalink / raw)
To: andrii.nakryiko, kernel-team, ast, daniel, bpf, netdev
Cc: Andrii Nakryiko, Krzesimir Nowak
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(-)
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)
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: remove logic duplication in test_verifier.c
2019-07-11 1:08 [PATCH bpf-next] selftests/bpf: remove logic duplication in test_verifier.c Andrii Nakryiko
@ 2019-07-11 12:13 ` Krzesimir Nowak
2019-07-11 14:43 ` Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: Krzesimir Nowak @ 2019-07-11 12:13 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, kernel-team, ast, Daniel Borkmann, bpf, Networking
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
>
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: remove logic duplication in test_verifier.c
2019-07-11 12:13 ` Krzesimir Nowak
@ 2019-07-11 14:43 ` Andrii Nakryiko
2019-07-12 7:53 ` Krzesimir Nowak
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2019-07-11 14:43 UTC (permalink / raw)
To: Krzesimir Nowak
Cc: Andrii Nakryiko, Kernel Team, Alexei Starovoitov,
Daniel Borkmann, bpf, Networking
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: remove logic duplication in test_verifier.c
2019-07-11 14:43 ` Andrii Nakryiko
@ 2019-07-12 7:53 ` Krzesimir Nowak
2019-07-12 13:57 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Krzesimir Nowak @ 2019-07-12 7:53 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, Kernel Team, Alexei Starovoitov,
Daniel Borkmann, bpf, Networking
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
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: remove logic duplication in test_verifier.c
2019-07-12 7:53 ` Krzesimir Nowak
@ 2019-07-12 13:57 ` Daniel Borkmann
2019-07-12 15:45 ` Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2019-07-12 13:57 UTC (permalink / raw)
To: Krzesimir Nowak, Andrii Nakryiko
Cc: Andrii Nakryiko, Kernel Team, Alexei Starovoitov, bpf, Networking
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: remove logic duplication in test_verifier.c
2019-07-12 13:57 ` Daniel Borkmann
@ 2019-07-12 15:45 ` Andrii Nakryiko
0 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2019-07-12 15:45 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Krzesimir Nowak, Andrii Nakryiko, Kernel Team,
Alexei Starovoitov, bpf, Networking
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-12 15:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-11 1:08 [PATCH bpf-next] selftests/bpf: remove logic duplication in test_verifier.c Andrii Nakryiko
2019-07-11 12:13 ` Krzesimir Nowak
2019-07-11 14:43 ` Andrii Nakryiko
2019-07-12 7:53 ` Krzesimir Nowak
2019-07-12 13:57 ` Daniel Borkmann
2019-07-12 15:45 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).