| Message ID | 20210223124554.1375051-1-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f566aac4e05363addfa21477a6daa9d78ac9faaa |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: fix missing * in bpf.h | expand |
On Tue, 23 Feb 2021 20:45:54 +0800 Hangbin Liu <liuhangbin@gmail.com> wrote: > Commit 34b2021cc616 ("bpf: Add BPF-helper for MTU checking") lost a * > in bpf.h. This will make bpf_helpers_doc.py stop building > bpf_helper_defs.h immediately after bpf_check_mtu, which will affect > future add functions. > > Fixes: 34b2021cc616 ("bpf: Add BPF-helper for MTU checking") > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > include/uapi/linux/bpf.h | 2 +- > tools/include/uapi/linux/bpf.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) Thanks for fixing that! Acked-by: Jesper Dangaard Brouer <brouer@redhat.com> I though I had already fix that, but I must have missed or reintroduced this, when I rolling back broken ideas in V13. I usually run this command to check the man-page (before submitting): ./scripts/bpf_helpers_doc.py | rst2man | man -l - > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index 4c24daa43bac..46248f8e024b 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -3850,7 +3850,7 @@ union bpf_attr { > * > * long bpf_check_mtu(void *ctx, u32 ifindex, u32 *mtu_len, s32 len_diff, u64 flags) > * Description > - > + * > * Check ctx packet size against exceeding MTU of net device (based > * on *ifindex*). This helper will likely be used in combination > * with helpers that adjust/change the packet size. > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > index 4c24daa43bac..46248f8e024b 100644 > --- a/tools/include/uapi/linux/bpf.h > +++ b/tools/include/uapi/linux/bpf.h > @@ -3850,7 +3850,7 @@ union bpf_attr { > * > * long bpf_check_mtu(void *ctx, u32 ifindex, u32 *mtu_len, s32 len_diff, u64 flags) > * Description > - > + * > * Check ctx packet size against exceeding MTU of net device (based > * on *ifindex*). This helper will likely be used in combination > * with helpers that adjust/change the packet size.
On 2/23/21 3:43 PM, Jesper Dangaard Brouer wrote: > On Tue, 23 Feb 2021 20:45:54 +0800 > Hangbin Liu <liuhangbin@gmail.com> wrote: > >> Commit 34b2021cc616 ("bpf: Add BPF-helper for MTU checking") lost a * >> in bpf.h. This will make bpf_helpers_doc.py stop building >> bpf_helper_defs.h immediately after bpf_check_mtu, which will affect >> future add functions. >> >> Fixes: 34b2021cc616 ("bpf: Add BPF-helper for MTU checking") >> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> >> --- >> include/uapi/linux/bpf.h | 2 +- >> tools/include/uapi/linux/bpf.h | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) > > Thanks for fixing that! > > Acked-by: Jesper Dangaard Brouer <brouer@redhat.com> Thanks guys, applied! > I though I had already fix that, but I must have missed or reintroduced > this, when I rolling back broken ideas in V13. > > I usually run this command to check the man-page (before submitting): > > ./scripts/bpf_helpers_doc.py | rst2man | man -l - [+ Andrii] maybe this could be included to run as part of CI to catch such things in advance? >> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h >> index 4c24daa43bac..46248f8e024b 100644 >> --- a/include/uapi/linux/bpf.h >> +++ b/include/uapi/linux/bpf.h >> @@ -3850,7 +3850,7 @@ union bpf_attr { >> * >> * long bpf_check_mtu(void *ctx, u32 ifindex, u32 *mtu_len, s32 len_diff, u64 flags) >> * Description >> - >> + * >> * Check ctx packet size against exceeding MTU of net device (based >> * on *ifindex*). This helper will likely be used in combination >> * with helpers that adjust/change the packet size. >> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h >> index 4c24daa43bac..46248f8e024b 100644 >> --- a/tools/include/uapi/linux/bpf.h >> +++ b/tools/include/uapi/linux/bpf.h >> @@ -3850,7 +3850,7 @@ union bpf_attr { >> * >> * long bpf_check_mtu(void *ctx, u32 ifindex, u32 *mtu_len, s32 len_diff, u64 flags) >> * Description >> - >> + * >> * Check ctx packet size against exceeding MTU of net device (based >> * on *ifindex*). This helper will likely be used in combination >> * with helpers that adjust/change the packet size. > > >
Hello: This patch was applied to bpf/bpf.git (refs/heads/master): On Tue, 23 Feb 2021 20:45:54 +0800 you wrote: > Commit 34b2021cc616 ("bpf: Add BPF-helper for MTU checking") lost a * > in bpf.h. This will make bpf_helpers_doc.py stop building > bpf_helper_defs.h immediately after bpf_check_mtu, which will affect > future add functions. > > Fixes: 34b2021cc616 ("bpf: Add BPF-helper for MTU checking") > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > > [...] Here is the summary with links: - [bpf-next] bpf: fix missing * in bpf.h https://git.kernel.org/bpf/bpf/c/f566aac4e053 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On Wed, Feb 24, 2021 at 7:55 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 2/23/21 3:43 PM, Jesper Dangaard Brouer wrote: > > On Tue, 23 Feb 2021 20:45:54 +0800 > > Hangbin Liu <liuhangbin@gmail.com> wrote: > > > >> Commit 34b2021cc616 ("bpf: Add BPF-helper for MTU checking") lost a * > >> in bpf.h. This will make bpf_helpers_doc.py stop building > >> bpf_helper_defs.h immediately after bpf_check_mtu, which will affect > >> future add functions. > >> > >> Fixes: 34b2021cc616 ("bpf: Add BPF-helper for MTU checking") > >> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > >> --- > >> include/uapi/linux/bpf.h | 2 +- > >> tools/include/uapi/linux/bpf.h | 2 +- > >> 2 files changed, 2 insertions(+), 2 deletions(-) > > > > Thanks for fixing that! > > > > Acked-by: Jesper Dangaard Brouer <brouer@redhat.com> > > Thanks guys, applied! > > > I though I had already fix that, but I must have missed or reintroduced > > this, when I rolling back broken ideas in V13. > > > > I usually run this command to check the man-page (before submitting): > > > > ./scripts/bpf_helpers_doc.py | rst2man | man -l - > > [+ Andrii] maybe this could be included to run as part of CI to catch such > things in advance? We do something like that as part of bpftool build, so there is no reason we can't add this to selftests/bpf/Makefile as well. > > >> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > >> index 4c24daa43bac..46248f8e024b 100644 > >> --- a/include/uapi/linux/bpf.h > >> +++ b/include/uapi/linux/bpf.h > >> @@ -3850,7 +3850,7 @@ union bpf_attr { > >> * > >> * long bpf_check_mtu(void *ctx, u32 ifindex, u32 *mtu_len, s32 len_diff, u64 flags) > >> * Description > >> - > >> + * > >> * Check ctx packet size against exceeding MTU of net device (based > >> * on *ifindex*). This helper will likely be used in combination > >> * with helpers that adjust/change the packet size. > >> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > >> index 4c24daa43bac..46248f8e024b 100644 > >> --- a/tools/include/uapi/linux/bpf.h > >> +++ b/tools/include/uapi/linux/bpf.h > >> @@ -3850,7 +3850,7 @@ union bpf_attr { > >> * > >> * long bpf_check_mtu(void *ctx, u32 ifindex, u32 *mtu_len, s32 len_diff, u64 flags) > >> * Description > >> - > >> + * > >> * Check ctx packet size against exceeding MTU of net device (based > >> * on *ifindex*). This helper will likely be used in combination > >> * with helpers that adjust/change the packet size. > > > > > > >
2021-02-24 10:59 UTC-0800 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > On Wed, Feb 24, 2021 at 7:55 AM Daniel Borkmann <daniel@iogearbox.net> wrote: >> >> On 2/23/21 3:43 PM, Jesper Dangaard Brouer wrote: >>> On Tue, 23 Feb 2021 20:45:54 +0800 >>> Hangbin Liu <liuhangbin@gmail.com> wrote: >>> >>>> Commit 34b2021cc616 ("bpf: Add BPF-helper for MTU checking") lost a * >>>> in bpf.h. This will make bpf_helpers_doc.py stop building >>>> bpf_helper_defs.h immediately after bpf_check_mtu, which will affect >>>> future add functions. >>>> >>>> Fixes: 34b2021cc616 ("bpf: Add BPF-helper for MTU checking") >>>> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> >>>> --- >>>> include/uapi/linux/bpf.h | 2 +- >>>> tools/include/uapi/linux/bpf.h | 2 +- >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> Thanks for fixing that! >>> >>> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com> >> >> Thanks guys, applied! >> >>> I though I had already fix that, but I must have missed or reintroduced >>> this, when I rolling back broken ideas in V13. >>> >>> I usually run this command to check the man-page (before submitting): >>> >>> ./scripts/bpf_helpers_doc.py | rst2man | man -l - >> >> [+ Andrii] maybe this could be included to run as part of CI to catch such >> things in advance? > > We do something like that as part of bpftool build, so there is no > reason we can't add this to selftests/bpf/Makefile as well. Hi, pretty sure this is the case already? [0] This helps catching RST formatting issues, for example if a description is using invalid markup, and reported by rst2man. My understanding is that in the current case, the missing star simply ends the block for the helpers documentation from the parser point of view, it's not considered an error. I see two possible workarounds: 1) Check that the number of helpers found ("len(self.helpers)") is equal to the number of helpers in the file, but that requires knowing how many helpers we have in the first place (e.g. parsing "__BPF_FUNC_MAPPER(FN)"). 2) Add some ending tag to the documentation block, and make sure we eventually reach it. This is probably a much simpler solution. I could work on this (or sync with Joe (+Cc) who is also working on these bits for documenting the bpf() syscall). [0] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/tools/testing/selftests/bpf/Makefile?h=v5.11#n189 Quentin
On Fri, Feb 26, 2021 at 8:50 AM Quentin Monnet <quentin@isovalent.com> wrote: > > 2021-02-24 10:59 UTC-0800 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > > On Wed, Feb 24, 2021 at 7:55 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > >> > >> On 2/23/21 3:43 PM, Jesper Dangaard Brouer wrote: > >>> On Tue, 23 Feb 2021 20:45:54 +0800 > >>> Hangbin Liu <liuhangbin@gmail.com> wrote: > >>> > >>>> Commit 34b2021cc616 ("bpf: Add BPF-helper for MTU checking") lost a * > >>>> in bpf.h. This will make bpf_helpers_doc.py stop building > >>>> bpf_helper_defs.h immediately after bpf_check_mtu, which will affect > >>>> future add functions. > >>>> > >>>> Fixes: 34b2021cc616 ("bpf: Add BPF-helper for MTU checking") > >>>> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > >>>> --- > >>>> include/uapi/linux/bpf.h | 2 +- > >>>> tools/include/uapi/linux/bpf.h | 2 +- > >>>> 2 files changed, 2 insertions(+), 2 deletions(-) > >>> > >>> Thanks for fixing that! > >>> > >>> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com> > >> > >> Thanks guys, applied! > >> > >>> I though I had already fix that, but I must have missed or reintroduced > >>> this, when I rolling back broken ideas in V13. > >>> > >>> I usually run this command to check the man-page (before submitting): > >>> > >>> ./scripts/bpf_helpers_doc.py | rst2man | man -l - > >> > >> [+ Andrii] maybe this could be included to run as part of CI to catch such > >> things in advance? > > > > We do something like that as part of bpftool build, so there is no > > reason we can't add this to selftests/bpf/Makefile as well. > > Hi, pretty sure this is the case already? [0] > > This helps catching RST formatting issues, for example if a description > is using invalid markup, and reported by rst2man. My understanding is > that in the current case, the missing star simply ends the block for the > helpers documentation from the parser point of view, it's not considered > an error. > > I see two possible workarounds: > > 1) Check that the number of helpers found ("len(self.helpers)") is equal > to the number of helpers in the file, but that requires knowing how many > helpers we have in the first place (e.g. parsing "__BPF_FUNC_MAPPER(FN)"). It's a bit hacky, but you could also just count a number of '* \tDescription' lines. > > 2) Add some ending tag to the documentation block, and make sure we > eventually reach it. This is probably a much simpler solution. I could > work on this (or sync with Joe (+Cc) who is also working on these bits > for documenting the bpf() syscall). Fine by me as well. > > [0] > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/tools/testing/selftests/bpf/Makefile?h=v5.11#n189 > > Quentin
On Fri, Feb 26, 2021 at 8:51 AM Quentin Monnet <quentin@isovalent.com> wrote: > > 2021-02-24 10:59 UTC-0800 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > > On Wed, Feb 24, 2021 at 7:55 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > >> > >> On 2/23/21 3:43 PM, Jesper Dangaard Brouer wrote: > >>> On Tue, 23 Feb 2021 20:45:54 +0800 > >>> Hangbin Liu <liuhangbin@gmail.com> wrote: > >>> > >>>> Commit 34b2021cc616 ("bpf: Add BPF-helper for MTU checking") lost a * > >>>> in bpf.h. This will make bpf_helpers_doc.py stop building > >>>> bpf_helper_defs.h immediately after bpf_check_mtu, which will affect > >>>> future add functions. > >>>> > >>>> Fixes: 34b2021cc616 ("bpf: Add BPF-helper for MTU checking") > >>>> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > >>>> --- > >>>> include/uapi/linux/bpf.h | 2 +- > >>>> tools/include/uapi/linux/bpf.h | 2 +- > >>>> 2 files changed, 2 insertions(+), 2 deletions(-) > >>> > >>> Thanks for fixing that! > >>> > >>> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com> > >> > >> Thanks guys, applied! > >> > >>> I though I had already fix that, but I must have missed or reintroduced > >>> this, when I rolling back broken ideas in V13. > >>> > >>> I usually run this command to check the man-page (before submitting): > >>> > >>> ./scripts/bpf_helpers_doc.py | rst2man | man -l - > >> > >> [+ Andrii] maybe this could be included to run as part of CI to catch such > >> things in advance? > > > > We do something like that as part of bpftool build, so there is no > > reason we can't add this to selftests/bpf/Makefile as well. > > Hi, pretty sure this is the case already? [0] > > This helps catching RST formatting issues, for example if a description > is using invalid markup, and reported by rst2man. My understanding is > that in the current case, the missing star simply ends the block for the > helpers documentation from the parser point of view, it's not considered > an error. > > I see two possible workarounds: > > 1) Check that the number of helpers found ("len(self.helpers)") is equal > to the number of helpers in the file, but that requires knowing how many > helpers we have in the first place (e.g. parsing "__BPF_FUNC_MAPPER(FN)"). This is not so difficult as long as we stick to one symbol per line: diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py index e2ffac2b7695..74cdcc2bbf18 100755 --- a/scripts/bpf_doc.py +++ b/scripts/bpf_doc.py @@ -183,25 +183,51 @@ class HeaderParser(object): self.reader.readline() self.line = self.reader.readline() + def get_elem_count(self, target): + self.seek_to(target, 'Could not find symbol "%s"' % target) + end_re = re.compile('^$') + count = 0 + while True: + capture = end_re.match(self.line) + if capture: + break + self.line = self.reader.readline() + count += 1 + + # The last line (either '};' or '/* */' doesn't count. + return count + I can either roll this into my docs update v2, or hold onto it for another dedicated patch fixup. Either way I'm trialing this out locally to regression-test my own docs update PR and make sure I'm not breaking one of the various output formats.
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 4c24daa43bac..46248f8e024b 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -3850,7 +3850,7 @@ union bpf_attr { * * long bpf_check_mtu(void *ctx, u32 ifindex, u32 *mtu_len, s32 len_diff, u64 flags) * Description - + * * Check ctx packet size against exceeding MTU of net device (based * on *ifindex*). This helper will likely be used in combination * with helpers that adjust/change the packet size. diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 4c24daa43bac..46248f8e024b 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -3850,7 +3850,7 @@ union bpf_attr { * * long bpf_check_mtu(void *ctx, u32 ifindex, u32 *mtu_len, s32 len_diff, u64 flags) * Description - + * * Check ctx packet size against exceeding MTU of net device (based * on *ifindex*). This helper will likely be used in combination * with helpers that adjust/change the packet size.
Commit 34b2021cc616 ("bpf: Add BPF-helper for MTU checking") lost a * in bpf.h. This will make bpf_helpers_doc.py stop building bpf_helper_defs.h immediately after bpf_check_mtu, which will affect future add functions. Fixes: 34b2021cc616 ("bpf: Add BPF-helper for MTU checking") Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- include/uapi/linux/bpf.h | 2 +- tools/include/uapi/linux/bpf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)