| Message ID | 20181112185328.125589-1-sdf@google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] bpftool: make libbfd optional | expand |
On Mon, 12 Nov 2018 10:53:28 -0800, Stanislav Fomichev wrote: > Make it possible to build bpftool without libbfd. This excludes support for > disassembling jit-ted code and prints an error if the user tries to use > these features. > > Tested by: > cat > FEATURES_DUMP.bpftool <<EOF > feature-libbfd=0 > feature-disassembler-four-args=1 > feature-reallocarray=0 > feature-libelf=1 > feature-libelf-mmap=1 > feature-bpf=1 > EOF > FEATURES_DUMP=$PWD/FEATURES_DUMP.bpftool make > ldd bpftool | grep libbfd > > Signed-off-by: Stanislav Fomichev <sdf@google.com> Would you mind spelling out the motivation? > diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h > index 61d82020af58..ec1bc2ae3c71 100644 > --- a/tools/bpf/bpftool/main.h > +++ b/tools/bpf/bpftool/main.h > @@ -147,8 +147,19 @@ int prog_parse_fd(int *argc, char ***argv); > int map_parse_fd(int *argc, char ***argv); > int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len); > > +#ifdef HAVE_LIBBFD_SUPPORT > void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes, > const char *arch, const char *disassembler_options); > +void disasm_init(void); > +#else > +static inline > +void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes, > + const char *arch, const char *disassembler_options) > +{ > + p_err("No libbfd support"); > +} I think an error per instruction is a bit much, could we make sure we error out earlier? > +static inline void disasm_init(void) {} > +#endif > void print_data_json(uint8_t *data, size_t len); > void print_hex_data_json(uint8_t *data, size_t len); Otherwise LGTM.
On 11/12, Jakub Kicinski wrote: > On Mon, 12 Nov 2018 10:53:28 -0800, Stanislav Fomichev wrote: > > Make it possible to build bpftool without libbfd. This excludes support for > > disassembling jit-ted code and prints an error if the user tries to use > > these features. > > > > Tested by: > > cat > FEATURES_DUMP.bpftool <<EOF > > feature-libbfd=0 > > feature-disassembler-four-args=1 > > feature-reallocarray=0 > > feature-libelf=1 > > feature-libelf-mmap=1 > > feature-bpf=1 > > EOF > > FEATURES_DUMP=$PWD/FEATURES_DUMP.bpftool make > > ldd bpftool | grep libbfd > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > Would you mind spelling out the motivation? Ack, will do in v2 commit message. tldr - some fleet machines don't have dev/dbg stuff installed; would like an option to have bpftool that works everywhere for introspection. > > diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h > > index 61d82020af58..ec1bc2ae3c71 100644 > > --- a/tools/bpf/bpftool/main.h > > +++ b/tools/bpf/bpftool/main.h > > @@ -147,8 +147,19 @@ int prog_parse_fd(int *argc, char ***argv); > > int map_parse_fd(int *argc, char ***argv); > > int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len); > > > > +#ifdef HAVE_LIBBFD_SUPPORT > > void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes, > > const char *arch, const char *disassembler_options); > > +void disasm_init(void); > > +#else > > +static inline > > +void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes, > > + const char *arch, const char *disassembler_options) > > +{ > > + p_err("No libbfd support"); > > +} > > I think an error per instruction is a bit much, could we make sure we > error out earlier? I can return int from disasm_print_insn as an indication to stop/continue. Let me know if you have better ideas, will post v2 later today. > > +static inline void disasm_init(void) {} > > +#endif > > void print_data_json(uint8_t *data, size_t len); > > void print_hex_data_json(uint8_t *data, size_t len); > > Otherwise LGTM. Thanks!
On Mon, 12 Nov 2018 11:58:27 -0800, Stanislav Fomichev wrote: > On 11/12, Jakub Kicinski wrote: > > On Mon, 12 Nov 2018 10:53:28 -0800, Stanislav Fomichev wrote: > > > diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h > > > index 61d82020af58..ec1bc2ae3c71 100644 > > > --- a/tools/bpf/bpftool/main.h > > > +++ b/tools/bpf/bpftool/main.h > > > @@ -147,8 +147,19 @@ int prog_parse_fd(int *argc, char ***argv); > > > int map_parse_fd(int *argc, char ***argv); > > > int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len); > > > > > > +#ifdef HAVE_LIBBFD_SUPPORT > > > void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes, > > > const char *arch, const char *disassembler_options); > > > +void disasm_init(void); > > > +#else > > > +static inline > > > +void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes, > > > + const char *arch, const char *disassembler_options) > > > +{ > > > + p_err("No libbfd support"); > > > +} > > > > I think an error per instruction is a bit much, could we make sure we > > error out earlier? > > I can return int from disasm_print_insn as an indication to > stop/continue. Let me know if you have better ideas, will post v2 later > today. Why not simply: diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c index 5302ee282409..44edf6705ad2 100644 --- a/tools/bpf/bpftool/prog.c +++ b/tools/bpf/bpftool/prog.c @@ -466,6 +466,10 @@ static int do_dump(int argc, char **argv) int fd; if (is_prefix(*argv, "jited")) { +#ifndef HAVE_LIBBFD_SUPPORT + p_err("No libbfd support"); + return -1; +#endif member_len = &info.jited_prog_len; member_ptr = &info.jited_prog_insns; } else if (is_prefix(*argv, "xlated")) { Perhaps wrapped into some helper in the header to avoid the yuckiness.
On 11/12, Jakub Kicinski wrote: > On Mon, 12 Nov 2018 11:58:27 -0800, Stanislav Fomichev wrote: > > On 11/12, Jakub Kicinski wrote: > > > On Mon, 12 Nov 2018 10:53:28 -0800, Stanislav Fomichev wrote: > > > > diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h > > > > index 61d82020af58..ec1bc2ae3c71 100644 > > > > --- a/tools/bpf/bpftool/main.h > > > > +++ b/tools/bpf/bpftool/main.h > > > > @@ -147,8 +147,19 @@ int prog_parse_fd(int *argc, char ***argv); > > > > int map_parse_fd(int *argc, char ***argv); > > > > int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len); > > > > > > > > +#ifdef HAVE_LIBBFD_SUPPORT > > > > void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes, > > > > const char *arch, const char *disassembler_options); > > > > +void disasm_init(void); > > > > +#else > > > > +static inline > > > > +void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes, > > > > + const char *arch, const char *disassembler_options) > > > > +{ > > > > + p_err("No libbfd support"); > > > > +} > > > > > > I think an error per instruction is a bit much, could we make sure we > > > error out earlier? > > > > I can return int from disasm_print_insn as an indication to > > stop/continue. Let me know if you have better ideas, will post v2 later > > today. > > Why not simply: Makes sense. I think I can actually repurpose disasm_init for that (the function I added to wrap bfd_init, I think I can move the callsite here without any problems). > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > index 5302ee282409..44edf6705ad2 100644 > --- a/tools/bpf/bpftool/prog.c > +++ b/tools/bpf/bpftool/prog.c > @@ -466,6 +466,10 @@ static int do_dump(int argc, char **argv) > int fd; > > if (is_prefix(*argv, "jited")) { > +#ifndef HAVE_LIBBFD_SUPPORT > + p_err("No libbfd support"); > + return -1; > +#endif > member_len = &info.jited_prog_len; > member_ptr = &info.jited_prog_insns; > } else if (is_prefix(*argv, "xlated")) { > > Perhaps wrapped into some helper in the header to avoid the yuckiness.
diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile index dac7eff4c7e5..1bea6b979082 100644 --- a/tools/bpf/bpftool/Makefile +++ b/tools/bpf/bpftool/Makefile @@ -53,7 +53,7 @@ ifneq ($(EXTRA_LDFLAGS),) LDFLAGS += $(EXTRA_LDFLAGS) endif -LIBS = -lelf -lbfd -lopcodes $(LIBBPF) +LIBS = -lelf $(LIBBPF) INSTALL ?= install RM ?= rm -f @@ -90,7 +90,16 @@ include $(wildcard $(OUTPUT)*.d) all: $(OUTPUT)bpftool -SRCS = $(wildcard *.c) +BFD_SRCS = jit_disasm.c + +SRCS = $(filter-out $(BFD_SRCS),$(wildcard *.c)) + +ifeq ($(feature-libbfd),1) +CFLAGS += -DHAVE_LIBBFD_SUPPORT +SRCS += $(BFD_SRCS) +LIBS += -lbfd -lopcodes +endif + OBJS = $(patsubst %.c,$(OUTPUT)%.o,$(SRCS)) $(OUTPUT)disasm.o $(OUTPUT)disasm.o: $(srctree)/kernel/bpf/disasm.c diff --git a/tools/bpf/bpftool/jit_disasm.c b/tools/bpf/bpftool/jit_disasm.c index c75ffd9ce2bb..6176bfe66f22 100644 --- a/tools/bpf/bpftool/jit_disasm.c +++ b/tools/bpf/bpftool/jit_disasm.c @@ -109,7 +109,7 @@ void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes, if (inf) { bfdf->arch_info = inf; } else { - p_err("No libfd support for %s", arch); + p_err("No libbfd support for %s", arch); return; } } @@ -183,3 +183,8 @@ void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes, bfd_close(bfdf); } + +void disasm_init(void) +{ + bfd_init(); +} diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c index 75a3296dc0bc..51aa71069b14 100644 --- a/tools/bpf/bpftool/main.c +++ b/tools/bpf/bpftool/main.c @@ -31,7 +31,6 @@ * SOFTWARE. */ -#include <bfd.h> #include <ctype.h> #include <errno.h> #include <getopt.h> @@ -399,7 +398,7 @@ int main(int argc, char **argv) if (argc < 0) usage(); - bfd_init(); + disasm_init(); ret = cmd_select(cmds, argc, argv, do_help); diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h index 61d82020af58..ec1bc2ae3c71 100644 --- a/tools/bpf/bpftool/main.h +++ b/tools/bpf/bpftool/main.h @@ -147,8 +147,19 @@ int prog_parse_fd(int *argc, char ***argv); int map_parse_fd(int *argc, char ***argv); int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len); +#ifdef HAVE_LIBBFD_SUPPORT void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes, const char *arch, const char *disassembler_options); +void disasm_init(void); +#else +static inline +void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes, + const char *arch, const char *disassembler_options) +{ + p_err("No libbfd support"); +} +static inline void disasm_init(void) {} +#endif void print_data_json(uint8_t *data, size_t len); void print_hex_data_json(uint8_t *data, size_t len);
Make it possible to build bpftool without libbfd. This excludes support for disassembling jit-ted code and prints an error if the user tries to use these features. Tested by: cat > FEATURES_DUMP.bpftool <<EOF feature-libbfd=0 feature-disassembler-four-args=1 feature-reallocarray=0 feature-libelf=1 feature-libelf-mmap=1 feature-bpf=1 EOF FEATURES_DUMP=$PWD/FEATURES_DUMP.bpftool make ldd bpftool | grep libbfd Signed-off-by: Stanislav Fomichev <sdf@google.com> --- tools/bpf/bpftool/Makefile | 13 +++++++++++-- tools/bpf/bpftool/jit_disasm.c | 7 ++++++- tools/bpf/bpftool/main.c | 3 +-- tools/bpf/bpftool/main.h | 11 +++++++++++ 4 files changed, 29 insertions(+), 5 deletions(-)