On Thu, Sep 1, 2022 at 10:55 PM Martin KaFai Lau <kafai@xxxxxx> wrote:
>
> On Thu, Sep 01, 2022 at 07:15:10PM +0000, YiFei Zhu wrote:
> > diff --git a/tools/testing/selftests/bpf/prog_tests/connect_ping.c b/tools/testing/selftests/bpf/prog_tests/connect_ping.c
> > new file mode 100644
> > index 0000000000000..99b1a2f0c4921
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/connect_ping.c
> > @@ -0,0 +1,318 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +/*
> > + * Copyright 2022 Google LLC.
> > + */
> > +
> > +#define _GNU_SOURCE
> > +#include <sys/mount.h>
> > +
> > +#include <test_progs.h>
> > +#include <cgroup_helpers.h>
> > +#include <network_helpers.h>
> > +
> > +#include "connect_ping.skel.h"
> > +
> > +/* 2001:db8::1 */
> > +#define BINDADDR_V6 { { { 0x20,0x01,0x0d,0xb8,0,0,0,0,0,0,0,0,0,0,0,1 } } }
> > +const struct in6_addr bindaddr_v6 = BINDADDR_V6;
> static
ack.
> > +
> > +static bool write_sysctl(const char *sysctl, const char *value)
> This has been copied >2 times now which probably shows it will
> also be useful in the future.
> Take this chance to move it to testing_helpers.{h,c}.
ack.
> > +{
> > + int fd, err, len;
> > +
> > + fd = open(sysctl, O_WRONLY);
> > + if (!ASSERT_GE(fd, 0, "open-sysctl"))
> > + return false;
> > +
> > + len = strlen(value);
> > + err = write(fd, value, len);
> > + close(fd);
> > + if (!ASSERT_EQ(err, len, "write-sysctl"))
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +static void test_ipv4(int cgroup_fd)
> > +{
> > + struct sockaddr_in sa = {
> > + .sin_family = AF_INET,
> > + .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
> > + };
> > + socklen_t sa_len = sizeof(sa);
> > + struct connect_ping *obj;
> > + int sock_fd;
> > +
> > + sock_fd = socket(AF_INET, SOCK_DGRAM, IPPROTO_ICMP);
> > + if (!ASSERT_GE(sock_fd, 0, "sock-create"))
> > + return;
> > +
> > + obj = connect_ping__open_and_load();
> > + if (!ASSERT_OK_PTR(obj, "skel-load"))
> > + goto close_sock;
> > +
> > + obj->bss->do_bind = 0;
> > +
> > + /* Attach connect v4 and connect v6 progs, connect a v4 ping socket to
> > + * localhost, assert that only v4 is called, and called exactly once,
> > + * and that the socket's bound address is original loopback address.
> > + */
> > + obj->links.connect_v4_prog =
> > + bpf_program__attach_cgroup(obj->progs.connect_v4_prog, cgroup_fd);
> > + if (!ASSERT_OK_PTR(obj->links.connect_v4_prog, "cg-attach-v4"))
> > + goto close_bpf_object;
> > + obj->links.connect_v6_prog =
> > + bpf_program__attach_cgroup(obj->progs.connect_v6_prog, cgroup_fd);
> > + if (!ASSERT_OK_PTR(obj->links.connect_v6_prog, "cg-attach-v6"))
> > + goto close_bpf_object;
> Overall, it seems like a lot of dup code can be saved
> between test_ipv4, test_ipv6, and their _bind() version.
>
> eg. The skel setup can be done once and the bss variables can be reset
> at the beginning of each test by memset(skel->bss, 0, sizeof(*skel->bss)).
> The result checking part is essentially checking the expected bss values
> and the getsockname result also.
ack.
> btw, does it make sense to do it as a subtest in
> connect_force_port.c or they are very different?
I could try, but they are structured differently; that checks the
ports whereas this checks the bound IPs. That test also doesn't use
skels or sets up netns whereas this test does. I think I would prefer
to have two tests since tests are cheap, but I can try to restructure
connect_force_port.c in a way that is compatible with both if you
insist.
