On Wed, Sep 15, 2021 at 9:24 PM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 9/15/21 6:58 PM, Andrii Nakryiko wrote:
> > Switch fexit_bpf2bpf selftest to bpf_program__set_attach_target()
> > instead of using bpf_object_open_opts.attach_prog_fd, which is going to
> > be deprecated. These changes also demonstrate the new mode of
> > set_attach_target() in which it allows NULL when the target is BPF
> > program (attach_prog_fd != 0).
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> Ack with a minor nit below.
> Acked-by: Yonghong Song <yhs@xxxxxx>
>
> > ---
> > .../selftests/bpf/prog_tests/fexit_bpf2bpf.c | 43 +++++++++++--------
> > 1 file changed, 26 insertions(+), 17 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
> > index 73b4c76e6b86..c7c1816899bf 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
> > @@ -60,7 +60,7 @@ static void test_fexit_bpf2bpf_common(const char *obj_file,
> > struct bpf_object *obj = NULL, *tgt_obj;
> > __u32 retval, tgt_prog_id, info_len;
> > struct bpf_prog_info prog_info = {};
> > - struct bpf_program **prog = NULL;
> > + struct bpf_program **prog = NULL, *p;
> > struct bpf_link **link = NULL;
> > int err, tgt_fd, i;
> > struct btf *btf;
> > @@ -69,9 +69,6 @@ static void test_fexit_bpf2bpf_common(const char *obj_file,
> > &tgt_obj, &tgt_fd);
> > if (!ASSERT_OK(err, "tgt_prog_load"))
> > return;
> > - DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
> > - .attach_prog_fd = tgt_fd,
> > - );
> >
> > info_len = sizeof(prog_info);
> > err = bpf_obj_get_info_by_fd(tgt_fd, &prog_info, &info_len);
> > @@ -89,10 +86,15 @@ static void test_fexit_bpf2bpf_common(const char *obj_file,
> > if (!ASSERT_OK_PTR(prog, "prog_ptr"))
> > goto close_prog;
> >
> > - obj = bpf_object__open_file(obj_file, &opts);
> > + obj = bpf_object__open_file(obj_file, NULL);
> > if (!ASSERT_OK_PTR(obj, "obj_open"))
> > goto close_prog;
> >
> > + bpf_object__for_each_program(p, obj) {
> > + err = bpf_program__set_attach_target(p, tgt_fd, NULL);
> > + ASSERT_OK(err, "set_attach_target");
> > + }
> > +
> > err = bpf_object__load(obj);
> > if (!ASSERT_OK(err, "obj_load"))
> > goto close_prog;
> > @@ -270,7 +272,7 @@ static void test_fmod_ret_freplace(void)
> > struct bpf_link *freplace_link = NULL;
> > struct bpf_program *prog;
> > __u32 duration = 0;
> > - int err, pkt_fd;
> > + int err, pkt_fd, attach_prog_fd;
> >
> > err = bpf_prog_load(tgt_name, BPF_PROG_TYPE_UNSPEC,
> > &pkt_obj, &pkt_fd);
> > @@ -278,26 +280,32 @@ static void test_fmod_ret_freplace(void)
> > if (CHECK(err, "tgt_prog_load", "file %s err %d errno %d\n",
> > tgt_name, err, errno))
> > return;
> > - opts.attach_prog_fd = pkt_fd;
> >
> > - freplace_obj = bpf_object__open_file(freplace_name, &opts);
> > + freplace_obj = bpf_object__open_file(freplace_name, NULL);
> > if (!ASSERT_OK_PTR(freplace_obj, "freplace_obj_open"))
> > goto out;
> >
> > + prog = bpf_program__next(NULL, freplace_obj);
> > + err = bpf_program__set_attach_target(prog, pkt_fd, NULL);
> > + ASSERT_OK(err, "freplace__set_attach_target");
>
> The above pattern appears 3 times. Maybe it is worthwhile to
> have a small helper. But the current code is also fine,
> so I won't insist.
I think it's acceptable for test code. It actually makes it easier to
follow, because every extra helper function is a bit of distraction
and context switch when reading the code.
My hope that with time we'll clean this up to use BPF skeleton and the
code will be clean without unnecessary repetition.
>
> > +
> > err = bpf_object__load(freplace_obj);
> > if (CHECK(err, "freplace_obj_load", "err %d\n", err))
> > goto out;
> >
> > - prog = bpf_program__next(NULL, freplace_obj);
> > freplace_link = bpf_program__attach_trace(prog);
> > if (!ASSERT_OK_PTR(freplace_link, "freplace_attach_trace"))
> > goto out;
> >
> > - opts.attach_prog_fd = bpf_program__fd(prog);
> > - fmod_obj = bpf_object__open_file(fmod_ret_name, &opts);
> > + fmod_obj = bpf_object__open_file(fmod_ret_name, NULL);
> > if (!ASSERT_OK_PTR(fmod_obj, "fmod_obj_open"))
> > goto out;
> >
> > + attach_prog_fd = bpf_program__fd(prog);
> > + prog = bpf_program__next(NULL, fmod_obj);
> > + err = bpf_program__set_attach_target(prog, attach_prog_fd, NULL);
> > + ASSERT_OK(err, "fmod_ret_set_attach_target");
> > +
> > err = bpf_object__load(fmod_obj);
> > if (CHECK(!err, "fmod_obj_load", "loading fmod_ret should fail\n"))
> > goto out;
> > @@ -322,14 +330,14 @@ static void test_func_sockmap_update(void)
> > }
> >
> > static void test_obj_load_failure_common(const char *obj_file,
> > - const char *target_obj_file)
> > -
> > + const char *target_obj_file)
> > {
> > /*
> > * standalone test that asserts failure to load freplace prog
> > * because of invalid return code.
> > */
> > struct bpf_object *obj = NULL, *pkt_obj;
> > + struct bpf_program *prog;
> > int err, pkt_fd;
> > __u32 duration = 0;
> >
> > @@ -339,14 +347,15 @@ static void test_obj_load_failure_common(const char *obj_file,
> > if (CHECK(err, "tgt_prog_load", "file %s err %d errno %d\n",
> > target_obj_file, err, errno))
> > return;
> > - DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
> > - .attach_prog_fd = pkt_fd,
> > - );
> >
> > - obj = bpf_object__open_file(obj_file, &opts);
> > + obj = bpf_object__open_file(obj_file, NULL);
> > if (!ASSERT_OK_PTR(obj, "obj_open"))
> > goto close_prog;
> >
> > + prog = bpf_program__next(NULL, obj);
> > + err = bpf_program__set_attach_target(prog, pkt_fd, NULL);
> > + ASSERT_OK(err, "set_attach_target");
> > +
> > /* It should fail to load the program */
> > err = bpf_object__load(obj);
> > if (CHECK(!err, "bpf_obj_load should fail", "err %d\n", err))
> >
