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

[Martin KaFai Lau <martin.lau@linux.dev>]

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.

> 
>>
>>
>>> +        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.