On Tue, Jan 11, 2022 at 3:17 AM Usama Arif <usama.arif@xxxxxxxxxxxxx> wrote:
>
>
>
> On 10/01/2022 22:43, Song Liu wrote:
> > On Mon, Jan 10, 2022 at 6:31 AM Usama Arif <usama.arif@xxxxxxxxxxxxx> wrote:
> >>
> >> 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
> >
> > Shall we fail instead of warning?
> >
>
> I am ok with either warning or error. The only thing with error is that
> it will cause the eBPF program to fail compilation even if its not using
> the helper with missing/misformatted doc, which i thought might be a bit
> extreme as the eBPF program will work if it doesnt use it.
> If error is recommended approach i can send v4 with #error replacing
> #warning.
>
Even the BPF program is not using the missing helper, it may still get wrong
helper ID. I think we should fail to compile in just cases.
[...]
> >> + 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)
> >
> > How about we only save nr_define_unique_helpers in self?
> >
>
> self.define_unique_helpers is used to give the first
> missing/misformatted helper to print in "#warning The description for %s
> is not present or formatted correctly" below.
I see. This is useful information, so let's keep it.
Thanks,
Song
