Re: [PATCH bpf-next 5/5] selftests/bpf: add test cases for bpf_strncmp()

Hi,
On 12/7/2021 11:09 AM, Andrii Nakryiko wrote:

quoted

+static struct strncmp_test *strncmp_test_open_and_disable_autoload(void)
+{
+       struct strncmp_test *skel;
+       struct bpf_program *prog;
+
+       skel = strncmp_test__open();
+       if (libbpf_get_error(skel))
+               return skel;
+
+       bpf_object__for_each_program(prog, skel->obj)
+               bpf_program__set_autoload(prog, false);

I think this is a wrong "code economy". You save few lines of code,
but make tests harder to follow. Just do 4 lines of code for each
subtest:

skel = strncmp_test__open();
if (!ASSERT_OK_PTR(skel, "skel_open"))
    return;

bpf_object__for_each_program(prog, skel->obj)
    bpf_program__set_autoload(prog, false);


It makes tests more self-contained and easier to follow. Also if some
tests need to do something slightly different it's easier to modify
them, as they are not coupled to some common helper. DRY is good where
it makes sense, but it also increases code coupling and more "jumping
around" in code, so it shouldn't be applied blindly.

Thanks for your suggestion on DRY topic. Will do in v2.

+
+static int trigger_strncmp(const struct strncmp_test *skel)
+{
+       struct timespec wait = {.tv_sec = 0, .tv_nsec = 1};
+
+       nanosleep(&wait, NULL);
all the other tests are just doing usleep(1), why using this more verbose way?

Will do in v2.

quoted

+
+static __always_inline bool called_by_target_pid(void)
+{
+       __u32 pid = bpf_get_current_pid_tgid() >> 32;
+
+       return pid == target_pid;
+}

again, what's the point of this helper? it's used once and you'd
actually save the code by doing the following inline:

if ((bpf_get_current_pid_tgid() >> 32) != target_pid)
    return 0;

Will do in v2.

quoted

+
+SEC("tp/syscalls/sys_enter_nanosleep")
+int do_strncmp(void *ctx)
+{
+       if (!called_by_target_pid())
+               return 0;
+
+       cmp_ret = bpf_strncmp(str, STRNCMP_STR_SZ, target);
+
+       return 0;
+}
+
+SEC("tp/syscalls/sys_enter_nanosleep")
+int strncmp_bad_not_const_str_size(void *ctx)
+{

probably worth leaving a short comment explaining that this program
should fail because ...

OK. Will do in v2.

Regards,
Tao