| Message ID | 20190809003911.7852-1-peter@lekensteyn.nl |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [v3] tools: bpftool: fix reading from /proc/config.gz | expand |
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> > --- > v3: replace popen(gunzip) by linking directly to zlib. Reword commit > message, remove "Fixes" line. (this patch) > v2: fix style (reorder vars as reverse xmas tree, rename function, > braces), fallback to /proc/config.gz if uname() fails. > https://lkml.kernel.org/r/20190806010702.3303-1-peter@lekensteyn.nl > v1: https://lkml.kernel.org/r/20190805001541.8096-1-peter@lekensteyn.nl > > Hi, > > Thanks to Jakub for observing that zlib is already used by libelf, this > simplifies the patch tremendously as the same API can be used for both > compressed and uncompressed files. No special case exists anymore for > fclose/pclose. > > According to configure.ac in elfutils, zlib is mandatory, so I just > assume it to be available. For simplicity I also silently assume lines > to be less than 4096 characters. If that is not the case, then lines > will appear truncated, but that should not be an issue for the > CONFIG_xyz lines that we are scanning for. > > Jakub requested the handle leak fix to be posted separately against the > bpf tree, but since the whole code is rewritten I am not sure if it is > worth it. It is an unusual edge case: /boot/config-$(uname -r) could be > opened, but starts with unexpected data. > > Kind regards, > Peter > --- > 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? > INSTALL ?= install > RM ?= rm -f > diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c > index d672d9086fff..03bdc5b3ac49 100644 > --- a/tools/bpf/bpftool/feature.c > +++ b/tools/bpf/bpftool/feature.c > @@ -14,6 +14,7 @@ > > #include <bpf.h> > #include <libbpf.h> > +#include <zlib.h> > > #include "main.h" > > @@ -284,34 +285,32 @@ static void probe_jit_limit(void) > } > } > > -static char *get_kernel_config_option(FILE *fd, const char *option) > +static bool read_next_kernel_config_option(gzFile file, char *buf, size_t n, > + char **value) > { > - size_t line_n = 0, optlen = strlen(option); > - char *res, *strval, *line = NULL; > - ssize_t n; > + char *sep; > > - rewind(fd); > - while ((n = getline(&line, &line_n, fd)) > 0) { > - if (strncmp(line, option, optlen)) > + while (gzgets(file, buf, n)) { > + if (strncmp(buf, "CONFIG_", 7)) > continue; > - /* Check we have at least '=', value, and '\n' */ > - if (strlen(line) < optlen + 3) > - continue; > - if (*(line + optlen) != '=') > + > + sep = strchr(buf, '='); > + if (!sep) > continue; > > /* Trim ending '\n' */ > - line[strlen(line) - 1] = '\0'; > + buf[strlen(buf) - 1] = '\0'; > + > + /* Split on '=' and ensure that a value is present. */ > + *sep = '\0'; > + if (!sep[1]) > + continue; > > - /* Copy and return config option value */ > - strval = line + optlen + 1; > - res = strdup(strval); > - free(line); > - return res; > + *value = sep + 1; > + return true; > } > - free(line); > > - return NULL; > + return false; > } > > static void probe_kernel_image_config(void) > @@ -386,59 +385,61 @@ static void probe_kernel_image_config(void) > /* test_bpf module for BPF tests */ > "CONFIG_TEST_BPF", > }; > - char *value, *buf = NULL; > + char *values[ARRAY_SIZE(options)] = { }; > struct utsname utsn; > char path[PATH_MAX]; > - size_t i, n; > - ssize_t ret; > - FILE *fd; > + gzFile file = NULL; > + char buf[4096]; > + char *value; > + size_t i; > > - if (uname(&utsn)) > - goto no_config; > + if (!uname(&utsn)) { > + snprintf(path, sizeof(path), "/boot/config-%s", utsn.release); > > - snprintf(path, sizeof(path), "/boot/config-%s", utsn.release); > + /* gzopen also accepts uncompressed files. */ > + file = gzopen(path, "r"); > + } > > - fd = fopen(path, "r"); > - if (!fd && errno == ENOENT) { > - /* Some distributions put the config file at /proc/config, give > - * it a try. > - * Sometimes it is also at /proc/config.gz but we do not try > - * this one for now, it would require linking against libz. > + if (!file) { > + /* Some distributions build with CONFIG_IKCONFIG=y and put the > + * config file at /proc/config.gz. > */ > - fd = fopen("/proc/config", "r"); > + file = gzopen("/proc/config.gz", "r"); > } > - if (!fd) { > + if (!file) { > p_info("skipping kernel config, can't open file: %s", > strerror(errno)); > - goto no_config; > + goto end_parse; > } > /* Sanity checks */ > - ret = getline(&buf, &n, fd); > - ret = getline(&buf, &n, fd); > - if (!buf || !ret) { > + if (!gzgets(file, buf, sizeof(buf)) || > + !gzgets(file, buf, sizeof(buf))) { > p_info("skipping kernel config, can't read from file: %s", > strerror(errno)); > - free(buf); > - goto no_config; > + goto end_parse; > } > if (strcmp(buf, "# Automatically generated file; DO NOT EDIT.\n")) { > p_info("skipping kernel config, can't find correct file"); > - free(buf); > - goto no_config; > + goto end_parse; > } > - free(buf); > > - for (i = 0; i < ARRAY_SIZE(options); i++) { > - value = get_kernel_config_option(fd, options[i]); > - print_kernel_option(options[i], value); > - free(value); > + while (read_next_kernel_config_option(file, buf, sizeof(buf), &value)) { > + for (i = 0; i < ARRAY_SIZE(options); i++) { > + if (values[i] || strcmp(buf, options[i])) > + continue; > + > + values[i] = strdup(value); > + } > } > - fclose(fd); > - return; > > -no_config: > - for (i = 0; i < ARRAY_SIZE(options); i++) > - print_kernel_option(options[i], NULL); > +end_parse: > + if (file) > + gzclose(file); > + > + for (i = 0; i < ARRAY_SIZE(options); i++) { > + print_kernel_option(options[i], values[i]); > + free(values[i]); > + } > } > > static bool probe_bpf_syscall(const char *define_prefix) > -- > 2.22.0 >
2019-08-09 01:39 UTC+0100 ~ Peter Wu <peter@lekensteyn.nl> > /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> > --- > v3: replace popen(gunzip) by linking directly to zlib. Reword commit > message, remove "Fixes" line. (this patch) > v2: fix style (reorder vars as reverse xmas tree, rename function, > braces), fallback to /proc/config.gz if uname() fails. > https://lkml.kernel.org/r/20190806010702.3303-1-peter@lekensteyn.nl > v1: https://lkml.kernel.org/r/20190805001541.8096-1-peter@lekensteyn.nl > > Hi, > > Thanks to Jakub for observing that zlib is already used by libelf, this > simplifies the patch tremendously as the same API can be used for both > compressed and uncompressed files. No special case exists anymore for > fclose/pclose. > > According to configure.ac in elfutils, zlib is mandatory, so I just > assume it to be available. For simplicity I also silently assume lines > to be less than 4096 characters. If that is not the case, then lines > will appear truncated, but that should not be an issue for the > CONFIG_xyz lines that we are scanning for. > > Jakub requested the handle leak fix to be posted separately against the > bpf tree, but since the whole code is rewritten I am not sure if it is > worth it. It is an unusual edge case: /boot/config-$(uname -r) could be > opened, but starts with unexpected data. > > Kind regards, > Peter This version seems good to me, thanks! Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
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.
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.
On Fri, 9 Aug 2019 14:48:31 -0700, Stanislav Fomichev wrote: > 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. Mm. Sounds reasonable. Fixing EXTRA_CFLAGS=-static would be really cool, too, I always struggle to get a statically linked build. > RE $(error): we don't do it for -lelf, right? So probably not worth > the hassle for zlib. Right, OTOH bpftool doesn't really care about -lelf, it's libbpf that needs it, and libbpf does test.
Hi all, Thanks for the lovely feedback :) On Fri, Aug 09, 2019 at 02:57:26PM -0700, Jakub Kicinski wrote: > On Fri, 9 Aug 2019 14:48:31 -0700, Stanislav Fomichev wrote: > > I'm just being nit picky :-) > > Because changelog says we already depend on -lz, but then in the patch > > we explicitly add it. What I meant by that is that zlib is not a new dependency since it is already a mandatory dependency of libelf which is currently marked as mandatory dependency in bpftool. That is why I did not bother with adding a feature test either since it would be redundant. Adding an explicit dependency helps if you want to build bpftool as static binary, or if libelf somehow drops zlib in the future.
On 8/9/19 2:39 AM, 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> Applied, thanks. Please follow-up with a zlib feature test as suggested by Jakub.
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) INSTALL ?= install RM ?= rm -f diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c index d672d9086fff..03bdc5b3ac49 100644 --- a/tools/bpf/bpftool/feature.c +++ b/tools/bpf/bpftool/feature.c @@ -14,6 +14,7 @@ #include <bpf.h> #include <libbpf.h> +#include <zlib.h> #include "main.h" @@ -284,34 +285,32 @@ static void probe_jit_limit(void) } } -static char *get_kernel_config_option(FILE *fd, const char *option) +static bool read_next_kernel_config_option(gzFile file, char *buf, size_t n, + char **value) { - size_t line_n = 0, optlen = strlen(option); - char *res, *strval, *line = NULL; - ssize_t n; + char *sep; - rewind(fd); - while ((n = getline(&line, &line_n, fd)) > 0) { - if (strncmp(line, option, optlen)) + while (gzgets(file, buf, n)) { + if (strncmp(buf, "CONFIG_", 7)) continue; - /* Check we have at least '=', value, and '\n' */ - if (strlen(line) < optlen + 3) - continue; - if (*(line + optlen) != '=') + + sep = strchr(buf, '='); + if (!sep) continue; /* Trim ending '\n' */ - line[strlen(line) - 1] = '\0'; + buf[strlen(buf) - 1] = '\0'; + + /* Split on '=' and ensure that a value is present. */ + *sep = '\0'; + if (!sep[1]) + continue; - /* Copy and return config option value */ - strval = line + optlen + 1; - res = strdup(strval); - free(line); - return res; + *value = sep + 1; + return true; } - free(line); - return NULL; + return false; } static void probe_kernel_image_config(void) @@ -386,59 +385,61 @@ static void probe_kernel_image_config(void) /* test_bpf module for BPF tests */ "CONFIG_TEST_BPF", }; - char *value, *buf = NULL; + char *values[ARRAY_SIZE(options)] = { }; struct utsname utsn; char path[PATH_MAX]; - size_t i, n; - ssize_t ret; - FILE *fd; + gzFile file = NULL; + char buf[4096]; + char *value; + size_t i; - if (uname(&utsn)) - goto no_config; + if (!uname(&utsn)) { + snprintf(path, sizeof(path), "/boot/config-%s", utsn.release); - snprintf(path, sizeof(path), "/boot/config-%s", utsn.release); + /* gzopen also accepts uncompressed files. */ + file = gzopen(path, "r"); + } - fd = fopen(path, "r"); - if (!fd && errno == ENOENT) { - /* Some distributions put the config file at /proc/config, give - * it a try. - * Sometimes it is also at /proc/config.gz but we do not try - * this one for now, it would require linking against libz. + if (!file) { + /* Some distributions build with CONFIG_IKCONFIG=y and put the + * config file at /proc/config.gz. */ - fd = fopen("/proc/config", "r"); + file = gzopen("/proc/config.gz", "r"); } - if (!fd) { + if (!file) { p_info("skipping kernel config, can't open file: %s", strerror(errno)); - goto no_config; + goto end_parse; } /* Sanity checks */ - ret = getline(&buf, &n, fd); - ret = getline(&buf, &n, fd); - if (!buf || !ret) { + if (!gzgets(file, buf, sizeof(buf)) || + !gzgets(file, buf, sizeof(buf))) { p_info("skipping kernel config, can't read from file: %s", strerror(errno)); - free(buf); - goto no_config; + goto end_parse; } if (strcmp(buf, "# Automatically generated file; DO NOT EDIT.\n")) { p_info("skipping kernel config, can't find correct file"); - free(buf); - goto no_config; + goto end_parse; } - free(buf); - for (i = 0; i < ARRAY_SIZE(options); i++) { - value = get_kernel_config_option(fd, options[i]); - print_kernel_option(options[i], value); - free(value); + while (read_next_kernel_config_option(file, buf, sizeof(buf), &value)) { + for (i = 0; i < ARRAY_SIZE(options); i++) { + if (values[i] || strcmp(buf, options[i])) + continue; + + values[i] = strdup(value); + } } - fclose(fd); - return; -no_config: - for (i = 0; i < ARRAY_SIZE(options); i++) - print_kernel_option(options[i], NULL); +end_parse: + if (file) + gzclose(file); + + for (i = 0; i < ARRAY_SIZE(options); i++) { + print_kernel_option(options[i], values[i]); + free(values[i]); + } } static bool probe_bpf_syscall(const char *define_prefix)
/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> --- v3: replace popen(gunzip) by linking directly to zlib. Reword commit message, remove "Fixes" line. (this patch) v2: fix style (reorder vars as reverse xmas tree, rename function, braces), fallback to /proc/config.gz if uname() fails. https://lkml.kernel.org/r/20190806010702.3303-1-peter@lekensteyn.nl v1: https://lkml.kernel.org/r/20190805001541.8096-1-peter@lekensteyn.nl Hi, Thanks to Jakub for observing that zlib is already used by libelf, this simplifies the patch tremendously as the same API can be used for both compressed and uncompressed files. No special case exists anymore for fclose/pclose. According to configure.ac in elfutils, zlib is mandatory, so I just assume it to be available. For simplicity I also silently assume lines to be less than 4096 characters. If that is not the case, then lines will appear truncated, but that should not be an issue for the CONFIG_xyz lines that we are scanning for. Jakub requested the handle leak fix to be posted separately against the bpf tree, but since the whole code is rewritten I am not sure if it is worth it. It is an unusual edge case: /boot/config-$(uname -r) could be opened, but starts with unexpected data. Kind regards, Peter --- tools/bpf/bpftool/Makefile | 2 +- tools/bpf/bpftool/feature.c | 105 ++++++++++++++++++------------------ 2 files changed, 54 insertions(+), 53 deletions(-)