Re: [PATCH bpf-next v4 1/6] selftests/bpf: Add traffic monitor functions.

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

On 8/6/24 16:41, Martin KaFai Lau wrote:
> On 8/6/24 4:18 PM, Kui-Feng Lee wrote:
>>
>>
>> On 8/6/24 15:22, Martin KaFai Lau wrote:
>>> On 8/6/24 3:07 PM, Kui-Feng Lee wrote:
>>>>
>>>>
>>>> On 8/1/24 21:35, Kui-Feng Lee wrote:
>>>>>
>>>>>
>>>>> On 8/1/24 20:43, Martin KaFai Lau wrote:
>>>>>> On 7/31/24 12:31 PM, Kui-Feng Lee wrote:
>>>>>>> diff --git a/tools/testing/selftests/bpf/test_progs.h 
>>>>>>> b/tools/testing/ selftests/bpf/test_progs.h
>>>>>>> index cb9d6d46826b..5d4e61fa26a1 100644
>>>>>>> --- a/tools/testing/selftests/bpf/test_progs.h
>>>>>>> +++ b/tools/testing/selftests/bpf/test_progs.h
>>>>>>> @@ -473,4 +473,20 @@ extern void test_loader_fini(struct 
>>>>>>> test_loader *tester);
>>>>>>>       test_loader_fini(&tester);                           \
>>>>>>>   })
>>>>>>> +struct tmonitor_ctx;
>>>>>>> +
>>>>>>> +#ifdef TRAFFIC_MONITOR
>>>>>>> +struct tmonitor_ctx *traffic_monitor_start(const char *netns);
>>>>>>> +void traffic_monitor_stop(struct tmonitor_ctx *ctx);
>>>>>>> +#else
>>>>>>> +static inline struct tmonitor_ctx *traffic_monitor_start(const 
>>>>>>> char *netns)
>>>>>>> +{
>>>>>>> +    return (struct tmonitor_ctx *)-1;
>>>>>>
>>>>>> hmm... from peeking patch 3, only NULL is checked.
>>>>
>>>> When traffic monitor is disable, these two functions are noop.
>>>> Returning -1 (not NULL) is convenient for the callers. They don't need
>>>> to tell if the error caused by a real error or by the disabled
>>>> feature.
>>>
>>> I pasted the code from patch 3 here only to ensure I understand the 
>>> above explanation correctly:
>>>
>>> +        netns_obj->tmon = traffic_monitor_start(name);
>>> +        if (!netns_obj->tmon)
>>>              ^^^^^^^^^^^^^^^^
>>>
>>> +            goto fail;
>>>
>>> Does it mean the traffic_monitor_start() above will never be called 
>>> if TRAFFIC_MONITOR macro is not defined such that 
>>> traffic_monitor_start() returning -1 but testing for NULL here does 
>>> not matter?
>>
>> Correct!
> 
> Got it. Then I missed some understanding. Can you explain why the above 
> traffic_monitor_start() will never be called?
> 

Sorry! Forget my previous word.

In the case that TRAFFIC_MONITOR is not defined,
traffic_monitor_start(name) always returns -1.
So, "!netns_obj->tmon" is always false. "goto fail" is never executed.
That means the test will keep going just like that traffic monitor is
enabled and started correctly.