* [PATCH bpf-next] bpf/scripts: use helper enum value instead of relying on comment order
@ 2022-08-19 9:12 Eyal Birger
2022-08-23 20:49 ` Quentin Monnet
0 siblings, 1 reply; 5+ messages in thread
From: Eyal Birger @ 2022-08-19 9:12 UTC (permalink / raw)
To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
Cc: bpf, Eyal Birger
The helper value is ABI as defined by enum bpf_func_id.
As bpf_helper_defs.h is used for the userpace part, it must be consistent
with this enum.
Before this change, the enumerated value was derived from the comment
order, which assumes comments are always appended, however, there doesn't
seem to be an enforcement anywhere for maintaining a strict order.
When adding new helpers it is very puzzling when the userspace application
breaks in weird places if the comment is inserted instead of appended -
because the generated helper ABI is incorrect and shifted.
This commit attempts to ease this by always using bpf_func_id order as
the helper value.
Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
---
scripts/bpf_doc.py | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
index dfb260de17a8..7797aa032eca 100755
--- a/scripts/bpf_doc.py
+++ b/scripts/bpf_doc.py
@@ -88,7 +88,7 @@ class HeaderParser(object):
self.helpers = []
self.commands = []
self.desc_unique_helpers = set()
- self.define_unique_helpers = []
+ self.define_unique_helpers = {}
self.desc_syscalls = []
self.enum_syscalls = []
@@ -245,24 +245,24 @@ class HeaderParser(object):
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.
+ # Parse FN(...) in #define __BPF_FUNC_MAPPER to compare later with the
+ # number of unique function names present in description and use the
+ # correct enumeration value.
# 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 = ''
+ p = re.compile('\s*FN\((\w+)\)+|\\\\')
+ i = 1 # 'unspec' is skipped as mentioned above
while True:
capture = p.match(self.line)
if capture:
- fn_defines_str += self.line
+ self.define_unique_helpers[capture.expand(r'bpf_\1')] = i
+ i += 1
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_desc_syscall()
@@ -573,6 +573,7 @@ class PrinterHelpers(Printer):
def __init__(self, parser):
self.elements = parser.helpers
self.elem_number_check(parser.desc_unique_helpers, parser.define_unique_helpers, 'helper', '__BPF_FUNC_MAPPER')
+ self.define_unique_helpers = parser.define_unique_helpers
type_fwds = [
'struct bpf_fib_lookup',
@@ -761,7 +762,7 @@ class PrinterHelpers(Printer):
comma = ', '
print(one_arg, end='')
- print(') = (void *) %d;' % len(self.seen_helpers))
+ print(') = (void *) %d;' % self.define_unique_helpers[proto['name']])
print('')
###############################################################################
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] bpf/scripts: use helper enum value instead of relying on comment order
2022-08-19 9:12 [PATCH bpf-next] bpf/scripts: use helper enum value instead of relying on comment order Eyal Birger
@ 2022-08-23 20:49 ` Quentin Monnet
2022-08-23 23:05 ` Eyal Birger
0 siblings, 1 reply; 5+ messages in thread
From: Quentin Monnet @ 2022-08-23 20:49 UTC (permalink / raw)
To: Eyal Birger
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf
On Fri, 19 Aug 2022 at 10:13, Eyal Birger <eyal.birger@gmail.com> wrote:
>
> The helper value is ABI as defined by enum bpf_func_id.
> As bpf_helper_defs.h is used for the userpace part, it must be consistent
> with this enum.
>
> Before this change, the enumerated value was derived from the comment
> order, which assumes comments are always appended, however, there doesn't
> seem to be an enforcement anywhere for maintaining a strict order.
>
> When adding new helpers it is very puzzling when the userspace application
> breaks in weird places if the comment is inserted instead of appended -
> because the generated helper ABI is incorrect and shifted.
>
> This commit attempts to ease this by always using bpf_func_id order as
> the helper value.
>
> Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
> ---
> scripts/bpf_doc.py | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
> index dfb260de17a8..7797aa032eca 100755
> --- a/scripts/bpf_doc.py
> +++ b/scripts/bpf_doc.py
> @@ -88,7 +88,7 @@ class HeaderParser(object):
> self.helpers = []
> self.commands = []
> self.desc_unique_helpers = set()
> - self.define_unique_helpers = []
> + self.define_unique_helpers = {}
> self.desc_syscalls = []
> self.enum_syscalls = []
>
> @@ -245,24 +245,24 @@ class HeaderParser(object):
> 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.
> + # Parse FN(...) in #define __BPF_FUNC_MAPPER to compare later with the
> + # number of unique function names present in description and use the
> + # correct enumeration value.
> # 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 = ''
> + p = re.compile('\s*FN\((\w+)\)+|\\\\')
Nit: I think the second '+' should be removed, I don't think you can
have consecutive "FN(...)" without at least a comma. But you didn't
add and it is harmless, so it can be a follow-up or wait until a
future clean-up.
> + i = 1 # 'unspec' is skipped as mentioned above
> while True:
> capture = p.match(self.line)
> if capture:
> - fn_defines_str += self.line
> + self.define_unique_helpers[capture.expand(r'bpf_\1')] = i
> + i += 1
> 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_desc_syscall()
> @@ -573,6 +573,7 @@ class PrinterHelpers(Printer):
> def __init__(self, parser):
> self.elements = parser.helpers
> self.elem_number_check(parser.desc_unique_helpers, parser.define_unique_helpers, 'helper', '__BPF_FUNC_MAPPER')
> + self.define_unique_helpers = parser.define_unique_helpers
>
> type_fwds = [
> 'struct bpf_fib_lookup',
> @@ -761,7 +762,7 @@ class PrinterHelpers(Printer):
> comma = ', '
> print(one_arg, end='')
>
> - print(') = (void *) %d;' % len(self.seen_helpers))
> + print(') = (void *) %d;' % self.define_unique_helpers[proto['name']])
> print('')
The code seems correct and should make the script more robust, and I
checked that the man page and header file are generated identically.
Reviewed-by: Quentin Monnet <quentin@isovalent.com>
However, I would recommend against inserting the description of new
helpers in the middle of the current documentation. Having the helpers
listed in order of creation is maybe not ideal, but at least they are
ordered, and the list remains consistent with the items of enum
bpf_func_id. I'm not opposed to reworking the list to have them
displayed in a more logical order, but in that case I think we should
reorganise the whole list, not just start inserting new descriptions
in the middle.
Thanks,
Quentin
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] bpf/scripts: use helper enum value instead of relying on comment order
2022-08-23 20:49 ` Quentin Monnet
@ 2022-08-23 23:05 ` Eyal Birger
2022-08-24 8:44 ` Quentin Monnet
0 siblings, 1 reply; 5+ messages in thread
From: Eyal Birger @ 2022-08-23 23:05 UTC (permalink / raw)
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf
Hi Quentin,
On Tue, Aug 23, 2022 at 11:49 PM Quentin Monnet <quentin@isovalent.com> wrote:
>
> On Fri, 19 Aug 2022 at 10:13, Eyal Birger <eyal.birger@gmail.com> wrote:
> >
> > The helper value is ABI as defined by enum bpf_func_id.
> > As bpf_helper_defs.h is used for the userpace part, it must be consistent
> > with this enum.
> >
> > Before this change, the enumerated value was derived from the comment
> > order, which assumes comments are always appended, however, there doesn't
> > seem to be an enforcement anywhere for maintaining a strict order.
> >
> > When adding new helpers it is very puzzling when the userspace application
> > breaks in weird places if the comment is inserted instead of appended -
> > because the generated helper ABI is incorrect and shifted.
> >
> > This commit attempts to ease this by always using bpf_func_id order as
> > the helper value.
> >
> > Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
> > ---
> > scripts/bpf_doc.py | 19 ++++++++++---------
> > 1 file changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
> > index dfb260de17a8..7797aa032eca 100755
> > --- a/scripts/bpf_doc.py
> > +++ b/scripts/bpf_doc.py
> > @@ -88,7 +88,7 @@ class HeaderParser(object):
> > self.helpers = []
> > self.commands = []
> > self.desc_unique_helpers = set()
> > - self.define_unique_helpers = []
> > + self.define_unique_helpers = {}
> > self.desc_syscalls = []
> > self.enum_syscalls = []
> >
> > @@ -245,24 +245,24 @@ class HeaderParser(object):
> > 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.
> > + # Parse FN(...) in #define __BPF_FUNC_MAPPER to compare later with the
> > + # number of unique function names present in description and use the
> > + # correct enumeration value.
> > # 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 = ''
> > + p = re.compile('\s*FN\((\w+)\)+|\\\\')
>
> Nit: I think the second '+' should be removed, I don't think you can
> have consecutive "FN(...)" without at least a comma. But you didn't
> add and it is harmless, so it can be a follow-up or wait until a
> future clean-up.
>
Sure. I can remove that.
> > + i = 1 # 'unspec' is skipped as mentioned above
> > while True:
> > capture = p.match(self.line)
> > if capture:
> > - fn_defines_str += self.line
> > + self.define_unique_helpers[capture.expand(r'bpf_\1')] = i
> > + i += 1
> > 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_desc_syscall()
> > @@ -573,6 +573,7 @@ class PrinterHelpers(Printer):
> > def __init__(self, parser):
> > self.elements = parser.helpers
> > self.elem_number_check(parser.desc_unique_helpers, parser.define_unique_helpers, 'helper', '__BPF_FUNC_MAPPER')
> > + self.define_unique_helpers = parser.define_unique_helpers
> >
> > type_fwds = [
> > 'struct bpf_fib_lookup',
> > @@ -761,7 +762,7 @@ class PrinterHelpers(Printer):
> > comma = ', '
> > print(one_arg, end='')
> >
> > - print(') = (void *) %d;' % len(self.seen_helpers))
> > + print(') = (void *) %d;' % self.define_unique_helpers[proto['name']])
> > print('')
>
> The code seems correct and should make the script more robust, and I
> checked that the man page and header file are generated identically.
>
> Reviewed-by: Quentin Monnet <quentin@isovalent.com>
Thanks for the review.
>
> However, I would recommend against inserting the description of new
> helpers in the middle of the current documentation. Having the helpers
> listed in order of creation is maybe not ideal, but at least they are
> ordered, and the list remains consistent with the items of enum
> bpf_func_id. I'm not opposed to reworking the list to have them
> displayed in a more logical order, but in that case I think we should
> reorganise the whole list, not just start inserting new descriptions
> in the middle.
>
I understand. Personally I don't mind the fact that they're ordered
relative to their enum value, only that this is implicitly enforced.
Since we know both the enum value and the comment value, would it be
acceptible to add an assertion here so that at least wrongful insertions
break the file generation instead of skewing the values?
Eyal.
> Thanks,
> Quentin
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] bpf/scripts: use helper enum value instead of relying on comment order
2022-08-23 23:05 ` Eyal Birger
@ 2022-08-24 8:44 ` Quentin Monnet
2022-08-24 9:04 ` Eyal Birger
0 siblings, 1 reply; 5+ messages in thread
From: Quentin Monnet @ 2022-08-24 8:44 UTC (permalink / raw)
To: Eyal Birger
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf
On 24/08/2022 00:05, Eyal Birger wrote:
> Hi Quentin,
>
> On Tue, Aug 23, 2022 at 11:49 PM Quentin Monnet <quentin@isovalent.com> wrote:
>>
>> On Fri, 19 Aug 2022 at 10:13, Eyal Birger <eyal.birger@gmail.com> wrote:
>>>
>>> The helper value is ABI as defined by enum bpf_func_id.
>>> As bpf_helper_defs.h is used for the userpace part, it must be consistent
>>> with this enum.
>>>
>>> Before this change, the enumerated value was derived from the comment
>>> order, which assumes comments are always appended, however, there doesn't
>>> seem to be an enforcement anywhere for maintaining a strict order.
>>>
>>> When adding new helpers it is very puzzling when the userspace application
>>> breaks in weird places if the comment is inserted instead of appended -
>>> because the generated helper ABI is incorrect and shifted.
>>>
>>> This commit attempts to ease this by always using bpf_func_id order as
>>> the helper value.
>>>
>>> Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
>>> ---
>>> scripts/bpf_doc.py | 19 ++++++++++---------
>>> 1 file changed, 10 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
>>> index dfb260de17a8..7797aa032eca 100755
>>> --- a/scripts/bpf_doc.py
>>> +++ b/scripts/bpf_doc.py
>>> @@ -88,7 +88,7 @@ class HeaderParser(object):
>>> self.helpers = []
>>> self.commands = []
>>> self.desc_unique_helpers = set()
>>> - self.define_unique_helpers = []
>>> + self.define_unique_helpers = {}
>>> self.desc_syscalls = []
>>> self.enum_syscalls = []
>>>
>>> @@ -245,24 +245,24 @@ class HeaderParser(object):
>>> 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.
>>> + # Parse FN(...) in #define __BPF_FUNC_MAPPER to compare later with the
>>> + # number of unique function names present in description and use the
>>> + # correct enumeration value.
>>> # 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 = ''
>>> + p = re.compile('\s*FN\((\w+)\)+|\\\\')
>>
>> Nit: I think the second '+' should be removed, I don't think you can
>> have consecutive "FN(...)" without at least a comma. But you didn't
>> add and it is harmless, so it can be a follow-up or wait until a
>> future clean-up.
>>
>
> Sure. I can remove that.
>
>>> + i = 1 # 'unspec' is skipped as mentioned above
>>> while True:
>>> capture = p.match(self.line)
>>> if capture:
>>> - fn_defines_str += self.line
>>> + self.define_unique_helpers[capture.expand(r'bpf_\1')] = i
>>> + i += 1
>>> 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_desc_syscall()
>>> @@ -573,6 +573,7 @@ class PrinterHelpers(Printer):
>>> def __init__(self, parser):
>>> self.elements = parser.helpers
>>> self.elem_number_check(parser.desc_unique_helpers, parser.define_unique_helpers, 'helper', '__BPF_FUNC_MAPPER')
>>> + self.define_unique_helpers = parser.define_unique_helpers
>>>
>>> type_fwds = [
>>> 'struct bpf_fib_lookup',
>>> @@ -761,7 +762,7 @@ class PrinterHelpers(Printer):
>>> comma = ', '
>>> print(one_arg, end='')
>>>
>>> - print(') = (void *) %d;' % len(self.seen_helpers))
>>> + print(') = (void *) %d;' % self.define_unique_helpers[proto['name']])
>>> print('')
>>
>> The code seems correct and should make the script more robust, and I
>> checked that the man page and header file are generated identically.
>>
>> Reviewed-by: Quentin Monnet <quentin@isovalent.com>
>
> Thanks for the review.
>
>>
>> However, I would recommend against inserting the description of new
>> helpers in the middle of the current documentation. Having the helpers
>> listed in order of creation is maybe not ideal, but at least they are
>> ordered, and the list remains consistent with the items of enum
>> bpf_func_id. I'm not opposed to reworking the list to have them
>> displayed in a more logical order, but in that case I think we should
>> reorganise the whole list, not just start inserting new descriptions
>> in the middle.
>>
>
> I understand. Personally I don't mind the fact that they're ordered
> relative to their enum value, only that this is implicitly enforced.
>
> Since we know both the enum value and the comment value, would it be
> acceptible to add an assertion here so that at least wrongful insertions
> break the file generation instead of skewing the values?
As I understand it, your patch already solves the issue by making sure
we use the correct value even if the descriptions do not come in the
same order as the enum items. Do you mean adding an additional check to
enforce that the description items are in the same order, in addition to
your patch?
I don't have a strong opinion, if anything I'd say it's probably not the
role of this script to ensure that the description items are in a
particular order (provided your patch is applied and the values are
correct). I'm not sure we want to strongly enforce the order; I would
definitely recommend against inserting new items in the middle, but at
the same time I wouldn't oppose some reorganisation into logical
sections. On the other hand, it's probably cleaner to have the
definitions in the generated header file listed in the correct order of
the enum values, so why not having the assertion for now, and lifting it
if we ever want to rework the order.
Quentin
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] bpf/scripts: use helper enum value instead of relying on comment order
2022-08-24 8:44 ` Quentin Monnet
@ 2022-08-24 9:04 ` Eyal Birger
0 siblings, 0 replies; 5+ messages in thread
From: Eyal Birger @ 2022-08-24 9:04 UTC (permalink / raw)
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf
On Wed, Aug 24, 2022 at 11:44 AM Quentin Monnet <quentin@isovalent.com> wrote:
>
> On 24/08/2022 00:05, Eyal Birger wrote:
> > Hi Quentin,
> >
> > On Tue, Aug 23, 2022 at 11:49 PM Quentin Monnet <quentin@isovalent.com> wrote:
> >>
> >> On Fri, 19 Aug 2022 at 10:13, Eyal Birger <eyal.birger@gmail.com> wrote:
> >>>
> >>> The helper value is ABI as defined by enum bpf_func_id.
> >>> As bpf_helper_defs.h is used for the userpace part, it must be consistent
> >>> with this enum.
> >>>
> >>> Before this change, the enumerated value was derived from the comment
> >>> order, which assumes comments are always appended, however, there doesn't
> >>> seem to be an enforcement anywhere for maintaining a strict order.
> >>>
> >>> When adding new helpers it is very puzzling when the userspace application
> >>> breaks in weird places if the comment is inserted instead of appended -
> >>> because the generated helper ABI is incorrect and shifted.
> >>>
> >>> This commit attempts to ease this by always using bpf_func_id order as
> >>> the helper value.
> >>>
> >>> Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
> >>> ---
> >>> scripts/bpf_doc.py | 19 ++++++++++---------
> >>> 1 file changed, 10 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
> >>> index dfb260de17a8..7797aa032eca 100755
> >>> --- a/scripts/bpf_doc.py
> >>> +++ b/scripts/bpf_doc.py
> >>> @@ -88,7 +88,7 @@ class HeaderParser(object):
> >>> self.helpers = []
> >>> self.commands = []
> >>> self.desc_unique_helpers = set()
> >>> - self.define_unique_helpers = []
> >>> + self.define_unique_helpers = {}
> >>> self.desc_syscalls = []
> >>> self.enum_syscalls = []
> >>>
> >>> @@ -245,24 +245,24 @@ class HeaderParser(object):
> >>> 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.
> >>> + # Parse FN(...) in #define __BPF_FUNC_MAPPER to compare later with the
> >>> + # number of unique function names present in description and use the
> >>> + # correct enumeration value.
> >>> # 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 = ''
> >>> + p = re.compile('\s*FN\((\w+)\)+|\\\\')
> >>
> >> Nit: I think the second '+' should be removed, I don't think you can
> >> have consecutive "FN(...)" without at least a comma. But you didn't
> >> add and it is harmless, so it can be a follow-up or wait until a
> >> future clean-up.
> >>
> >
> > Sure. I can remove that.
> >
> >>> + i = 1 # 'unspec' is skipped as mentioned above
> >>> while True:
> >>> capture = p.match(self.line)
> >>> if capture:
> >>> - fn_defines_str += self.line
> >>> + self.define_unique_helpers[capture.expand(r'bpf_\1')] = i
> >>> + i += 1
> >>> 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_desc_syscall()
> >>> @@ -573,6 +573,7 @@ class PrinterHelpers(Printer):
> >>> def __init__(self, parser):
> >>> self.elements = parser.helpers
> >>> self.elem_number_check(parser.desc_unique_helpers, parser.define_unique_helpers, 'helper', '__BPF_FUNC_MAPPER')
> >>> + self.define_unique_helpers = parser.define_unique_helpers
> >>>
> >>> type_fwds = [
> >>> 'struct bpf_fib_lookup',
> >>> @@ -761,7 +762,7 @@ class PrinterHelpers(Printer):
> >>> comma = ', '
> >>> print(one_arg, end='')
> >>>
> >>> - print(') = (void *) %d;' % len(self.seen_helpers))
> >>> + print(') = (void *) %d;' % self.define_unique_helpers[proto['name']])
> >>> print('')
> >>
> >> The code seems correct and should make the script more robust, and I
> >> checked that the man page and header file are generated identically.
> >>
> >> Reviewed-by: Quentin Monnet <quentin@isovalent.com>
> >
> > Thanks for the review.
> >
> >>
> >> However, I would recommend against inserting the description of new
> >> helpers in the middle of the current documentation. Having the helpers
> >> listed in order of creation is maybe not ideal, but at least they are
> >> ordered, and the list remains consistent with the items of enum
> >> bpf_func_id. I'm not opposed to reworking the list to have them
> >> displayed in a more logical order, but in that case I think we should
> >> reorganise the whole list, not just start inserting new descriptions
> >> in the middle.
> >>
> >
> > I understand. Personally I don't mind the fact that they're ordered
> > relative to their enum value, only that this is implicitly enforced.
> >
> > Since we know both the enum value and the comment value, would it be
> > acceptible to add an assertion here so that at least wrongful insertions
> > break the file generation instead of skewing the values?
>
> As I understand it, your patch already solves the issue by making sure
> we use the correct value even if the descriptions do not come in the
> same order as the enum items. Do you mean adding an additional check to
> enforce that the description items are in the same order, in addition to
> your patch?
>
Yes. The patch would fetch the value from the enum, but also assert that
the enum value is the same as the comment order value.
> I don't have a strong opinion, if anything I'd say it's probably not the
> role of this script to ensure that the description items are in a
> particular order (provided your patch is applied and the values are
> correct). I'm not sure we want to strongly enforce the order; I would
> definitely recommend against inserting new items in the middle, but at
> the same time I wouldn't oppose some reorganisation into logical
> sections. On the other hand, it's probably cleaner to have the
> definitions in the generated header file listed in the correct order of
> the enum values, so why not having the assertion for now, and lifting it
> if we ever want to rework the order.
>
I agree. I think for now we can have the assertion, and lift it when
a reorganization of the comments is done. I'll send a v2.
> Quentin
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-24 9:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-19 9:12 [PATCH bpf-next] bpf/scripts: use helper enum value instead of relying on comment order Eyal Birger
2022-08-23 20:49 ` Quentin Monnet
2022-08-23 23:05 ` Eyal Birger
2022-08-24 8:44 ` Quentin Monnet
2022-08-24 9:04 ` Eyal Birger
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.