[PATCH bpf-next v2 0/5] libbpf: rename btf__get_from_id() and btf__load() APIs, support split BTF

[PATCH bpf-next v2 0/5] libbpf: rename btf__get_from_id() and btf__load() APIs, support split BTF

[Quentin Monnet]

As part of the effort to move towards a v1.0 for libbpf [0], this set
improves some confusing function names related to BTF loading from and to
the kernel:

- btf__load() becomes btf__load_into_kernel().
- btf__get_from_id becomes btf__load_from_kernel_by_id().
- A new version btf__load_from_kernel_by_id_split() extends the former to
  add support for split BTF.

The old functions are not removed or marked as deprecated yet, there
should be in a future libbpf version.

The last patch is a trivial change to bpftool to add support for dumping
split BTF objects by referencing them by their id (and not only by their
BTF path).

[0] https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis

v2:
- Remove deprecation marking of legacy functions (patch 4/6 from v1).
- Make btf__load_from_kernel_by_id{,_split}() return the btf struct.
- Add new functions to v0.5.0 API (and not v0.6.0).

Quentin Monnet (5):
  libbpf: rename btf__load() as btf__load_into_kernel()
  libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id()
  tools: replace btf__get_from_id() with btf__load_from_kernel_by_id()
  libbpf: add split BTF support for btf__load_from_kernel_by_id()
  tools: bpftool: support dumping split BTF by id

 tools/bpf/bpftool/btf.c                      |  8 ++---
 tools/bpf/bpftool/btf_dumper.c               |  6 ++--
 tools/bpf/bpftool/map.c                      | 16 +++++-----
 tools/bpf/bpftool/prog.c                     | 29 +++++++++++------
 tools/lib/bpf/btf.c                          | 33 ++++++++++++++------
 tools/lib/bpf/btf.h                          |  4 +++
 tools/lib/bpf/libbpf.c                       |  7 +++--
 tools/lib/bpf/libbpf.map                     |  3 ++
 tools/perf/util/bpf-event.c                  | 11 ++++---
 tools/perf/util/bpf_counter.c                | 12 +++++--
 tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
 11 files changed, 86 insertions(+), 47 deletions(-)

-- 
2.30.2

[PATCH bpf-next v2 1/5] libbpf: rename btf__load() as btf__load_into_kernel()

[Quentin Monnet]

As part of the effort to move towards a v1.0 for libbpf, rename
btf__load() function, used to "upload" BTF information into the kernel,
as btf__load_into_kernel(). This new name better reflects what the
function does.

References:

- https://github.com/libbpf/libbpf/issues/278
- https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis

v2: Declare the new symbol in libbpf.map as v0.5.0 API (and not v0.6.0).

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Acked-by: John Fastabend <john.fastabend@gmail.com>
---
 tools/lib/bpf/btf.c      | 3 ++-
 tools/lib/bpf/btf.h      | 1 +
 tools/lib/bpf/libbpf.c   | 2 +-
 tools/lib/bpf/libbpf.map | 1 +
 4 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index b46760b93bb4..7e0de560490e 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -1180,7 +1180,7 @@ int btf__finalize_data(struct bpf_object *obj, struct btf *btf)
 
 static void *btf_get_raw_data(const struct btf *btf, __u32 *size, bool swap_endian);
 
-int btf__load(struct btf *btf)
+int btf__load_into_kernel(struct btf *btf)
 {
 	__u32 log_buf_size = 0, raw_size;
 	char *log_buf = NULL;
@@ -1228,6 +1228,7 @@ int btf__load(struct btf *btf)
 	free(log_buf);
 	return libbpf_err(err);
 }
+int btf__load(struct btf *) __attribute__((alias("btf__load_into_kernel")));
 
 int btf__fd(const struct btf *btf)
 {
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 374e9f15de2e..fd8a21d936ef 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -46,6 +46,7 @@ LIBBPF_API struct btf *btf__parse_raw_split(const char *path, struct btf *base_b
 
 LIBBPF_API int btf__finalize_data(struct bpf_object *obj, struct btf *btf);
 LIBBPF_API int btf__load(struct btf *btf);
+LIBBPF_API int btf__load_into_kernel(struct btf *btf);
 LIBBPF_API __s32 btf__find_by_name(const struct btf *btf,
 				   const char *type_name);
 LIBBPF_API __s32 btf__find_by_name_kind(const struct btf *btf,
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 4c153c379989..242e97892043 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -2774,7 +2774,7 @@ static int bpf_object__sanitize_and_load_btf(struct bpf_object *obj)
 		 */
 		btf__set_fd(kern_btf, 0);
 	} else {
-		err = btf__load(kern_btf);
+		err = btf__load_into_kernel(kern_btf);
 	}
 	if (sanitize) {
 		if (!err) {
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 5bfc10722647..f7d52d76ca3a 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -373,6 +373,7 @@ LIBBPF_0.5.0 {
 		bpf_map__initial_value;
 		bpf_map_lookup_and_delete_elem_flags;
 		bpf_object__gen_loader;
+		btf__load_into_kernel;
 		btf_dump__dump_type_data;
 		libbpf_set_strict_mode;
 } LIBBPF_0.4.0;
-- 
2.30.2

[PATCH bpf-next v2 2/5] libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id()

[Quentin Monnet]

Rename function btf__get_from_id() as btf__load_from_kernel_by_id() to
better indicate what the function does. Change the new function so that,
instead of requiring a pointer to the pointer to update and returning
with an error code, it takes a single argument (the id of the BTF
object) and returns the corresponding pointer. This is more in line with
the existing constructors.

The other tools calling the deprecated btf__get_from_id() function will
be updated in a future commit.

References:

- https://github.com/libbpf/libbpf/issues/278
- https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis

v2:
- Instead of a simple renaming, change the new function to make it
  return the pointer to the btf struct.
- API v0.5.0 instead of v0.6.0.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Acked-by: John Fastabend <john.fastabend@gmail.com>
---
 tools/lib/bpf/btf.c      | 25 +++++++++++++++++--------
 tools/lib/bpf/btf.h      |  1 +
 tools/lib/bpf/libbpf.c   |  5 +++--
 tools/lib/bpf/libbpf.map |  1 +
 4 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 7e0de560490e..6654bdee7ad7 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -1383,21 +1383,30 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
 	return btf;
 }
 
+struct btf *btf__load_from_kernel_by_id(__u32 id)
+{
+	struct btf *btf;
+	int btf_fd;
+
+	btf_fd = bpf_btf_get_fd_by_id(id);
+	if (btf_fd < 0)
+		return ERR_PTR(-errno);
+
+	btf = btf_get_from_fd(btf_fd, NULL);
+	close(btf_fd);
+
+	return libbpf_ptr(btf);
+}
+
 int btf__get_from_id(__u32 id, struct btf **btf)
 {
 	struct btf *res;
-	int err, btf_fd;
+	int err;
 
 	*btf = NULL;
-	btf_fd = bpf_btf_get_fd_by_id(id);
-	if (btf_fd < 0)
-		return libbpf_err(-errno);
-
-	res = btf_get_from_fd(btf_fd, NULL);
+	res = btf__load_from_kernel_by_id(id);
 	err = libbpf_get_error(res);
 
-	close(btf_fd);
-
 	if (err)
 		return libbpf_err(err);
 
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index fd8a21d936ef..3db9446bc133 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -68,6 +68,7 @@ LIBBPF_API const void *btf__get_raw_data(const struct btf *btf, __u32 *size);
 LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
 LIBBPF_API const char *btf__str_by_offset(const struct btf *btf, __u32 offset);
 LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
+LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id);
 LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name,
 				    __u32 expected_key_size,
 				    __u32 expected_value_size,
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 242e97892043..eff005b1eba1 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -9576,8 +9576,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
 {
 	struct bpf_prog_info_linear *info_linear;
 	struct bpf_prog_info *info;
-	struct btf *btf = NULL;
 	int err = -EINVAL;
+	struct btf *btf;
 
 	info_linear = bpf_program__get_prog_info_linear(attach_prog_fd, 0);
 	err = libbpf_get_error(info_linear);
@@ -9591,7 +9591,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
 		pr_warn("The target program doesn't have BTF\n");
 		goto out;
 	}
-	if (btf__get_from_id(info->btf_id, &btf)) {
+	btf = btf__load_from_kernel_by_id(info->btf_id);
+	if (libbpf_get_error(btf)) {
 		pr_warn("Failed to get BTF of the program\n");
 		goto out;
 	}
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index f7d52d76ca3a..ca8cc7a7faad 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -373,6 +373,7 @@ LIBBPF_0.5.0 {
 		bpf_map__initial_value;
 		bpf_map_lookup_and_delete_elem_flags;
 		bpf_object__gen_loader;
+		btf__load_from_kernel_by_id;
 		btf__load_into_kernel;
 		btf_dump__dump_type_data;
 		libbpf_set_strict_mode;
-- 
2.30.2

Re: [PATCH bpf-next v2 2/5] libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id()

[Andrii Nakryiko]

On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> Rename function btf__get_from_id() as btf__load_from_kernel_by_id() to
> better indicate what the function does. Change the new function so that,
> instead of requiring a pointer to the pointer to update and returning
> with an error code, it takes a single argument (the id of the BTF
> object) and returns the corresponding pointer. This is more in line with
> the existing constructors.
>
> The other tools calling the deprecated btf__get_from_id() function will
> be updated in a future commit.
>
> References:
>
> - https://github.com/libbpf/libbpf/issues/278
> - https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis
>
> v2:
> - Instead of a simple renaming, change the new function to make it
>   return the pointer to the btf struct.
> - API v0.5.0 instead of v0.6.0.

We generally keep such version changes to cover letters. It keeps each
individual commit clean and collects full history in the cover letter
which becomes a body of merge commit when the whole patch set is
applied. For next revision please consolidate the history in the cover
letter. Thanks!

>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> Acked-by: John Fastabend <john.fastabend@gmail.com>
> ---
>  tools/lib/bpf/btf.c      | 25 +++++++++++++++++--------
>  tools/lib/bpf/btf.h      |  1 +
>  tools/lib/bpf/libbpf.c   |  5 +++--
>  tools/lib/bpf/libbpf.map |  1 +
>  4 files changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 7e0de560490e..6654bdee7ad7 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -1383,21 +1383,30 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
>         return btf;
>  }
>
> +struct btf *btf__load_from_kernel_by_id(__u32 id)
> +{
> +       struct btf *btf;
> +       int btf_fd;
> +
> +       btf_fd = bpf_btf_get_fd_by_id(id);
> +       if (btf_fd < 0)
> +               return ERR_PTR(-errno);

please use libbpf_err_ptr() for consistency, see
bpf_object__open_mem() for an example

> +
> +       btf = btf_get_from_fd(btf_fd, NULL);
> +       close(btf_fd);
> +
> +       return libbpf_ptr(btf);
> +}
> +
>  int btf__get_from_id(__u32 id, struct btf **btf)
>  {
>         struct btf *res;
> -       int err, btf_fd;
> +       int err;
>
>         *btf = NULL;
> -       btf_fd = bpf_btf_get_fd_by_id(id);
> -       if (btf_fd < 0)
> -               return libbpf_err(-errno);
> -
> -       res = btf_get_from_fd(btf_fd, NULL);
> +       res = btf__load_from_kernel_by_id(id);
>         err = libbpf_get_error(res);
>
> -       close(btf_fd);
> -
>         if (err)
>                 return libbpf_err(err);
>
> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index fd8a21d936ef..3db9446bc133 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -68,6 +68,7 @@ LIBBPF_API const void *btf__get_raw_data(const struct btf *btf, __u32 *size);
>  LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
>  LIBBPF_API const char *btf__str_by_offset(const struct btf *btf, __u32 offset);
>  LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
> +LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id);

let's move this definition to after btf__parse() to keep all
"constructors" together (we can move btf__get_from_id() there for
completeness as well, I suppose).

>  LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name,
>                                     __u32 expected_key_size,
>                                     __u32 expected_value_size,
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 242e97892043..eff005b1eba1 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -9576,8 +9576,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
>  {
>         struct bpf_prog_info_linear *info_linear;
>         struct bpf_prog_info *info;
> -       struct btf *btf = NULL;
>         int err = -EINVAL;
> +       struct btf *btf;
>
>         info_linear = bpf_program__get_prog_info_linear(attach_prog_fd, 0);
>         err = libbpf_get_error(info_linear);
> @@ -9591,7 +9591,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
>                 pr_warn("The target program doesn't have BTF\n");
>                 goto out;
>         }
> -       if (btf__get_from_id(info->btf_id, &btf)) {
> +       btf = btf__load_from_kernel_by_id(info->btf_id);
> +       if (libbpf_get_error(btf)) {

there seems to be a bug in existing code and you are keeping it. On
error err will be 0. Let's fix it. Same for above if (!info->btf_id),
please fix that as well while you are at it.

>                 pr_warn("Failed to get BTF of the program\n");
>                 goto out;
>         }
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index f7d52d76ca3a..ca8cc7a7faad 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -373,6 +373,7 @@ LIBBPF_0.5.0 {
>                 bpf_map__initial_value;
>                 bpf_map_lookup_and_delete_elem_flags;
>                 bpf_object__gen_loader;
> +               btf__load_from_kernel_by_id;
>                 btf__load_into_kernel;
>                 btf_dump__dump_type_data;
>                 libbpf_set_strict_mode;
> --
> 2.30.2
>

Re: [PATCH bpf-next v2 2/5] libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id()

[Quentin Monnet]

2021-07-22 17:39 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> Rename function btf__get_from_id() as btf__load_from_kernel_by_id() to
>> better indicate what the function does. Change the new function so that,
>> instead of requiring a pointer to the pointer to update and returning
>> with an error code, it takes a single argument (the id of the BTF
>> object) and returns the corresponding pointer. This is more in line with
>> the existing constructors.
>>
>> The other tools calling the deprecated btf__get_from_id() function will
>> be updated in a future commit.
>>
>> References:
>>
>> - https://github.com/libbpf/libbpf/issues/278
>> - https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis
>>
>> v2:
>> - Instead of a simple renaming, change the new function to make it
>>   return the pointer to the btf struct.
>> - API v0.5.0 instead of v0.6.0.
> 
> We generally keep such version changes to cover letters. It keeps each
> individual commit clean and collects full history in the cover letter
> which becomes a body of merge commit when the whole patch set is
> applied. For next revision please consolidate the history in the cover
> letter. Thanks!

OK will do.
I've seen other folks detailing the changes on individual patches, and
done so in the past, although it's true the current trend is to have it
in the cover letter (and I understand the motivation).

> 
>>
>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
>> Acked-by: John Fastabend <john.fastabend@gmail.com>
>> ---
>>  tools/lib/bpf/btf.c      | 25 +++++++++++++++++--------
>>  tools/lib/bpf/btf.h      |  1 +
>>  tools/lib/bpf/libbpf.c   |  5 +++--
>>  tools/lib/bpf/libbpf.map |  1 +
>>  4 files changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
>> index 7e0de560490e..6654bdee7ad7 100644
>> --- a/tools/lib/bpf/btf.c
>> +++ b/tools/lib/bpf/btf.c
>> @@ -1383,21 +1383,30 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
>>         return btf;
>>  }
>>
>> +struct btf *btf__load_from_kernel_by_id(__u32 id)
>> +{
>> +       struct btf *btf;
>> +       int btf_fd;
>> +
>> +       btf_fd = bpf_btf_get_fd_by_id(id);
>> +       if (btf_fd < 0)
>> +               return ERR_PTR(-errno);
> 
> please use libbpf_err_ptr() for consistency, see
> bpf_object__open_mem() for an example

I can do that, but I'll need to uncouple btf__get_from_id() from the new
function. If it calls btf__load_from_kernel_by_id() and
LIBBPF_STRICT_CLEAN_PTRS is set, it would change its return value.

> 
>> +
>> +       btf = btf_get_from_fd(btf_fd, NULL);
>> +       close(btf_fd);
>> +
>> +       return libbpf_ptr(btf);
>> +}
>> +
>>  int btf__get_from_id(__u32 id, struct btf **btf)
>>  {
>>         struct btf *res;
>> -       int err, btf_fd;
>> +       int err;
>>
>>         *btf = NULL;
>> -       btf_fd = bpf_btf_get_fd_by_id(id);
>> -       if (btf_fd < 0)
>> -               return libbpf_err(-errno);
>> -
>> -       res = btf_get_from_fd(btf_fd, NULL);
>> +       res = btf__load_from_kernel_by_id(id);
>>         err = libbpf_get_error(res);
>>
>> -       close(btf_fd);
>> -
>>         if (err)
>>                 return libbpf_err(err);
>>
>> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
>> index fd8a21d936ef..3db9446bc133 100644
>> --- a/tools/lib/bpf/btf.h
>> +++ b/tools/lib/bpf/btf.h
>> @@ -68,6 +68,7 @@ LIBBPF_API const void *btf__get_raw_data(const struct btf *btf, __u32 *size);
>>  LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
>>  LIBBPF_API const char *btf__str_by_offset(const struct btf *btf, __u32 offset);
>>  LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
>> +LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id);
> 
> let's move this definition to after btf__parse() to keep all
> "constructors" together (we can move btf__get_from_id() there for
> completeness as well, I suppose).

I thought about that but wasn't sure, OK will do.

> 
>>  LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name,
>>                                     __u32 expected_key_size,
>>                                     __u32 expected_value_size,
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 242e97892043..eff005b1eba1 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -9576,8 +9576,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
>>  {
>>         struct bpf_prog_info_linear *info_linear;
>>         struct bpf_prog_info *info;
>> -       struct btf *btf = NULL;
>>         int err = -EINVAL;
>> +       struct btf *btf;
>>
>>         info_linear = bpf_program__get_prog_info_linear(attach_prog_fd, 0);
>>         err = libbpf_get_error(info_linear);
>> @@ -9591,7 +9591,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
>>                 pr_warn("The target program doesn't have BTF\n");
>>                 goto out;
>>         }
>> -       if (btf__get_from_id(info->btf_id, &btf)) {
>> +       btf = btf__load_from_kernel_by_id(info->btf_id);
>> +       if (libbpf_get_error(btf)) {
> 
> there seems to be a bug in existing code and you are keeping it. On
> error err will be 0. Let's fix it. Same for above if (!info->btf_id),
> please fix that as well while you are at it.

Oh right, I saw that err was initialised at -EINVAL and did not notice
it was changed for the info_linear. I'll address it.

Re: [PATCH bpf-next v2 2/5] libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id()

[Andrii Nakryiko]

On Fri, Jul 23, 2021 at 2:31 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> 2021-07-22 17:39 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>
> >> Rename function btf__get_from_id() as btf__load_from_kernel_by_id() to
> >> better indicate what the function does. Change the new function so that,
> >> instead of requiring a pointer to the pointer to update and returning
> >> with an error code, it takes a single argument (the id of the BTF
> >> object) and returns the corresponding pointer. This is more in line with
> >> the existing constructors.
> >>
> >> The other tools calling the deprecated btf__get_from_id() function will
> >> be updated in a future commit.
> >>
> >> References:
> >>
> >> - https://github.com/libbpf/libbpf/issues/278
> >> - https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis
> >>
> >> v2:
> >> - Instead of a simple renaming, change the new function to make it
> >>   return the pointer to the btf struct.
> >> - API v0.5.0 instead of v0.6.0.
> >
> > We generally keep such version changes to cover letters. It keeps each
> > individual commit clean and collects full history in the cover letter
> > which becomes a body of merge commit when the whole patch set is
> > applied. For next revision please consolidate the history in the cover
> > letter. Thanks!
>
> OK will do.
> I've seen other folks detailing the changes on individual patches, and
> done so in the past, although it's true the current trend is to have it
> in the cover letter (and I understand the motivation).
>
> >
> >>
> >> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> >> Acked-by: John Fastabend <john.fastabend@gmail.com>
> >> ---
> >>  tools/lib/bpf/btf.c      | 25 +++++++++++++++++--------
> >>  tools/lib/bpf/btf.h      |  1 +
> >>  tools/lib/bpf/libbpf.c   |  5 +++--
> >>  tools/lib/bpf/libbpf.map |  1 +
> >>  4 files changed, 22 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> >> index 7e0de560490e..6654bdee7ad7 100644
> >> --- a/tools/lib/bpf/btf.c
> >> +++ b/tools/lib/bpf/btf.c
> >> @@ -1383,21 +1383,30 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
> >>         return btf;
> >>  }
> >>
> >> +struct btf *btf__load_from_kernel_by_id(__u32 id)
> >> +{
> >> +       struct btf *btf;
> >> +       int btf_fd;
> >> +
> >> +       btf_fd = bpf_btf_get_fd_by_id(id);
> >> +       if (btf_fd < 0)
> >> +               return ERR_PTR(-errno);
> >
> > please use libbpf_err_ptr() for consistency, see
> > bpf_object__open_mem() for an example
>
> I can do that, but I'll need to uncouple btf__get_from_id() from the new
> function. If it calls btf__load_from_kernel_by_id() and
> LIBBPF_STRICT_CLEAN_PTRS is set, it would change its return value.

No it won't, if libbpf_get_error() is used right after the API call.
With CLEAN_PTRS the result pointer is NULL but actual error is passed
through errno. libbpf_get_error() knows about this and extracts error
from errno if passed NULL pointer. With returning ERR_PTR(-errno) from
btf__load_from_kernel_by_id() you are breaking CLEAN_PTRS guarantees.

>
> >
> >> +
> >> +       btf = btf_get_from_fd(btf_fd, NULL);
> >> +       close(btf_fd);
> >> +
> >> +       return libbpf_ptr(btf);
> >> +}
> >> +
> >>  int btf__get_from_id(__u32 id, struct btf **btf)
> >>  {
> >>         struct btf *res;
> >> -       int err, btf_fd;
> >> +       int err;
> >>
> >>         *btf = NULL;
> >> -       btf_fd = bpf_btf_get_fd_by_id(id);
> >> -       if (btf_fd < 0)
> >> -               return libbpf_err(-errno);
> >> -
> >> -       res = btf_get_from_fd(btf_fd, NULL);
> >> +       res = btf__load_from_kernel_by_id(id);
> >>         err = libbpf_get_error(res);
> >>
> >> -       close(btf_fd);
> >> -
> >>         if (err)
> >>                 return libbpf_err(err);
> >>
> >> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> >> index fd8a21d936ef..3db9446bc133 100644
> >> --- a/tools/lib/bpf/btf.h
> >> +++ b/tools/lib/bpf/btf.h
> >> @@ -68,6 +68,7 @@ LIBBPF_API const void *btf__get_raw_data(const struct btf *btf, __u32 *size);
> >>  LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
> >>  LIBBPF_API const char *btf__str_by_offset(const struct btf *btf, __u32 offset);
> >>  LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
> >> +LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id);
> >
> > let's move this definition to after btf__parse() to keep all
> > "constructors" together (we can move btf__get_from_id() there for
> > completeness as well, I suppose).
>
> I thought about that but wasn't sure, OK will do.

Ok, thanks.

>
> >
> >>  LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name,
> >>                                     __u32 expected_key_size,
> >>                                     __u32 expected_value_size,
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index 242e97892043..eff005b1eba1 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -9576,8 +9576,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
> >>  {
> >>         struct bpf_prog_info_linear *info_linear;
> >>         struct bpf_prog_info *info;
> >> -       struct btf *btf = NULL;
> >>         int err = -EINVAL;
> >> +       struct btf *btf;
> >>
> >>         info_linear = bpf_program__get_prog_info_linear(attach_prog_fd, 0);
> >>         err = libbpf_get_error(info_linear);
> >> @@ -9591,7 +9591,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
> >>                 pr_warn("The target program doesn't have BTF\n");
> >>                 goto out;
> >>         }
> >> -       if (btf__get_from_id(info->btf_id, &btf)) {
> >> +       btf = btf__load_from_kernel_by_id(info->btf_id);
> >> +       if (libbpf_get_error(btf)) {
> >
> > there seems to be a bug in existing code and you are keeping it. On
> > error err will be 0. Let's fix it. Same for above if (!info->btf_id),
> > please fix that as well while you are at it.
>
> Oh right, I saw that err was initialised at -EINVAL and did not notice
> it was changed for the info_linear. I'll address it.

cool, thanks

Re: [PATCH bpf-next v2 2/5] libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id()

[Quentin Monnet]

2021-07-23 08:54 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Fri, Jul 23, 2021 at 2:31 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> 2021-07-22 17:39 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
>>> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>>>
>>>> Rename function btf__get_from_id() as btf__load_from_kernel_by_id() to
>>>> better indicate what the function does. Change the new function so that,
>>>> instead of requiring a pointer to the pointer to update and returning
>>>> with an error code, it takes a single argument (the id of the BTF
>>>> object) and returns the corresponding pointer. This is more in line with
>>>> the existing constructors.
>>>>
>>>> The other tools calling the deprecated btf__get_from_id() function will
>>>> be updated in a future commit.
>>>>
>>>> References:
>>>>
>>>> - https://github.com/libbpf/libbpf/issues/278
>>>> - https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis
>>>>

>>>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
>>>> index 7e0de560490e..6654bdee7ad7 100644
>>>> --- a/tools/lib/bpf/btf.c
>>>> +++ b/tools/lib/bpf/btf.c
>>>> @@ -1383,21 +1383,30 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
>>>>         return btf;
>>>>  }
>>>>
>>>> +struct btf *btf__load_from_kernel_by_id(__u32 id)
>>>> +{
>>>> +       struct btf *btf;
>>>> +       int btf_fd;
>>>> +
>>>> +       btf_fd = bpf_btf_get_fd_by_id(id);
>>>> +       if (btf_fd < 0)
>>>> +               return ERR_PTR(-errno);
>>>
>>> please use libbpf_err_ptr() for consistency, see
>>> bpf_object__open_mem() for an example
>>
>> I can do that, but I'll need to uncouple btf__get_from_id() from the new
>> function. If it calls btf__load_from_kernel_by_id() and
>> LIBBPF_STRICT_CLEAN_PTRS is set, it would change its return value.
> 
> No it won't, if libbpf_get_error() is used right after the API call.

But we cannot be sure that users currently call libbpf_get_error() after
btf__get_from_id()? I'm fine if we assume they do (users currently
selecting the CLEAN_PTRS are probably savvy enough to call it I guess),
I'll update as you suggest.

> With CLEAN_PTRS the result pointer is NULL but actual error is passed
> through errno. libbpf_get_error() knows about this and extracts error
> from errno if passed NULL pointer. With returning ERR_PTR(-errno) from
> btf__load_from_kernel_by_id() you are breaking CLEAN_PTRS guarantees.
OK right, this makes sense to me for btf__load_from_kernel_by_id().

Re: [PATCH bpf-next v2 2/5] libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id()

[Andrii Nakryiko]

On Fri, Jul 23, 2021 at 9:13 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> 2021-07-23 08:54 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > On Fri, Jul 23, 2021 at 2:31 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>
> >> 2021-07-22 17:39 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> >>> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>>>
> >>>> Rename function btf__get_from_id() as btf__load_from_kernel_by_id() to
> >>>> better indicate what the function does. Change the new function so that,
> >>>> instead of requiring a pointer to the pointer to update and returning
> >>>> with an error code, it takes a single argument (the id of the BTF
> >>>> object) and returns the corresponding pointer. This is more in line with
> >>>> the existing constructors.
> >>>>
> >>>> The other tools calling the deprecated btf__get_from_id() function will
> >>>> be updated in a future commit.
> >>>>
> >>>> References:
> >>>>
> >>>> - https://github.com/libbpf/libbpf/issues/278
> >>>> - https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis
> >>>>
>
> >>>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> >>>> index 7e0de560490e..6654bdee7ad7 100644
> >>>> --- a/tools/lib/bpf/btf.c
> >>>> +++ b/tools/lib/bpf/btf.c
> >>>> @@ -1383,21 +1383,30 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
> >>>>         return btf;
> >>>>  }
> >>>>
> >>>> +struct btf *btf__load_from_kernel_by_id(__u32 id)
> >>>> +{
> >>>> +       struct btf *btf;
> >>>> +       int btf_fd;
> >>>> +
> >>>> +       btf_fd = bpf_btf_get_fd_by_id(id);
> >>>> +       if (btf_fd < 0)
> >>>> +               return ERR_PTR(-errno);
> >>>
> >>> please use libbpf_err_ptr() for consistency, see
> >>> bpf_object__open_mem() for an example
> >>
> >> I can do that, but I'll need to uncouple btf__get_from_id() from the new
> >> function. If it calls btf__load_from_kernel_by_id() and
> >> LIBBPF_STRICT_CLEAN_PTRS is set, it would change its return value.
> >
> > No it won't, if libbpf_get_error() is used right after the API call.
>
> But we cannot be sure that users currently call libbpf_get_error() after
> btf__get_from_id()? I'm fine if we assume they do (users currently
> selecting the CLEAN_PTRS are probably savvy enough to call it I guess),
> I'll update as you suggest.

I think you are still confused. It doesn't matter what the user does,
the contract is for libbpf API to either return ERR_PTR(err) if no
CLEAN_PTRS is requested, or return NULL and set errno to -err.
libbpf_err_ptr() does that from inside the libbpf API (so you don't
have to check CLEAN_PTRS explicitly, you are just passing an error to
be returned, regardless of libbpf mode).

If a user opted into CLEAN_PTRS, they don't have to use
libbpf_get_error(), it's enough to check for NULL. If they care about
the error code itself, they'll need to use -errno. If they haven't
opted into CLEAN_PTRS yet, they have to use libbpf_get_error(), as
that's the only supported way. Sure, they could check for NULL and
that's a bug (and that's a very common one, which motivated
CLEAN_PTRS), or they implement the IS_ERR() macro from the kernel
(which is not officially supported, but works, of course). But again,
all that is orthogonal to how libbpf has to return errors from inside
for pointer-returning APIs.

>
> > With CLEAN_PTRS the result pointer is NULL but actual error is passed
> > through errno. libbpf_get_error() knows about this and extracts error
> > from errno if passed NULL pointer. With returning ERR_PTR(-errno) from
> > btf__load_from_kernel_by_id() you are breaking CLEAN_PTRS guarantees.
> OK right, this makes sense to me for btf__load_from_kernel_by_id().

Re: [PATCH bpf-next v2 2/5] libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id()

[Quentin Monnet]

2021-07-23 10:18 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Fri, Jul 23, 2021 at 9:13 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> 2021-07-23 08:54 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
>>> On Fri, Jul 23, 2021 at 2:31 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>>>
>>>> 2021-07-22 17:39 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
>>>>> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>>>>>
>>>>>> Rename function btf__get_from_id() as btf__load_from_kernel_by_id() to
>>>>>> better indicate what the function does. Change the new function so that,
>>>>>> instead of requiring a pointer to the pointer to update and returning
>>>>>> with an error code, it takes a single argument (the id of the BTF
>>>>>> object) and returns the corresponding pointer. This is more in line with
>>>>>> the existing constructors.
>>>>>>
>>>>>> The other tools calling the deprecated btf__get_from_id() function will
>>>>>> be updated in a future commit.
>>>>>>
>>>>>> References:
>>>>>>
>>>>>> - https://github.com/libbpf/libbpf/issues/278
>>>>>> - https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis
>>>>>>
>>
>>>>>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
>>>>>> index 7e0de560490e..6654bdee7ad7 100644
>>>>>> --- a/tools/lib/bpf/btf.c
>>>>>> +++ b/tools/lib/bpf/btf.c
>>>>>> @@ -1383,21 +1383,30 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
>>>>>>         return btf;
>>>>>>  }
>>>>>>
>>>>>> +struct btf *btf__load_from_kernel_by_id(__u32 id)
>>>>>> +{
>>>>>> +       struct btf *btf;
>>>>>> +       int btf_fd;
>>>>>> +
>>>>>> +       btf_fd = bpf_btf_get_fd_by_id(id);
>>>>>> +       if (btf_fd < 0)
>>>>>> +               return ERR_PTR(-errno);
>>>>>
>>>>> please use libbpf_err_ptr() for consistency, see
>>>>> bpf_object__open_mem() for an example
>>>>
>>>> I can do that, but I'll need to uncouple btf__get_from_id() from the new
>>>> function. If it calls btf__load_from_kernel_by_id() and
>>>> LIBBPF_STRICT_CLEAN_PTRS is set, it would change its return value.
>>>
>>> No it won't, if libbpf_get_error() is used right after the API call.
>>
>> But we cannot be sure that users currently call libbpf_get_error() after
>> btf__get_from_id()? I'm fine if we assume they do (users currently
>> selecting the CLEAN_PTRS are probably savvy enough to call it I guess),
>> I'll update as you suggest.
> 
> I think you are still confused.

OK, I think I was.
I'm not arguing against the contract, but I thought your suggestion
would introduce a change in btf__get_from_id()'s behaviour. Reading
again through the code and your explanations, there should be no change
indeed, I just misunderstood in the first place. Apologies, and thanks
for your patience :). I'll prepare v3 soon.

Quentin

[PATCH bpf-next v2 3/5] tools: replace btf__get_from_id() with btf__load_from_kernel_by_id()

[Quentin Monnet]

Replace the calls to deprecated function btf__get_from_id() with calls
to btf__load_from_kernel_by_id() in tools/ (bpftool, perf, selftests).
Update the surrounding code accordingly (instead of passing a pointer to
the btf struct, get it as a return value from the function). Also make
sure that btf__free() is called on the pointer after use.

v2:
- Given that btf__load_from_kernel_by_id() has changed since v1, adapt
  the code accordingly instead of just renaming the function. Also add a
  few calls to btf__free() when necessary.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Acked-by: John Fastabend <john.fastabend@gmail.com>
---
 tools/bpf/bpftool/btf.c                      |  8 ++----
 tools/bpf/bpftool/btf_dumper.c               |  6 ++--
 tools/bpf/bpftool/map.c                      | 16 +++++------
 tools/bpf/bpftool/prog.c                     | 29 ++++++++++++++------
 tools/perf/util/bpf-event.c                  | 11 ++++----
 tools/perf/util/bpf_counter.c                | 12 ++++++--
 tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
 7 files changed, 51 insertions(+), 35 deletions(-)

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 385d5c955cf3..9162a18e84c0 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -580,16 +580,12 @@ static int do_dump(int argc, char **argv)
 	}
 
 	if (!btf) {
-		err = btf__get_from_id(btf_id, &btf);
+		btf = btf__load_from_kernel_by_id(btf_id);
+		err = libbpf_get_error(btf);
 		if (err) {
 			p_err("get btf by id (%u): %s", btf_id, strerror(err));
 			goto done;
 		}
-		if (!btf) {
-			err = -ENOENT;
-			p_err("can't find btf with ID (%u)", btf_id);
-			goto done;
-		}
 	}
 
 	if (dump_c) {
diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c
index 7ca54d046362..9c25286a5c73 100644
--- a/tools/bpf/bpftool/btf_dumper.c
+++ b/tools/bpf/bpftool/btf_dumper.c
@@ -64,8 +64,10 @@ static int dump_prog_id_as_func_ptr(const struct btf_dumper *d,
 	}
 	info = &prog_info->info;
 
-	if (!info->btf_id || !info->nr_func_info ||
-	    btf__get_from_id(info->btf_id, &prog_btf))
+	if (!info->btf_id || !info->nr_func_info)
+		goto print;
+	prog_btf = btf__load_from_kernel_by_id(info->btf_id);
+	if (libbpf_get_error(prog_btf))
 		goto print;
 	finfo = u64_to_ptr(info->func_info);
 	func_type = btf__type_by_id(prog_btf, finfo->type_id);
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 09ae0381205b..12787758ce03 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -805,12 +805,11 @@ static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
 		}
 		return btf_vmlinux;
 	} else if (info->btf_value_type_id) {
-		int err;
-
-		err = btf__get_from_id(info->btf_id, &btf);
-		if (err || !btf) {
+		btf = btf__load_from_kernel_by_id(info->btf_id);
+		if (libbpf_get_error(btf)) {
 			p_err("failed to get btf");
-			btf = err ? ERR_PTR(err) : ERR_PTR(-ESRCH);
+			if (!btf)
+				btf = ERR_PTR(-ESRCH);
 		}
 	}
 
@@ -1039,11 +1038,10 @@ static void print_key_value(struct bpf_map_info *info, void *key,
 			    void *value)
 {
 	json_writer_t *btf_wtr;
-	struct btf *btf = NULL;
-	int err;
+	struct btf *btf;
 
-	err = btf__get_from_id(info->btf_id, &btf);
-	if (err) {
+	btf = btf__load_from_kernel_by_id(info->btf_id);
+	if (libbpf_get_error(btf)) {
 		p_err("failed to get btf");
 		return;
 	}
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index cc48726740ad..b1996b8f1d42 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -249,10 +249,10 @@ static void show_prog_metadata(int fd, __u32 num_maps)
 	struct bpf_map_info map_info;
 	struct btf_var_secinfo *vsi;
 	bool printed_header = false;
-	struct btf *btf = NULL;
 	unsigned int i, vlen;
 	void *value = NULL;
 	const char *name;
+	struct btf *btf;
 	int err;
 
 	if (!num_maps)
@@ -263,8 +263,8 @@ static void show_prog_metadata(int fd, __u32 num_maps)
 	if (!value)
 		return;
 
-	err = btf__get_from_id(map_info.btf_id, &btf);
-	if (err || !btf)
+	btf = btf__load_from_kernel_by_id(map_info.btf_id);
+	if (libbpf_get_error(btf))
 		goto out_free;
 
 	t_datasec = btf__type_by_id(btf, map_info.btf_value_type_id);
@@ -646,9 +646,12 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
 		member_len = info->xlated_prog_len;
 	}
 
-	if (info->btf_id && btf__get_from_id(info->btf_id, &btf)) {
-		p_err("failed to get btf");
-		return -1;
+	if (info->btf_id) {
+		btf = btf__load_from_kernel_by_id(info->btf_id);
+		if (libbpf_get_error(btf)) {
+			p_err("failed to get btf");
+			return -1;
+		}
 	}
 
 	func_info = u64_to_ptr(info->func_info);
@@ -781,6 +784,8 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
 		kernel_syms_destroy(&dd);
 	}
 
+	btf__free(btf);
+
 	return 0;
 }
 
@@ -2002,8 +2007,8 @@ static char *profile_target_name(int tgt_fd)
 	struct bpf_prog_info_linear *info_linear;
 	struct bpf_func_info *func_info;
 	const struct btf_type *t;
+	struct btf *btf = NULL;
 	char *name = NULL;
-	struct btf *btf;
 
 	info_linear = bpf_program__get_prog_info_linear(
 		tgt_fd, 1UL << BPF_PROG_INFO_FUNC_INFO);
@@ -2012,12 +2017,17 @@ static char *profile_target_name(int tgt_fd)
 		return NULL;
 	}
 
-	if (info_linear->info.btf_id == 0 ||
-	    btf__get_from_id(info_linear->info.btf_id, &btf)) {
+	if (info_linear->info.btf_id == 0) {
 		p_err("prog FD %d doesn't have valid btf", tgt_fd);
 		goto out;
 	}
 
+	btf = btf__load_from_kernel_by_id(info_linear->info.btf_id);
+	if (libbpf_get_error(btf)) {
+		p_err("failed to load btf for prog FD %d", tgt_fd);
+		goto out;
+	}
+
 	func_info = u64_to_ptr(info_linear->info.func_info);
 	t = btf__type_by_id(btf, func_info[0].type_id);
 	if (!t) {
@@ -2027,6 +2037,7 @@ static char *profile_target_name(int tgt_fd)
 	}
 	name = strdup(btf__name_by_offset(btf, t->name_off));
 out:
+	btf__free(btf);
 	free(info_linear);
 	return name;
 }
diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
index cdecda1ddd36..996d025b8ed8 100644
--- a/tools/perf/util/bpf-event.c
+++ b/tools/perf/util/bpf-event.c
@@ -223,10 +223,10 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_session *session,
 			free(info_linear);
 			return -1;
 		}
-		if (btf__get_from_id(info->btf_id, &btf)) {
+		btf = btf__load_from_kernel_by_id(info->btf_id);
+		if (libbpf_get_error(btf)) {
 			pr_debug("%s: failed to get BTF of id %u, aborting\n", __func__, info->btf_id);
 			err = -1;
-			btf = NULL;
 			goto out;
 		}
 		perf_env__fetch_btf(env, info->btf_id, btf);
@@ -296,7 +296,7 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_session *session,
 
 out:
 	free(info_linear);
-	free(btf);
+	btf__free(btf);
 	return err ? -1 : 0;
 }
 
@@ -478,7 +478,8 @@ static void perf_env__add_bpf_info(struct perf_env *env, u32 id)
 	if (btf_id == 0)
 		goto out;
 
-	if (btf__get_from_id(btf_id, &btf)) {
+	btf = btf__load_from_kernel_by_id(btf_id);
+	if (libbpf_get_error(btf)) {
 		pr_debug("%s: failed to get BTF of id %u, aborting\n",
 			 __func__, btf_id);
 		goto out;
@@ -486,7 +487,7 @@ static void perf_env__add_bpf_info(struct perf_env *env, u32 id)
 	perf_env__fetch_btf(env, btf_id, btf);
 
 out:
-	free(btf);
+	btf__free(btf);
 	close(fd);
 }
 
diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
index 8150e03367bb..ba0f20853651 100644
--- a/tools/perf/util/bpf_counter.c
+++ b/tools/perf/util/bpf_counter.c
@@ -64,8 +64,8 @@ static char *bpf_target_prog_name(int tgt_fd)
 	struct bpf_prog_info_linear *info_linear;
 	struct bpf_func_info *func_info;
 	const struct btf_type *t;
+	struct btf *btf = NULL;
 	char *name = NULL;
-	struct btf *btf;
 
 	info_linear = bpf_program__get_prog_info_linear(
 		tgt_fd, 1UL << BPF_PROG_INFO_FUNC_INFO);
@@ -74,12 +74,17 @@ static char *bpf_target_prog_name(int tgt_fd)
 		return NULL;
 	}
 
-	if (info_linear->info.btf_id == 0 ||
-	    btf__get_from_id(info_linear->info.btf_id, &btf)) {
+	if (info_linear->info.btf_id == 0) {
 		pr_debug("prog FD %d doesn't have valid btf\n", tgt_fd);
 		goto out;
 	}
 
+	btf = btf__load_from_kernel_by_id(info_linear->info.btf_id);
+	if (libbpf_get_error(btf)) {
+		pr_debug("failed to load btf for prog FD %d\n", tgt_fd);
+		goto out;
+	}
+
 	func_info = u64_to_ptr(info_linear->info.func_info);
 	t = btf__type_by_id(btf, func_info[0].type_id);
 	if (!t) {
@@ -89,6 +94,7 @@ static char *bpf_target_prog_name(int tgt_fd)
 	}
 	name = strdup(btf__name_by_offset(btf, t->name_off));
 out:
+	btf__free(btf);
 	free(info_linear);
 	return name;
 }
diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c
index 857e3f26086f..649f87382c8d 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf.c
@@ -4350,7 +4350,8 @@ static void do_test_file(unsigned int test_num)
 		goto done;
 	}
 
-	err = btf__get_from_id(info.btf_id, &btf);
+	btf = btf__load_from_kernel_by_id(info.btf_id);
+	err = libbpf_get_error(btf);
 	if (CHECK(err, "cannot get btf from kernel, err: %d", err))
 		goto done;
 
@@ -4386,6 +4387,7 @@ static void do_test_file(unsigned int test_num)
 	fprintf(stderr, "OK");
 
 done:
+	btf__free(btf);
 	free(func_info);
 	bpf_object__close(obj);
 }
-- 
2.30.2

Re: [PATCH bpf-next v2 3/5] tools: replace btf__get_from_id() with btf__load_from_kernel_by_id()

[Andrii Nakryiko]

On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> Replace the calls to deprecated function btf__get_from_id() with calls
> to btf__load_from_kernel_by_id() in tools/ (bpftool, perf, selftests).
> Update the surrounding code accordingly (instead of passing a pointer to
> the btf struct, get it as a return value from the function). Also make
> sure that btf__free() is called on the pointer after use.
>
> v2:
> - Given that btf__load_from_kernel_by_id() has changed since v1, adapt
>   the code accordingly instead of just renaming the function. Also add a
>   few calls to btf__free() when necessary.
>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> Acked-by: John Fastabend <john.fastabend@gmail.com>
> ---
>  tools/bpf/bpftool/btf.c                      |  8 ++----
>  tools/bpf/bpftool/btf_dumper.c               |  6 ++--
>  tools/bpf/bpftool/map.c                      | 16 +++++------
>  tools/bpf/bpftool/prog.c                     | 29 ++++++++++++++------
>  tools/perf/util/bpf-event.c                  | 11 ++++----
>  tools/perf/util/bpf_counter.c                | 12 ++++++--
>  tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
>  7 files changed, 51 insertions(+), 35 deletions(-)
>

[...]

> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 09ae0381205b..12787758ce03 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -805,12 +805,11 @@ static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
>                 }
>                 return btf_vmlinux;
>         } else if (info->btf_value_type_id) {
> -               int err;
> -
> -               err = btf__get_from_id(info->btf_id, &btf);
> -               if (err || !btf) {
> +               btf = btf__load_from_kernel_by_id(info->btf_id);
> +               if (libbpf_get_error(btf)) {
>                         p_err("failed to get btf");
> -                       btf = err ? ERR_PTR(err) : ERR_PTR(-ESRCH);
> +                       if (!btf)
> +                               btf = ERR_PTR(-ESRCH);

why not do a simpler (less conditionals)

err = libbpf_get_error(btf);
if (err) {
    btf = ERR_PTR(err);
}

?

>                 }
>         }
>
> @@ -1039,11 +1038,10 @@ static void print_key_value(struct bpf_map_info *info, void *key,
>                             void *value)
>  {
>         json_writer_t *btf_wtr;
> -       struct btf *btf = NULL;
> -       int err;
> +       struct btf *btf;
>
> -       err = btf__get_from_id(info->btf_id, &btf);
> -       if (err) {
> +       btf = btf__load_from_kernel_by_id(info->btf_id);
> +       if (libbpf_get_error(btf)) {
>                 p_err("failed to get btf");
>                 return;
>         }

[...]

>
>         func_info = u64_to_ptr(info->func_info);
> @@ -781,6 +784,8 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
>                 kernel_syms_destroy(&dd);
>         }
>
> +       btf__free(btf);
> +

warrants a Fixes: tag?

>         return 0;
>  }
>
> @@ -2002,8 +2007,8 @@ static char *profile_target_name(int tgt_fd)
>         struct bpf_prog_info_linear *info_linear;
>         struct bpf_func_info *func_info;
>         const struct btf_type *t;
> +       struct btf *btf = NULL;
>         char *name = NULL;
> -       struct btf *btf;
>
>         info_linear = bpf_program__get_prog_info_linear(
>                 tgt_fd, 1UL << BPF_PROG_INFO_FUNC_INFO);
> @@ -2012,12 +2017,17 @@ static char *profile_target_name(int tgt_fd)
>                 return NULL;
>         }
>
> -       if (info_linear->info.btf_id == 0 ||
> -           btf__get_from_id(info_linear->info.btf_id, &btf)) {
> +       if (info_linear->info.btf_id == 0) {
>                 p_err("prog FD %d doesn't have valid btf", tgt_fd);
>                 goto out;
>         }
>
> +       btf = btf__load_from_kernel_by_id(info_linear->info.btf_id);
> +       if (libbpf_get_error(btf)) {
> +               p_err("failed to load btf for prog FD %d", tgt_fd);
> +               goto out;
> +       }
> +
>         func_info = u64_to_ptr(info_linear->info.func_info);
>         t = btf__type_by_id(btf, func_info[0].type_id);
>         if (!t) {
> @@ -2027,6 +2037,7 @@ static char *profile_target_name(int tgt_fd)
>         }
>         name = strdup(btf__name_by_offset(btf, t->name_off));
>  out:
> +       btf__free(btf);

and another Fixes? :) and two more below

>         free(info_linear);
>         return name;
>  }

[...]

Re: [PATCH bpf-next v2 3/5] tools: replace btf__get_from_id() with btf__load_from_kernel_by_id()

[Quentin Monnet]

2021-07-22 17:48 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> Replace the calls to deprecated function btf__get_from_id() with calls
>> to btf__load_from_kernel_by_id() in tools/ (bpftool, perf, selftests).
>> Update the surrounding code accordingly (instead of passing a pointer to
>> the btf struct, get it as a return value from the function). Also make
>> sure that btf__free() is called on the pointer after use.
>>
>> v2:
>> - Given that btf__load_from_kernel_by_id() has changed since v1, adapt
>>   the code accordingly instead of just renaming the function. Also add a
>>   few calls to btf__free() when necessary.
>>
>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
>> Acked-by: John Fastabend <john.fastabend@gmail.com>
>> ---
>>  tools/bpf/bpftool/btf.c                      |  8 ++----
>>  tools/bpf/bpftool/btf_dumper.c               |  6 ++--
>>  tools/bpf/bpftool/map.c                      | 16 +++++------
>>  tools/bpf/bpftool/prog.c                     | 29 ++++++++++++++------
>>  tools/perf/util/bpf-event.c                  | 11 ++++----
>>  tools/perf/util/bpf_counter.c                | 12 ++++++--
>>  tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
>>  7 files changed, 51 insertions(+), 35 deletions(-)
>>
> 
> [...]
> 
>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>> index 09ae0381205b..12787758ce03 100644
>> --- a/tools/bpf/bpftool/map.c
>> +++ b/tools/bpf/bpftool/map.c
>> @@ -805,12 +805,11 @@ static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
>>                 }
>>                 return btf_vmlinux;
>>         } else if (info->btf_value_type_id) {
>> -               int err;
>> -
>> -               err = btf__get_from_id(info->btf_id, &btf);
>> -               if (err || !btf) {
>> +               btf = btf__load_from_kernel_by_id(info->btf_id);
>> +               if (libbpf_get_error(btf)) {
>>                         p_err("failed to get btf");
>> -                       btf = err ? ERR_PTR(err) : ERR_PTR(-ESRCH);
>> +                       if (!btf)
>> +                               btf = ERR_PTR(-ESRCH);
> 
> why not do a simpler (less conditionals)
> 
> err = libbpf_get_error(btf);
> if (err) {
>     btf = ERR_PTR(err);
> }
> 
> ?

Because if btf is NULL at this stage, this would change the return value
from -ESRCH to NULL. This would be problematic in mapdump(), since we
check this value ("if (IS_ERR(btf))") to detect a failure in
get_map_kv_btf().

I could change that check in mapdump() to use libbpf_get_error()
instead, but in that case it would similarly change the return value for
mapdump() (and errno), which I think would be propagated up to main()
and would return 0 instead of -ESRCH. This does not seem suitable and
would play badly with batch mode, among other things.

So I'm considering keeping the one additional if.

> 
>>                 }
>>         }
>>
>> @@ -1039,11 +1038,10 @@ static void print_key_value(struct bpf_map_info *info, void *key,
>>                             void *value)
>>  {
>>         json_writer_t *btf_wtr;
>> -       struct btf *btf = NULL;
>> -       int err;
>> +       struct btf *btf;
>>
>> -       err = btf__get_from_id(info->btf_id, &btf);
>> -       if (err) {
>> +       btf = btf__load_from_kernel_by_id(info->btf_id);
>> +       if (libbpf_get_error(btf)) {
>>                 p_err("failed to get btf");
>>                 return;
>>         }
> 
> [...]
> 
>>
>>         func_info = u64_to_ptr(info->func_info);
>> @@ -781,6 +784,8 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
>>                 kernel_syms_destroy(&dd);
>>         }
>>
>> +       btf__free(btf);
>> +
> 
> warrants a Fixes: tag?

I don't mind adding the tags, but do they have any advantage here? My
understanding is that they tend to be neon signs for backports to stable
branches, but this patch depends on btf__load_from_kernel_by_id(),
meaning more patches to pull. I'll see if I can move the btf__free()
fixes to a separate commit, maybe.

Re: [PATCH bpf-next v2 3/5] tools: replace btf__get_from_id() with btf__load_from_kernel_by_id()

[Andrii Nakryiko]

On Fri, Jul 23, 2021 at 2:52 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> 2021-07-22 17:48 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>
> >> Replace the calls to deprecated function btf__get_from_id() with calls
> >> to btf__load_from_kernel_by_id() in tools/ (bpftool, perf, selftests).
> >> Update the surrounding code accordingly (instead of passing a pointer to
> >> the btf struct, get it as a return value from the function). Also make
> >> sure that btf__free() is called on the pointer after use.
> >>
> >> v2:
> >> - Given that btf__load_from_kernel_by_id() has changed since v1, adapt
> >>   the code accordingly instead of just renaming the function. Also add a
> >>   few calls to btf__free() when necessary.
> >>
> >> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> >> Acked-by: John Fastabend <john.fastabend@gmail.com>
> >> ---
> >>  tools/bpf/bpftool/btf.c                      |  8 ++----
> >>  tools/bpf/bpftool/btf_dumper.c               |  6 ++--
> >>  tools/bpf/bpftool/map.c                      | 16 +++++------
> >>  tools/bpf/bpftool/prog.c                     | 29 ++++++++++++++------
> >>  tools/perf/util/bpf-event.c                  | 11 ++++----
> >>  tools/perf/util/bpf_counter.c                | 12 ++++++--
> >>  tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
> >>  7 files changed, 51 insertions(+), 35 deletions(-)
> >>
> >
> > [...]
> >
> >> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> >> index 09ae0381205b..12787758ce03 100644
> >> --- a/tools/bpf/bpftool/map.c
> >> +++ b/tools/bpf/bpftool/map.c
> >> @@ -805,12 +805,11 @@ static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
> >>                 }
> >>                 return btf_vmlinux;
> >>         } else if (info->btf_value_type_id) {
> >> -               int err;
> >> -
> >> -               err = btf__get_from_id(info->btf_id, &btf);
> >> -               if (err || !btf) {
> >> +               btf = btf__load_from_kernel_by_id(info->btf_id);
> >> +               if (libbpf_get_error(btf)) {
> >>                         p_err("failed to get btf");
> >> -                       btf = err ? ERR_PTR(err) : ERR_PTR(-ESRCH);
> >> +                       if (!btf)
> >> +                               btf = ERR_PTR(-ESRCH);
> >
> > why not do a simpler (less conditionals)
> >
> > err = libbpf_get_error(btf);
> > if (err) {
> >     btf = ERR_PTR(err);
> > }
> >
> > ?
>
> Because if btf is NULL at this stage, this would change the return value
> from -ESRCH to NULL. This would be problematic in mapdump(), since we
> check this value ("if (IS_ERR(btf))") to detect a failure in
> get_map_kv_btf().

see my reply on previous patch. libbpf_get_error() handles this
transparently regardless of CLEAN_PTRS mode, as long as it is called
right after API call. So the above sample will work as you'd expect,
preserving errors.

>
> I could change that check in mapdump() to use libbpf_get_error()
> instead, but in that case it would similarly change the return value for
> mapdump() (and errno), which I think would be propagated up to main()
> and would return 0 instead of -ESRCH. This does not seem suitable and
> would play badly with batch mode, among other things.
>
> So I'm considering keeping the one additional if.
>
> >
> >>                 }
> >>         }
> >>
> >> @@ -1039,11 +1038,10 @@ static void print_key_value(struct bpf_map_info *info, void *key,
> >>                             void *value)
> >>  {
> >>         json_writer_t *btf_wtr;
> >> -       struct btf *btf = NULL;
> >> -       int err;
> >> +       struct btf *btf;
> >>
> >> -       err = btf__get_from_id(info->btf_id, &btf);
> >> -       if (err) {
> >> +       btf = btf__load_from_kernel_by_id(info->btf_id);
> >> +       if (libbpf_get_error(btf)) {
> >>                 p_err("failed to get btf");
> >>                 return;
> >>         }
> >
> > [...]
> >
> >>
> >>         func_info = u64_to_ptr(info->func_info);
> >> @@ -781,6 +784,8 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
> >>                 kernel_syms_destroy(&dd);
> >>         }
> >>
> >> +       btf__free(btf);
> >> +
> >
> > warrants a Fixes: tag?
>
> I don't mind adding the tags, but do they have any advantage here? My
> understanding is that they tend to be neon signs for backports to stable
> branches, but this patch depends on btf__load_from_kernel_by_id(),
> meaning more patches to pull. I'll see if I can move the btf__free()
> fixes to a separate commit, maybe.

Having Fixes: allows to keep track of where the issue originated. It
doesn't necessarily mean something has to be backported, as far as I
understand. So it's good to do regardless. Splitting fixes into a
separate patch works for me as well, but I don't care all that much
given they are small.

Re: [PATCH bpf-next v2 3/5] tools: replace btf__get_from_id() with btf__load_from_kernel_by_id()

[Quentin Monnet]

2021-07-23 08:57 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Fri, Jul 23, 2021 at 2:52 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> 2021-07-22 17:48 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
>>> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>>>
>>>> Replace the calls to deprecated function btf__get_from_id() with calls
>>>> to btf__load_from_kernel_by_id() in tools/ (bpftool, perf, selftests).
>>>> Update the surrounding code accordingly (instead of passing a pointer to
>>>> the btf struct, get it as a return value from the function). Also make
>>>> sure that btf__free() is called on the pointer after use.
>>>>
>>>> v2:
>>>> - Given that btf__load_from_kernel_by_id() has changed since v1, adapt
>>>>   the code accordingly instead of just renaming the function. Also add a
>>>>   few calls to btf__free() when necessary.
>>>>
>>>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
>>>> Acked-by: John Fastabend <john.fastabend@gmail.com>
>>>> ---
>>>>  tools/bpf/bpftool/btf.c                      |  8 ++----
>>>>  tools/bpf/bpftool/btf_dumper.c               |  6 ++--
>>>>  tools/bpf/bpftool/map.c                      | 16 +++++------
>>>>  tools/bpf/bpftool/prog.c                     | 29 ++++++++++++++------
>>>>  tools/perf/util/bpf-event.c                  | 11 ++++----
>>>>  tools/perf/util/bpf_counter.c                | 12 ++++++--
>>>>  tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
>>>>  7 files changed, 51 insertions(+), 35 deletions(-)
>>>>
>>>
>>> [...]
>>>
>>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>>>> index 09ae0381205b..12787758ce03 100644
>>>> --- a/tools/bpf/bpftool/map.c
>>>> +++ b/tools/bpf/bpftool/map.c
>>>> @@ -805,12 +805,11 @@ static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
>>>>                 }
>>>>                 return btf_vmlinux;
>>>>         } else if (info->btf_value_type_id) {
>>>> -               int err;
>>>> -
>>>> -               err = btf__get_from_id(info->btf_id, &btf);
>>>> -               if (err || !btf) {
>>>> +               btf = btf__load_from_kernel_by_id(info->btf_id);
>>>> +               if (libbpf_get_error(btf)) {
>>>>                         p_err("failed to get btf");
>>>> -                       btf = err ? ERR_PTR(err) : ERR_PTR(-ESRCH);
>>>> +                       if (!btf)
>>>> +                               btf = ERR_PTR(-ESRCH);
>>>
>>> why not do a simpler (less conditionals)
>>>
>>> err = libbpf_get_error(btf);
>>> if (err) {
>>>     btf = ERR_PTR(err);
>>> }
>>>
>>> ?
>>
>> Because if btf is NULL at this stage, this would change the return value
>> from -ESRCH to NULL. This would be problematic in mapdump(), since we
>> check this value ("if (IS_ERR(btf))") to detect a failure in
>> get_map_kv_btf().
> 
> see my reply on previous patch. libbpf_get_error() handles this
> transparently regardless of CLEAN_PTRS mode, as long as it is called
> right after API call. So the above sample will work as you'd expect,
> preserving errors.

Right, it looks like I got confused on this one. I'll update it.

> 
>>
>> I could change that check in mapdump() to use libbpf_get_error()
>> instead, but in that case it would similarly change the return value for
>> mapdump() (and errno), which I think would be propagated up to main()
>> and would return 0 instead of -ESRCH. This does not seem suitable and
>> would play badly with batch mode, among other things.
>>
>> So I'm considering keeping the one additional if.
>>
>>>
>>>>                 }
>>>>         }
>>>>
>>>> @@ -1039,11 +1038,10 @@ static void print_key_value(struct bpf_map_info *info, void *key,
>>>>                             void *value)
>>>>  {
>>>>         json_writer_t *btf_wtr;
>>>> -       struct btf *btf = NULL;
>>>> -       int err;
>>>> +       struct btf *btf;
>>>>
>>>> -       err = btf__get_from_id(info->btf_id, &btf);
>>>> -       if (err) {
>>>> +       btf = btf__load_from_kernel_by_id(info->btf_id);
>>>> +       if (libbpf_get_error(btf)) {
>>>>                 p_err("failed to get btf");
>>>>                 return;
>>>>         }
>>>
>>> [...]
>>>
>>>>
>>>>         func_info = u64_to_ptr(info->func_info);
>>>> @@ -781,6 +784,8 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
>>>>                 kernel_syms_destroy(&dd);
>>>>         }
>>>>
>>>> +       btf__free(btf);
>>>> +
>>>
>>> warrants a Fixes: tag?
>>
>> I don't mind adding the tags, but do they have any advantage here? My
>> understanding is that they tend to be neon signs for backports to stable
>> branches, but this patch depends on btf__load_from_kernel_by_id(),
>> meaning more patches to pull. I'll see if I can move the btf__free()
>> fixes to a separate commit, maybe.
> 
> Having Fixes: allows to keep track of where the issue originated. It
> doesn't necessarily mean something has to be backported, as far as I
> understand. So it's good to do regardless. Splitting fixes into a
> separate patch works for me as well, but I don't care all that much
> given they are small.
> 

OK, thank you for the clarification :).
I'll keep a single patch in that case.

[PATCH bpf-next v2 4/5] libbpf: add split BTF support for btf__load_from_kernel_by_id()

[Quentin Monnet]

Add a new API function btf__load_from_kernel_by_id_split(), which takes
a pointer to a base BTF object in order to support split BTF objects
when retrieving BTF information from the kernel.

Reference: https://github.com/libbpf/libbpf/issues/314

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Acked-by: John Fastabend <john.fastabend@gmail.com>
---
 tools/lib/bpf/btf.c      | 9 +++++++--
 tools/lib/bpf/btf.h      | 2 ++
 tools/lib/bpf/libbpf.map | 1 +
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 6654bdee7ad7..38a901e3a483 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -1383,7 +1383,7 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
 	return btf;
 }
 
-struct btf *btf__load_from_kernel_by_id(__u32 id)
+struct btf *btf__load_from_kernel_by_id_split(__u32 id, struct btf *base_btf)
 {
 	struct btf *btf;
 	int btf_fd;
@@ -1392,12 +1392,17 @@ struct btf *btf__load_from_kernel_by_id(__u32 id)
 	if (btf_fd < 0)
 		return ERR_PTR(-errno);
 
-	btf = btf_get_from_fd(btf_fd, NULL);
+	btf = btf_get_from_fd(btf_fd, base_btf);
 	close(btf_fd);
 
 	return libbpf_ptr(btf);
 }
 
+struct btf *btf__load_from_kernel_by_id(__u32 id)
+{
+	return btf__load_from_kernel_by_id_split(id, NULL);
+}
+
 int btf__get_from_id(__u32 id, struct btf **btf)
 {
 	struct btf *res;
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 3db9446bc133..c9407d57d096 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -69,6 +69,8 @@ LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
 LIBBPF_API const char *btf__str_by_offset(const struct btf *btf, __u32 offset);
 LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
 LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id);
+LIBBPF_API struct btf *btf__load_from_kernel_by_id_split(__u32 id,
+							 struct btf *base_btf);
 LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name,
 				    __u32 expected_key_size,
 				    __u32 expected_value_size,
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index ca8cc7a7faad..eecf77227aeb 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -374,6 +374,7 @@ LIBBPF_0.5.0 {
 		bpf_map_lookup_and_delete_elem_flags;
 		bpf_object__gen_loader;
 		btf__load_from_kernel_by_id;
+		btf__load_from_kernel_by_id_split;
 		btf__load_into_kernel;
 		btf_dump__dump_type_data;
 		libbpf_set_strict_mode;
-- 
2.30.2

Re: [PATCH bpf-next v2 4/5] libbpf: add split BTF support for btf__load_from_kernel_by_id()

[Andrii Nakryiko]

On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> Add a new API function btf__load_from_kernel_by_id_split(), which takes
> a pointer to a base BTF object in order to support split BTF objects
> when retrieving BTF information from the kernel.
>
> Reference: https://github.com/libbpf/libbpf/issues/314
>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
> Acked-by: John Fastabend <john.fastabend@gmail.com>
> ---
>  tools/lib/bpf/btf.c      | 9 +++++++--
>  tools/lib/bpf/btf.h      | 2 ++
>  tools/lib/bpf/libbpf.map | 1 +
>  3 files changed, 10 insertions(+), 2 deletions(-)
>

[...]

> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index 3db9446bc133..c9407d57d096 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -69,6 +69,8 @@ LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
>  LIBBPF_API const char *btf__str_by_offset(const struct btf *btf, __u32 offset);
>  LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
>  LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id);
> +LIBBPF_API struct btf *btf__load_from_kernel_by_id_split(__u32 id,
> +                                                        struct btf *base_btf);

nit: please keep it on a single line, it's not that long

>  LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name,
>                                     __u32 expected_key_size,
>                                     __u32 expected_value_size,
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index ca8cc7a7faad..eecf77227aeb 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -374,6 +374,7 @@ LIBBPF_0.5.0 {
>                 bpf_map_lookup_and_delete_elem_flags;
>                 bpf_object__gen_loader;
>                 btf__load_from_kernel_by_id;
> +               btf__load_from_kernel_by_id_split;
>                 btf__load_into_kernel;
>                 btf_dump__dump_type_data;
>                 libbpf_set_strict_mode;
> --
> 2.30.2
>

[PATCH bpf-next v2 5/5] tools: bpftool: support dumping split BTF by id

[Quentin Monnet]

Split BTF objects are typically BTF objects for kernel modules, which
are incrementally built on top of kernel BTF instead of redefining all
kernel symbols they need. We can use bpftool with its -B command-line
option to dump split BTF objects. It works well when the handle provided
for the BTF object to dump is a "path" to the BTF object, typically
under /sys/kernel/btf, because bpftool internally calls
btf__parse_split() which can take a "base_btf" pointer and resolve the
BTF reconstruction (although in that case, the "-B" option is
unnecessary because bpftool performs autodetection).

However, it did not work so far when passing the BTF object through its
id, because bpftool would call btf__get_from_id() which did not provide
a way to pass a "base_btf" pointer.

In other words, the following works:

    # bpftool btf dump file /sys/kernel/btf/i2c_smbus -B /sys/kernel/btf/vmlinux

But this was not possible:

    # bpftool btf dump id 6 -B /sys/kernel/btf/vmlinux

The libbpf API has recently changed, and btf__get_from_id() has been
replaced with btf__load_from_kernel_by_id() and its version with support
for split BTF, btf__load_from_kernel_by_id_split(). Let's update bpftool
to make it able to dump the BTF object in the second case as well.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Acked-by: John Fastabend <john.fastabend@gmail.com>
---
 tools/bpf/bpftool/btf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 9162a18e84c0..0ce3643278d4 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -580,7 +580,7 @@ static int do_dump(int argc, char **argv)
 	}
 
 	if (!btf) {
-		btf = btf__load_from_kernel_by_id(btf_id);
+		btf = btf__load_from_kernel_by_id_split(btf_id, base_btf);
 		err = libbpf_get_error(btf);
 		if (err) {
 			p_err("get btf by id (%u): %s", btf_id, strerror(err));
-- 
2.30.2

Re: [PATCH bpf-next v2 0/5] libbpf: rename btf__get_from_id() and btf__load() APIs, support split BTF

[Andrii Nakryiko]

On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> As part of the effort to move towards a v1.0 for libbpf [0], this set
> improves some confusing function names related to BTF loading from and to
> the kernel:
>
> - btf__load() becomes btf__load_into_kernel().
> - btf__get_from_id becomes btf__load_from_kernel_by_id().
> - A new version btf__load_from_kernel_by_id_split() extends the former to
>   add support for split BTF.
>
> The old functions are not removed or marked as deprecated yet, there
> should be in a future libbpf version.

Oh, and I was thinking about this whole deprecation having to be done
in two steps. It's super annoying to keep track of that. Ideally, we'd
have some macro that can mark API deprecated "in the future", when
actual libbpf version is >= to defined version. So something like
this:

LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6")


We'd need to make sure that during the build time we have some
LIBBPF_VERSION macro available against which we compare the expected
version and add or not the __attribute__((deprecated)).

Does this make sense? WDYT? I haven't looked into how hard it would be
to implement this, but it should be easy enough, so if you'd like some
macro challenge, please take a stab at it.

Having this it would be possible to make all the deprecations at the
same time that we add replacement APIs and not ask anyone to follow-up
potentially a month or two later, right?

>
> The last patch is a trivial change to bpftool to add support for dumping
> split BTF objects by referencing them by their id (and not only by their
> BTF path).
>
> [0] https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis
>
> v2:
> - Remove deprecation marking of legacy functions (patch 4/6 from v1).
> - Make btf__load_from_kernel_by_id{,_split}() return the btf struct.
> - Add new functions to v0.5.0 API (and not v0.6.0).
>
> Quentin Monnet (5):
>   libbpf: rename btf__load() as btf__load_into_kernel()
>   libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id()
>   tools: replace btf__get_from_id() with btf__load_from_kernel_by_id()
>   libbpf: add split BTF support for btf__load_from_kernel_by_id()
>   tools: bpftool: support dumping split BTF by id
>
>  tools/bpf/bpftool/btf.c                      |  8 ++---
>  tools/bpf/bpftool/btf_dumper.c               |  6 ++--
>  tools/bpf/bpftool/map.c                      | 16 +++++-----
>  tools/bpf/bpftool/prog.c                     | 29 +++++++++++------
>  tools/lib/bpf/btf.c                          | 33 ++++++++++++++------
>  tools/lib/bpf/btf.h                          |  4 +++
>  tools/lib/bpf/libbpf.c                       |  7 +++--
>  tools/lib/bpf/libbpf.map                     |  3 ++
>  tools/perf/util/bpf-event.c                  | 11 ++++---
>  tools/perf/util/bpf_counter.c                | 12 +++++--
>  tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
>  11 files changed, 86 insertions(+), 47 deletions(-)
>
> --
> 2.30.2
>

Re: [PATCH bpf-next v2 0/5] libbpf: rename btf__get_from_id() and btf__load() APIs, support split BTF

[Andrii Nakryiko]

On Thu, Jul 22, 2021 at 5:58 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >
> > As part of the effort to move towards a v1.0 for libbpf [0], this set
> > improves some confusing function names related to BTF loading from and to
> > the kernel:
> >
> > - btf__load() becomes btf__load_into_kernel().
> > - btf__get_from_id becomes btf__load_from_kernel_by_id().
> > - A new version btf__load_from_kernel_by_id_split() extends the former to
> >   add support for split BTF.
> >
> > The old functions are not removed or marked as deprecated yet, there
> > should be in a future libbpf version.
>
> Oh, and I was thinking about this whole deprecation having to be done
> in two steps. It's super annoying to keep track of that. Ideally, we'd
> have some macro that can mark API deprecated "in the future", when
> actual libbpf version is >= to defined version. So something like
> this:
>
> LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6")

Better:

LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6")

>
>
> We'd need to make sure that during the build time we have some
> LIBBPF_VERSION macro available against which we compare the expected
> version and add or not the __attribute__((deprecated)).
>
> Does this make sense? WDYT? I haven't looked into how hard it would be
> to implement this, but it should be easy enough, so if you'd like some
> macro challenge, please take a stab at it.
>
> Having this it would be possible to make all the deprecations at the
> same time that we add replacement APIs and not ask anyone to follow-up
> potentially a month or two later, right?
>
> >
> > The last patch is a trivial change to bpftool to add support for dumping
> > split BTF objects by referencing them by their id (and not only by their
> > BTF path).
> >
> > [0] https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis
> >
> > v2:
> > - Remove deprecation marking of legacy functions (patch 4/6 from v1).
> > - Make btf__load_from_kernel_by_id{,_split}() return the btf struct.
> > - Add new functions to v0.5.0 API (and not v0.6.0).
> >
> > Quentin Monnet (5):
> >   libbpf: rename btf__load() as btf__load_into_kernel()
> >   libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id()
> >   tools: replace btf__get_from_id() with btf__load_from_kernel_by_id()
> >   libbpf: add split BTF support for btf__load_from_kernel_by_id()
> >   tools: bpftool: support dumping split BTF by id
> >
> >  tools/bpf/bpftool/btf.c                      |  8 ++---
> >  tools/bpf/bpftool/btf_dumper.c               |  6 ++--
> >  tools/bpf/bpftool/map.c                      | 16 +++++-----
> >  tools/bpf/bpftool/prog.c                     | 29 +++++++++++------
> >  tools/lib/bpf/btf.c                          | 33 ++++++++++++++------
> >  tools/lib/bpf/btf.h                          |  4 +++
> >  tools/lib/bpf/libbpf.c                       |  7 +++--
> >  tools/lib/bpf/libbpf.map                     |  3 ++
> >  tools/perf/util/bpf-event.c                  | 11 ++++---
> >  tools/perf/util/bpf_counter.c                | 12 +++++--
> >  tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
> >  11 files changed, 86 insertions(+), 47 deletions(-)
> >
> > --
> > 2.30.2
> >

Re: [PATCH bpf-next v2 0/5] libbpf: rename btf__get_from_id() and btf__load() APIs, support split BTF

[Quentin Monnet]

2021-07-22 19:45 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Thu, Jul 22, 2021 at 5:58 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>>
>>> As part of the effort to move towards a v1.0 for libbpf [0], this set
>>> improves some confusing function names related to BTF loading from and to
>>> the kernel:
>>>
>>> - btf__load() becomes btf__load_into_kernel().
>>> - btf__get_from_id becomes btf__load_from_kernel_by_id().
>>> - A new version btf__load_from_kernel_by_id_split() extends the former to
>>>   add support for split BTF.
>>>
>>> The old functions are not removed or marked as deprecated yet, there
>>> should be in a future libbpf version.
>>
>> Oh, and I was thinking about this whole deprecation having to be done
>> in two steps. It's super annoying to keep track of that. Ideally, we'd
>> have some macro that can mark API deprecated "in the future", when
>> actual libbpf version is >= to defined version. So something like
>> this:
>>
>> LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6")
> 
> Better:
> 
> LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6")

I was considering a very advanced feature called “opening a new GitHub
issue” to track this :). But the macro game sounds interesting, I'll
look into it for next version.

One nit with LIBBPF_DEPRECATED_SINCE() is that the warning mentions a
version (here v0.6) that we are unsure will exist (say we jump from v0.5
to v1.0). But I don't suppose that's a real issue.

Thanks for the feedback!
Quentin

Re: [PATCH bpf-next v2 0/5] libbpf: rename btf__get_from_id() and btf__load() APIs, support split BTF

[Andrii Nakryiko]

On Fri, Jul 23, 2021 at 2:58 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> 2021-07-22 19:45 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > On Thu, Jul 22, 2021 at 5:58 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >>
> >> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>>
> >>> As part of the effort to move towards a v1.0 for libbpf [0], this set
> >>> improves some confusing function names related to BTF loading from and to
> >>> the kernel:
> >>>
> >>> - btf__load() becomes btf__load_into_kernel().
> >>> - btf__get_from_id becomes btf__load_from_kernel_by_id().
> >>> - A new version btf__load_from_kernel_by_id_split() extends the former to
> >>>   add support for split BTF.
> >>>
> >>> The old functions are not removed or marked as deprecated yet, there
> >>> should be in a future libbpf version.
> >>
> >> Oh, and I was thinking about this whole deprecation having to be done
> >> in two steps. It's super annoying to keep track of that. Ideally, we'd
> >> have some macro that can mark API deprecated "in the future", when
> >> actual libbpf version is >= to defined version. So something like
> >> this:
> >>
> >> LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6")
> >
> > Better:
> >
> > LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6")
>
> I was considering a very advanced feature called “opening a new GitHub

Someone gotta track and ping people at the right time even with
issues, so yeah, it's suboptimal.

> issue” to track this :). But the macro game sounds interesting, I'll
> look into it for next version.
>
> One nit with LIBBPF_DEPRECATED_SINCE() is that the warning mentions a
> version (here v0.6) that we are unsure will exist (say we jump from v0.5
> to v1.0). But I don't suppose that's a real issue.

There will always be a +0.1 version just to get deprecation activated.
This is for the reason I explained: we add replacement API in 0.X, but
can mark deprecated API in 0.(X+1), so we won't skip it, even if we
have to wait 2 extra months before 1.0. So I wouldn't worry about
this.

>
> Thanks for the feedback!
> Quentin

Re: [PATCH bpf-next v2 0/5] libbpf: rename btf__get_from_id() and btf__load() APIs, support split BTF

[Quentin Monnet]

2021-07-23 08:51 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Fri, Jul 23, 2021 at 2:58 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> 2021-07-22 19:45 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
>>> On Thu, Jul 22, 2021 at 5:58 PM Andrii Nakryiko
>>> <andrii.nakryiko@gmail.com> wrote:
>>>>
>>>> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>>>>
>>>>> As part of the effort to move towards a v1.0 for libbpf [0], this set
>>>>> improves some confusing function names related to BTF loading from and to
>>>>> the kernel:
>>>>>
>>>>> - btf__load() becomes btf__load_into_kernel().
>>>>> - btf__get_from_id becomes btf__load_from_kernel_by_id().
>>>>> - A new version btf__load_from_kernel_by_id_split() extends the former to
>>>>>   add support for split BTF.
>>>>>
>>>>> The old functions are not removed or marked as deprecated yet, there
>>>>> should be in a future libbpf version.
>>>>
>>>> Oh, and I was thinking about this whole deprecation having to be done
>>>> in two steps. It's super annoying to keep track of that. Ideally, we'd
>>>> have some macro that can mark API deprecated "in the future", when
>>>> actual libbpf version is >= to defined version. So something like
>>>> this:
>>>>
>>>> LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6")
>>>
>>> Better:
>>>
>>> LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6")

So I've been looking into this, and it's not _that_ simple to do. Unless
I missed something about preprocessing macros, I cannot bake a "#if" in
a "#define", to have the attribute printed if and only if the current
version is >= 0.6 in this example.

I've come up with something, but it is not optimal because I have to
write a check and macros for each version number used with the
LIBBPF_DEPRECATED_SINCE macro. If we really wanted to automate that part
I guess we could generate a header with those macros from the Makefile
and include it in libbpf_common.h, but that does not really look much
cleaner to me.

Here's my current code, below - does it correspond to what you had in
mind? Or did you think of something else?

------

diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index ec14aa725bb0..095d5dc30d50 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -8,6 +8,7 @@ LIBBPF_VERSION := $(shell \
 	grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
 	sort -rV | head -n1 | cut -d'_' -f2)
 LIBBPF_MAJOR_VERSION := $(firstword $(subst ., ,$(LIBBPF_VERSION)))
+LIBBPF_MINOR_VERSION := $(firstword $(subst ., ,$(subst $(LIBBPF_MAJOR_VERSION)., ,$(LIBBPF_VERSION))))
 
 MAKEFLAGS += --no-print-directory
 
@@ -86,6 +87,8 @@ override CFLAGS += -Werror -Wall
 override CFLAGS += $(INCLUDES)
 override CFLAGS += -fvisibility=hidden
 override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
+override CFLAGS += -DLIBBPF_MAJOR_VERSION=$(LIBBPF_MAJOR_VERSION)
+override CFLAGS += -DLIBBPF_MINOR_VERSION=$(LIBBPF_MINOR_VERSION)
 
 # flags specific for shared library
 SHLIB_FLAGS := -DSHARED -fPIC
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index cf8490f95641..8b6b5442dbd8 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -45,7 +45,8 @@ LIBBPF_API struct btf *btf__parse_raw(const char *path);
 LIBBPF_API struct btf *btf__parse_raw_split(const char *path, struct btf *base_btf);
 LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id);
 LIBBPF_API struct btf *btf__load_from_kernel_by_id_split(__u32 id, struct btf *base_btf);
-LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
+LIBBPF_API LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead")
+int btf__get_from_id(__u32 id, struct btf **btf);
 
 LIBBPF_API int btf__finalize_data(struct bpf_object *obj, struct btf *btf);
 LIBBPF_API int btf__load(struct btf *btf);
diff --git a/tools/lib/bpf/libbpf_common.h b/tools/lib/bpf/libbpf_common.h
index 947d8bd8a7bb..9ba9f8135dc8 100644
--- a/tools/lib/bpf/libbpf_common.h
+++ b/tools/lib/bpf/libbpf_common.h
@@ -17,6 +17,28 @@
 
 #define LIBBPF_DEPRECATED(msg) __attribute__((deprecated(msg)))
 
+#ifndef LIBBPF_DEPRECATED_SINCE
+#define __LIBBPF_VERSION_CHECK(major, minor) \
+	LIBBPF_MAJOR_VERSION > major || \
+		(LIBBPF_MAJOR_VERSION == major && LIBBPF_MINOR_VERSION >= minor)
+
+/* Add checks for other versions below when planning deprecation of API symbols
+ * with the LIBBPF_DEPRECATED_SINCE macro.
+ */
+#if __LIBBPF_VERSION_CHECK(0, 6)
+#define __LIBBPF_MARK_DEPRECATED_0_6(X) X
+#else
+#define __LIBBPF_MARK_DEPRECATED_0_6(X)
+#endif
+
+#define __LIBBPF_DEPRECATED_SINCE(major, minor, msg) \
+	__LIBBPF_MARK_DEPRECATED_ ## major ## _ ## minor (LIBBPF_DEPRECATED("v" # major "." # minor "+, " msg))
+
+/* Mark a symbol as deprecated when libbpf version is >= {major}.{minor} */
+#define LIBBPF_DEPRECATED_SINCE(major, minor, msg) \
+	__LIBBPF_DEPRECATED_SINCE(major, minor, msg)
+#endif /* LIBBPF_DEPRECATED_SINCE */
+
 /* Helper macro to declare and initialize libbpf options struct
  *
  * This dance with uninitialized declaration, followed by memset to zero,

Re: [PATCH bpf-next v2 0/5] libbpf: rename btf__get_from_id() and btf__load() APIs, support split BTF

[Andrii Nakryiko]

On Tue, Jul 27, 2021 at 4:39 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> 2021-07-23 08:51 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > On Fri, Jul 23, 2021 at 2:58 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>
> >> 2021-07-22 19:45 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> >>> On Thu, Jul 22, 2021 at 5:58 PM Andrii Nakryiko
> >>> <andrii.nakryiko@gmail.com> wrote:
> >>>>
> >>>> On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>>>>
> >>>>> As part of the effort to move towards a v1.0 for libbpf [0], this set
> >>>>> improves some confusing function names related to BTF loading from and to
> >>>>> the kernel:
> >>>>>
> >>>>> - btf__load() becomes btf__load_into_kernel().
> >>>>> - btf__get_from_id becomes btf__load_from_kernel_by_id().
> >>>>> - A new version btf__load_from_kernel_by_id_split() extends the former to
> >>>>>   add support for split BTF.
> >>>>>
> >>>>> The old functions are not removed or marked as deprecated yet, there
> >>>>> should be in a future libbpf version.
> >>>>
> >>>> Oh, and I was thinking about this whole deprecation having to be done
> >>>> in two steps. It's super annoying to keep track of that. Ideally, we'd
> >>>> have some macro that can mark API deprecated "in the future", when
> >>>> actual libbpf version is >= to defined version. So something like
> >>>> this:
> >>>>
> >>>> LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6")
> >>>
> >>> Better:
> >>>
> >>> LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6")
>
> So I've been looking into this, and it's not _that_ simple to do. Unless
> I missed something about preprocessing macros, I cannot bake a "#if" in
> a "#define", to have the attribute printed if and only if the current
> version is >= 0.6 in this example.
>
> I've come up with something, but it is not optimal because I have to
> write a check and macros for each version number used with the
> LIBBPF_DEPRECATED_SINCE macro. If we really wanted to automate that part
> I guess we could generate a header with those macros from the Makefile
> and include it in libbpf_common.h, but that does not really look much
> cleaner to me.

Yeah, let's not add unnecessary code generation. It sucks, of course,
that we can't do #ifdef inside a macro :(

So it's either do something like what you did with defining
version-specific macros, which is actually not too bad, because it's
not like we have tons of those versions anyways.

LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead")
LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);

Alternatively, we can go with:

#if LIBBPF_AT_OR_NEWER(0, 6)
LIBBPF_DEPRECATED("use btf__load_from_kernel_by_id instead")
#endif
LIBBPF API int btf__get_from_id(__u32 id, struct btf **btf);

I don't really dislike the second variant too much either, but
LIBBPF_DEPRECATED_SINCE() reads nicer. Let's go with that. See some
comments below about implementation.

>
> Here's my current code, below - does it correspond to what you had in
> mind? Or did you think of something else?
>
> ------
>
> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> index ec14aa725bb0..095d5dc30d50 100644
> --- a/tools/lib/bpf/Makefile
> +++ b/tools/lib/bpf/Makefile
> @@ -8,6 +8,7 @@ LIBBPF_VERSION := $(shell \
>         grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
>         sort -rV | head -n1 | cut -d'_' -f2)
>  LIBBPF_MAJOR_VERSION := $(firstword $(subst ., ,$(LIBBPF_VERSION)))
> +LIBBPF_MINOR_VERSION := $(firstword $(subst ., ,$(subst $(LIBBPF_MAJOR_VERSION)., ,$(LIBBPF_VERSION))))

Given all this is for internal use, I'd instead define something like
__LIBBPF_CURVER as an integer that is easy to compare against:

#define __LIBBPF_CURVER (LIBBPF_MAJOR_VERSION * 100 +
LIBBPF_MINOR_VERSION) * 100 + LIBBPF_PATCH_VERSION

That will simplify some stuff below and is generally easier to use in
code, if we will need this somewhere to use explicitly.

>
>  MAKEFLAGS += --no-print-directory
>
> @@ -86,6 +87,8 @@ override CFLAGS += -Werror -Wall
>  override CFLAGS += $(INCLUDES)
>  override CFLAGS += -fvisibility=hidden
>  override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
> +override CFLAGS += -DLIBBPF_MAJOR_VERSION=$(LIBBPF_MAJOR_VERSION)
> +override CFLAGS += -DLIBBPF_MINOR_VERSION=$(LIBBPF_MINOR_VERSION)
>
>  # flags specific for shared library
>  SHLIB_FLAGS := -DSHARED -fPIC
> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index cf8490f95641..8b6b5442dbd8 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -45,7 +45,8 @@ LIBBPF_API struct btf *btf__parse_raw(const char *path);
>  LIBBPF_API struct btf *btf__parse_raw_split(const char *path, struct btf *base_btf);
>  LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id);
>  LIBBPF_API struct btf *btf__load_from_kernel_by_id_split(__u32 id, struct btf *base_btf);
> -LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
> +LIBBPF_API LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead")

nit: given how long those deprecations will be, let's keep them at a
separate (first) line and keep LIBBPF_API near the function
declaration itself

> +int btf__get_from_id(__u32 id, struct btf **btf);
>
>  LIBBPF_API int btf__finalize_data(struct bpf_object *obj, struct btf *btf);
>  LIBBPF_API int btf__load(struct btf *btf);
> diff --git a/tools/lib/bpf/libbpf_common.h b/tools/lib/bpf/libbpf_common.h
> index 947d8bd8a7bb..9ba9f8135dc8 100644
> --- a/tools/lib/bpf/libbpf_common.h
> +++ b/tools/lib/bpf/libbpf_common.h
> @@ -17,6 +17,28 @@
>
>  #define LIBBPF_DEPRECATED(msg) __attribute__((deprecated(msg)))
>
> +#ifndef LIBBPF_DEPRECATED_SINCE

why #ifndef conditional?

> +#define __LIBBPF_VERSION_CHECK(major, minor) \
> +       LIBBPF_MAJOR_VERSION > major || \
> +               (LIBBPF_MAJOR_VERSION == major && LIBBPF_MINOR_VERSION >= minor)

so we don't need this if we do __LIBBPF_CURVER

> +
> +/* Add checks for other versions below when planning deprecation of API symbols
> + * with the LIBBPF_DEPRECATED_SINCE macro.
> + */
> +#if __LIBBPF_VERSION_CHECK(0, 6)
> +#define __LIBBPF_MARK_DEPRECATED_0_6(X) X
> +#else
> +#define __LIBBPF_MARK_DEPRECATED_0_6(X)
> +#endif
> +
> +#define __LIBBPF_DEPRECATED_SINCE(major, minor, msg) \
> +       __LIBBPF_MARK_DEPRECATED_ ## major ## _ ## minor (LIBBPF_DEPRECATED("v" # major "." # minor "+, " msg))
> +
> +/* Mark a symbol as deprecated when libbpf version is >= {major}.{minor} */
> +#define LIBBPF_DEPRECATED_SINCE(major, minor, msg) \
> +       __LIBBPF_DEPRECATED_SINCE(major, minor, msg)

Is it needed for some macro value concatenation magic to have this
nested __LIBBPF_DEPRECATED_SINCE?

> +#endif /* LIBBPF_DEPRECATED_SINCE */
> +
>  /* Helper macro to declare and initialize libbpf options struct
>   *
>   * This dance with uninitialized declaration, followed by memset to zero,

Re: [PATCH bpf-next v2 0/5] libbpf: rename btf__get_from_id() and btf__load() APIs, support split BTF

[Quentin Monnet]

On Tue, 27 Jul 2021 at 21:49, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> > >>>
> > >>> LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6")
> >
> > So I've been looking into this, and it's not _that_ simple to do. Unless
> > I missed something about preprocessing macros, I cannot bake a "#if" in
> > a "#define", to have the attribute printed if and only if the current
> > version is >= 0.6 in this example.
> >
> > I've come up with something, but it is not optimal because I have to
> > write a check and macros for each version number used with the
> > LIBBPF_DEPRECATED_SINCE macro. If we really wanted to automate that part
> > I guess we could generate a header with those macros from the Makefile
> > and include it in libbpf_common.h, but that does not really look much
> > cleaner to me.
>
> Yeah, let's not add unnecessary code generation. It sucks, of course,
> that we can't do #ifdef inside a macro :(
>
> So it's either do something like what you did with defining
> version-specific macros, which is actually not too bad, because it's
> not like we have tons of those versions anyways.
>
> LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead")
> LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
>
> Alternatively, we can go with:
>
> #if LIBBPF_AT_OR_NEWER(0, 6)
> LIBBPF_DEPRECATED("use btf__load_from_kernel_by_id instead")
> #endif
> LIBBPF API int btf__get_from_id(__u32 id, struct btf **btf);
>
> I don't really dislike the second variant too much either, but
> LIBBPF_DEPRECATED_SINCE() reads nicer. Let's go with that. See some
> comments below about implementation.

Ok.

>
> >
> > Here's my current code, below - does it correspond to what you had in
> > mind? Or did you think of something else?
> >
> > ------
> >
> > diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> > index ec14aa725bb0..095d5dc30d50 100644
> > --- a/tools/lib/bpf/Makefile
> > +++ b/tools/lib/bpf/Makefile
> > @@ -8,6 +8,7 @@ LIBBPF_VERSION := $(shell \
> >         grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
> >         sort -rV | head -n1 | cut -d'_' -f2)
> >  LIBBPF_MAJOR_VERSION := $(firstword $(subst ., ,$(LIBBPF_VERSION)))
> > +LIBBPF_MINOR_VERSION := $(firstword $(subst ., ,$(subst $(LIBBPF_MAJOR_VERSION)., ,$(LIBBPF_VERSION))))
>
> Given all this is for internal use, I'd instead define something like
> __LIBBPF_CURVER as an integer that is easy to compare against:
>
> #define __LIBBPF_CURVER (LIBBPF_MAJOR_VERSION * 100 +
> LIBBPF_MINOR_VERSION) * 100 + LIBBPF_PATCH_VERSION
>
> That will simplify some stuff below and is generally easier to use in
> code, if we will need this somewhere to use explicitly.

Did you mean computing __LIBBPF_CURVER in the Makefile, or in the
header?

I can do that if you want, although I'm not convinced it will simplify
much. Instead of having one long-ish condition, we'll have to compute
the integer for the current version, as well as for each of the versions
that we list for deprecating functions. I suppose I can add another
dedicated macro.

Do you actually want the patch version? I chose to leave it aside
because 1) I thought it would not be relevant for deprecating symbols,
and 2) if anything like a -rc1 suffix is ever appended to the version,
it makes it more complex to parse from the version string.

>
> >
> >  MAKEFLAGS += --no-print-directory
> >
> > @@ -86,6 +87,8 @@ override CFLAGS += -Werror -Wall
> >  override CFLAGS += $(INCLUDES)
> >  override CFLAGS += -fvisibility=hidden
> >  override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
> > +override CFLAGS += -DLIBBPF_MAJOR_VERSION=$(LIBBPF_MAJOR_VERSION)
> > +override CFLAGS += -DLIBBPF_MINOR_VERSION=$(LIBBPF_MINOR_VERSION)
> >
> >  # flags specific for shared library
> >  SHLIB_FLAGS := -DSHARED -fPIC
> > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> > index cf8490f95641..8b6b5442dbd8 100644
> > --- a/tools/lib/bpf/btf.h
> > +++ b/tools/lib/bpf/btf.h
> > @@ -45,7 +45,8 @@ LIBBPF_API struct btf *btf__parse_raw(const char *path);
> >  LIBBPF_API struct btf *btf__parse_raw_split(const char *path, struct btf *base_btf);
> >  LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id);
> >  LIBBPF_API struct btf *btf__load_from_kernel_by_id_split(__u32 id, struct btf *base_btf);
> > -LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
> > +LIBBPF_API LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead")
>
> nit: given how long those deprecations will be, let's keep them at a
> separate (first) line and keep LIBBPF_API near the function
> declaration itself

I thought having the LIBBPF_API on a separate line would slightly reduce
the risk, when moving lines around, to move the function prototype but
not the deprecation attribute. But ok, fine.

>
> > +int btf__get_from_id(__u32 id, struct btf **btf);
> >
> >  LIBBPF_API int btf__finalize_data(struct bpf_object *obj, struct btf *btf);
> >  LIBBPF_API int btf__load(struct btf *btf);
> > diff --git a/tools/lib/bpf/libbpf_common.h b/tools/lib/bpf/libbpf_common.h
> > index 947d8bd8a7bb..9ba9f8135dc8 100644
> > --- a/tools/lib/bpf/libbpf_common.h
> > +++ b/tools/lib/bpf/libbpf_common.h
> > @@ -17,6 +17,28 @@
> >
> >  #define LIBBPF_DEPRECATED(msg) __attribute__((deprecated(msg)))
> >
> > +#ifndef LIBBPF_DEPRECATED_SINCE
>
> why #ifndef conditional?

Right, we don't expect to have the macro defined elsewhere. I'll remove
it.

>
> > +#define __LIBBPF_VERSION_CHECK(major, minor) \
> > +       LIBBPF_MAJOR_VERSION > major || \
> > +               (LIBBPF_MAJOR_VERSION == major && LIBBPF_MINOR_VERSION >= minor)
>
> so we don't need this if we do __LIBBPF_CURVER

Right, but we do need to compute an integer for each of the versions
listed below (0.6 for now). I'll see if I can come up with something
short.

>
> > +
> > +/* Add checks for other versions below when planning deprecation of API symbols
> > + * with the LIBBPF_DEPRECATED_SINCE macro.
> > + */
> > +#if __LIBBPF_VERSION_CHECK(0, 6)
> > +#define __LIBBPF_MARK_DEPRECATED_0_6(X) X
> > +#else
> > +#define __LIBBPF_MARK_DEPRECATED_0_6(X)
> > +#endif
> > +
> > +#define __LIBBPF_DEPRECATED_SINCE(major, minor, msg) \
> > +       __LIBBPF_MARK_DEPRECATED_ ## major ## _ ## minor (LIBBPF_DEPRECATED("v" # major "." # minor "+, " msg))
> > +
> > +/* Mark a symbol as deprecated when libbpf version is >= {major}.{minor} */
> > +#define LIBBPF_DEPRECATED_SINCE(major, minor, msg) \
> > +       __LIBBPF_DEPRECATED_SINCE(major, minor, msg)
>
> Is it needed for some macro value concatenation magic to have this
> nested __LIBBPF_DEPRECATED_SINCE?

I double-checked (I needed to, anyway), and it seems not. It's a
leftover from an earlier version of my code, I'll clean it up before
the proper submission.

Thanks!
Quentin

Re: [PATCH bpf-next v2 0/5] libbpf: rename btf__get_from_id() and btf__load() APIs, support split BTF

[Andrii Nakryiko]

On Wed, Jul 28, 2021 at 2:54 PM Quentin Monnet <quentin@isovalent.com> wrote:
>
> On Tue, 27 Jul 2021 at 21:49, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> > > >>>
> > > >>> LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6")
> > >
> > > So I've been looking into this, and it's not _that_ simple to do. Unless
> > > I missed something about preprocessing macros, I cannot bake a "#if" in
> > > a "#define", to have the attribute printed if and only if the current
> > > version is >= 0.6 in this example.
> > >
> > > I've come up with something, but it is not optimal because I have to
> > > write a check and macros for each version number used with the
> > > LIBBPF_DEPRECATED_SINCE macro. If we really wanted to automate that part
> > > I guess we could generate a header with those macros from the Makefile
> > > and include it in libbpf_common.h, but that does not really look much
> > > cleaner to me.
> >
> > Yeah, let's not add unnecessary code generation. It sucks, of course,
> > that we can't do #ifdef inside a macro :(
> >
> > So it's either do something like what you did with defining
> > version-specific macros, which is actually not too bad, because it's
> > not like we have tons of those versions anyways.
> >
> > LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead")
> > LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
> >
> > Alternatively, we can go with:
> >
> > #if LIBBPF_AT_OR_NEWER(0, 6)
> > LIBBPF_DEPRECATED("use btf__load_from_kernel_by_id instead")
> > #endif
> > LIBBPF API int btf__get_from_id(__u32 id, struct btf **btf);
> >
> > I don't really dislike the second variant too much either, but
> > LIBBPF_DEPRECATED_SINCE() reads nicer. Let's go with that. See some
> > comments below about implementation.
>
> Ok.
>
> >
> > >
> > > Here's my current code, below - does it correspond to what you had in
> > > mind? Or did you think of something else?
> > >
> > > ------
> > >
> > > diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> > > index ec14aa725bb0..095d5dc30d50 100644
> > > --- a/tools/lib/bpf/Makefile
> > > +++ b/tools/lib/bpf/Makefile
> > > @@ -8,6 +8,7 @@ LIBBPF_VERSION := $(shell \
> > >         grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
> > >         sort -rV | head -n1 | cut -d'_' -f2)
> > >  LIBBPF_MAJOR_VERSION := $(firstword $(subst ., ,$(LIBBPF_VERSION)))
> > > +LIBBPF_MINOR_VERSION := $(firstword $(subst ., ,$(subst $(LIBBPF_MAJOR_VERSION)., ,$(LIBBPF_VERSION))))
> >
> > Given all this is for internal use, I'd instead define something like
> > __LIBBPF_CURVER as an integer that is easy to compare against:
> >
> > #define __LIBBPF_CURVER (LIBBPF_MAJOR_VERSION * 100 +
> > LIBBPF_MINOR_VERSION) * 100 + LIBBPF_PATCH_VERSION
> >
> > That will simplify some stuff below and is generally easier to use in
> > code, if we will need this somewhere to use explicitly.
>
> Did you mean computing __LIBBPF_CURVER in the Makefile, or in the
> header?

I was thinking Makefile, but if it's simpler to do in the header
that's fine as well.

>
> I can do that if you want, although I'm not convinced it will simplify
> much. Instead of having one long-ish condition, we'll have to compute
> the integer for the current version, as well as for each of the versions
> that we list for deprecating functions. I suppose I can add another
> dedicated macro.

feels like if we need to do some comparisons, then writing

#if __LIBBPF_VER > 102
/* do something */
#endif

is much simpler than comparing MAJOR_VERSION and MINOR_VERSION
separately. It's just that currently with 0 major version it might
look a bit awkward right now, but that's temporary.

>
> Do you actually want the patch version? I chose to leave it aside
> because 1) I thought it would not be relevant for deprecating symbols,
> and 2) if anything like a -rc1 suffix is ever appended to the version,
> it makes it more complex to parse from the version string.

yeah, you are probably right. major  and minor should be enough

>
> >
> > >
> > >  MAKEFLAGS += --no-print-directory
> > >
> > > @@ -86,6 +87,8 @@ override CFLAGS += -Werror -Wall
> > >  override CFLAGS += $(INCLUDES)
> > >  override CFLAGS += -fvisibility=hidden
> > >  override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
> > > +override CFLAGS += -DLIBBPF_MAJOR_VERSION=$(LIBBPF_MAJOR_VERSION)
> > > +override CFLAGS += -DLIBBPF_MINOR_VERSION=$(LIBBPF_MINOR_VERSION)
> > >
> > >  # flags specific for shared library
> > >  SHLIB_FLAGS := -DSHARED -fPIC
> > > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> > > index cf8490f95641..8b6b5442dbd8 100644
> > > --- a/tools/lib/bpf/btf.h
> > > +++ b/tools/lib/bpf/btf.h
> > > @@ -45,7 +45,8 @@ LIBBPF_API struct btf *btf__parse_raw(const char *path);
> > >  LIBBPF_API struct btf *btf__parse_raw_split(const char *path, struct btf *base_btf);
> > >  LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id);
> > >  LIBBPF_API struct btf *btf__load_from_kernel_by_id_split(__u32 id, struct btf *base_btf);
> > > -LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
> > > +LIBBPF_API LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead")
> >
> > nit: given how long those deprecations will be, let's keep them at a
> > separate (first) line and keep LIBBPF_API near the function
> > declaration itself
>
> I thought having the LIBBPF_API on a separate line would slightly reduce
> the risk, when moving lines around, to move the function prototype but
> not the deprecation attribute. But ok, fine.

highly improbable and then we'll most probably catch it during build anyways

>
> >
> > > +int btf__get_from_id(__u32 id, struct btf **btf);
> > >
> > >  LIBBPF_API int btf__finalize_data(struct bpf_object *obj, struct btf *btf);
> > >  LIBBPF_API int btf__load(struct btf *btf);
> > > diff --git a/tools/lib/bpf/libbpf_common.h b/tools/lib/bpf/libbpf_common.h
> > > index 947d8bd8a7bb..9ba9f8135dc8 100644
> > > --- a/tools/lib/bpf/libbpf_common.h
> > > +++ b/tools/lib/bpf/libbpf_common.h
> > > @@ -17,6 +17,28 @@
> > >
> > >  #define LIBBPF_DEPRECATED(msg) __attribute__((deprecated(msg)))
> > >
> > > +#ifndef LIBBPF_DEPRECATED_SINCE
> >
> > why #ifndef conditional?
>
> Right, we don't expect to have the macro defined elsewhere. I'll remove
> it.
>
> >
> > > +#define __LIBBPF_VERSION_CHECK(major, minor) \
> > > +       LIBBPF_MAJOR_VERSION > major || \
> > > +               (LIBBPF_MAJOR_VERSION == major && LIBBPF_MINOR_VERSION >= minor)
> >
> > so we don't need this if we do __LIBBPF_CURVER
>
> Right, but we do need to compute an integer for each of the versions
> listed below (0.6 for now). I'll see if I can come up with something
> short.

see above, I'd just do 102 etc. I wonder if 006 will be treated as an
octal number, in that case probably fine to do just 6. Or we can have
a small macro for this, of course. Don't know, doesn't seem to matter
all that much

>
> >
> > > +
> > > +/* Add checks for other versions below when planning deprecation of API symbols
> > > + * with the LIBBPF_DEPRECATED_SINCE macro.
> > > + */
> > > +#if __LIBBPF_VERSION_CHECK(0, 6)
> > > +#define __LIBBPF_MARK_DEPRECATED_0_6(X) X
> > > +#else
> > > +#define __LIBBPF_MARK_DEPRECATED_0_6(X)
> > > +#endif
> > > +
> > > +#define __LIBBPF_DEPRECATED_SINCE(major, minor, msg) \
> > > +       __LIBBPF_MARK_DEPRECATED_ ## major ## _ ## minor (LIBBPF_DEPRECATED("v" # major "." # minor "+, " msg))
> > > +
> > > +/* Mark a symbol as deprecated when libbpf version is >= {major}.{minor} */
> > > +#define LIBBPF_DEPRECATED_SINCE(major, minor, msg) \
> > > +       __LIBBPF_DEPRECATED_SINCE(major, minor, msg)
> >
> > Is it needed for some macro value concatenation magic to have this
> > nested __LIBBPF_DEPRECATED_SINCE?
>
> I double-checked (I needed to, anyway), and it seems not. It's a
> leftover from an earlier version of my code, I'll clean it up before
> the proper submission.

ok, thanks

>
> Thanks!
> Quentin