Message-ID: <7d24f2fd-0cba-e6dc-8b12-881583978ba9@fb.com>
Date:   Wed, 21 Nov 2018 20:18:35 +0000
From:   Yonghong Song <yhs@...com>
To:     Andrey Ignatov <rdna@...com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next 1/2] libbpf: Add version script for DSO



On 11/21/18 9:40 AM, Andrey Ignatov wrote:
> More and more projects use libbpf and one day it'll likely be packaged
> and distributed as DSO and that requires ABI versioning so that both
> compatible and incompatible changes to ABI can be introduced in a safe
> way in the future without breaking executables dynamically linked with a
> previous version of the library.
> 
> Usual way to do ABI versioning is version script for the linker. Add
> such a script for libbpf. All global symbols currently exported via
> LIBBPF_API macro are added to the version script libbpf.map.
> 
> The version name LIBBPF_0.0.1 is constructed from the name of the
> library + version specified by $(LIBBPF_VERSION) in Makefile.
> 
> Version script does not duplicate the work done by LIBBPF_API macro, it
> rather complements it. The macro is used at compile time and can be used
> by compiler to do optimization that can't be done at link time, it is
> purely about global symbol visibility. The version script, in turn, is
> used at link time and takes care of ABI versioning. Both techniques are
> described in details in [1].
> 
> Whenever ABI is changed in the future, version script should be changed
> appropriately.

Maybe we should clarify the policy of how version numbers should be 
change? Each commit which changes default global symbol ABI? Each kernel 
release?

> 
> [1] https://www.akkadia.org/drepper/dsohowto.pdf
> 
> Signed-off-by: Andrey Ignatov <rdna@...com>
> ---
>   tools/lib/bpf/Makefile   |   4 +-
>   tools/lib/bpf/libbpf.map | 120 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 123 insertions(+), 1 deletion(-)
>   create mode 100644 tools/lib/bpf/libbpf.map
> 
> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> index 425b480bda75..d76c41fa2d39 100644
> --- a/tools/lib/bpf/Makefile
> +++ b/tools/lib/bpf/Makefile
> @@ -145,6 +145,7 @@ include $(srctree)/tools/build/Makefile.include
>   
>   BPF_IN    := $(OUTPUT)libbpf-in.o
>   LIB_FILE := $(addprefix $(OUTPUT),$(LIB_FILE))
> +VERSION_SCRIPT := libbpf.map
>   
>   CMD_TARGETS = $(LIB_FILE)
>   
> @@ -170,7 +171,8 @@ $(BPF_IN): force elfdep bpfdep
>   	$(Q)$(MAKE) $(build)=libbpf
>   
>   $(OUTPUT)libbpf.so: $(BPF_IN)
> -	$(QUIET_LINK)$(CC) --shared $^ -o $@
> +	$(QUIET_LINK)$(CC) --shared -Wl,--version-script=$(VERSION_SCRIPT) \
> +		$^ -o $@
>   
>   $(OUTPUT)libbpf.a: $(BPF_IN)
>   	$(QUIET_LINK)$(RM) $@; $(AR) rcs $@ $^
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> new file mode 100644
> index 000000000000..9fe416b68c7d
> --- /dev/null
> +++ b/tools/lib/bpf/libbpf.map
> @@ -0,0 +1,120 @@
> +LIBBPF_0.0.1 {
> +	global:
> +		bpf_btf_get_fd_by_id;

Do you think we could use this opportunities to
make naming more consistent? For example,
bpf_btf_get_fd_by_id => btf__get_fd_by_id?

> +		bpf_create_map;
> +		bpf_create_map_in_map;
> +		bpf_create_map_in_map_node;
> +		bpf_create_map_name;
> +		bpf_create_map_node;
> +		bpf_create_map_xattr;
> +		bpf_load_btf;
> +		bpf_load_program;
> +		bpf_load_program_xattr;
> +		bpf_map__btf_key_type_id;
> +		bpf_map__btf_value_type_id;
> +		bpf_map__def;
> +		bpf_map_delete_elem; > +		bpf_map__fd;
> +		bpf_map_get_fd_by_id;
> +		bpf_map_get_next_id;
> +		bpf_map_get_next_key; > +		bpf_map__is_offload_neutral;
> +		bpf_map_lookup_and_delete_elem;
> +		bpf_map_lookup_elem;
> +		bpf_map__name;
> +		bpf_map__next;
> +		bpf_map__pin;
> +		bpf_map__prev;
> +		bpf_map__priv;
> +		bpf_map__reuse_fd;
> +		bpf_map__set_ifindex;
> +		bpf_map__set_priv;
> +		bpf_map__unpin;
> +		bpf_map_update_elem;
> +		bpf_object__btf_fd;
> +		bpf_object__close;
> +		bpf_object__find_map_by_name;
> +		bpf_object__find_map_by_offset;
> +		bpf_object__find_program_by_title;
> +		bpf_object__kversion;
> +		bpf_object__load;
> +		bpf_object__name;
> +		bpf_object__next;
> +		bpf_object__open;
> +		bpf_object__open_buffer;
> +		bpf_object__open_xattr;
> +		bpf_object__pin;
> +		bpf_object__pin_maps;
> +		bpf_object__pin_programs;
> +		bpf_object__priv;
> +		bpf_object__set_priv;
> +		bpf_object__unload;
> +		bpf_object__unpin_maps;
> +		bpf_object__unpin_programs;
> +		bpf_obj_get;
> +		bpf_obj_get_info_by_fd;
> +		bpf_obj_pin;
> +		bpf_perf_event_read_simple;
> +		bpf_prog_attach;
> +		bpf_prog_detach;
> +		bpf_prog_detach2;
> +		bpf_prog_get_fd_by_id;
> +		bpf_prog_get_next_id;
> +		bpf_prog_load;
> +		bpf_prog_load_xattr;
> +		bpf_prog_query;
> +		bpf_program__fd;
> +		bpf_program__is_kprobe;
> +		bpf_program__is_perf_event;
> +		bpf_program__is_raw_tracepoint;
> +		bpf_program__is_sched_act;
> +		bpf_program__is_sched_cls;
> +		bpf_program__is_socket_filter;
> +		bpf_program__is_tracepoint;
> +		bpf_program__is_xdp;
> +		bpf_program__load;
> +		bpf_program__next;
> +		bpf_program__nth_fd;
> +		bpf_program__pin;
> +		bpf_program__pin_instance;
> +		bpf_program__prev;
> +		bpf_program__priv;
> +		bpf_program__set_expected_attach_type;
> +		bpf_program__set_ifindex;
> +		bpf_program__set_kprobe;
> +		bpf_program__set_perf_event;
> +		bpf_program__set_prep;
> +		bpf_program__set_priv;
> +		bpf_program__set_raw_tracepoint;
> +		bpf_program__set_sched_act;
> +		bpf_program__set_sched_cls;
> +		bpf_program__set_socket_filter;
> +		bpf_program__set_tracepoint;
> +		bpf_program__set_type;
> +		bpf_program__set_xdp;
> +		bpf_program__title;
> +		bpf_program__unload;
> +		bpf_program__unpin;
> +		bpf_program__unpin_instance;
> +		bpf_prog_test_run;
> +		bpf_raw_tracepoint_open;
> +		bpf_set_link_xdp_fd;
> +		bpf_task_fd_query;
> +		bpf_verify_program;
> +		btf__fd;
> +		btf__find_by_name;
> +		btf__free;
> +		btf_get_from_id;
btf_get_from_id => btf__get_from_id?
> +		btf__name_by_offset;
> +		btf__new;
> +		btf__resolve_size;
> +		btf__resolve_type;
> +		btf__type_by_id;
> +		libbpf_attach_type_by_name;
> +		libbpf_get_error;
> +		libbpf_prog_type_by_name;
> +		libbpf_set_print;
> +		libbpf_strerror;

Anything else? We have btf__, bpf_program__ prefixes with double "_"
while with libbpf_, bpf_<wrapper> as single "_". Not sure whether they 
need change or not.

> +	local:
> +		*;
> +};
> 

Powered by blists - more mailing lists