Re: [PATCH bpf] selftests/bpf: Rewrite test_tc_redirect.sh as prog_tests/tc_redirect.c

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, Apr 29, 2021 at 8:32 AM Jussi Maki <joamaki@gmail.com> wrote:
>
> Ports test_tc_redirect.sh to the test_progs framework and removes the
> old test. This makes it more in line with rest of the tests and makes
> it possible to run this test with vmtest.sh and under the bpf CI.
>
> Signed-off-by: Jussi Maki <joamaki@gmail.com>
> ---

Aren't there any Makefile changes that need to be done as well, given
you are removing "old style" test script?

>  tools/testing/selftests/bpf/network_helpers.c |   2 +-
>  tools/testing/selftests/bpf/network_helpers.h |   1 +
>  .../selftests/bpf/prog_tests/tc_redirect.c    | 481 ++++++++++++++++++
>  .../selftests/bpf/progs/test_tc_neigh.c       |  33 +-
>  .../selftests/bpf/progs/test_tc_neigh_fib.c   |   9 +-
>  .../selftests/bpf/progs/test_tc_peer.c        |  33 +-
>  .../testing/selftests/bpf/test_tc_redirect.sh | 216 --------
>  7 files changed, 509 insertions(+), 266 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/tc_redirect.c
>  delete mode 100755 tools/testing/selftests/bpf/test_tc_redirect.sh
>

[...]

> +
> +#define TIMEOUT_MILLIS 10000
> +
> +static const char * const namespaces[] = {NS_SRC, NS_FWD, NS_DST, NULL};
> +static int root_netns_fd = -1;
> +static __u32 duration;
> +
> +static void restore_root_netns(void)
> +{
> +       CHECK_FAIL(setns(root_netns_fd, CLONE_NEWNET));

can you please also switch to ASSERT_xxx() macros while converting
this selftest? Thanks!

> +}
> +
> +int setns_by_name(const char *name)

static?

> +{
> +       int nsfd;
> +       char nspath[PATH_MAX];
> +       int err;
> +
> +       snprintf(nspath, sizeof(nspath), "%s/%s", "/var/run/netns", name);
> +       nsfd = open(nspath, O_RDONLY | O_CLOEXEC);
> +       if (CHECK(nsfd < 0, nspath, "failed to open\n"))
> +               return -EINVAL;
> +
> +       err = setns(nsfd, CLONE_NEWNET);
> +       close(nsfd);
> +
> +       if (CHECK(err, name, "failed to setns\n"))
> +               return -1;
> +
> +       return 0;
> +}
> +

[...]

> +
> +#define SYS(fmt, ...)                                          \
> +       ({                                                      \
> +               char cmd[1024];                                 \
> +               snprintf(cmd, sizeof(cmd), fmt, ##__VA_ARGS__); \
> +               if (CHECK(system(cmd), cmd, "failed\n"))        \
> +                       goto fail;                              \
> +       })
> +
> +static int netns_setup_links_and_routes(struct netns_setup_result *result)
> +{
> +       char veth_src_fwd_addr[IFADDR_STR_LEN+1] = {0,};
> +       char veth_dst_fwd_addr[IFADDR_STR_LEN+1] = {0,};
> +
> +       SYS("ip link add veth_src type veth peer name veth_src_fwd");
> +       SYS("ip link add veth_dst type veth peer name veth_dst_fwd");
> +       if (CHECK_FAIL(get_ifaddr("veth_src_fwd", veth_src_fwd_addr)))

please no CHECK_FAIL, they are invisible in test_progs logs, so it's
harder to debug any failures (and use ASSERT_xxx() instead of CHECK).

> +               goto fail;
> +       if (CHECK_FAIL(get_ifaddr("veth_dst_fwd", veth_dst_fwd_addr)))
> +               goto fail;
> +
> +       result->ifindex_veth_src_fwd = get_ifindex("veth_src_fwd");
> +       if (CHECK_FAIL(result->ifindex_veth_src_fwd < 0))
> +               goto fail;
> +       result->ifindex_veth_dst_fwd = get_ifindex("veth_dst_fwd");
> +       if (CHECK_FAIL(result->ifindex_veth_dst_fwd < 0))
> +               goto fail;
> +

[...]

> +
> +       /* bpf_fib_lookup() checks if forwarding is enabled */
> +       system("ip netns exec " NS_FWD " sysctl -q -w "
> +              "net.ipv4.ip_forward=1 "
> +              "net.ipv6.conf.veth_src_fwd.forwarding=1 "
> +              "net.ipv6.conf.veth_dst_fwd.forwarding=1");

no SYS() and/or error checking?
> +
> +       test_connectivity();
> +done:
> +       system("ip netns exec " NS_FWD " sysctl -q -w "
> +              "net.ipv4.ip_forward=0 "
> +              "net.ipv6.conf.veth_src_fwd.forwarding=0 "
> +              "net.ipv6.conf.veth_dst_fwd.forwarding=0");
> +

same?

> +       bpf_program__unpin(skel->progs.tc_src, SRC_PROG_PIN_FILE);
> +       bpf_program__unpin(skel->progs.tc_chk, CHK_PROG_PIN_FILE);
> +       bpf_program__unpin(skel->progs.tc_dst, DST_PROG_PIN_FILE);
> +       test_tc_neigh_fib__destroy(skel);
> +       netns_unload_bpf();
> +       restore_root_netns();
> +}
> +

[...]