On Mon, 2022-01-31 at 17:24 -0800, Andrii Nakryiko wrote:
> for simple case like this you can just keep it single line:
>
> LIBBPF_OPTS(bpf_test_run_opts, topts, .repeat = 1)
>
> But, it seems like the kernel does `if (!repeat) repeat = 1;` logic
> for program types that support repeat feature, so I'd suggest just
> drop .repeat = 1 and keep it simple.
Sure.
>
> let's not add new CHECK*() variants, CHECK() and its derivative are
> deprecated, we are using ASSERT_xxx() macros for all new code.
>
> In this case, I think it's best to replace CHECK() with just:
>
> ASSERT_OK(err, "run_err");
> ASSERT_OK(topts.retval, "run_ret_val");
>
> I don't think logging duration is useful for most, if not all, tests,
> so I wouldn't bother.
Ah, this makes a lot of sense, I was wondering what was going on with these
quite unreadable CHECK{,_ATTR} macros. I'll rewrite the ones I'm changing in
this series to use ASSERT_* if you're okay with that amount of churn?
> You didn't have to touch this code, you could have just kept duration
> = 0 (which CHECK() internally assumes, unfortunately).
I didn't *have* to but leaving the variable around to satisfy a macro feels
super awkward. Happy to minimize these changes, if they're going overboard but
I'd rather clean up the CHECK usage where I can.
>
> Alternatively, just switch to ASSERT_OK_PTR(fentry_skel,
> "fentry_skel_load"); and be done with it. As I mentioned, we are in a
> gradual process of removing CHECK()s,
> [...]
> At some point we'll do the final push and remove CHECK() altogether,
> but it doesn't have to be part of this patch set (unless you are
> interested in doing some more mechanical conversions, of course, it's
> greatly appreciated, but I didn't yet have heart to ask anyone to do
> those 2000 conversions...).
I don't think I have it in me to volunteer for the whole refactor but I'll do my
bit in the usages this series touches.
