Re: [PATCH bpf-next] selftests/bpf: test_progs use another shell exit on non-actions

[Yonghong Song <yhs@fb.com>]

On 7/3/20 4:59 AM, Jesper Dangaard Brouer wrote:
> On Thu, 2 Jul 2020 14:24:14 -0700
> Yonghong Song <yhs@fb.com> wrote:
> 
>> On 7/2/20 10:59 AM, Jesper Dangaard Brouer wrote:
>>> This is a follow up adjustment to commit 6c92bd5cd465 ("selftests/bpf:
>>> Test_progs indicate to shell on non-actions"), that returns shell exit
>>> indication EXIT_FAILURE (value 1) when user selects a non-existing test.
>>>
>>> The problem with using EXIT_FAILURE is that a shell script cannot tell
>>> the difference between a non-existing test and the test failing.
>>>
>>> This patch uses value 2 as shell exit indication.
>>> (Aside note unrecognized option parameters use value 64).
>>>
>>> Fixes: 6c92bd5cd465 ("selftests/bpf: Test_progs indicate to shell on non-actions")
>>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>> ---
>>>    tools/testing/selftests/bpf/test_progs.c |    4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
>>> index 104e833d0087..e8f7cd5dbae4 100644
>>> --- a/tools/testing/selftests/bpf/test_progs.c
>>> +++ b/tools/testing/selftests/bpf/test_progs.c
>>> @@ -12,6 +12,8 @@
>>>    #include <string.h>
>>>    #include <execinfo.h> /* backtrace */
>>>    
>>> +#define EXIT_NO_TEST 2
>>
>> How do you ensure this won't collide with other exit code
>> from other library functions (e.g., error code 64 is used
>> for unrecognized option which I have no idea what 64 means)?
> 
> I expect 64 comes from: /usr/include/sysexits.h
>   #define EX_USAGE        64      /* command line usage error */

Thanks for the pointer.

> 
> 
>> Maybe -2 for the exit code?
> 
> No. The process's exit status must be a number between 0 and 255, as
> defined in man exit(3). (run: 'man 3 exit' as there are many manpages
> named exit).

Yes, if user app exits with -2, it actually prints 254, -1 for 255...

> 
> But don't use above 127, because that is usually used for indicating
> signals.  E.g. 139 means 11=SIGSEGV $((139 & 127))=11.  POSIX defines
> in man wait(3p) check WIFSIGNALED(STATUS) and WTERMSIG(139)=11.
> (Hint: cmd 'kill -l' list signals and their numbers).
> 
> I bring up Segmentation fault explicitly, as we are seeing these happen
> with different tests (that are part of test_progs).  CI people writing
> these shell-scripts could pickup these hints and report them, if that
> makes sense.

Make sense to use from 1 - 127 range for normal exit.

> 
> 
>> test_progs already uses -1.
> 
> Well that is a bug then.  This will be seen by the shell (parent
> process) as 255.

I think previously people may just check test_progs return 0 or non-0.
Since here you will try to check different error return code, It makes
sense to do an audit to explicitly define all return values. So this
way, tools can have a reliable way to check exit code.

> 
>   
>>> +
>>>    /* defined in test_progs.h */
>>>    struct test_env env = {};
>>>    
>>> @@ -740,7 +742,7 @@ int main(int argc, char **argv)
>>>    	close(env.saved_netns_fd);
>>>    
>>>    	if (env.succ_cnt + env.fail_cnt + env.skip_cnt == 0)
>>> -		return EXIT_FAILURE;
>>> +		return EXIT_NO_TEST;
>>>    
>>>    	return env.fail_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
>>>    }
>>>
>