Andrii Nakryiko wrote: > On Tue, Oct 22, 2019 at 12:20 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote: > > > > On Mon, Oct 21, 2019 at 10:07:59PM -0700, John Fastabend wrote: > > > Andrii Nakryiko wrote: > > > > On Sat, Oct 19, 2019 at 1:30 AM John Fastabend <john.fastabend@xxxxxxxxx> wrote: > > > > > > > > > > Following ./Documentation/trace/kprobetrace.rst add support for loading > > > > > kprobes programs on older kernels. > > > > > > > > My main concern with this is that this code is born bit-rotten, > > > > because selftests are never testing the legacy code path. How did you > > > > think about testing this and ensuring that this keeps working going > > > > forward? > > > > > > Well we use it, but I see your point and actually I even broke the retprobe > > > piece hastily fixing merge conflicts in this patch. When I ran tests on it > > > I missed running retprobe tests on the set of kernels that would hit that > > > code. > > > > If it also gets explicitly exposed as bpf_program__attach_legacy_kprobe() or > > such, it should be easy to add BPF selftests for that API to address the test > > coverage concern. Generally more selftests for exposed libbpf APIs is good to > > have anyway. > > > > Agree about tests. Disagree about more APIs, especially that the only > difference will be which underlying kernel machinery they are using to > set everything up. We should ideally avoid exposing that to users. Maybe a build flag to build with only the older style supported for testing? Then we could build, test in selftests at least. Be clear the flag is only for testing and can not be relied upon. > > > Cheers, > > Daniel
