Hi Song,
Thanks for the feedback !
On Mon, Sep 14, 2020 at 8:07 PM Song Liu song@kernel.org wrote:
On Fri, Sep 11, 2020 at 8:02 AM Nicolas Rybowski nicolas.rybowski@tessares.net wrote:
This patch adds a base for MPTCP specific tests.
It is currently limited to the is_mptcp field in case of plain TCP connection because for the moment there is no easy way to get the subflow sk from a msk in userspace. This implies that we cannot lookup the sk_storage attached to the subflow sk in the sockops program.
Acked-by: Matthieu Baerts matthieu.baerts@tessares.net Signed-off-by: Nicolas Rybowski nicolas.rybowski@tessares.net
Acked-by: Song Liu songliubraving@fb.com
With some nitpicks below.
Notes: v1 -> v2: - new patch: mandatory selftests (Alexei)
[...]
int timeout_ms);diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c new file mode 100644 index 000000000000..0e65d64868e9 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c @@ -0,0 +1,119 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <test_progs.h> +#include "cgroup_helpers.h" +#include "network_helpers.h"
+struct mptcp_storage {
__u32 invoked;__u32 is_mptcp;+};
+static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp) +{
int err = 0, cfd = client_fd;struct mptcp_storage val;/* Currently there is no easy way to get back the subflow sk from the MPTCP* sk, thus we cannot access here the sk_storage associated to the subflow* sk. Also, there is no sk_storage associated with the MPTCP sk since it* does not trigger sockops events.* We silently pass this situation at the moment.*/if (is_mptcp == 1)return 0;if (CHECK_FAIL(bpf_map_lookup_elem(map_fd, &cfd, &val) < 0)) {perror("Failed to read socket storage");Maybe simplify this with CHECK(), which contains a customized error message? Same for some other calls.
The whole logic here is strongly inspired from prog_tests/tcp_rtt.c where CHECK_FAIL is used. Also the CHECK macro will print a PASS message on successful map lookup, which is not expected at this point of the tests. I think it would be more interesting to leave it as it is to keep a cohesion between TCP and MPTCP selftests. What do you think?
If there are no objections, I will send a v3 with the other requested changes and a rebase on the latest bpf-next.
return -1;}if (val.invoked != 1) {log_err("%s: unexpected invoked count %d != %d",msg, val.invoked, 1);err++;}if (val.is_mptcp != is_mptcp) {log_err("%s: unexpected bpf_tcp_sock.is_mptcp %d != %d",msg, val.is_mptcp, is_mptcp);err++;}return err;+}
+static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
[...]
client_fd = is_mptcp ? connect_to_mptcp_fd(server_fd, 0) :connect_to_fd(server_fd, 0);if (client_fd < 0) {err = -1;goto close_client_fd;This should be "goto close_bpf_object;", and we don't really need the label close_client_fd.
}err += is_mptcp ? verify_sk(map_fd, client_fd, "MPTCP subflow socket", 1) :It doesn't really change the logic, but I guess we only need "err = xxx"?
verify_sk(map_fd, client_fd, "plain TCP socket", 0);+close_client_fd:
close(client_fd);+close_bpf_object:
bpf_object__close(obj);return err;+}