Re: [PATCH bpf-next v2 2/3] bpftool: don't append / to the progtype
From: Andrii Nakryiko <hidden>
Date: 2021-10-20 18:12:34
Also in:
bpf
On Tue, Oct 12, 2021 at 9:15 AM Stanislav Fomichev [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Otherwise, attaching with bpftool doesn't work with strict section names. Also, switch to libbpf strict mode to use the latest conventions (note, I don't think we have any cli api guarantees?). Signed-off-by: Stanislav Fomichev <redacted> --- tools/bpf/bpftool/main.c | 4 ++++ tools/bpf/bpftool/prog.c | 15 +-------------- 2 files changed, 5 insertions(+), 14 deletions(-)diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c index 02eaaf065f65..8223bac1e401 100644 --- a/tools/bpf/bpftool/main.c +++ b/tools/bpf/bpftool/main.c@@ -409,6 +409,10 @@ int main(int argc, char **argv) block_mount = false; bin_name = argv[0]; + ret = libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
Quentin, any concerns about turning strict mode for bpftool? Either way we should audit bpftool code to ensure that at least error handling is done properly (see my comments on Dave's patch set about == -1 checks).
quoted hunk ↗ jump to hunk
+ if (ret) + p_err("failed to enable libbpf strict mode: %d", ret); + hash_init(prog_table.table); hash_init(map_table.table); hash_init(link_table.table);diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c index 277d51c4c5d9..17505dc1243e 100644 --- a/tools/bpf/bpftool/prog.c +++ b/tools/bpf/bpftool/prog.c@@ -1396,8 +1396,6 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) while (argc) { if (is_prefix(*argv, "type")) { - char *type; - NEXT_ARG(); if (common_prog_type != BPF_PROG_TYPE_UNSPEC) {@@ -1407,19 +1405,8 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) if (!REQ_ARGS(1)) goto err_free_reuse_maps; - /* Put a '/' at the end of type to appease libbpf */ - type = malloc(strlen(*argv) + 2); - if (!type) { - p_err("mem alloc failed"); - goto err_free_reuse_maps; - } - *type = 0; - strcat(type, *argv); - strcat(type, "/"); - - err = get_prog_type_by_name(type, &common_prog_type, + err = get_prog_type_by_name(*argv, &common_prog_type, &expected_attach_type);
Question not specifically to Stanislav, but anyone who's using bpftool to load programs. Do we need to support program type overrides? Libbpf has been inferring the program type for a long time now, are there realistic use cases where this override logic is necessary? I see there is bpf_object__for_each_program() loop down the code, it essentially repeats what libbpf is already doing on bpf_object__open(), why keep the duplicated logic? libbpf_prog_type_by_name() is also a bad idea (IMO) and I'd like to get rid of that in libbpf 1.0, so if we can stop using that from bpftool, it would be great. Thoughts?
- free(type);
if (err < 0)
goto err_free_reuse_maps;
--
2.33.0.882.g93a45727a2-goog