> On Apr 3, 2023, at 6:41 PM, Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> 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.
Got it. I'll take care of the test.
>
> 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.
>
Yes, this was the only open item for now. Thanks, I'll take a look at your RFC patch.
