On Mon, Aug 1, 2022 at 3:55 PM Hao Luo [off-list ref] wrote:

On Mon, Aug 1, 2022 at 2:51 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Mon, Aug 1, 2022 at 10:54 AM Hao Luo [off-list ref] wrote:

quoted

Add a selftest for cgroup_iter. The selftest creates a mini cgroup tree
of the following structure:

    ROOT (working cgroup)
     |
   PARENT
  /      \
CHILD1  CHILD2

and tests the following scenarios:

 - invalid cgroup fd.
 - pre-order walk over descendants from PARENT.
 - post-order walk over descendants from PARENT.
 - walk of ancestors from PARENT.
 - early termination.

Acked-by: Yonghong Song <redacted>
Signed-off-by: Hao Luo <redacted>
---
 .../selftests/bpf/prog_tests/cgroup_iter.c    | 193 ++++++++++++++++++
 tools/testing/selftests/bpf/progs/bpf_iter.h  |   7 +
 .../testing/selftests/bpf/progs/cgroup_iter.c |  39 ++++
 3 files changed, 239 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_iter.c
 create mode 100644 tools/testing/selftests/bpf/progs/cgroup_iter.c

[...]

quoted

quoted

+#define format_expected_output3(cg_id1, cg_id2, cg_id3) \
+       snprintf(expected_output, sizeof(expected_output), \
+                PROLOGUE "%8llu\n%8llu\n%8llu\n" EPILOGUE, \
+                (cg_id1), (cg_id2), (cg_id3))
+

you use format_expected_output{1,2} just once and
format_expected_output3 twice. Is it worth defining macros for that?

If not, we'd see this snprintf and format all over the place. It looks
worse than the current one I think, prefer leave as-is.

All over the place == 4 places where it matters.

We are not trying to write the most beautiful code through macro
obfuscation. The point is to write tests that are easy to follow,
debug, understand, and potentially modify. Adding extra layers of
macros goes against this. Instead of clearly seeing in each individual
subtest that we expect "%llu\n%llu\n", I need to search what
"format_expected_output3" is actually doing, then I'm wondering where
expected_output is coming from (I scan macro input args, see nothing,
then I conclude it must be coming from the environment; I jump to one
of the format_expected_output3 invocation sites, see no local variable
named "expected_output", then I look around and see global variable;
aha, finally!) Sure it's a rather trivial thing, but this adds up.

*Unnecessary* macros are bad and a hindrance. Please avoid them, if
possible. Saving 20 characters is not a sufficient justification in my
view.

quoted

quoted

+const char *cg_path[] = {
+       "/", "/parent", "/parent/child1", "/parent/child2"
+};
+

[...]

quoted

quoted

+       link = bpf_program__attach_iter(skel->progs.cgroup_id_printer, &opts);
+       if (!ASSERT_ERR_PTR(link, "attach_iter"))
+               bpf_link__destroy(link);

nit: you can call bpf_link__destroy() even if link is NULL or IS_ERR

Ack. Still need to ASSERT on 'link' though, so the saving is probably
just an indentation. Anyway, will change.

Yeah, of course you need to assert. But it's nice to have
unconditional assertion.

quoted

quoted

+}
+

[...]

[...]