On Tue, Feb 1, 2022 at 4:01 PM Delyan Kratunov <delyank@xxxxxx> wrote:
>
> 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?
No, it's fine with me. As I said, I'd like all CHECK()s to be gone, so
the sooner and closer we get to that the better. We have BPF CI setup
to make sure that we don't accidentally screw up checks in a major
way, so it's pretty safe to do.
>
> > 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.
Heh, we do have some selftests doing `static int duration;` in a file
to satisfy CHECK() :) But I don't mind more selftest code churn to
convert to ASSERT_xxx() macros.
>
> >
> > 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.
No worries and thanks!
