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 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?

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

>> +
>> +    return netns_obj;
>> +fail:
>> +    close_netns(netns_obj->nstoken);
>> +    remove_netns(nsname);
>> +    free(netns_obj->nsname);
>> +    free(netns_obj);
>> +    return NULL;
>> +}
>> +
>> +/* Delete the network namespace.
>> + *
>> + * This function should be paired with netns_new() to delete the 
>> namespace
>> + * created by netns_new().
>> + */
>> +void netns_free(struct netns_obj *netns_obj)
>> +{
>> +    if (!netns_obj)
>> +        return;
>> +    if (netns_obj->tmon)
>> +        traffic_monitor_stop(netns_obj->tmon);
>> +    close_netns(netns_obj->nstoken);
>> +    remove_netns(netns_obj->nsname);
>> +    free(netns_obj->nsname);
>> +    free(netns_obj);
>> +}
>> +
>>   /* extern declarations for test funcs */
>>   #define DEFINE_TEST(name)                \
>>       extern void test_##name(void) __weak;        \
>> diff --git a/tools/testing/selftests/bpf/test_progs.h 
>> b/tools/testing/selftests/bpf/test_progs.h
>> index 966011eb7ec8..3ad131de14c6 100644
>> --- a/tools/testing/selftests/bpf/test_progs.h
>> +++ b/tools/testing/selftests/bpf/test_progs.h
>> @@ -430,6 +430,10 @@ int write_sysctl(const char *sysctl, const char 
>> *value);
>>   int get_bpf_max_tramp_links_from(struct btf *btf);
>>   int get_bpf_max_tramp_links(void);
>> +struct netns_obj;
>> +struct netns_obj *netns_new(const char *name, bool open);
>> +void netns_free(struct netns_obj *netns);
>> +
>>   #ifdef __x86_64__
>>   #define SYS_NANOSLEEP_KPROBE_NAME "__x64_sys_nanosleep"
>>   #elif defined(__s390x__)
>