Hi Peter,
2019-08-06 00:54 UTC+0100 ~ Peter Wu <pe...@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
