On 08/09, Jakub Kicinski wrote: > On Fri, 9 Aug 2019 08:32:10 -0700, Stanislav Fomichev wrote: > > On 08/09, Peter Wu wrote: > > > /proc/config has never existed as far as I can see, but /proc/config.gz > > > is present on Arch Linux. Add support for decompressing config.gz using > > > zlib which is a mandatory dependency of libelf. Replace existing stdio > > > functions with gzFile operations since the latter transparently handles > > > uncompressed and gzip-compressed files. > > > > > > Cc: Quentin Monnet <quentin.mon...@netronome.com> > > > Signed-off-by: Peter Wu <pe...@lekensteyn.nl> > > Thanks for the patch, looks good to me now! > > > > tools/bpf/bpftool/Makefile | 2 +- > > > tools/bpf/bpftool/feature.c | 105 ++++++++++++++++++------------------ > > > 2 files changed, 54 insertions(+), 53 deletions(-) > > > > > > diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile > > > index a7afea4dec47..078bd0dcfba5 100644 > > > --- a/tools/bpf/bpftool/Makefile > > > +++ b/tools/bpf/bpftool/Makefile > > > @@ -52,7 +52,7 @@ ifneq ($(EXTRA_LDFLAGS),) > > > LDFLAGS += $(EXTRA_LDFLAGS) > > > endif > > > > > > -LIBS = -lelf $(LIBBPF) > > > +LIBS = -lelf -lz $(LIBBPF) > > You're saying in the commit description that bpftool already links > > against -lz (via -lelf), but then explicitly add -lz here, why? > > It probably won't hurt to enable the zlib test: > > diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile > index 078bd0dcfba5..8176632e519c 100644 > --- a/tools/bpf/bpftool/Makefile > +++ b/tools/bpf/bpftool/Makefile > @@ -58,8 +58,8 @@ INSTALL ?= install > RM ?= rm -f > > FEATURE_USER = .bpftool > -FEATURE_TESTS = libbfd disassembler-four-args reallocarray > -FEATURE_DISPLAY = libbfd disassembler-four-args > +FEATURE_TESTS = libbfd disassembler-four-args reallocarray zlib > +FEATURE_DISPLAY = libbfd disassembler-four-args zlib > > check_feat := 1 > NON_CHECK_FEAT_TARGETS := clean uninstall doc doc-clean doc-install > doc-uninstall > > And then we can test for it the way libbpf tests for elf: > > all: zdep $(OUTPUT)bpftool > > PHONY += zdep > > zdep: > @if [ "$(feature-zlib)" != "1" ]; then echo "No zlib found"; exit 1 ; fi > > Or maybe just $(error ...), Stan what's your preference here? > We don't have a precedent for hard tests of features in bpftool. I'm just being nit picky :-) Because changelog says we already depend on -lz, but then in the patch we explicitly add it.
I think you were right in pointing out that we already implicitly depend on -lz via -lelf and/or -lbfd. And it works for non-static builds. We don't need an explicit -lz unless somebody puts '-static' in EXTRA_CFLAGS. So maybe we should just submit the patch as is because it fixes make EXTRA_CFLAGS=-static. RE $(error): we don't do it for -lelf, right? So probably not worth the hassle for zlib.
