Re: [PATCH bpf-next v6 3/6] selftests/bpf: netns_new() and netns_free() helpers.

[Kui-Feng Lee <sinquersw@gmail.com>]

On 8/8/24 14:56, Martin KaFai Lau wrote:
> On 8/8/24 1:38 PM, Kui-Feng Lee wrote:
>>
>>
>> On 8/8/24 13:27, Martin KaFai Lau wrote:
>>> On 8/7/24 11:31 AM, Kui-Feng Lee wrote:
>>>> +struct netns_obj *netns_new(const char *nsname, bool open)
>>>> +{
>>>> +    struct netns_obj *netns_obj = malloc(sizeof(*netns_obj));
>>>> +    const char *test_name, *subtest_name;
>>>> +    int r;
>>>> +
>>>> +    if (!netns_obj)
>>>> +        return NULL;
>>>> +    memset(netns_obj, 0, sizeof(*netns_obj));
>>>> +
>>>> +    netns_obj->nsname = strdup(nsname);
>>>> +    if (!netns_obj->nsname)
>>>> +        goto fail;
>>>> +
>>>> +    /* Create the network namespace */
>>>> +    r = make_netns(nsname);
>>>> +    if (r)
>>>> +        goto fail;
>>>> +
>>>> +    /* Set the network namespace of the current process */
>>>> +    if (open) {
>>>> +        netns_obj->nstoken = open_netns(nsname);
>>>> +        if (!netns_obj->nstoken)
>>>> +            goto fail;
>>>> +    }
>>>> +
>>>> +    /* Start traffic monitor */
>>>> +    if (env.test->should_tmon ||
>>>> +        (env.subtest_state && env.subtest_state->should_tmon)) {
>>>> +        test_name = env.test->test_name;
>>>> +        subtest_name = env.subtest_state ? env.subtest_state->name 
>>>> : NULL;
>>>> +        netns_obj->tmon = traffic_monitor_start(nsname, test_name, 
>>>> subtest_name);
>>>
>>> The traffic_monitor_start() does open/close_netns(). close_netns() 
>>> will restore to the previous netns. Is it better to do 
>>> traffic_monitor_start() before the above open_netns() such that we 
>>> don't have to worry about the stacking open_netns and which netns the 
>>> close_netns will restore?
>>
>> Do you mean to open_netns() in another thread at the same time and
>> interleave with the open_netns()/close_netns() pairs in the current 
>> thread?
> 
> I didn't mean this case. I don't think there will be a test calling 
> open/close_nets() in different threads... but will it be an issue?
> 
> I was trying to say having the close_netns() restoring to the init_netns 
> for the common case. Easier for the brain to reason without too much 
> unnecessary open_netns stacking. Not saying there is an issue in the patch.

Got it!

> 
>>
>>>
>>>
>>>> +        if (!netns_obj->tmon)
>>>> +            fprintf(stderr, "Failed to start traffic monitor for 
>>>> %s\n", nsname);
>>>> +    } else {
>>>> +        netns_obj->tmon = NULL;
>>>> +    }
>>>> +
>>>> +    system("ip link set lo up");
>>>
>>> The "bool open" could be false here. This command could be acted on 
>>> the > init_netns and the intention is to set lo up at the newly 
>>> created netns.
>>>
>>
>> You are right! I should enclose this call in-between a pair of
>> open_netns() & close_netns().
> 
> I would just move it to make_netns() and do "ip -n nsname link set lo up".
> Yes, the traffic_monitor_start() is after the lo is up but I think it is 
> fine.
> 
> 

Ok!