* [PATCH bpf-next 1/2] bpf/scripts: Make description and returns section for helpers/syscalls mandatory
@ 2022-01-18 11:56 Usama Arif
2022-01-18 11:56 ` [PATCH bpf-next 2/2] bpf/scripts: Raise an exception if the correct number of sycalls are not generated Usama Arif
2022-01-18 16:04 ` [PATCH bpf-next 1/2] bpf/scripts: Make description and returns section for helpers/syscalls mandatory Quentin Monnet
0 siblings, 2 replies; 6+ messages in thread
From: Usama Arif @ 2022-01-18 11:56 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, fam.zheng, cong.wang, song, quentin,
andrii.nakryiko, Usama Arif
This enforce a minimal formatting consistency for the documentation. The
description and returns missing for a few helpers have also been added.
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
---
include/uapi/linux/bpf.h | 13 +++++++++++++
scripts/bpf_doc.py | 30 ++++++++++++++++++------------
tools/include/uapi/linux/bpf.h | 13 +++++++++++++
3 files changed, 44 insertions(+), 12 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index b0383d371..8cbb3737e 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1775,6 +1775,8 @@ union bpf_attr {
* 0 on success, or a negative error in case of failure.
*
* u64 bpf_get_current_pid_tgid(void)
+ * Description
+ * Get the current pid and tgid.
* Return
* A 64-bit integer containing the current tgid and pid, and
* created as such:
@@ -1782,6 +1784,8 @@ union bpf_attr {
* *current_task*\ **->pid**.
*
* u64 bpf_get_current_uid_gid(void)
+ * Description
+ * Get the current uid and gid.
* Return
* A 64-bit integer containing the current GID and UID, and
* created as such: *current_gid* **<< 32 \|** *current_uid*.
@@ -2256,6 +2260,8 @@ union bpf_attr {
* The 32-bit hash.
*
* u64 bpf_get_current_task(void)
+ * Description
+ * Get the current task.
* Return
* A pointer to the current task struct.
*
@@ -2369,6 +2375,8 @@ union bpf_attr {
* indicate that the hash is outdated and to trigger a
* recalculation the next time the kernel tries to access this
* hash or when the **bpf_get_hash_recalc**\ () helper is called.
+ * Return
+ * void.
*
* long bpf_get_numa_node_id(void)
* Description
@@ -2466,6 +2474,8 @@ union bpf_attr {
* A 8-byte long unique number or 0 if *sk* is NULL.
*
* u32 bpf_get_socket_uid(struct sk_buff *skb)
+ * Description
+ * Get the owner UID of the socked associated to *skb*.
* Return
* The owner UID of the socket associated to *skb*. If the socket
* is **NULL**, or if it is not a full socket (i.e. if it is a
@@ -3240,6 +3250,9 @@ union bpf_attr {
* The id is returned or 0 in case the id could not be retrieved.
*
* u64 bpf_get_current_cgroup_id(void)
+ * Description
+ * Get the current cgroup id based on the cgroup within which
+ * the current task is running.
* Return
* A 64-bit integer containing the current cgroup id based
* on the cgroup within which the current task is running.
diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
index 5cf8ae2e7..20441e5d2 100755
--- a/scripts/bpf_doc.py
+++ b/scripts/bpf_doc.py
@@ -92,14 +92,14 @@ class HeaderParser(object):
def parse_element(self):
proto = self.parse_symbol()
- desc = self.parse_desc()
- ret = self.parse_ret()
+ desc = self.parse_desc(proto)
+ ret = self.parse_ret(proto)
return APIElement(proto=proto, desc=desc, ret=ret)
def parse_helper(self):
proto = self.parse_proto()
- desc = self.parse_desc()
- ret = self.parse_ret()
+ desc = self.parse_desc(proto)
+ ret = self.parse_ret(proto)
return Helper(proto=proto, desc=desc, ret=ret)
def parse_symbol(self):
@@ -129,16 +129,15 @@ class HeaderParser(object):
self.line = self.reader.readline()
return capture.group(1)
- def parse_desc(self):
+ def parse_desc(self, proto):
p = re.compile(' \* ?(?:\t| {5,8})Description$')
capture = p.match(self.line)
if not capture:
- # Helper can have empty description and we might be parsing another
- # attribute: return but do not consume.
- return ''
+ raise Exception("No description section found for " + proto)
# Description can be several lines, some of them possibly empty, and it
# stops when another subsection title is met.
desc = ''
+ desc_present = False
while True:
self.line = self.reader.readline()
if self.line == ' *\n':
@@ -147,21 +146,24 @@ class HeaderParser(object):
p = re.compile(' \* ?(?:\t| {5,8})(?:\t| {8})(.*)')
capture = p.match(self.line)
if capture:
+ desc_present = True
desc += capture.group(1) + '\n'
else:
break
+
+ if not desc_present:
+ raise Exception("No description found for " + proto)
return desc
- def parse_ret(self):
+ def parse_ret(self, proto):
p = re.compile(' \* ?(?:\t| {5,8})Return$')
capture = p.match(self.line)
if not capture:
- # Helper can have empty retval and we might be parsing another
- # attribute: return but do not consume.
- return ''
+ raise Exception("No return section found for " + proto)
# Return value description can be several lines, some of them possibly
# empty, and it stops when another subsection title is met.
ret = ''
+ ret_present = False
while True:
self.line = self.reader.readline()
if self.line == ' *\n':
@@ -170,9 +172,13 @@ class HeaderParser(object):
p = re.compile(' \* ?(?:\t| {5,8})(?:\t| {8})(.*)')
capture = p.match(self.line)
if capture:
+ ret_present = True
ret += capture.group(1) + '\n'
else:
break
+
+ if not ret_present:
+ raise Exception("No return found for " + proto)
return ret
def seek_to(self, target, help_message):
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index b0383d371..8cbb3737e 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1775,6 +1775,8 @@ union bpf_attr {
* 0 on success, or a negative error in case of failure.
*
* u64 bpf_get_current_pid_tgid(void)
+ * Description
+ * Get the current pid and tgid.
* Return
* A 64-bit integer containing the current tgid and pid, and
* created as such:
@@ -1782,6 +1784,8 @@ union bpf_attr {
* *current_task*\ **->pid**.
*
* u64 bpf_get_current_uid_gid(void)
+ * Description
+ * Get the current uid and gid.
* Return
* A 64-bit integer containing the current GID and UID, and
* created as such: *current_gid* **<< 32 \|** *current_uid*.
@@ -2256,6 +2260,8 @@ union bpf_attr {
* The 32-bit hash.
*
* u64 bpf_get_current_task(void)
+ * Description
+ * Get the current task.
* Return
* A pointer to the current task struct.
*
@@ -2369,6 +2375,8 @@ union bpf_attr {
* indicate that the hash is outdated and to trigger a
* recalculation the next time the kernel tries to access this
* hash or when the **bpf_get_hash_recalc**\ () helper is called.
+ * Return
+ * void.
*
* long bpf_get_numa_node_id(void)
* Description
@@ -2466,6 +2474,8 @@ union bpf_attr {
* A 8-byte long unique number or 0 if *sk* is NULL.
*
* u32 bpf_get_socket_uid(struct sk_buff *skb)
+ * Description
+ * Get the owner UID of the socked associated to *skb*.
* Return
* The owner UID of the socket associated to *skb*. If the socket
* is **NULL**, or if it is not a full socket (i.e. if it is a
@@ -3240,6 +3250,9 @@ union bpf_attr {
* The id is returned or 0 in case the id could not be retrieved.
*
* u64 bpf_get_current_cgroup_id(void)
+ * Description
+ * Get the current cgroup id based on the cgroup within which
+ * the current task is running.
* Return
* A 64-bit integer containing the current cgroup id based
* on the cgroup within which the current task is running.
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next 2/2] bpf/scripts: Raise an exception if the correct number of sycalls are not generated
2022-01-18 11:56 [PATCH bpf-next 1/2] bpf/scripts: Make description and returns section for helpers/syscalls mandatory Usama Arif
@ 2022-01-18 11:56 ` Usama Arif
2022-01-18 16:04 ` Quentin Monnet
2022-01-18 16:04 ` [PATCH bpf-next 1/2] bpf/scripts: Make description and returns section for helpers/syscalls mandatory Quentin Monnet
1 sibling, 1 reply; 6+ messages in thread
From: Usama Arif @ 2022-01-18 11:56 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, fam.zheng, cong.wang, song, quentin,
andrii.nakryiko, Usama Arif
Currently the syscalls rst and subsequently man page are auto-generated
using function documentation present in bpf.h. If the documentation for the
syscall is missing or doesn't follow a specific format, then that syscall
is not dumped in the auto-generated rst.
This patch checks the number of syscalls documented within the header file
with those present as part of the enum bpf_cmd and raises an Exception if
they don't match. It is not needed with the currently documented upstream
syscalls, but can help in debugging when developing new syscalls when
there might be missing or misformatted documentation.
The function helper_number_check is moved to the Printer parent
class and renamed to elem_number_check as all the most derived children
classes are using this function now.
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
---
scripts/bpf_doc.py | 88 ++++++++++++++++++++++++++++++++--------------
1 file changed, 61 insertions(+), 27 deletions(-)
diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
index 20441e5d2..11304427e 100755
--- a/scripts/bpf_doc.py
+++ b/scripts/bpf_doc.py
@@ -89,6 +89,8 @@ class HeaderParser(object):
self.commands = []
self.desc_unique_helpers = set()
self.define_unique_helpers = []
+ self.desc_syscalls = []
+ self.enum_syscalls = []
def parse_element(self):
proto = self.parse_symbol()
@@ -103,7 +105,7 @@ class HeaderParser(object):
return Helper(proto=proto, desc=desc, ret=ret)
def parse_symbol(self):
- p = re.compile(' \* ?(.+)$')
+ p = re.compile(' \* ?(BPF\w+)$')
capture = p.match(self.line)
if not capture:
raise NoSyscallCommandFound
@@ -181,26 +183,55 @@ class HeaderParser(object):
raise Exception("No return found for " + proto)
return ret
- def seek_to(self, target, help_message):
+ def seek_to(self, target, help_message, discard_lines = 1):
self.reader.seek(0)
offset = self.reader.read().find(target)
if offset == -1:
raise Exception(help_message)
self.reader.seek(offset)
self.reader.readline()
- self.reader.readline()
+ for _ in range(discard_lines):
+ self.reader.readline()
self.line = self.reader.readline()
- def parse_syscall(self):
+ def parse_desc_syscall(self):
self.seek_to('* DOC: eBPF Syscall Commands',
'Could not find start of eBPF syscall descriptions list')
while True:
try:
command = self.parse_element()
self.commands.append(command)
+ self.desc_syscalls.append(command.proto)
+
except NoSyscallCommandFound:
break
+ def parse_enum_syscall(self):
+ self.seek_to('enum bpf_cmd {',
+ 'Could not find start of bpf_cmd enum', 0)
+ # Searches for either one or more BPF\w+ enums
+ bpf_p = re.compile('\s*(BPF\w+)+')
+ # Searches for an enum entry assigned to another entry,
+ # for e.g. BPF_PROG_RUN = BPF_PROG_TEST_RUN, which is
+ # not documented hence should be skipped in check to
+ # determine if the right number of syscalls are documented
+ assign_p = re.compile('\s*(BPF\w+)\s*=\s*(BPF\w+)')
+ bpf_cmd_str = ''
+ while True:
+ capture = assign_p.match(self.line)
+ if capture:
+ # Skip line if an enum entry is assigned to another entry
+ self.line = self.reader.readline()
+ continue
+ capture = bpf_p.match(self.line)
+ if capture:
+ bpf_cmd_str += self.line
+ else:
+ break
+ self.line = self.reader.readline()
+ # Find the number of occurences of BPF\w+
+ self.enum_syscalls = re.findall('(BPF\w+)+', bpf_cmd_str)
+
def parse_desc_helpers(self):
self.seek_to('* Start of BPF helper function descriptions:',
'Could not find start of eBPF helper descriptions list')
@@ -234,7 +265,8 @@ class HeaderParser(object):
self.define_unique_helpers = re.findall('FN\(\w+\)', fn_defines_str)
def run(self):
- self.parse_syscall()
+ self.parse_desc_syscall()
+ self.parse_enum_syscall()
self.parse_desc_helpers()
self.parse_define_helpers()
self.reader.close()
@@ -266,6 +298,27 @@ class Printer(object):
self.print_one(elem)
self.print_footer()
+ def elem_number_check(self, desc_unique_elem, define_unique_elem, type, instance):
+ """
+ Checks the number of helpers/syscalls documented within the header file
+ description with those defined as part of enum/macro and raise an
+ Exception if they don't match.
+ """
+ nr_desc_unique_elem = len(desc_unique_elem)
+ nr_define_unique_elem = len(define_unique_elem)
+ if nr_desc_unique_elem != nr_define_unique_elem:
+ exception_msg = '''
+ The number of unique %s in description (%d) doesn\'t match the number of unique %s defined in %s (%d)
+ ''' % (type, nr_desc_unique_elem, type, instance, nr_define_unique_elem)
+ if nr_desc_unique_elem < nr_define_unique_elem:
+ # Function description is parsed until no helper is found (which can be due to
+ # misformatting). Hence, only print the first missing/misformatted helper/enum.
+ exception_msg += '''
+ The description for %s is not present or formatted correctly.
+ ''' % (define_unique_elem[nr_desc_unique_elem])
+ print(define_unique_elem)
+ print(desc_unique_elem)
+ raise Exception(exception_msg)
class PrinterRST(Printer):
"""
@@ -326,26 +379,6 @@ class PrinterRST(Printer):
print('')
-def helper_number_check(desc_unique_helpers, define_unique_helpers):
- """
- Checks the number of functions documented within the header file
- with those present as part of #define __BPF_FUNC_MAPPER and raise an
- Exception if they don't match.
- """
- nr_desc_unique_helpers = len(desc_unique_helpers)
- nr_define_unique_helpers = len(define_unique_helpers)
- if nr_desc_unique_helpers != nr_define_unique_helpers:
- helper_exception = '''
-The number of unique helpers in description (%d) doesn\'t match the number of unique helpers defined in __BPF_FUNC_MAPPER (%d)
-''' % (nr_desc_unique_helpers, nr_define_unique_helpers)
- if nr_desc_unique_helpers < nr_define_unique_helpers:
- # Function description is parsed until no helper is found (which can be due to
- # misformatting). Hence, only print the first missing/misformatted function.
- helper_exception += '''
-The description for %s is not present or formatted correctly.
-''' % (define_unique_helpers[nr_desc_unique_helpers])
- raise Exception(helper_exception)
-
class PrinterHelpersRST(PrinterRST):
"""
A printer for dumping collected information about helpers as a ReStructured
@@ -355,7 +388,7 @@ class PrinterHelpersRST(PrinterRST):
"""
def __init__(self, parser):
self.elements = parser.helpers
- helper_number_check(parser.desc_unique_helpers, parser.define_unique_helpers)
+ self.elem_number_check(parser.desc_unique_helpers, parser.define_unique_helpers, 'helper', '__BPF_FUNC_MAPPER')
def print_header(self):
header = '''\
@@ -529,6 +562,7 @@ class PrinterSyscallRST(PrinterRST):
"""
def __init__(self, parser):
self.elements = parser.commands
+ self.elem_number_check(parser.desc_syscalls, parser.enum_syscalls, 'syscall', 'bpf_cmd')
def print_header(self):
header = '''\
@@ -560,7 +594,7 @@ class PrinterHelpers(Printer):
"""
def __init__(self, parser):
self.elements = parser.helpers
- helper_number_check(parser.desc_unique_helpers, parser.define_unique_helpers)
+ self.elem_number_check(parser.desc_unique_helpers, parser.define_unique_helpers, 'helper', '__BPF_FUNC_MAPPER')
type_fwds = [
'struct bpf_fib_lookup',
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf/scripts: Make description and returns section for helpers/syscalls mandatory
2022-01-18 11:56 [PATCH bpf-next 1/2] bpf/scripts: Make description and returns section for helpers/syscalls mandatory Usama Arif
2022-01-18 11:56 ` [PATCH bpf-next 2/2] bpf/scripts: Raise an exception if the correct number of sycalls are not generated Usama Arif
@ 2022-01-18 16:04 ` Quentin Monnet
2022-01-18 16:05 ` Quentin Monnet
1 sibling, 1 reply; 6+ messages in thread
From: Quentin Monnet @ 2022-01-18 16:04 UTC (permalink / raw)
To: Usama Arif, bpf; +Cc: ast, daniel, fam.zheng, cong.wang, song, andrii.nakryiko
2022-01-18 11:56 UTC+0000 ~ Usama Arif <usama.arif@bytedance.com>
> This enforce a minimal formatting consistency for the documentation. The
> description and returns missing for a few helpers have also been added.
>
> Signed-off-by: Usama Arif <usama.arif@bytedance.com>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Thank you!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 2/2] bpf/scripts: Raise an exception if the correct number of sycalls are not generated
2022-01-18 11:56 ` [PATCH bpf-next 2/2] bpf/scripts: Raise an exception if the correct number of sycalls are not generated Usama Arif
@ 2022-01-18 16:04 ` Quentin Monnet
2022-01-18 17:19 ` [External] " Usama Arif
0 siblings, 1 reply; 6+ messages in thread
From: Quentin Monnet @ 2022-01-18 16:04 UTC (permalink / raw)
To: Usama Arif, bpf; +Cc: ast, daniel, fam.zheng, cong.wang, song, andrii.nakryiko
2022-01-18 11:56 UTC+0000 ~ Usama Arif <usama.arif@bytedance.com>
> Currently the syscalls rst and subsequently man page are auto-generated
> using function documentation present in bpf.h. If the documentation for the
> syscall is missing or doesn't follow a specific format, then that syscall
> is not dumped in the auto-generated rst.
>
> This patch checks the number of syscalls documented within the header file
> with those present as part of the enum bpf_cmd and raises an Exception if
> they don't match. It is not needed with the currently documented upstream
> syscalls, but can help in debugging when developing new syscalls when
> there might be missing or misformatted documentation.
>
> The function helper_number_check is moved to the Printer parent
> class and renamed to elem_number_check as all the most derived children
> classes are using this function now.
>
> Signed-off-by: Usama Arif <usama.arif@bytedance.com>
Thanks for the follow-up, looks good and seems to work well. Please find
just a few nitpicks below.
> ---
> scripts/bpf_doc.py | 88 ++++++++++++++++++++++++++++++++--------------
> 1 file changed, 61 insertions(+), 27 deletions(-)
>
> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
> index 20441e5d2..11304427e 100755
> --- a/scripts/bpf_doc.py
> +++ b/scripts/bpf_doc.py
> @@ -89,6 +89,8 @@ class HeaderParser(object):
> self.commands = []
> self.desc_unique_helpers = set()
> self.define_unique_helpers = []
> + self.desc_syscalls = []
> + self.enum_syscalls = []
>
> def parse_element(self):
> proto = self.parse_symbol()
> @@ -103,7 +105,7 @@ class HeaderParser(object):
> return Helper(proto=proto, desc=desc, ret=ret)
>
> def parse_symbol(self):
> - p = re.compile(' \* ?(.+)$')
> + p = re.compile(' \* ?(BPF\w+)$')
> capture = p.match(self.line)
> if not capture:
> raise NoSyscallCommandFound
> @@ -181,26 +183,55 @@ class HeaderParser(object):
> raise Exception("No return found for " + proto)
> return ret
>
> - def seek_to(self, target, help_message):
> + def seek_to(self, target, help_message, discard_lines = 1):
> self.reader.seek(0)
> offset = self.reader.read().find(target)
> if offset == -1:
> raise Exception(help_message)
> self.reader.seek(offset)
> self.reader.readline()
> - self.reader.readline()
> + for _ in range(discard_lines):
> + self.reader.readline()
> self.line = self.reader.readline()
>
> - def parse_syscall(self):
> + def parse_desc_syscall(self):
> self.seek_to('* DOC: eBPF Syscall Commands',
> 'Could not find start of eBPF syscall descriptions list')
> while True:
> try:
> command = self.parse_element()
> self.commands.append(command)
> + self.desc_syscalls.append(command.proto)
> +
> except NoSyscallCommandFound:
> break
>
> + def parse_enum_syscall(self):
> + self.seek_to('enum bpf_cmd {',
> + 'Could not find start of bpf_cmd enum', 0)
> + # Searches for either one or more BPF\w+ enums
> + bpf_p = re.compile('\s*(BPF\w+)+')
> + # Searches for an enum entry assigned to another entry,
> + # for e.g. BPF_PROG_RUN = BPF_PROG_TEST_RUN, which is
> + # not documented hence should be skipped in check to
> + # determine if the right number of syscalls are documented
Sounds good. If you respin, would you mind taking this opportunity to
add, at the end of BPF_PROG_TEST_RUN's description in
{tools/,}include/uapi/linux/bpf.h, that BPF_PROG_RUN is an alias to this
command? It may be useful for users looking for BPF_PROG_RUN in the
generated man page.
> + assign_p = re.compile('\s*(BPF\w+)\s*=\s*(BPF\w+)')
> + bpf_cmd_str = ''
> + while True:
> + capture = assign_p.match(self.line)
> + if capture:
> + # Skip line if an enum entry is assigned to another entry
> + self.line = self.reader.readline()
> + continue
> + capture = bpf_p.match(self.line)
> + if capture:
> + bpf_cmd_str += self.line
> + else:
> + break
> + self.line = self.reader.readline()
> + # Find the number of occurences of BPF\w+
> + self.enum_syscalls = re.findall('(BPF\w+)+', bpf_cmd_str)
> +
> def parse_desc_helpers(self):
> self.seek_to('* Start of BPF helper function descriptions:',
> 'Could not find start of eBPF helper descriptions list')
> @@ -234,7 +265,8 @@ class HeaderParser(object):
> self.define_unique_helpers = re.findall('FN\(\w+\)', fn_defines_str)
>
> def run(self):
> - self.parse_syscall()
> + self.parse_desc_syscall()
> + self.parse_enum_syscall()
> self.parse_desc_helpers()
> self.parse_define_helpers()
> self.reader.close()
> @@ -266,6 +298,27 @@ class Printer(object):
> self.print_one(elem)
> self.print_footer()
>
> + def elem_number_check(self, desc_unique_elem, define_unique_elem, type, instance):
> + """
> + Checks the number of helpers/syscalls documented within the header file
> + description with those defined as part of enum/macro and raise an
> + Exception if they don't match.
> + """
> + nr_desc_unique_elem = len(desc_unique_elem)
> + nr_define_unique_elem = len(define_unique_elem)
> + if nr_desc_unique_elem != nr_define_unique_elem:
> + exception_msg = '''
> + The number of unique %s in description (%d) doesn\'t match the number of unique %s defined in %s (%d)
> + ''' % (type, nr_desc_unique_elem, type, instance, nr_define_unique_elem)
Nit: I think you didn't mean to indent the two lines above?
> + if nr_desc_unique_elem < nr_define_unique_elem:
> + # Function description is parsed until no helper is found (which can be due to
> + # misformatting). Hence, only print the first missing/misformatted helper/enum.
> + exception_msg += '''
> + The description for %s is not present or formatted correctly.
> + ''' % (define_unique_elem[nr_desc_unique_elem])
Same thing for the indent
> + print(define_unique_elem)
> + print(desc_unique_elem)
These two objects should accompany the error message from the Exception.
Did you consider adding them to exception_msg? If not convenient, should
we at least send them to stderr instead of stdout?
> + raise Exception(exception_msg)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf/scripts: Make description and returns section for helpers/syscalls mandatory
2022-01-18 16:04 ` [PATCH bpf-next 1/2] bpf/scripts: Make description and returns section for helpers/syscalls mandatory Quentin Monnet
@ 2022-01-18 16:05 ` Quentin Monnet
0 siblings, 0 replies; 6+ messages in thread
From: Quentin Monnet @ 2022-01-18 16:05 UTC (permalink / raw)
To: Usama Arif, bpf; +Cc: ast, daniel, fam.zheng, cong.wang, song, andrii.nakryiko
2022-01-18 16:04 UTC+0000 ~ Quentin Monnet <quentin@isovalent.com>
> 2022-01-18 11:56 UTC+0000 ~ Usama Arif <usama.arif@bytedance.com>
>> This enforce a minimal formatting consistency for the documentation. The
>> description and returns missing for a few helpers have also been added.
>>
>> Signed-off-by: Usama Arif <usama.arif@bytedance.com>
>
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Apologies, this is obviously not a Signed-off, I meant:
Reviewed-by: Quentin Monnet <quentin@isovalent.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [External] Re: [PATCH bpf-next 2/2] bpf/scripts: Raise an exception if the correct number of sycalls are not generated
2022-01-18 16:04 ` Quentin Monnet
@ 2022-01-18 17:19 ` Usama Arif
0 siblings, 0 replies; 6+ messages in thread
From: Usama Arif @ 2022-01-18 17:19 UTC (permalink / raw)
To: Quentin Monnet, bpf
Cc: ast, daniel, fam.zheng, cong.wang, song, andrii.nakryiko
On 18/01/2022 16:04, Quentin Monnet wrote:
> 2022-01-18 11:56 UTC+0000 ~ Usama Arif <usama.arif@bytedance.com>
>> Currently the syscalls rst and subsequently man page are auto-generated
>> using function documentation present in bpf.h. If the documentation for the
>> syscall is missing or doesn't follow a specific format, then that syscall
>> is not dumped in the auto-generated rst.
>>
>> This patch checks the number of syscalls documented within the header file
>> with those present as part of the enum bpf_cmd and raises an Exception if
>> they don't match. It is not needed with the currently documented upstream
>> syscalls, but can help in debugging when developing new syscalls when
>> there might be missing or misformatted documentation.
>>
>> The function helper_number_check is moved to the Printer parent
>> class and renamed to elem_number_check as all the most derived children
>> classes are using this function now.
>>
>> Signed-off-by: Usama Arif <usama.arif@bytedance.com>
>
> Thanks for the follow-up, looks good and seems to work well. Please find
> just a few nitpicks below.
>
>> ---
>> scripts/bpf_doc.py | 88 ++++++++++++++++++++++++++++++++--------------
>> 1 file changed, 61 insertions(+), 27 deletions(-)
>>
>> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
>> index 20441e5d2..11304427e 100755
>> --- a/scripts/bpf_doc.py
>> +++ b/scripts/bpf_doc.py
>> @@ -89,6 +89,8 @@ class HeaderParser(object):
>> self.commands = []
>> self.desc_unique_helpers = set()
>> self.define_unique_helpers = []
>> + self.desc_syscalls = []
>> + self.enum_syscalls = []
>>
>> def parse_element(self):
>> proto = self.parse_symbol()
>> @@ -103,7 +105,7 @@ class HeaderParser(object):
>> return Helper(proto=proto, desc=desc, ret=ret)
>>
>> def parse_symbol(self):
>> - p = re.compile(' \* ?(.+)$')
>> + p = re.compile(' \* ?(BPF\w+)$')
>> capture = p.match(self.line)
>> if not capture:
>> raise NoSyscallCommandFound
>> @@ -181,26 +183,55 @@ class HeaderParser(object):
>> raise Exception("No return found for " + proto)
>> return ret
>>
>> - def seek_to(self, target, help_message):
>> + def seek_to(self, target, help_message, discard_lines = 1):
>> self.reader.seek(0)
>> offset = self.reader.read().find(target)
>> if offset == -1:
>> raise Exception(help_message)
>> self.reader.seek(offset)
>> self.reader.readline()
>> - self.reader.readline()
>> + for _ in range(discard_lines):
>> + self.reader.readline()
>> self.line = self.reader.readline()
>>
>> - def parse_syscall(self):
>> + def parse_desc_syscall(self):
>> self.seek_to('* DOC: eBPF Syscall Commands',
>> 'Could not find start of eBPF syscall descriptions list')
>> while True:
>> try:
>> command = self.parse_element()
>> self.commands.append(command)
>> + self.desc_syscalls.append(command.proto)
>> +
>> except NoSyscallCommandFound:
>> break
>>
>> + def parse_enum_syscall(self):
>> + self.seek_to('enum bpf_cmd {',
>> + 'Could not find start of bpf_cmd enum', 0)
>> + # Searches for either one or more BPF\w+ enums
>> + bpf_p = re.compile('\s*(BPF\w+)+')
>> + # Searches for an enum entry assigned to another entry,
>> + # for e.g. BPF_PROG_RUN = BPF_PROG_TEST_RUN, which is
>> + # not documented hence should be skipped in check to
>> + # determine if the right number of syscalls are documented
>
> Sounds good. If you respin, would you mind taking this opportunity to
> add, at the end of BPF_PROG_TEST_RUN's description in
> {tools/,}include/uapi/linux/bpf.h, that BPF_PROG_RUN is an alias to this
> command? It may be useful for users looking for BPF_PROG_RUN in the
> generated man page.
Thanks for the reviews! Added in v2.
>
>> + assign_p = re.compile('\s*(BPF\w+)\s*=\s*(BPF\w+)')
>> + bpf_cmd_str = ''
>> + while True:
>> + capture = assign_p.match(self.line)
>> + if capture:
>> + # Skip line if an enum entry is assigned to another entry
>> + self.line = self.reader.readline()
>> + continue
>> + capture = bpf_p.match(self.line)
>> + if capture:
>> + bpf_cmd_str += self.line
>> + else:
>> + break
>> + self.line = self.reader.readline()
>> + # Find the number of occurences of BPF\w+
>> + self.enum_syscalls = re.findall('(BPF\w+)+', bpf_cmd_str)
>> +
>> def parse_desc_helpers(self):
>> self.seek_to('* Start of BPF helper function descriptions:',
>> 'Could not find start of eBPF helper descriptions list')
>> @@ -234,7 +265,8 @@ class HeaderParser(object):
>> self.define_unique_helpers = re.findall('FN\(\w+\)', fn_defines_str)
>>
>> def run(self):
>> - self.parse_syscall()
>> + self.parse_desc_syscall()
>> + self.parse_enum_syscall()
>> self.parse_desc_helpers()
>> self.parse_define_helpers()
>> self.reader.close()
>> @@ -266,6 +298,27 @@ class Printer(object):
>> self.print_one(elem)
>> self.print_footer()
>>
>> + def elem_number_check(self, desc_unique_elem, define_unique_elem, type, instance):
>> + """
>> + Checks the number of helpers/syscalls documented within the header file
>> + description with those defined as part of enum/macro and raise an
>> + Exception if they don't match.
>> + """
>> + nr_desc_unique_elem = len(desc_unique_elem)
>> + nr_define_unique_elem = len(define_unique_elem)
>> + if nr_desc_unique_elem != nr_define_unique_elem:
>> + exception_msg = '''
>> + The number of unique %s in description (%d) doesn\'t match the number of unique %s defined in %s (%d)
>> + ''' % (type, nr_desc_unique_elem, type, instance, nr_define_unique_elem)
>
> Nit: I think you didn't mean to indent the two lines above?
Thanks, removed indent here and below in v2.
>
>> + if nr_desc_unique_elem < nr_define_unique_elem:
>> + # Function description is parsed until no helper is found (which can be due to
>> + # misformatting). Hence, only print the first missing/misformatted helper/enum.
>> + exception_msg += '''
>> + The description for %s is not present or formatted correctly.
>> + ''' % (define_unique_elem[nr_desc_unique_elem])
>
> Same thing for the indent
>
>> + print(define_unique_elem)
>> + print(desc_unique_elem)
>
> These two objects should accompany the error message from the Exception.
> Did you consider adding them to exception_msg? If not convenient, should
> we at least send them to stderr instead of stdout?
>
They were debug prints that i was using while writing the patch. I
should have removed them before sending to the mailing list! Removed in
v2, Thanks!
>> + raise Exception(exception_msg)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-18 17:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-18 11:56 [PATCH bpf-next 1/2] bpf/scripts: Make description and returns section for helpers/syscalls mandatory Usama Arif
2022-01-18 11:56 ` [PATCH bpf-next 2/2] bpf/scripts: Raise an exception if the correct number of sycalls are not generated Usama Arif
2022-01-18 16:04 ` Quentin Monnet
2022-01-18 17:19 ` [External] " Usama Arif
2022-01-18 16:04 ` [PATCH bpf-next 1/2] bpf/scripts: Make description and returns section for helpers/syscalls mandatory Quentin Monnet
2022-01-18 16:05 ` Quentin Monnet
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.