netdev - Re: [PATCH bpf-next] bpf: relax constraints on formatting for eBPF helper documentation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2f2f31e1-3638-fac8-cd15-628de11d2f52@netronome.com>
Date:   Mon, 30 Apr 2018 18:45:32 +0100
From:   Quentin Monnet <quentin.monnet@...ronome.com>
To:     Edward Cree <ecree@...arflare.com>, daniel@...earbox.net,
        ast@...nel.org
Cc:     dsahern@...il.com, yhs@...com, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [PATCH bpf-next] bpf: relax constraints on formatting for eBPF
 helper documentation

2018-04-30 17:33 UTC+0100 ~ Edward Cree <ecree@...arflare.com>
> On 30/04/18 16:59, Quentin Monnet wrote:
>> The Python script used to parse and extract eBPF helpers documentation
>> from include/uapi/linux/bpf.h expects a very specific formatting for the
>> descriptions (single dots represent a space, '>' stands for a tab):
>>
>>     /*
>>      ...
>>      *.int bpf_helper(list of arguments)
>>      *.>    Description
>>      *.>    >       Start of description
>>      *.>    >       Another line of description
>>      *.>    >       And yet another line of description
>>      *.>    Return
>>      *.>    >       0 on success, or a negative error in case of failure
>>      ...
>>      */
>>
>> This is too strict, and painful for developers who wants to add
>> documentation for new helpers. Worse, it is extremelly difficult to
>> check that the formatting is correct during reviews. Change the
>> format expected by the script and make it more flexible. The script now
>> works whether or not the initial space (right after the star) is
>> present, and accepts both tabs and white spaces (or a combination of
>> both) for indenting description sections and contents.
>>
>> Concretely, something like the following would now be supported:
>>
>>     /*
>>      ...
>>      *int bpf_helper(list of arguments)
>>      *......Description
>>      *.>    >       Start of description...
>>      *>     >       Another line of description
>>      *..............And yet another line of description
>>      *>     Return
>>      *.>    ........0 on success, or a negative error in case of failure
>>      ...
>>      */
>>
>> Signed-off-by: Quentin Monnet <quentin.monnet@...ronome.com>
>> ---
>>  scripts/bpf_helpers_doc.py | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py
>> index 30ba0fee36e4..717547e6f0a6 100755
>> --- a/scripts/bpf_helpers_doc.py
>> +++ b/scripts/bpf_helpers_doc.py
>> @@ -87,7 +87,7 @@ class HeaderParser(object):
>>          #   - Same as above, with "const" and/or "struct" in front of type
>>          #   - "..." (undefined number of arguments, for bpf_trace_printk())
>>          # There is at least one term ("void"), and at most five arguments.
>> -        p = re.compile('^ \* ((.+) \**\w+\((((const )?(struct )?(\w+|\.\.\.)( \**\w+)?)(, )?){1,5}\))$')
>> +        p = re.compile('^ \* ?((.+) \**\w+\((((const )?(struct )?(\w+|\.\.\.)( \**\w+)?)(, )?){1,5}\))$')
> The proper coding style for such things is to go straight to tabs after
>  the star and not have the space.  So if we're going to make the script
>  flexible here (and leave coding style enforcement to other tools such
>  as checkpatch), maybe the regexen should just begin '^ \*\s+' and avoid
>  relying on counting indentation to delimit sections (e.g. scan for the
>  section headers like '^ \*\s+Description$' instead).

Thanks Edward! I agree it would be cleaner. However, with the current
format of the doc, I see two shortcomings.

- First we need a way to detect the end of a section. There is no
"Return" section for helper returning void, so we cannot rely on it to
end the "Description" section. And there is no delimiter to indicate the
end of the description of a given helper. We cannot assume that a string
matching a function definition, alone on its line, indicate the start of
a new helper (this is not the case). So as I see it, this would at least
require some delimiter between the descriptions of different functions
in bpf.h. I could add them if you think this is better.

- Also, we loose the possibility to further indent the text from the
description. Think about code snippets in descriptions: were we to
extract the lines with a regex such as / *\s+(.*)/, I see no way to get
the additional indent that should appear in the man page, if we do not
know what indent level was used for the helper description. I do not see
any simple workaround.

This being said, I am ready to bring whatever changes are needed to make
writing new helper doc easier, so I am open to suggestions if you have
workarounds for these or if the consensus is that the formatting should
be completely revised.

> Btw, leading '^' is unnecessary as re.match() is already implicitly
>  anchored at start-of-string.  (The trailing '$' are still needed.)

Oh, thanks! I'll fix that.

Quentin