On Sun, 13 Nov 2022 at 10:15, Sahid Orentino Ferdjaoui
<sahid.ferdjaoui@xxxxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> bpftool is now totally compliant with libbpf 1.0 mode and is not
> expected to be compiled with pre-1.0, let's clean-up the usage of
> libbpf_get_error().
>
> The changes stay aligned with returned errors always negative.
>
> - In tools/bpf/bpftool/btf.c This fixes an unintialized local variable
> `err` in function do_dump() because it may now be returned without
> having been set.
> - This also removes the checks on NULL pointers before calling
> btf__free() because that function already does the check.
>
> Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@xxxxxxxxxxxxxxxxxxxxxxxx>
> ---
> tools/bpf/bpftool/btf.c | 19 ++++++++-----------
> tools/bpf/bpftool/btf_dumper.c | 2 +-
> tools/bpf/bpftool/gen.c | 11 ++++-------
> tools/bpf/bpftool/iter.c | 6 ++----
> tools/bpf/bpftool/main.c | 7 +++----
> tools/bpf/bpftool/map.c | 15 +++++++--------
> tools/bpf/bpftool/prog.c | 10 +++++-----
> tools/bpf/bpftool/struct_ops.c | 12 ++++++------
> 8 files changed, 36 insertions(+), 46 deletions(-)
>
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index b87e4a7fd689..352290ba7b29 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -467,9 +467,8 @@ static int dump_btf_c(const struct btf *btf,
> int err = 0, i;
>
> d = btf_dump__new(btf, btf_dump_printf, NULL, NULL);
> - err = libbpf_get_error(d);
> - if (err)
> - return err;
> + if (!d)
> + return -errno;
>
> printf("#ifndef __VMLINUX_H__\n");
> printf("#define __VMLINUX_H__\n");
> @@ -512,11 +511,9 @@ static struct btf *get_vmlinux_btf_from_sysfs(void)
> struct btf *base;
>
> base = btf__parse(sysfs_vmlinux, NULL);
> - if (libbpf_get_error(base)) {
> - p_err("failed to parse vmlinux BTF at '%s': %ld\n",
> - sysfs_vmlinux, libbpf_get_error(base));
> - base = NULL;
> - }
> + if (!base)
> + p_err("failed to parse vmlinux BTF at '%s': %d\n",
> + sysfs_vmlinux, -errno);
>
> return base;
> }
> @@ -559,7 +556,7 @@ static int do_dump(int argc, char **argv)
> __u32 btf_id = -1;
> const char *src;
> int fd = -1;
> - int err;
> + int err = 0;
>
> if (!REQ_ARGS(2)) {
> usage();
> @@ -634,8 +631,8 @@ static int do_dump(int argc, char **argv)
> base = get_vmlinux_btf_from_sysfs();
>
> btf = btf__parse_split(*argv, base ?: base_btf);
> - err = libbpf_get_error(btf);
> if (!btf) {
> + err = -errno;
> p_err("failed to load BTF from %s: %s",
> *argv, strerror(errno));
> goto done;
> @@ -681,8 +678,8 @@ static int do_dump(int argc, char **argv)
> }
>
> btf = btf__load_from_kernel_by_id_split(btf_id, base_btf);
> - err = libbpf_get_error(btf);
> if (!btf) {
> + err = -errno;
> p_err("get btf by id (%u): %s", btf_id, strerror(errno));
> goto done;
> }
> diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c
> index 19924b6ce796..eda71fdfe95a 100644
> --- a/tools/bpf/bpftool/btf_dumper.c
> +++ b/tools/bpf/bpftool/btf_dumper.c
> @@ -75,7 +75,7 @@ static int dump_prog_id_as_func_ptr(const struct btf_dumper *d,
> goto print;
>
> prog_btf = btf__load_from_kernel_by_id(info.btf_id);
> - if (libbpf_get_error(prog_btf))
> + if (!prog_btf)
> goto print;
> func_type = btf__type_by_id(prog_btf, finfo.type_id);
> if (!func_type || !btf_is_func(func_type))
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index 01bb8d8f5568..5c68b0983491 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -252,9 +252,8 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name)
> int err = 0;
>
> d = btf_dump__new(btf, codegen_btf_dump_printf, NULL, NULL);
> - err = libbpf_get_error(d);
> - if (err)
> - return err;
> + if (!d)
> + return -errno;
>
> bpf_object__for_each_map(map, obj) {
> /* only generate definitions for memory-mapped internal maps */
> @@ -976,13 +975,11 @@ static int do_skeleton(int argc, char **argv)
> /* log_level1 + log_level2 + stats, but not stable UAPI */
> opts.kernel_log_level = 1 + 2 + 4;
> obj = bpf_object__open_mem(obj_data, file_sz, &opts);
> - err = libbpf_get_error(obj);
> - if (err) {
> + if (!obj) {
> char err_buf[256];
>
> - libbpf_strerror(err, err_buf, sizeof(err_buf));
> + libbpf_strerror(errno, err_buf, sizeof(err_buf));
> p_err("failed to open BPF object file: %s", err_buf);
> - obj = NULL;
> goto out;
> }
>
> diff --git a/tools/bpf/bpftool/iter.c b/tools/bpf/bpftool/iter.c
> index a3e6b167153d..ab6f1b2befe7 100644
> --- a/tools/bpf/bpftool/iter.c
> +++ b/tools/bpf/bpftool/iter.c
> @@ -48,8 +48,7 @@ static int do_pin(int argc, char **argv)
> }
>
> obj = bpf_object__open(objfile);
> - err = libbpf_get_error(obj);
> - if (err) {
> + if (!obj) {
We could maybe set err to -errno here, so the function returns it
instead of the default -1 on error. Doesn't matter too much because
the return value is not used by the caller (other than to compare it
to 0), but it would be more consistent with the surrounding checks in
my opinion.
> p_err("can't open objfile %s", objfile);
> goto close_map_fd;
> }
> @@ -67,8 +66,7 @@ static int do_pin(int argc, char **argv)
> }
>
> link = bpf_program__attach_iter(prog, &iter_opts);
> - err = libbpf_get_error(link);
> - if (err) {
> + if (!link) {
Same
> p_err("attach_iter failed for program %s",
> bpf_program__name(prog));
> goto close_obj;
> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index 87ceafa4b9b8..da43ba596610 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -510,10 +510,9 @@ int main(int argc, char **argv)
> break;
> case 'B':
> base_btf = btf__parse(optarg, NULL);
> - if (libbpf_get_error(base_btf)) {
> - p_err("failed to parse base BTF at '%s': %ld\n",
> - optarg, libbpf_get_error(base_btf));
> - base_btf = NULL;
> + if (!base_btf) {
> + p_err("failed to parse base BTF at '%s': %d\n",
> + optarg, errno);
You fixed the errno -> -errno occurrences reported by Andrii, but you
have a few remaining: here...
> return -1;
> }
> break;
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index d884070a2314..26d4022ec374 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -786,18 +786,18 @@ static int get_map_kv_btf(const struct bpf_map_info *info, struct btf **btf)
> if (info->btf_vmlinux_value_type_id) {
> if (!btf_vmlinux) {
> btf_vmlinux = libbpf_find_kernel_btf();
> - err = libbpf_get_error(btf_vmlinux);
> - if (err) {
> + if (!btf_vmlinux) {
> p_err("failed to get kernel btf");
> - return err;
> + return errno;
... and here...
> }
> }
> *btf = btf_vmlinux;
> } else if (info->btf_value_type_id) {
> *btf = btf__load_from_kernel_by_id(info->btf_id);
> - err = libbpf_get_error(*btf);
> - if (err)
> + if (!*btf) {
> + err = errno;
... and here. Please double-check in case I missed some too.
> p_err("failed to get btf");
> + }
> } else {
> *btf = NULL;
> }
> @@ -807,14 +807,13 @@ static int get_map_kv_btf(const struct bpf_map_info *info, struct btf **btf)
>
> static void free_map_kv_btf(struct btf *btf)
> {
> - if (!libbpf_get_error(btf) && btf != btf_vmlinux)
> + if (btf != btf_vmlinux)
> btf__free(btf);
> }
>
> static void free_btf_vmlinux(void)
> {
> - if (!libbpf_get_error(btf_vmlinux))
> - btf__free(btf_vmlinux);
> + btf__free(btf_vmlinux);
> }
>
> static int
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 2266958f203f..cfc9fdc1e863 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -322,7 +322,7 @@ static void show_prog_metadata(int fd, __u32 num_maps)
> return;
>
> btf = btf__load_from_kernel_by_id(map_info.btf_id);
> - if (libbpf_get_error(btf))
> + if (!btf)
> goto out_free;
>
> t_datasec = btf__type_by_id(btf, map_info.btf_value_type_id);
> @@ -726,7 +726,7 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
>
> if (info->btf_id) {
> btf = btf__load_from_kernel_by_id(info->btf_id);
> - if (libbpf_get_error(btf)) {
> + if (!btf) {
> p_err("failed to get btf");
> return -1;
> }
> @@ -1663,7 +1663,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> open_opts.kernel_log_level = 1 + 2 + 4;
>
> obj = bpf_object__open_file(file, &open_opts);
> - if (libbpf_get_error(obj)) {
> + if (!obj) {
> p_err("failed to open object file");
> goto err_free_reuse_maps;
> }
> @@ -1882,7 +1882,7 @@ static int do_loader(int argc, char **argv)
> open_opts.kernel_log_level = 1 + 2 + 4;
>
> obj = bpf_object__open_file(file, &open_opts);
> - if (libbpf_get_error(obj)) {
> + if (!obj) {
> p_err("failed to open object file");
> goto err_close_obj;
> }
> @@ -2199,7 +2199,7 @@ static char *profile_target_name(int tgt_fd)
> }
>
> btf = btf__load_from_kernel_by_id(info.btf_id);
> - if (libbpf_get_error(btf)) {
> + if (!btf) {
> p_err("failed to load btf for prog FD %d", tgt_fd);
> goto out;
> }
> diff --git a/tools/bpf/bpftool/struct_ops.c b/tools/bpf/bpftool/struct_ops.c
> index a6c6d5b9551e..903b80ff4e9a 100644
> --- a/tools/bpf/bpftool/struct_ops.c
> +++ b/tools/bpf/bpftool/struct_ops.c
> @@ -32,7 +32,7 @@ static const struct btf *get_btf_vmlinux(void)
> return btf_vmlinux;
>
> btf_vmlinux = libbpf_find_kernel_btf();
> - if (libbpf_get_error(btf_vmlinux))
> + if (!btf_vmlinux)
> p_err("struct_ops requires kernel CONFIG_DEBUG_INFO_BTF=y");
>
> return btf_vmlinux;
> @@ -45,7 +45,7 @@ static const char *get_kern_struct_ops_name(const struct bpf_map_info *info)
> const char *st_ops_name;
>
> kern_btf = get_btf_vmlinux();
> - if (libbpf_get_error(kern_btf))
> + if (!kern_btf)
> return "<btf_vmlinux_not_found>";
>
> t = btf__type_by_id(kern_btf, info->btf_vmlinux_value_type_id);
> @@ -62,6 +62,7 @@ static __s32 get_map_info_type_id(void)
> if (map_info_type_id)
> return map_info_type_id;
>
> + kern_btf = get_btf_vmlinux();
Looks like that line you removed by mistake in a previous patch?
> if (!kern_btf)
> return 0;
>
> @@ -412,7 +413,7 @@ static int do_dump(int argc, char **argv)
> }
>
> kern_btf = get_btf_vmlinux();
> - if (libbpf_get_error(kern_btf))
> + if (!kern_btf)
> return -1;
>
> if (!json_output) {
> @@ -495,7 +496,7 @@ static int do_register(int argc, char **argv)
> open_opts.kernel_log_level = 1 + 2 + 4;
>
> obj = bpf_object__open_file(file, &open_opts);
> - if (libbpf_get_error(obj))
> + if (!obj)
> return -1;
>
> set_max_rlimit();
> @@ -589,8 +590,7 @@ int do_struct_ops(int argc, char **argv)
>
> err = cmd_select(cmds, argc, argv, do_help);
>
> - if (!libbpf_get_error(btf_vmlinux))
> - btf__free(btf_vmlinux);
> + btf__free(btf_vmlinux);
>
> return err;
> }
> --
> 2.34.1
>
>
