Re: [PATCH bpf-next] bpf/scripts: use helper enum value instead of relying on comment order

[Quentin Monnet <quentin@isovalent.com>]

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