[PATCH v6] bpf/scripts: raise an exception if the correct number of helpers are not generated

* [PATCH v6] bpf/scripts: raise an exception if the correct number of helpers are not generated
@ 2022-01-12 11:49 Usama Arif
  2022-01-12 12:15 ` Quentin Monnet
  0 siblings, 1 reply; 5+ messages in thread
From: Usama Arif @ 2022-01-12 11:49 UTC (permalink / raw)
  To: bpf
  Cc: ast, daniel, joe, fam.zheng, cong.wang, alexei.starovoitov, song,
	quentin, Usama Arif

Currently bpf_helper_defs.h and the bpf helpers man page are auto-generated
using function documentation present in bpf.h. If the documentation for the
helper is missing or doesn't follow a specific format for e.g. if a function
is documented as:
 * long bpf_kallsyms_lookup_name( const char *name, int name_sz, int flags, u64 *res )
instead of
 * long bpf_kallsyms_lookup_name(const char *name, int name_sz, int flags, u64 *res)
(notice the extra space at the start and end of function arguments)
then that helper is not dumped in the auto-generated header and results in
an invalid call during eBPF runtime, even if all the code specific to the
helper is correct.

This patch checks the number of functions documented within the header file
with those present as part of #define __BPF_FUNC_MAPPER and raises an
Exception if they don't match. It is not needed with the currently documented
upstream functions, but can help in debugging when developing new helpers
when there might be missing or misformatted documentation.

Signed-off-by: Usama Arif <usama.arif@bytedance.com>

---
v5->v6:
- change from error in auto-generated files to exception in bpf_doc.py
  if the correct number of helpers are not generated (suggested by
  Quentin Monnet)

v4->v5:
- Converted warning to error incase of missing/misformatted helper doc
  (suggested by Song Liu)

v3->v4:
- Added comments to make code clearer
- Added warning to man page as well (suggested by Quentin Monnet)

v2->v3:
- Removed check if value is already in set (suggested by Song Liu)

v1->v2:
- Fix CI error reported by Alexei Starovoitov
---
 scripts/bpf_doc.py | 50 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
index a6403ddf5de7..76c96df095e3 100755
--- a/scripts/bpf_doc.py
+++ b/scripts/bpf_doc.py
@@ -87,6 +87,8 @@ class HeaderParser(object):
         self.line = ''
         self.helpers = []
         self.commands = []
+        self.desc_unique_helpers = set()
+        self.define_unique_helpers = []
 
     def parse_element(self):
         proto    = self.parse_symbol()
@@ -193,19 +195,42 @@ class HeaderParser(object):
             except NoSyscallCommandFound:
                 break
 
-    def parse_helpers(self):
+    def parse_desc_helpers(self):
         self.seek_to('* Start of BPF helper function descriptions:',
                      'Could not find start of eBPF helper descriptions list')
         while True:
             try:
                 helper = self.parse_helper()
                 self.helpers.append(helper)
+                proto = helper.proto_break_down()
+                self.desc_unique_helpers.add(proto['name'])
             except NoHelperFound:
                 break
 
+    def parse_define_helpers(self):
+        # Parse the number of FN(...) in #define __BPF_FUNC_MAPPER to compare
+        # later with the number of unique function names present in description.
+        # Note: seek_to(..) discards the first line below the target search text,
+        # resulting in FN(unspec) being skipped and not added to self.define_unique_helpers.
+        self.seek_to('#define __BPF_FUNC_MAPPER(FN)',
+                     'Could not find start of eBPF helper definition list')
+        # Searches for either one or more FN(\w+) defines or a backslash for newline
+        p = re.compile('\s*(FN\(\w+\))+|\\\\')
+        fn_defines_str = ''
+        while True:
+            capture = p.match(self.line)
+            if capture:
+                fn_defines_str += self.line
+            else:
+                break
+            self.line = self.reader.readline()
+        # Find the number of occurences of FN(\w+)
+        self.define_unique_helpers = re.findall('FN\(\w+\)', fn_defines_str)
+
     def run(self):
         self.parse_syscall()
-        self.parse_helpers()
+        self.parse_desc_helpers()
+        self.parse_define_helpers()
         self.reader.close()
 
 ###############################################################################
@@ -295,6 +320,25 @@ 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) don\'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):
     """
@@ -305,6 +349,7 @@ class PrinterHelpersRST(PrinterRST):
     """
     def __init__(self, parser):
         self.elements = parser.helpers
+        helper_number_check(parser.desc_unique_helpers, parser.define_unique_helpers)
 
     def print_header(self):
         header = '''\
@@ -509,6 +554,7 @@ class PrinterHelpers(Printer):
     """
     def __init__(self, parser):
         self.elements = parser.helpers
+        helper_number_check(parser.desc_unique_helpers, parser.define_unique_helpers)
 
     type_fwds = [
             'struct bpf_fib_lookup',
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread