Re: [PATCH v3] tools: bpftool: fix reading from /proc/config.gz

[Stanislav Fomichev <sdf@fomichev.me>]

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.monnet@netronome.com>
> > > Signed-off-by: Peter Wu <peter@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.