On 4/3/23 5:15 PM, Aditi Ghag wrote:
On Apr 3, 2023, at 10:35 AM, Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
On 4/3/23 8:55 AM, Aditi Ghag wrote:
On Mar 31, 2023, at 3:32 PM, Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
On 3/30/23 11:46 AM, Stanislav Fomichev wrote:
+void test_sock_destroy(void)
+{
+ struct sock_destroy_prog *skel;
+ int cgroup_fd = 0;
+
+ skel = sock_destroy_prog__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "skel_open"))
+ return;
+
+ cgroup_fd = test__join_cgroup("/sock_destroy");
Please run this test in its own netns also to avoid affecting other tests as much as possible.
Is it okay if I defer this nit to a follow-up patch? It's not conflicting with other tests at the moment.
Is it sure it won't affect other tests when running in parallel (test -j)?
This test is iterating all in the current netns and only checks for port before aborting.
It is easy to add. eg. ~10 lines of codes can be borrowed from prog_tests/mptcp.c which has recently done the same in commit 02d6a057c7be to run the test in its own netns to set a sysctl.
I haven't run the tests in parallel, but the tests are not using hard-coded ports anymore. Anyway I'm willing to run it in a separate netns as a follow-up for additional guards, but do you still think it's a blocker for this patch?
Testing port is not good enough. It is only like ~10 lines of codes that can be
borrowed from other existing tests that I mentioned earlier. What is the reason
to cut corners here? The time spent in replying on this topic is more than
enough to add the netns support. I don't want to spend time to figure out why
other tests running in parallel become flaky while waiting for the follow up,
so no.
Please run the test in its own netns. All new network tests must run in its own
netns.
btw, since I don't hear any comment on patch 5 regarding to restricting the
destroy kfunc to BPF_TRACE_ITER only. It is the major piece missing. I am
putting some pseudo code that is more flexible than adding
BTF_KFUNC_HOOK_TRACING_ITER that I mentioned earlier to see how it may look
like. Will update that patch's thread soon.
