Re: [PATCH bpf-next 1/2] btf: separate btf creation and loading

[Yonghong Song <yhs@fb.com>]

On 2/5/19 11:48 AM, Andrii Nakryiko wrote:
> This change splits out previous btf__new functionality of constructing
> struct btf and loading it into kernel into two:
> - btf__new() just creates and initializes struct btf
> - btf__load() attempts to load existing struct btf into kernel
> 
> btf__free will still close BTF fd, if it was ever loaded successfully
> into kernel.
> 
> This change allows users of libbpf to manipulate BTF using its API,
> without the need to unnecessarily load it into kernel.
> 
> One of the intended use cases is pahole using libbpf to do DWARF to BTF
> conversion and deduplication using libbpf, while handling ELF sections
> overwrites and other concerns on its own.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> Acked-by: Song Liu <songliubraving@fb.com>
> ---
>   tools/lib/bpf/btf.c      | 53 ++++++++++++++++++++++------------------
>   tools/lib/bpf/btf.h      |  1 +
>   tools/lib/bpf/libbpf.c   |  2 +-
>   tools/lib/bpf/libbpf.map |  1 +
>   4 files changed, 32 insertions(+), 25 deletions(-)
> 
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 4949f8840bda..065d51fa63e5 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -366,8 +366,6 @@ void btf__free(struct btf *btf)
>   
>   struct btf *btf__new(__u8 *data, __u32 size)
>   {
> -	__u32 log_buf_size = 0;
> -	char *log_buf = NULL;
>   	struct btf *btf;
>   	int err;
>   
> @@ -377,15 +375,6 @@ struct btf *btf__new(__u8 *data, __u32 size)
>   
>   	btf->fd = -1;
>   
> -	log_buf = malloc(BPF_LOG_BUF_SIZE);
> -	if (!log_buf) {
> -		err = -ENOMEM;
> -		goto done;
> -	}
> -
> -	*log_buf = 0;
> -	log_buf_size = BPF_LOG_BUF_SIZE;
> -
>   	btf->data = malloc(size);
>   	if (!btf->data) {
>   		err = -ENOMEM;
> @@ -395,17 +384,6 @@ struct btf *btf__new(__u8 *data, __u32 size)
>   	memcpy(btf->data, data, size);
>   	btf->data_size = size;
>   
> -	btf->fd = bpf_load_btf(btf->data, btf->data_size,
> -			       log_buf, log_buf_size, false);
> -
> -	if (btf->fd == -1) {
> -		err = -errno;
> -		pr_warning("Error loading BTF: %s(%d)\n", strerror(errno), errno);
> -		if (log_buf && *log_buf)
> -			pr_warning("%s\n", log_buf);
> -		goto done;
> -	}
> -
>   	err = btf_parse_hdr(btf);
>   	if (err)
>   		goto done;
> @@ -417,8 +395,6 @@ struct btf *btf__new(__u8 *data, __u32 size)
>   	err = btf_parse_type_sec(btf);
>   
>   done:
> -	free(log_buf);
> -
>   	if (err) {
>   		btf__free(btf);
>   		return ERR_PTR(err);
> @@ -427,6 +403,35 @@ struct btf *btf__new(__u8 *data, __u32 size)
>   	return btf;
>   }
>   
> +int btf__load(struct btf* btf) {
> +	__u32 log_buf_size = BPF_LOG_BUF_SIZE;
> +	char *log_buf = NULL;
> +
> +	if (btf->fd >= 0) {
> +		return -EEXIST;
> +	}
> +
> +	log_buf = malloc(log_buf_size);
> +	if (!log_buf)
> +		return -ENOMEM;
> +
> +	*log_buf = 0;
> +
> +	btf->fd = bpf_load_btf(btf->data, btf->data_size,
> +			       log_buf, log_buf_size, false);
> +	if (btf->fd < 0) {
> +		btf->fd = -errno;

Why you set btf->fd = -errno? Do you have any intended use for it later.
If not, I would still prefer the existing value -1. This will be
consistent with all other fd field convention in libbpf.

> +		pr_warning("Error loading BTF: %s(%d)\n", strerror(errno), errno);
> +		if (*log_buf)
> +			pr_warning("%s\n", log_buf);
> +		goto done;
> +	}
> +
> +done:
> +	free(log_buf);
> +	return btf->fd;
> +}
> +
>   int btf__fd(const struct btf *btf)
>   {
>   	return btf->fd;
> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index 25a9d2db035d..e8410887f93a 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -57,6 +57,7 @@ struct btf_ext_header {
>   
>   LIBBPF_API void btf__free(struct btf *btf);
>   LIBBPF_API struct btf *btf__new(__u8 *data, __u32 size);
> +LIBBPF_API int btf__load(struct btf* btf);
>   LIBBPF_API __s32 btf__find_by_name(const struct btf *btf,
>   				   const char *type_name);
>   LIBBPF_API __u32 btf__get_nr_types(const struct btf *btf);
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 47969aa0faf8..75b82c1cfc72 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -835,7 +835,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
>   			obj->efile.maps_shndx = idx;
>   		else if (strcmp(name, BTF_ELF_SEC) == 0) {
>   			obj->btf = btf__new(data->d_buf, data->d_size);
> -			if (IS_ERR(obj->btf)) {
> +			if (IS_ERR(obj->btf) || btf__load(obj->btf) < 0) {
>   				pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n",
>   					   BTF_ELF_SEC, PTR_ERR(obj->btf));
>   				obj->btf = NULL;
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 89c1149e32ee..ffa1fe044f6a 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -134,6 +134,7 @@ LIBBPF_0.0.2 {
>   		bpf_object__find_map_fd_by_name;
>   		bpf_get_link_xdp_id;
>   		btf__dedup;
> +		btf__load;

Maybe put btf__load after btf__get_strings based on alphabetical order.
With the above changes,


>   		btf__get_map_kv_tids;
>   		btf__get_nr_types;
>   		btf__get_strings;
>