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
