Re: [PATCH bpf-next 6/6] selftests: bpf: test sockmap update from BPF

[Yonghong Song <yhs@fb.com>]

On 8/20/20 4:58 AM, Lorenz Bauer wrote:
> On Wed, 19 Aug 2020 at 21:46, Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 8/19/20 2:24 AM, Lorenz Bauer wrote:
>>> Add a test which copies a socket from a sockmap into another sockmap
>>> or sockhash. This excercises bpf_map_update_elem support from BPF
>>> context. Compare the socket cookies from source and destination to
>>> ensure that the copy succeeded.
>>>
>>> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
>>> ---
>>>    .../selftests/bpf/prog_tests/sockmap_basic.c  | 76 +++++++++++++++++++
>>>    .../selftests/bpf/progs/test_sockmap_copy.c   | 48 ++++++++++++
>>>    2 files changed, 124 insertions(+)
>>>    create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_copy.c
>>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>>> index 96e7b7f84c65..d30cabc00e9e 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>>> @@ -4,6 +4,7 @@
>>>
>>>    #include "test_progs.h"
>>>    #include "test_skmsg_load_helpers.skel.h"
>>> +#include "test_sockmap_copy.skel.h"
>>>
>>>    #define TCP_REPAIR          19      /* TCP sock is under repair right now */
>>>
>>> @@ -101,6 +102,77 @@ static void test_skmsg_helpers(enum bpf_map_type map_type)
>>>        test_skmsg_load_helpers__destroy(skel);
>>>    }
>>>
>>> +static void test_sockmap_copy(enum bpf_map_type map_type)
>>> +{
>>> +     struct bpf_prog_test_run_attr attr;
>>> +     struct test_sockmap_copy *skel;
>>> +     __u64 src_cookie, dst_cookie;
>>> +     int err, prog, s, src, dst;
>>> +     const __u32 zero = 0;
>>> +     char dummy[14] = {0};
>>> +
>>> +     s = connected_socket_v4();
>>
>> Maybe change variable name to "sk" for better clarity?
> 
> Yup!
> 
>>
>>> +     if (CHECK_FAIL(s == -1))
>>> +             return;
>>> +
>>> +     skel = test_sockmap_copy__open_and_load();
>>> +     if (CHECK_FAIL(!skel)) {
>>> +             close(s);
>>> +             perror("test_sockmap_copy__open_and_load");
>>> +             return;
>>> +     }
>>
>> Could you use CHECK instead of CHECK_FAIL?
>> With CHECK, you can print additional information without perror.
> 
> I avoid CHECK because it requires `duration`, which doesn't make sense
> for most things that I call CHECK_FAIL on here. So either it outputs 0
> nsec (which is bogus) or it outputs the value from the last
> bpf_prog_test_run call (which is also bogus). How do other tests
> handle this? Just ignore it?

Just ignore it. You can define a static variable duration in the 
beginning of file and then use CHECK in the rest of file.

> 
>>
>>
>>> +
>>> +     prog = bpf_program__fd(skel->progs.copy_sock_map);
>>> +     src = bpf_map__fd(skel->maps.src);
>>> +     if (map_type == BPF_MAP_TYPE_SOCKMAP)
>>> +             dst = bpf_map__fd(skel->maps.dst_sock_map);
>>> +     else
>>> +             dst = bpf_map__fd(skel->maps.dst_sock_hash);
>>> +
[...]