On Thu, May 28, 2020 at 6:29 AM Jakub Sitnicki [off-list ref] wrote:

On Thu, May 28, 2020 at 08:08 AM CEST, Andrii Nakryiko wrote:

quoted

On Wed, May 27, 2020 at 12:16 PM Jakub Sitnicki [off-list ref] wrote:

quoted

Extend the existing test case for flow dissector attaching to cover:

 - link creation,
 - link updates,
 - link info querying,
 - mixing links with direct prog attachment.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---

You are not using bpf_program__attach_netns() at all. Would be nice to
actually use higher-level API here...

That's true. I didn't exercise the high-level API. I can cover that.

quoted

Also... what's up with people using CHECK_FAIL + perror instead of
CHECK? Is CHECK being avoided for some reason or people are just not
aware of it (which is strange, because CHECK was there before
CHECK_FAIL)?

I can only speak for myself. Funnily enough I think I've switched from
CHECK to CHECK_FAIL when I touched on BPF flow dissector last time [0].

CHECK needs and "external" duration variable to be in scope, and so it
was suggested to me that if I'm not measuring run-time with
bpf_prog_test_run, CHECK_FAIL might be a better choice.

duration is unfortunate and historical, we can eventually fix that,
but it simply didn't feel worthwhile to me. I just do `static int
duration;` at the top of the test and forget about it.

CHECK is also perhaps too verbose because it emits a log message on
success (to report duration, I assume).

I actually find that CHECK emitting message regardless of success or
failure (in verbose mode or when test fails) helps a lot to narrow
down where exactly the failure happens (it's not always obvious from
error message). So unless we are talking about 100+ element loops, I'm
all for CHECK vebosity. Again, you'll see it only when something fails
or you run tests in verbose mode.

CHECK_FAIL is the worst in that case, because it makes me blind in
terms of tracking progress of test.

You have a better overview of all the tests than me, but if I had the
cycles I'd see if renaming CHECK to something more specific, for those
test that actually track prog run time, can work.

I'd make CHECK not depend on duration instead, and convert existing
cases that do rely on duration to something like CHECK_TIMED.

-jkbs


[0] https://lore.kernel.org/bpf/87imov1y5m.fsf@cloudflare.com/ (local)

quoted

quoted

 .../bpf/prog_tests/flow_dissector_reattach.c  | 500 +++++++++++++++++-
 1 file changed, 471 insertions(+), 29 deletions(-)

[...]