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

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

[Peter Wu]

/proc/config has never existed as far as I can see, but /proc/config.gz
is present on Arch Linux. Execute an external gunzip program to avoid
linking to zlib and rework the option scanning code since a pipe is not
seekable. This also fixes a file handle leak on some error paths.

Fixes: 4567b983f78c ("tools: bpftool: add probes for kernel configuration options")
Cc: Quentin Monnet <quentin.monnet@netronome.com>
Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 tools/bpf/bpftool/feature.c | 92 +++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 40 deletions(-)

diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
index d672d9086fff..e9e10f582047 100644
--- a/tools/bpf/bpftool/feature.c
+++ b/tools/bpf/bpftool/feature.c
@@ -284,34 +284,34 @@ static void probe_jit_limit(void)
 	}
 }
 
-static char *get_kernel_config_option(FILE *fd, const char *option)
+static bool get_kernel_config_option(FILE *fd, char **buf_p, size_t *n_p,
+				     char **value)
 {
-	size_t line_n = 0, optlen = strlen(option);
-	char *res, *strval, *line = NULL;
-	ssize_t n;
+	char *sep;
+	ssize_t linelen;
 
-	rewind(fd);
-	while ((n = getline(&line, &line_n, fd)) > 0) {
-		if (strncmp(line, option, optlen))
+	while ((linelen = getline(buf_p, n_p, fd)) > 0) {
+		char *line = *buf_p;
+		if (strncmp(line, "CONFIG_", 7))
 			continue;
-		/* Check we have at least '=', value, and '\n' */
-		if (strlen(line) < optlen + 3)
-			continue;
-		if (*(line + optlen) != '=')
+
+		sep = memchr(line, '=', linelen);
+		if (!sep)
 			continue;
 
 		/* Trim ending '\n' */
-		line[strlen(line) - 1] = '\0';
+		line[linelen - 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,31 +386,34 @@ static void probe_kernel_image_config(void)
 		/* test_bpf module for BPF tests */
 		"CONFIG_TEST_BPF",
 	};
+	char *values[ARRAY_SIZE(options)] = { };
 	char *value, *buf = NULL;
 	struct utsname utsn;
 	char path[PATH_MAX];
 	size_t i, n;
 	ssize_t ret;
-	FILE *fd;
+	FILE *fd = NULL;
+	bool is_pipe = false;
 
 	if (uname(&utsn))
-		goto no_config;
+		goto end_parse;
 
 	snprintf(path, sizeof(path), "/boot/config-%s", utsn.release);
 
 	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.
+		/* Some distributions build with CONFIG_IKCONFIG=y and put the
+		 * config file at /proc/config.gz. We try to invoke an external
+		 * gzip program to avoid linking to libz.
+		 * Hide stderr to avoid interference with the JSON output.
 		 */
-		fd = fopen("/proc/config", "r");
+		fd = popen("gunzip -c /proc/config.gz 2>/dev/null", "r");
+		is_pipe = true;
 	}
 	if (!fd) {
 		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);
@@ -418,27 +421,36 @@ static void probe_kernel_image_config(void)
 	if (!buf || !ret) {
 		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;
+	}
+
+	while (get_kernel_config_option(fd, &buf, &n, &value)) {
+		for (i = 0; i < ARRAY_SIZE(options); i++) {
+			if (values[i] || strcmp(buf, options[i]))
+				continue;
+
+			values[i] = strdup(value);
+		}
+	}
+
+end_parse:
+	if (fd) {
+		if (is_pipe) {
+			if (pclose(fd))
+				p_info("failed to read /proc/config.gz");
+		} else
+			fclose(fd);
 	}
 	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);
+		print_kernel_option(options[i], values[i]);
+		free(values[i]);
 	}
-	fclose(fd);
-	return;
-
-no_config:
-	for (i = 0; i < ARRAY_SIZE(options); i++)
-		print_kernel_option(options[i], NULL);
 }
 
 static bool probe_bpf_syscall(const char *define_prefix)
-- 
2.22.0

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

[Quentin Monnet]

Hi Peter,

Thanks for looking into this (and for the fixes)! Some comments below.

2019-08-05 01:15 UTC+0100 ~ Peter Wu <peter@lekensteyn.nl>
> /proc/config has never existed as far as I can see, but /proc/config.gz

As far as I understood (from examining Cilium [0]), /proc/config _is_
used by some distributions, such as CoreOS. This is why we look at that
location in bpftool.

[0] https://github.com/cilium/cilium/blob/master/bpf/run_probes.sh#L42

> is present on Arch Linux. Execute an external gunzip program to avoid
> linking to zlib and rework the option scanning code since a pipe is not
> seekable. This also fixes a file handle leak on some error paths.
> 
> Fixes: 4567b983f78c ("tools: bpftool: add probes for kernel configuration options")
> Cc: Quentin Monnet <quentin.monnet@netronome.com>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>  tools/bpf/bpftool/feature.c | 92 +++++++++++++++++++++----------------
>  1 file changed, 52 insertions(+), 40 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
> index d672d9086fff..e9e10f582047 100644
> --- a/tools/bpf/bpftool/feature.c
> +++ b/tools/bpf/bpftool/feature.c
> @@ -284,34 +284,34 @@ static void probe_jit_limit(void)
>  	}
>  }
>  
> -static char *get_kernel_config_option(FILE *fd, const char *option)
> +static bool get_kernel_config_option(FILE *fd, char **buf_p, size_t *n_p,
> +				     char **value)

Maybe we could rename this function, and have "next" appear in it
somewhere? After your changes, it does not return the value for a
specific option anymore.

>  {
> -	size_t line_n = 0, optlen = strlen(option);
> -	char *res, *strval, *line = NULL;
> -	ssize_t n;
> +	char *sep;
> +	ssize_t linelen;

Please order the declarations in reverse-Christmas tree style.

>  
> -	rewind(fd);
> -	while ((n = getline(&line, &line_n, fd)) > 0) {
> -		if (strncmp(line, option, optlen))
> +	while ((linelen = getline(buf_p, n_p, fd)) > 0) {
> +		char *line = *buf_p;

Please leave a blank line after declarations.

> +		if (strncmp(line, "CONFIG_", 7))
>  			continue;
> -		/* Check we have at least '=', value, and '\n' */
> -		if (strlen(line) < optlen + 3)
> -			continue;
> -		if (*(line + optlen) != '=')
> +
> +		sep = memchr(line, '=', linelen);
> +		if (!sep)
>  			continue;
>  
>  		/* Trim ending '\n' */
> -		line[strlen(line) - 1] = '\0';
> +		line[linelen - 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,31 +386,34 @@ static void probe_kernel_image_config(void)
>  		/* test_bpf module for BPF tests */
>  		"CONFIG_TEST_BPF",
>  	};
> +	char *values[ARRAY_SIZE(options)] = { };
>  	char *value, *buf = NULL;
>  	struct utsname utsn;
>  	char path[PATH_MAX];
>  	size_t i, n;
>  	ssize_t ret;
> -	FILE *fd;
> +	FILE *fd = NULL;
> +	bool is_pipe = false;

Reverse-Christmas-tree style please.

>  
>  	if (uname(&utsn))
> -		goto no_config;
> +		goto end_parse;

Just thinking, maybe if uname() fails we can skip /boot/config-$(uname
-r) but still attempt to parse /proc/config{,.gz} instead of printing
only NULL options?

>  
>  	snprintf(path, sizeof(path), "/boot/config-%s", utsn.release);
>  
>  	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.
> +		/* Some distributions build with CONFIG_IKCONFIG=y and put the
> +		 * config file at /proc/config.gz. We try to invoke an external
> +		 * gzip program to avoid linking to libz.
> +		 * Hide stderr to avoid interference with the JSON output.

Because some distributions do use /proc/config, we should keep this. You
can probably add /proc/config.gz as another attempt below (or even
above) the current case?

>  		 */
> -		fd = fopen("/proc/config", "r");
> +		fd = popen("gunzip -c /proc/config.gz 2>/dev/null", "r");
> +		is_pipe = true;
>  	}
>  	if (!fd) {
>  		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);
> @@ -418,27 +421,36 @@ static void probe_kernel_image_config(void)
>  	if (!buf || !ret) {
>  		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;
> +	}
> +
> +	while (get_kernel_config_option(fd, &buf, &n, &value))> +		for (i = 0; i < ARRAY_SIZE(options); i++) {
> +			if (values[i] || strcmp(buf, options[i]))

Can we have an option set multiple times in the config file? If so,
maybe have a p_info() if values are different to warn users that
conflicting values were found?

(Reading the file just once is a nice improvement over my version, by
the way, thanks!)

> +				continue;
> +
> +			values[i] = strdup(value);
> +		}
> +	}
> +
> +end_parse:
> +	if (fd) {
> +		if (is_pipe) {
> +			if (pclose(fd))
> +				p_info("failed to read /proc/config.gz");
> +		} else
> +			fclose(fd);

Please use braces on all branches of the conditional statement (else).

Thanks,
Quentin

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

[Stanislav Fomichev]

On 08/05, Peter Wu wrote:
> /proc/config has never existed as far as I can see, but /proc/config.gz
> is present on Arch Linux. Execute an external gunzip program to avoid
> linking to zlib and rework the option scanning code since a pipe is not
> seekable. This also fixes a file handle leak on some error paths.
Thanks for doing that! One question: why not link against -lz instead?
With fork/execing gunzip you're just hiding this dependency.

You can add something like this to the Makefile:
ifeq ($(feature-zlib),1)
CLFAGS += -DHAVE_ZLIB
endif

And then conditionally add support for config.gz. Thoughts?

> 
> Fixes: 4567b983f78c ("tools: bpftool: add probes for kernel configuration options")
> Cc: Quentin Monnet <quentin.monnet@netronome.com>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>  tools/bpf/bpftool/feature.c | 92 +++++++++++++++++++++----------------
>  1 file changed, 52 insertions(+), 40 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
> index d672d9086fff..e9e10f582047 100644
> --- a/tools/bpf/bpftool/feature.c
> +++ b/tools/bpf/bpftool/feature.c
> @@ -284,34 +284,34 @@ static void probe_jit_limit(void)
>  	}
>  }
>  
> -static char *get_kernel_config_option(FILE *fd, const char *option)
> +static bool get_kernel_config_option(FILE *fd, char **buf_p, size_t *n_p,
> +				     char **value)
>  {
> -	size_t line_n = 0, optlen = strlen(option);
> -	char *res, *strval, *line = NULL;
> -	ssize_t n;
> +	char *sep;
> +	ssize_t linelen;
>  
> -	rewind(fd);
> -	while ((n = getline(&line, &line_n, fd)) > 0) {
> -		if (strncmp(line, option, optlen))
> +	while ((linelen = getline(buf_p, n_p, fd)) > 0) {
> +		char *line = *buf_p;
> +		if (strncmp(line, "CONFIG_", 7))
>  			continue;
> -		/* Check we have at least '=', value, and '\n' */
> -		if (strlen(line) < optlen + 3)
> -			continue;
> -		if (*(line + optlen) != '=')
> +
> +		sep = memchr(line, '=', linelen);
> +		if (!sep)
>  			continue;
>  
>  		/* Trim ending '\n' */
> -		line[strlen(line) - 1] = '\0';
> +		line[linelen - 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,31 +386,34 @@ static void probe_kernel_image_config(void)
>  		/* test_bpf module for BPF tests */
>  		"CONFIG_TEST_BPF",
>  	};
> +	char *values[ARRAY_SIZE(options)] = { };
>  	char *value, *buf = NULL;
>  	struct utsname utsn;
>  	char path[PATH_MAX];
>  	size_t i, n;
>  	ssize_t ret;
> -	FILE *fd;
> +	FILE *fd = NULL;
> +	bool is_pipe = false;
>  
>  	if (uname(&utsn))
> -		goto no_config;
> +		goto end_parse;
>  
>  	snprintf(path, sizeof(path), "/boot/config-%s", utsn.release);
>  
>  	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.
> +		/* Some distributions build with CONFIG_IKCONFIG=y and put the
> +		 * config file at /proc/config.gz. We try to invoke an external
> +		 * gzip program to avoid linking to libz.
> +		 * Hide stderr to avoid interference with the JSON output.
>  		 */
> -		fd = fopen("/proc/config", "r");
> +		fd = popen("gunzip -c /proc/config.gz 2>/dev/null", "r");
> +		is_pipe = true;
>  	}
>  	if (!fd) {
>  		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);
> @@ -418,27 +421,36 @@ static void probe_kernel_image_config(void)
>  	if (!buf || !ret) {
>  		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;
> +	}
> +
> +	while (get_kernel_config_option(fd, &buf, &n, &value)) {
> +		for (i = 0; i < ARRAY_SIZE(options); i++) {
> +			if (values[i] || strcmp(buf, options[i]))
> +				continue;
> +
> +			values[i] = strdup(value);
> +		}
> +	}
> +
> +end_parse:
> +	if (fd) {
> +		if (is_pipe) {
> +			if (pclose(fd))
> +				p_info("failed to read /proc/config.gz");
> +		} else
> +			fclose(fd);
>  	}
>  	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);
> +		print_kernel_option(options[i], values[i]);
> +		free(values[i]);
>  	}
> -	fclose(fd);
> -	return;
> -
> -no_config:
> -	for (i = 0; i < ARRAY_SIZE(options); i++)
> -		print_kernel_option(options[i], NULL);
>  }
>  
>  static bool probe_bpf_syscall(const char *define_prefix)
> -- 
> 2.22.0
> 

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

[Jakub Kicinski]

On Mon, 5 Aug 2019 08:29:36 -0700, Stanislav Fomichev wrote:
> On 08/05, Peter Wu wrote:
> > /proc/config has never existed as far as I can see, but /proc/config.gz
> > is present on Arch Linux. Execute an external gunzip program to avoid
> > linking to zlib and rework the option scanning code since a pipe is not
> > seekable. This also fixes a file handle leak on some error paths.  
> Thanks for doing that! One question: why not link against -lz instead?
> With fork/execing gunzip you're just hiding this dependency.
> 
> You can add something like this to the Makefile:
> ifeq ($(feature-zlib),1)
> CLFAGS += -DHAVE_ZLIB
> endif
> 
> And then conditionally add support for config.gz. Thoughts?

+1

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

[Peter Wu]

Hi all,

Thank you for your quick feedback, I will address them in the next
revision.

On Mon, Aug 05, 2019 at 11:41:09AM +0100, Quentin Monnet wrote:

> As far as I understood (from examining Cilium [0]), /proc/config _is_
> used by some distributions, such as CoreOS. This is why we look at that
> location in bpftool.
> 
> [0] https://github.com/cilium/cilium/blob/master/bpf/run_probes.sh#L42

This comment[1] says "CoreOS uses /proc/config", but I think that is a
typo and is supposed to say "/proc/config.gz". The original feature
request[2] uses "/boot/config" as example.

 [1]: https://github.com/cilium/cilium/pull/1065
 [2]: https://github.com/cilium/cilium/issues/891

Given that "/proc/config.gz" is the standard since at least v2.6.12-rc2,
and the official kernel has no mention of "/proc/config", I would like
to skip the latter. If someone has a need for this and it is not covered
by either /boot/config-$(uname -r) or /proc/config.gz, they could submit
a patch for it with links to documentation. How about that?

> > -static char *get_kernel_config_option(FILE *fd, const char *option)
> > +static bool get_kernel_config_option(FILE *fd, char **buf_p, size_t *n_p,
> > +				     char **value)
> 
> Maybe we could rename this function, and have "next" appear in it
> somewhere? After your changes, it does not return the value for a
> specific option anymore.

I have changed it to "read_next_kernel_config_option", let me know if
you prefer an alternative.

> >  {
> > -	size_t line_n = 0, optlen = strlen(option);
> > -	char *res, *strval, *line = NULL;
> > -	ssize_t n;
> > +	char *sep;
> > +	ssize_t linelen;
> 
> Please order the declarations in reverse-Christmas tree style.

Does this refer to the type, name, or full line length? I did not find
this in CodingStyle, the closest I could get is:
https://lore.kernel.org/patchwork/patch/732076/

I will assume the line length for now.

> >  static void probe_kernel_image_config(void)
> > @@ -386,31 +386,34 @@ static void probe_kernel_image_config(void)
> >  		/* test_bpf module for BPF tests */
> >  		"CONFIG_TEST_BPF",
> >  	};
> > +	char *values[ARRAY_SIZE(options)] = { };
> >  	char *value, *buf = NULL;
> >  	struct utsname utsn;
> >  	char path[PATH_MAX];
> >  	size_t i, n;
> >  	ssize_t ret;
> > -	FILE *fd;
> > +	FILE *fd = NULL;
> > +	bool is_pipe = false;
> 
> Reverse-Christmas-tree style please.

Even if that means moving lines? Something like this?

        char path[PATH_MAX];
   +    bool is_pipe = false;
   +    FILE *fd = NULL;
        size_t i, n;
        ssize_t ret;
   -    FILE *fd;

> >  	if (uname(&utsn))
> > -		goto no_config;
> > +		goto end_parse;
> 
> Just thinking, maybe if uname() fails we can skip /boot/config-$(uname
> -r) but still attempt to parse /proc/config{,.gz} instead of printing
> only NULL options?

Good idea, I'll try a bit harder if uname falls for whatever reason.

> Because some distributions do use /proc/config, we should keep this. You
> can probably add /proc/config.gz as another attempt below (or even
> above) the current case?

I doubt it is actually in use, it looks like a typo in the original PR.
This post only lists /proc/config.gz, /boot/config and
/boot/config-$(uname -r): https://superuser.com/questions/287371

> > +	while (get_kernel_config_option(fd, &buf, &n, &value))> +		for (i = 0; i < ARRAY_SIZE(options); i++) {
> > +			if (values[i] || strcmp(buf, options[i]))
> 
> Can we have an option set multiple times in the config file? If so,
> maybe have a p_info() if values are different to warn users that
> conflicting values were found?

scripts/kconfig/merge_config.sh seems to apply a merge strategy,
overwriting earlier values and warning about it. However this should be
rare given that it ended up at /proc/config.gz. For now I will favor
simplicity over complexity and keep the old situation. Let me know if
you prefer otherwise.


On Mon, Aug 05, 2019 at 12:06:49PM -0700, Jakub Kicinski wrote:
> On Mon, 5 Aug 2019 08:29:36 -0700, Stanislav Fomichev wrote:
> > On 08/05, Peter Wu wrote:
> > > /proc/config has never existed as far as I can see, but /proc/config.gz
> > > is present on Arch Linux. Execute an external gunzip program to avoid
> > > linking to zlib and rework the option scanning code since a pipe is not
> > > seekable. This also fixes a file handle leak on some error paths.  
> > Thanks for doing that! One question: why not link against -lz instead?
> > With fork/execing gunzip you're just hiding this dependency.
> > 
> > You can add something like this to the Makefile:
> > ifeq ($(feature-zlib),1)
> > CLFAGS += -DHAVE_ZLIB
> > endif
> > 
> > And then conditionally add support for config.gz. Thoughts?
> 
> +1

Given that the old code did not have this library dependency I did not
add it (the program would otherwise fail to run). Executing an external
process is similar to what tar does. I will look into linking directly
to zlib, thanks!
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl

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

[Quentin Monnet]

Hi Peter,

2019-08-06 00:54 UTC+0100 ~ Peter Wu <peter@lekensteyn.nl>
> Hi all,
> 
> Thank you for your quick feedback, I will address them in the next
> revision.
> 
> On Mon, Aug 05, 2019 at 11:41:09AM +0100, Quentin Monnet wrote:
> 
>> As far as I understood (from examining Cilium [0]), /proc/config _is_
>> used by some distributions, such as CoreOS. This is why we look at that
>> location in bpftool.
>>
>> [0] https://github.com/cilium/cilium/blob/master/bpf/run_probes.sh#L42
> 
> This comment[1] says "CoreOS uses /proc/config", but I think that is a
> typo and is supposed to say "/proc/config.gz". The original feature
> request[2] uses "/boot/config" as example.
> 
>  [1]: https://github.com/cilium/cilium/pull/1065
>  [2]: https://github.com/cilium/cilium/issues/891
> 
> Given that "/proc/config.gz" is the standard since at least v2.6.12-rc2,
> and the official kernel has no mention of "/proc/config", I would like
> to skip the latter. If someone has a need for this and it is not covered
> by either /boot/config-$(uname -r) or /proc/config.gz, they could submit
> a patch for it with links to documentation. How about that?

Ok, did a bit of research on my side as well, and I couldn't find a
solid reference to /proc/config either, so it seems you are correct.
Let's drop /proc/config for now. Thanks for investigating that!

> 
>>> -static char *get_kernel_config_option(FILE *fd, const char *option)
>>> +static bool get_kernel_config_option(FILE *fd, char **buf_p, size_t *n_p,
>>> +				     char **value)
>>
>> Maybe we could rename this function, and have "next" appear in it
>> somewhere? After your changes, it does not return the value for a
>> specific option anymore.
> 
> I have changed it to "read_next_kernel_config_option", let me know if
> you prefer an alternative.
> 

Sounds good to me.

>>>  {
>>> -	size_t line_n = 0, optlen = strlen(option);
>>> -	char *res, *strval, *line = NULL;
>>> -	ssize_t n;
>>> +	char *sep;
>>> +	ssize_t linelen;
>>
>> Please order the declarations in reverse-Christmas tree style.
> 
> Does this refer to the type, name, or full line length? I did not find
> this in CodingStyle, the closest I could get is:
> https://lore.kernel.org/patchwork/patch/732076/
> 
> I will assume the line length for now.

I am unsure this is in the CodingStyle, but fairly certain that this is
a convention for at least network-related code. And yes, as I understand
it refers to the length of the line.

> 
>>>  static void probe_kernel_image_config(void)
>>> @@ -386,31 +386,34 @@ static void probe_kernel_image_config(void)
>>>  		/* test_bpf module for BPF tests */
>>>  		"CONFIG_TEST_BPF",
>>>  	};
>>> +	char *values[ARRAY_SIZE(options)] = { };
>>>  	char *value, *buf = NULL;
>>>  	struct utsname utsn;
>>>  	char path[PATH_MAX];
>>>  	size_t i, n;
>>>  	ssize_t ret;
>>> -	FILE *fd;
>>> +	FILE *fd = NULL;
>>> +	bool is_pipe = false;
>>
>> Reverse-Christmas-tree style please.
> 
> Even if that means moving lines? Something like this?
> 
>         char path[PATH_MAX];
>    +    bool is_pipe = false;
>    +    FILE *fd = NULL;
>         size_t i, n;
>         ssize_t ret;
>    -    FILE *fd;

Yes, that's the idea (although "is_pipe" should be at the top in that
case, above "path" -- and this applies to your v2 patch, by the way).

> 
>>>  	if (uname(&utsn))
>>> -		goto no_config;
>>> +		goto end_parse;
>>
>> Just thinking, maybe if uname() fails we can skip /boot/config-$(uname
>> -r) but still attempt to parse /proc/config{,.gz} instead of printing
>> only NULL options?
> 
> Good idea, I'll try a bit harder if uname falls for whatever reason.

Thanks!

> 
>> Because some distributions do use /proc/config, we should keep this. You
>> can probably add /proc/config.gz as another attempt below (or even
>> above) the current case?
> 
> I doubt it is actually in use, it looks like a typo in the original PR.
> This post only lists /proc/config.gz, /boot/config and
> /boot/config-$(uname -r): https://superuser.com/questions/287371
> 
>>> +	while (get_kernel_config_option(fd, &buf, &n, &value))
>>> +		for (i = 0; i < ARRAY_SIZE(options); i++) {
>>> +			if (values[i] || strcmp(buf, options[i]))
>>
>> Can we have an option set multiple times in the config file? If so,
>> maybe have a p_info() if values are different to warn users that
>> conflicting values were found?
> 
> scripts/kconfig/merge_config.sh seems to apply a merge strategy,
> overwriting earlier values and warning about it. However this should be
> rare given that it ended up at /proc/config.gz. For now I will favor
> simplicity over complexity and keep the old situation. Let me know if
> you prefer otherwise.

Understood, let's keep it that way for now.

Thanks,
Quentin

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

[Alexei Starovoitov]

On Tue, Aug 6, 2019 at 2:36 AM Quentin Monnet
<quentin.monnet@netronome.com> wrote:
>
> >>>  {
> >>> -   size_t line_n = 0, optlen = strlen(option);
> >>> -   char *res, *strval, *line = NULL;
> >>> -   ssize_t n;
> >>> +   char *sep;
> >>> +   ssize_t linelen;
> >>
> >> Please order the declarations in reverse-Christmas tree style.
> >
> > Does this refer to the type, name, or full line length? I did not find
> > this in CodingStyle, the closest I could get is:
> > https://lore.kernel.org/patchwork/patch/732076/
> >
> > I will assume the line length for now.
>
> I am unsure this is in the CodingStyle, but fairly certain that this is
> a convention for at least network-related code. And yes, as I understand
> it refers to the length of the line.

Let's not over focus on this.
It's a preference, but not a strong requirement.
There are plenty of cases where it shouldn't be followed.
Like logical grouping of variables take precedence of xmas tree.