On Wed, Aug 19, 2020 at 5:57 PM Yonghong Song <yhs@xxxxxx> wrote: > > > > On 8/19/20 12:05 AM, Yauheni Kaliuta wrote: > > On Wed, Aug 19, 2020 at 8:19 AM Yonghong Song <yhs@xxxxxx> wrote: > >> > >> > >> > >> On 8/18/20 7:34 PM, Yauheni Kaliuta wrote: > >>> The error path in libbpf.c:load_program() has calls to pr_warn() > >>> which ends up for global_funcs tests to > >>> test_global_funcs.c:libbpf_debug_print(). > >>> > >>> For the tests with no struct test_def::err_str initialized with a > >>> string, it causes call of strstr() with NULL as the second argument > >>> and it segfaults. > >>> > >>> Fix it by calling strstr() only for non-NULL err_str. > >>> > >>> The patch does not fix the test itself. > >> > >> So this happens in older kernel, right? Could you clarify more > >> in which kernel and what environment? It probably no need to > >> fix the issue for really old kernel but some clarification > >> will be good. > > > > I'll test it with the very recent kernel on that architecture soon, > > for sure. But it's not related to the patch. > > The above "The patch does not fix the test itself" a little bit vague. > You can say that "The test may fail in old kernels where <why it fails > ...> and this patch is to fix the segfault rather the test failure.". > This way people can easily understand why and the purpose of this patch. Ok, I'll remove it completely (as I mentioned in the follow-up email, the test still fails for me for the Linus' HEAD). > > > > >> > >>> > >>> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@xxxxxxxxxx> > > Ok, ack with the above nit and one nit below. > Acked-by: Yonghong Song <yhs@xxxxxx> > I guess it is better to send a v2 carrying my ack. > > >>> --- > >>> tools/testing/selftests/bpf/prog_tests/test_global_funcs.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c > >>> index 25b068591e9a..6ad14c5465eb 100644 > >>> --- a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c > >>> +++ b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c > >>> @@ -19,7 +19,7 @@ static int libbpf_debug_print(enum libbpf_print_level level, > >>> log_buf = va_arg(args, char *); > >>> if (!log_buf) > >>> goto out; > >>> - if (strstr(log_buf, err_str) == 0) > >>> + if ((err_str != NULL) && (strstr(log_buf, err_str) == 0)) > >> > >> Looks good but the code can be simplified as > >> if (err_str && strstr(log_buf, err_str) == 0) > >> found = true; > > > > Yes, but I prefer to use NULL explicitly when I deal with pointers. It > > demonstrates intention better. You also can simplify strstr() == 0 > > with !. Actually, strstr() returns char *, so comparation to 0 > > (totally legal by standard) breaks my feelings too :). > > comparison with NULL is okay. You can just do > (err_str != NULL && strstr(log_buf, err_str) == 0) > there is no need for extra parenthesis. Ah, ok. Inconsistency with the strstr check bothers me, but it would be unrelated change. Thank you for review! > > > > > If you insist, I'll send v2 of course. > > > >>> found = true; > >>> out: > >>> printf(format, log_buf); > >>> > >> > > > > > -- WBR, Yauheni
