* [PATCH v3] bpf/scripts: add warning if the correct number of helpers are not auto-generated
@ 2022-01-11 11:08 Usama Arif
2022-01-11 11:35 ` Quentin Monnet
0 siblings, 1 reply; 3+ messages in thread
From: Usama Arif @ 2022-01-11 11:08 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, joe, fam.zheng, cong.wang, alexei.starovoitov, song,
Usama Arif
Currently bpf_helper_defs.h is 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 generates a
warning in the header file 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>
---
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 | 45 +++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 43 insertions(+), 2 deletions(-)
diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
index a6403ddf5de7..8d96f08ea7a6 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,40 @@ 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
+ 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()
###############################################################################
@@ -509,6 +532,8 @@ class PrinterHelpers(Printer):
"""
def __init__(self, parser):
self.elements = parser.helpers
+ self.desc_unique_helpers = parser.desc_unique_helpers
+ self.define_unique_helpers = parser.define_unique_helpers
type_fwds = [
'struct bpf_fib_lookup',
@@ -628,6 +653,22 @@ class PrinterHelpers(Printer):
/* Forward declarations of BPF structs */'''
print(header)
+
+ nr_desc_unique_helpers = len(self.desc_unique_helpers)
+ nr_define_unique_helpers = len(self.define_unique_helpers)
+ if nr_desc_unique_helpers != nr_define_unique_helpers:
+ header_warning = '''
+#warning 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.
+ header_warning += '''
+#warning The description for %s is not present or formatted correctly.
+''' % (self.define_unique_helpers[nr_desc_unique_helpers])
+ print(header_warning)
+
+
for fwd in self.type_fwds:
print('%s;' % fwd)
print('')
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] bpf/scripts: add warning if the correct number of helpers are not auto-generated
2022-01-11 11:08 [PATCH v3] bpf/scripts: add warning if the correct number of helpers are not auto-generated Usama Arif
@ 2022-01-11 11:35 ` Quentin Monnet
2022-01-11 14:46 ` [External] " Usama Arif
0 siblings, 1 reply; 3+ messages in thread
From: Quentin Monnet @ 2022-01-11 11:35 UTC (permalink / raw)
To: Usama Arif, bpf
Cc: ast, daniel, joe, fam.zheng, cong.wang, alexei.starovoitov, song
2022-01-11 11:08 UTC+0000 ~ Usama Arif <usama.arif@bytedance.com>
> Currently bpf_helper_defs.h is 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 generates a
> warning in the header file 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>
>
> ---
> 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 | 45 +++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
> index a6403ddf5de7..8d96f08ea7a6 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,40 @@ 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
> + self.seek_to('#define __BPF_FUNC_MAPPER(FN)',
> + 'Could not find start of eBPF helper definition list')
Hi, and thanks!
It might be worth a comment above the "seek_to()" to mention that
"FN(unspec)" is skipped here due to seek_to() discarding the first line
below the target (here, the first line below "#define
__BPF_FUNC_MAPPER(FN)").
> + # 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()
>
> ###############################################################################
> @@ -509,6 +532,8 @@ class PrinterHelpers(Printer):
> """
> def __init__(self, parser):
> self.elements = parser.helpers
> + self.desc_unique_helpers = parser.desc_unique_helpers
> + self.define_unique_helpers = parser.define_unique_helpers
>
> type_fwds = [
> 'struct bpf_fib_lookup',
> @@ -628,6 +653,22 @@ class PrinterHelpers(Printer):
> /* Forward declarations of BPF structs */'''
>
> print(header)
> +
> + nr_desc_unique_helpers = len(self.desc_unique_helpers)
> + nr_define_unique_helpers = len(self.define_unique_helpers)
> + if nr_desc_unique_helpers != nr_define_unique_helpers:
> + header_warning = '''
> +#warning 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.
> + header_warning += '''
> +#warning The description for %s is not present or formatted correctly.
> +''' % (self.define_unique_helpers[nr_desc_unique_helpers])
> + print(header_warning)
> +
> +
We should probably have the same error/warning when generating the man
page for the helpers (print_header() in class PrinterHelpersRST)?
> for fwd in self.type_fwds:
> print('%s;' % fwd)
> print('')
Thanks,
Quentin
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [External] Re: [PATCH v3] bpf/scripts: add warning if the correct number of helpers are not auto-generated
2022-01-11 11:35 ` Quentin Monnet
@ 2022-01-11 14:46 ` Usama Arif
0 siblings, 0 replies; 3+ messages in thread
From: Usama Arif @ 2022-01-11 14:46 UTC (permalink / raw)
To: Quentin Monnet, bpf
Cc: ast, daniel, joe, fam.zheng, cong.wang, alexei.starovoitov, song
On 11/01/2022 11:35, Quentin Monnet wrote:
> 2022-01-11 11:08 UTC+0000 ~ Usama Arif <usama.arif@bytedance.com>
>> Currently bpf_helper_defs.h is 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 generates a
>> warning in the header file 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>
>>
>> ---
>> 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 | 45 +++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 43 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
>> index a6403ddf5de7..8d96f08ea7a6 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,40 @@ 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
>> + self.seek_to('#define __BPF_FUNC_MAPPER(FN)',
>> + 'Could not find start of eBPF helper definition list')
>
> Hi, and thanks!
> It might be worth a comment above the "seek_to()" to mention that
> "FN(unspec)" is skipped here due to seek_to() discarding the first line
> below the target (here, the first line below "#define
> __BPF_FUNC_MAPPER(FN)").
Thanks! added to v4.
>
>> + # 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()
>>
>> ###############################################################################
>> @@ -509,6 +532,8 @@ class PrinterHelpers(Printer):
>> """
>> def __init__(self, parser):
>> self.elements = parser.helpers
>> + self.desc_unique_helpers = parser.desc_unique_helpers
>> + self.define_unique_helpers = parser.define_unique_helpers
>>
>> type_fwds = [
>> 'struct bpf_fib_lookup',
>> @@ -628,6 +653,22 @@ class PrinterHelpers(Printer):
>> /* Forward declarations of BPF structs */'''
>>
>> print(header)
>> +
>> + nr_desc_unique_helpers = len(self.desc_unique_helpers)
>> + nr_define_unique_helpers = len(self.define_unique_helpers)
>> + if nr_desc_unique_helpers != nr_define_unique_helpers:
>> + header_warning = '''
>> +#warning 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.
>> + header_warning += '''
>> +#warning The description for %s is not present or formatted correctly.
>> +''' % (self.define_unique_helpers[nr_desc_unique_helpers])
>> + print(header_warning)
>> +
>> +
>
> We should probably have the same error/warning when generating the man
> page for the helpers (print_header() in class PrinterHelpersRST)?
>
Thanks! added to v4.
>> for fwd in self.type_fwds:
>> print('%s;' % fwd)
>> print('')
>
> Thanks,
> Quentin
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-01-11 14:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-11 11:08 [PATCH v3] bpf/scripts: add warning if the correct number of helpers are not auto-generated Usama Arif
2022-01-11 11:35 ` Quentin Monnet
2022-01-11 14:46 ` [External] " Usama Arif
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).