* [PATCH v5] bpf/scripts: add an error if the correct number of helpers are not generated
@ 2022-01-11 18:44 Usama Arif
2022-01-11 21:49 ` Song Liu
2022-01-12 10:01 ` Quentin Monnet
0 siblings, 2 replies; 6+ messages in thread
From: Usama Arif @ 2022-01-11 18:44 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 generates an
error in the header file and the man page 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>
---
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 | 74 +++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 70 insertions(+), 4 deletions(-)
diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
index a6403ddf5de7..adf08fa963a4 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()
###############################################################################
@@ -305,9 +330,11 @@ class PrinterHelpersRST(PrinterRST):
"""
def __init__(self, parser):
self.elements = parser.helpers
+ self.desc_unique_helpers = parser.desc_unique_helpers
+ self.define_unique_helpers = parser.define_unique_helpers
def print_header(self):
- header = '''\
+ header_name = '''\
===========
BPF-HELPERS
===========
@@ -317,6 +344,8 @@ list of eBPF helper functions
:Manual section: 7
+'''
+ header_description = '''
DESCRIPTION
===========
@@ -349,7 +378,27 @@ HELPERS
=======
'''
PrinterRST.print_license(self)
- print(header)
+
+ print(header_name)
+
+ # Add an error if the correct number of helpers are not auto-generated.
+ 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_error = '''
+.. error::
+ 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_error += '''
+.. error::
+ The description for %s is not present or formatted correctly.
+''' % (self.define_unique_helpers[nr_desc_unique_helpers])
+ print(header_error)
+
+ print(header_description)
def print_footer(self):
footer = '''
@@ -509,6 +558,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 +679,21 @@ 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_error = '''
+#error 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_error += '''
+#error The description for %s is not present or formatted correctly.
+''' % (self.define_unique_helpers[nr_desc_unique_helpers])
+ print(header_error)
+
for fwd in self.type_fwds:
print('%s;' % fwd)
print('')
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v5] bpf/scripts: add an error if the correct number of helpers are not generated
2022-01-11 18:44 [PATCH v5] bpf/scripts: add an error if the correct number of helpers are not generated Usama Arif
@ 2022-01-11 21:49 ` Song Liu
2022-01-12 10:01 ` Quentin Monnet
1 sibling, 0 replies; 6+ messages in thread
From: Song Liu @ 2022-01-11 21:49 UTC (permalink / raw)
To: Usama Arif
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, joe, fam.zheng,
Cong Wang, Alexei Starovoitov, Quentin Monnet
On Tue, Jan 11, 2022 at 10:44 AM Usama Arif <usama.arif@bytedance.com> wrote:
>
> 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 generates an
> error in the header file and the man page 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>
Acked-by: Song Liu <songliubraving@fb.com>
[...]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v5] bpf/scripts: add an error if the correct number of helpers are not generated
2022-01-11 18:44 [PATCH v5] bpf/scripts: add an error if the correct number of helpers are not generated Usama Arif
2022-01-11 21:49 ` Song Liu
@ 2022-01-12 10:01 ` Quentin Monnet
2022-01-12 10:19 ` [External] " Usama Arif
1 sibling, 1 reply; 6+ messages in thread
From: Quentin Monnet @ 2022-01-12 10:01 UTC (permalink / raw)
To: Usama Arif, bpf
Cc: ast, daniel, joe, fam.zheng, cong.wang, alexei.starovoitov, song
2022-01-11 18:44 UTC+0000 ~ Usama Arif <usama.arif@bytedance.com>
> 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 generates an
> error in the header file and the man page 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>
>
> ---
> v4->v5:
> - Converted warning to error incase of missing/misformatted helper doc
> (suggested by Song Liu)
I don't think it was converted to an error in the sense that Song meant
it? Unless I'm missing something you simply changed the message so that
it prints "error" instead of "warning", but the script still goes on
without returning any error code, and a failure won't be detected by the
CI for example.
Could you make the script break out on errors, and print a message to
stderr so that it's visible even if the generated output is redirected
to a file, please?
>
> 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 | 74 +++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 70 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
> index a6403ddf5de7..adf08fa963a4 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()
>
> ###############################################################################
> @@ -305,9 +330,11 @@ class PrinterHelpersRST(PrinterRST):
> """
> def __init__(self, parser):
> self.elements = parser.helpers
> + self.desc_unique_helpers = parser.desc_unique_helpers
> + self.define_unique_helpers = parser.define_unique_helpers
>
> def print_header(self):
> - header = '''\
> + header_name = '''\
> ===========
> BPF-HELPERS
> ===========
> @@ -317,6 +344,8 @@ list of eBPF helper functions
>
> :Manual section: 7
>
> +'''
> + header_description = '''
> DESCRIPTION
> ===========
>
> @@ -349,7 +378,27 @@ HELPERS
> =======
> '''
> PrinterRST.print_license(self)
> - print(header)
> +
> + print(header_name)
> +
> + # Add an error if the correct number of helpers are not auto-generated.
> + 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_error = '''
> +.. error::
> + The number of unique helpers in description (%d) don\'t match the number of unique helpers defined in __BPF_FUNC_MAPPER (%d)
don\'t -> doesn\'t
> +''' % (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_error += '''
> +.. error::
> + The description for %s is not present or formatted correctly.
> +''' % (self.define_unique_helpers[nr_desc_unique_helpers])
> + print(header_error)
> +
> + print(header_description)
>
> def print_footer(self):
> footer = '''
> @@ -509,6 +558,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 +679,21 @@ 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_error = '''
> +#error 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_error += '''
> +#error The description for %s is not present or formatted correctly.
> +''' % (self.define_unique_helpers[nr_desc_unique_helpers])
> + print(header_error)
> +
> for fwd in self.type_fwds:
> print('%s;' % fwd)
> print('')
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [External] Re: [PATCH v5] bpf/scripts: add an error if the correct number of helpers are not generated
2022-01-12 10:01 ` Quentin Monnet
@ 2022-01-12 10:19 ` Usama Arif
2022-01-12 11:04 ` Quentin Monnet
0 siblings, 1 reply; 6+ messages in thread
From: Usama Arif @ 2022-01-12 10:19 UTC (permalink / raw)
To: Quentin Monnet, bpf
Cc: ast, daniel, joe, fam.zheng, cong.wang, alexei.starovoitov, song
On 12/01/2022 10:01, Quentin Monnet wrote:
> 2022-01-11 18:44 UTC+0000 ~ Usama Arif <usama.arif@bytedance.com>
>> 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 generates an
>> error in the header file and the man page 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>
>>
>> ---
>> v4->v5:
>> - Converted warning to error incase of missing/misformatted helper doc
>> (suggested by Song Liu)
>
> I don't think it was converted to an error in the sense that Song meant
> it? Unless I'm missing something you simply changed the message so that
> it prints "error" instead of "warning", but the script still goes on
> without returning any error code, and a failure won't be detected by the
> CI for example.
>
> Could you make the script break out on errors, and print a message to
> stderr so that it's visible even if the generated output is redirected
> to a file, please?
>
It does now print an error to stdout while building an eBPF application.
For e.g. if you introduce a space in the doc as in the commit message like:
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index ba5af15e25f5..5bf80dbb820b 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4908,7 +4908,7 @@ union bpf_attr {
*
* **-ENOENT** if architecture does not support branch
records.
*
- * long bpf_trace_vprintk(const char *fmt, u32 fmt_size, const void
*data, u32 data_len)
+ * long bpf_trace_vprintk( const char *fmt, u32 fmt_size, const void
*data, u32 data_len)
* Description
* Behaves like **bpf_trace_printk**\ () helper, but takes
an array of u64
* to format and can handle more format args as a result.
@@ -4938,6 +4938,12 @@ union bpf_attr {
* **-ENOENT** if symbol is not found.
*
* **-EPERM** if caller does not have permission to obtain
kernel address.
and build samples/bpf:
make LLVM_STRIP=llvm-strip-13 M=samples/bpf > /tmp/samplesbuild.out
you get the following at stderr returning an error code
make[2]: *** [Makefile:186:
/data/usaari01/ebpf/linux/samples/bpf/bpftool/pid_iter.bpf.o] Error 1
make[2]: *** Waiting for unfinished jobs....
In file included from skeleton/profiler.bpf.c:4:
In file included from
/data/usaari01/ebpf/linux/samples/bpf/bpftool//bootstrap/libbpf//include/bpf/bpf_helpers.h:11:
/data/usaari01/ebpf/linux/samples/bpf/bpftool//bootstrap/libbpf//include/bpf/bpf_helper_defs.h:5:2:
error: The number of unique helpers in description (176) don't match the
number of unique helpers defined in __BPF_FUNC_MAPPER (180)
#error The number of unique helpers in description (176) don't match the
number of unique helpers defined in __BPF_FUNC_MAPPER (180)
^
/data/usaari01/ebpf/linux/samples/bpf/bpftool//bootstrap/libbpf//include/bpf/bpf_helper_defs.h:7:2:
error: The description for FN(trace_vprintk) is not present or formatted
correctly.
#error The description for FN(trace_vprintk) is not present or formatted
correctly.
^
But i am guessing that you want an error while the script is run as well?
If we do this:
diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
index adf08fa963a4..4ce982ce58f2 100755
--- a/scripts/bpf_doc.py
+++ b/scripts/bpf_doc.py
@@ -397,6 +397,7 @@ HELPERS
The description for %s is not present or formatted correctly.
''' % (self.define_unique_helpers[nr_desc_unique_helpers])
print(header_error)
+ print(header_error, file = sys.stderr)
print(header_description)
@@ -693,6 +694,7 @@ class PrinterHelpers(Printer):
#error The description for %s is not present or formatted correctly.
''' % (self.define_unique_helpers[nr_desc_unique_helpers])
print(header_error)
+ print(header_error, file = sys.stderr)
for fwd in self.type_fwds:
print('%s;' % fwd)
then an error will be printed while the script is run and also later
while the eBPF application is compiled. I can send this in next version
if thats the preference?
>>
>> 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 | 74 +++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 70 insertions(+), 4 deletions(-)
>>
>> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
>> index a6403ddf5de7..adf08fa963a4 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()
>>
>> ###############################################################################
>> @@ -305,9 +330,11 @@ class PrinterHelpersRST(PrinterRST):
>> """
>> def __init__(self, parser):
>> self.elements = parser.helpers
>> + self.desc_unique_helpers = parser.desc_unique_helpers
>> + self.define_unique_helpers = parser.define_unique_helpers
>>
>> def print_header(self):
>> - header = '''\
>> + header_name = '''\
>> ===========
>> BPF-HELPERS
>> ===========
>> @@ -317,6 +344,8 @@ list of eBPF helper functions
>>
>> :Manual section: 7
>>
>> +'''
>> + header_description = '''
>> DESCRIPTION
>> ===========
>>
>> @@ -349,7 +378,27 @@ HELPERS
>> =======
>> '''
>> PrinterRST.print_license(self)
>> - print(header)
>> +
>> + print(header_name)
>> +
>> + # Add an error if the correct number of helpers are not auto-generated.
>> + 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_error = '''
>> +.. error::
>> + The number of unique helpers in description (%d) don\'t match the number of unique helpers defined in __BPF_FUNC_MAPPER (%d)
>
> don\'t -> doesn\'t
>
>> +''' % (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_error += '''
>> +.. error::
>> + The description for %s is not present or formatted correctly.
>> +''' % (self.define_unique_helpers[nr_desc_unique_helpers])
>> + print(header_error)
>> +
>> + print(header_description)
>>
>> def print_footer(self):
>> footer = '''
>> @@ -509,6 +558,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 +679,21 @@ 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_error = '''
>> +#error 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_error += '''
>> +#error The description for %s is not present or formatted correctly.
>> +''' % (self.define_unique_helpers[nr_desc_unique_helpers])
>> + print(header_error)
>> +
>> for fwd in self.type_fwds:
>> print('%s;' % fwd)
>> print('')
>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [External] Re: [PATCH v5] bpf/scripts: add an error if the correct number of helpers are not generated
2022-01-12 10:19 ` [External] " Usama Arif
@ 2022-01-12 11:04 ` Quentin Monnet
2022-01-12 11:52 ` Usama Arif
0 siblings, 1 reply; 6+ messages in thread
From: Quentin Monnet @ 2022-01-12 11:04 UTC (permalink / raw)
To: Usama Arif, bpf
Cc: ast, daniel, joe, fam.zheng, cong.wang, alexei.starovoitov, song
2022-01-12 10:19 UTC+0000 ~ Usama Arif <usama.arif@bytedance.com>
>
>
> On 12/01/2022 10:01, Quentin Monnet wrote:
>> 2022-01-11 18:44 UTC+0000 ~ Usama Arif <usama.arif@bytedance.com>
>>> 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 generates an
>>> error in the header file and the man page 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>
>>>
>>> ---
>>> v4->v5:
>>> - Converted warning to error incase of missing/misformatted helper doc
>>> (suggested by Song Liu)
>>
>> I don't think it was converted to an error in the sense that Song meant
>> it? Unless I'm missing something you simply changed the message so that
>> it prints "error" instead of "warning", but the script still goes on
>> without returning any error code, and a failure won't be detected by the
>> CI for example.
>>
>> Could you make the script break out on errors, and print a message to
>> stderr so that it's visible even if the generated output is redirected
>> to a file, please?
>>
>
> It does now print an error to stdout while building an eBPF application.
> For e.g. if you introduce a space in the doc as in the commit message like:
>
> diff --git a/tools/include/uapi/linux/bpf.h
> b/tools/include/uapi/linux/bpf.h
> index ba5af15e25f5..5bf80dbb820b 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -4908,7 +4908,7 @@ union bpf_attr {
> *
> * **-ENOENT** if architecture does not support branch
> records.
> *
> - * long bpf_trace_vprintk(const char *fmt, u32 fmt_size, const void
> *data, u32 data_len)
> + * long bpf_trace_vprintk( const char *fmt, u32 fmt_size, const void
> *data, u32 data_len)
> * Description
> * Behaves like **bpf_trace_printk**\ () helper, but takes
> an array of u64
> * to format and can handle more format args as a result.
> @@ -4938,6 +4938,12 @@ union bpf_attr {
> * **-ENOENT** if symbol is not found.
> *
> * **-EPERM** if caller does not have permission to obtain
> kernel address.
>
> and build samples/bpf:
>
> make LLVM_STRIP=llvm-strip-13 M=samples/bpf > /tmp/samplesbuild.out
>
> you get the following at stderr returning an error code
>
> make[2]: *** [Makefile:186:
> /data/usaari01/ebpf/linux/samples/bpf/bpftool/pid_iter.bpf.o] Error 1
> make[2]: *** Waiting for unfinished jobs....
> In file included from skeleton/profiler.bpf.c:4:
> In file included from
> /data/usaari01/ebpf/linux/samples/bpf/bpftool//bootstrap/libbpf//include/bpf/bpf_helpers.h:11:
>
> /data/usaari01/ebpf/linux/samples/bpf/bpftool//bootstrap/libbpf//include/bpf/bpf_helper_defs.h:5:2:
> error: The number of unique helpers in description (176) don't match the
> number of unique helpers defined in __BPF_FUNC_MAPPER (180)
> #error The number of unique helpers in description (176) don't match the
> number of unique helpers defined in __BPF_FUNC_MAPPER (180)
> ^
> /data/usaari01/ebpf/linux/samples/bpf/bpftool//bootstrap/libbpf//include/bpf/bpf_helper_defs.h:7:2:
> error: The description for FN(trace_vprintk) is not present or formatted
> correctly.
> #error The description for FN(trace_vprintk) is not present or formatted
> correctly.
> ^
Right, my bad, I tried your patch to generate the header but didn't go
so far as to include it and try to compile a program.
> But i am guessing that you want an error while the script is run as well?
> If we do this:
> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
> index adf08fa963a4..4ce982ce58f2 100755
> --- a/scripts/bpf_doc.py
> +++ b/scripts/bpf_doc.py
> @@ -397,6 +397,7 @@ HELPERS
> The description for %s is not present or formatted correctly.
> ''' % (self.define_unique_helpers[nr_desc_unique_helpers])
> print(header_error)
> + print(header_error, file = sys.stderr)
>
> print(header_description)
>
> @@ -693,6 +694,7 @@ class PrinterHelpers(Printer):
> #error The description for %s is not present or formatted correctly.
> ''' % (self.define_unique_helpers[nr_desc_unique_helpers])
> print(header_error)
> + print(header_error, file = sys.stderr)
>
> for fwd in self.type_fwds:
> print('%s;' % fwd)
>
> then an error will be printed while the script is run and also later
> while the eBPF application is compiled. I can send this in next version
> if thats the preference?
Yes, this is what I meant. From my point of view it would be best if we
had this message, and also if we could make bpf_doc.py raise an
Exception on such errors. Given that we can tell at this step already
that compiling will fail, we should as well break the workflow here,
there's not much point in carrying on and calling the compiler.
Thanks,
Quentin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v5] bpf/scripts: add an error if the correct number of helpers are not generated
2022-01-12 11:04 ` Quentin Monnet
@ 2022-01-12 11:52 ` Usama Arif
0 siblings, 0 replies; 6+ messages in thread
From: Usama Arif @ 2022-01-12 11:52 UTC (permalink / raw)
To: Quentin Monnet, bpf
Cc: ast, daniel, joe, fam.zheng, cong.wang, alexei.starovoitov, song
On 12/01/2022 11:04, Quentin Monnet wrote:
> 2022-01-12 10:19 UTC+0000 ~ Usama Arif <usama.arif@bytedance.com>
>>
>>
>> On 12/01/2022 10:01, Quentin Monnet wrote:
>>> 2022-01-11 18:44 UTC+0000 ~ Usama Arif <usama.arif@bytedance.com>
>>>> 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 generates an
>>>> error in the header file and the man page 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>
>>>>
>>>> ---
>>>> v4->v5:
>>>> - Converted warning to error incase of missing/misformatted helper doc
>>>> (suggested by Song Liu)
>>>
>>> I don't think it was converted to an error in the sense that Song meant
>>> it? Unless I'm missing something you simply changed the message so that
>>> it prints "error" instead of "warning", but the script still goes on
>>> without returning any error code, and a failure won't be detected by the
>>> CI for example.
>>>
>>> Could you make the script break out on errors, and print a message to
>>> stderr so that it's visible even if the generated output is redirected
>>> to a file, please?
>>>
>>
>> It does now print an error to stdout while building an eBPF application.
>> For e.g. if you introduce a space in the doc as in the commit message like:
>>
>> diff --git a/tools/include/uapi/linux/bpf.h
>> b/tools/include/uapi/linux/bpf.h
>> index ba5af15e25f5..5bf80dbb820b 100644
>> --- a/tools/include/uapi/linux/bpf.h
>> +++ b/tools/include/uapi/linux/bpf.h
>> @@ -4908,7 +4908,7 @@ union bpf_attr {
>> *
>> * **-ENOENT** if architecture does not support branch
>> records.
>> *
>> - * long bpf_trace_vprintk(const char *fmt, u32 fmt_size, const void
>> *data, u32 data_len)
>> + * long bpf_trace_vprintk( const char *fmt, u32 fmt_size, const void
>> *data, u32 data_len)
>> * Description
>> * Behaves like **bpf_trace_printk**\ () helper, but takes
>> an array of u64
>> * to format and can handle more format args as a result.
>> @@ -4938,6 +4938,12 @@ union bpf_attr {
>> * **-ENOENT** if symbol is not found.
>> *
>> * **-EPERM** if caller does not have permission to obtain
>> kernel address.
>>
>> and build samples/bpf:
>>
>> make LLVM_STRIP=llvm-strip-13 M=samples/bpf > /tmp/samplesbuild.out
>>
>> you get the following at stderr returning an error code
>>
>> make[2]: *** [Makefile:186:
>> /data/usaari01/ebpf/linux/samples/bpf/bpftool/pid_iter.bpf.o] Error 1
>> make[2]: *** Waiting for unfinished jobs....
>> In file included from skeleton/profiler.bpf.c:4:
>> In file included from
>> /data/usaari01/ebpf/linux/samples/bpf/bpftool//bootstrap/libbpf//include/bpf/bpf_helpers.h:11:
>>
>> /data/usaari01/ebpf/linux/samples/bpf/bpftool//bootstrap/libbpf//include/bpf/bpf_helper_defs.h:5:2:
>> error: The number of unique helpers in description (176) don't match the
>> number of unique helpers defined in __BPF_FUNC_MAPPER (180)
>> #error The number of unique helpers in description (176) don't match the
>> number of unique helpers defined in __BPF_FUNC_MAPPER (180)
>> ^
>> /data/usaari01/ebpf/linux/samples/bpf/bpftool//bootstrap/libbpf//include/bpf/bpf_helper_defs.h:7:2:
>> error: The description for FN(trace_vprintk) is not present or formatted
>> correctly.
>> #error The description for FN(trace_vprintk) is not present or formatted
>> correctly.
>> ^
>
> Right, my bad, I tried your patch to generate the header but didn't go
> so far as to include it and try to compile a program.
>
>> But i am guessing that you want an error while the script is run as well?
>> If we do this:
>> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
>> index adf08fa963a4..4ce982ce58f2 100755
>> --- a/scripts/bpf_doc.py
>> +++ b/scripts/bpf_doc.py
>> @@ -397,6 +397,7 @@ HELPERS
>> The description for %s is not present or formatted correctly.
>> ''' % (self.define_unique_helpers[nr_desc_unique_helpers])
>> print(header_error)
>> + print(header_error, file = sys.stderr)
>>
>> print(header_description)
>>
>> @@ -693,6 +694,7 @@ class PrinterHelpers(Printer):
>> #error The description for %s is not present or formatted correctly.
>> ''' % (self.define_unique_helpers[nr_desc_unique_helpers])
>> print(header_error)
>> + print(header_error, file = sys.stderr)
>>
>> for fwd in self.type_fwds:
>> print('%s;' % fwd)
>>
>> then an error will be printed while the script is run and also later
>> while the eBPF application is compiled. I can send this in next version
>> if thats the preference?
>
> Yes, this is what I meant. From my point of view it would be best if we
> had this message, and also if we could make bpf_doc.py raise an
> Exception on such errors. Given that we can tell at this step already
> that compiling will fail, we should as well break the workflow here,
> there's not much point in carrying on and calling the compiler.
>
Thanks, i have sent v6 which raises an Exception. No point in writing to
the auto-generated header/rst if the script raises an Exception as the
actual helpers wont be written so i have moved the check to the constructor.
> Thanks,
> Quentin
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-12 11:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-11 18:44 [PATCH v5] bpf/scripts: add an error if the correct number of helpers are not generated Usama Arif
2022-01-11 21:49 ` Song Liu
2022-01-12 10:01 ` Quentin Monnet
2022-01-12 10:19 ` [External] " Usama Arif
2022-01-12 11:04 ` Quentin Monnet
2022-01-12 11:52 ` 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).