On Fri, Jan 28, 2022 at 12:12 PM Delyan Kratunov <delyank@xxxxxx> wrote: > > Thanks for taking a look, Daniel! > > On Fri, 2022-01-28 at 14:07 +0100, Daniel Borkmann wrote: > > On 1/28/22 2:23 AM, Delyan Kratunov wrote: > > > Adding this behavior to prog_test_run_opts is one option, keeping the test as-is > > > and cloning it to use bpf_prog_test_run_opts is another possibility. > > > > I would suggest to do the former rather than duplicating, if there's nothing > > particularly blocking us from adding this to prog_test_run_opts. > > I looked into this a bit more. Unfortunately, bpf_test_finish unconditionally > copies data_size_out back into the uattr, even if data_out is NULL. > (net/bpf/test_run.c:180) > > This makes the ergonomics of reusing the same topts struct for multiple > bpf_prog_test_run calls pretty horrendous - you'd need to clear data_size_out > before every call, even if you don't care about it otherwise (and you don't, you > didn't set data_out!). > > In practicality, adding the logic from _xattr to _opts results in a significant > number of test failures. I'm a bit worried it might break libbpf users if they > use similar opts reuse patterns. > Seems like kernel doesn't enforce that data_size_out == 0 if data_out is NULL, so I'd say let's just keep bpf_test_run_opts() as is and defer to kernel for error checking (e.g., for raw_tp data_out isn't allowed to be NULL, but libbpf doesn't check that, right, it just lets kernel do all the nuanced error checking). So I'd say let's keep _opts() as is and let's remove parts of prog_run_xattr.c selftest that check this specific error check? > > As you have it looks good to me. One small nit, please also add a non-empty > > commit message with rationale to each of the patches rather than just SoB alone. > > Will do! >
