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

[Quentin Monnet]

2018-04-30 17:33 UTC+0100 ~ Edward Cree 
> 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 
>> ---
>>  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

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

[Edward Cree]

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

-Ed

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

[Quentin Monnet]

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 
---
 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}\))$')
 capture = p.match(self.line)
 if not capture:
 raise NoHelperFound
@@ -95,7 +95,7 @@ class HeaderParser(object):
 return capture.group(1)
 
 def parse_desc(self):
-p = re.compile('^ \* \tDescription$')
+p = re.compile('^ \* ?(?:\t| {6,8})Description$')
 capture = p.match(self.line)
 if not capture:
 # Helper can have empty description and we might be parsing another
@@ -109,7 +109,7 @@ class HeaderParser(object):
 if self.line == ' *\n':
 desc += '\n'
 else:
-p = re.compile('^ \* \t\t(.*)')
+p = re.compile('^ \* ?(?:\t| {6,8})(?:\t| {8})(.*)')
 capture = p.match(self.line)
 if capture:
 desc += capture.group(1) + '\n'
@@ -118,7 +118,7 @@ class HeaderParser(object):
 return desc
 
 def parse_ret(self):
-p = re.compile('^ \* \tReturn$')
+p = re.compile('^ \* ?(?:\t| {6,8})Return$')
 capture = p.match(self.line)
 if not capture:
 # Helper can have empty retval and we might be parsing another
@@ -132,7 +132,7 @@ class HeaderParser(object):
 if self.line == ' *\n':
 ret += '\n'
 else:
-p = re.compile('^ \* \t\t(.*)')
+p = re.compile('^ \* ?(?:\t| {6,8})(?:\t| {8})(.*)')
 capture = p.match(self.line)
 if capture:
 ret += capture.group(1) + '\n'
-- 
2.14.1