On Thu, May 28, 2020 at 6:29 AM Jakub Sitnicki <jakub@xxxxxxxxxxxxxx> wrote: > > On Thu, May 28, 2020 at 08:08 AM CEST, Andrii Nakryiko wrote: > > On Wed, May 27, 2020 at 12:16 PM Jakub Sitnicki <jakub@xxxxxxxxxxxxxx> wrote: > >> > >> 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@xxxxxxxxxxxxxx> > >> --- > > > > 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. > > > > > 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@xxxxxxxxxxxxxx/ > > > > > > >> .../bpf/prog_tests/flow_dissector_reattach.c | 500 +++++++++++++++++- > >> 1 file changed, 471 insertions(+), 29 deletions(-) > >> > > > > [...]
