Re: [PATCH 1/1] bpf: introduce new helper udp_flow_src_port

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Mon, Aug 5, 2019 at 5:11 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Sat, 3 Aug 2019 23:52:16 -0700, Y Song wrote:
> > >  include/uapi/linux/bpf.h                      | 21 +++++++--
> > >  net/core/filter.c                             | 20 ++++++++
> > >  tools/include/uapi/linux/bpf.h                | 21 +++++++--
> > >  tools/testing/selftests/bpf/bpf_helpers.h     |  2 +
> > >  .../bpf/prog_tests/udp_flow_src_port.c        | 28 +++++++++++
> > >  .../bpf/progs/test_udp_flow_src_port_kern.c   | 47 +++++++++++++++++++
> > >  6 files changed, 131 insertions(+), 8 deletions(-)
> > >  create mode 100644 tools/testing/selftests/bpf/prog_tests/udp_flow_src_port.c
> > >  create mode 100644 tools/testing/selftests/bpf/progs/test_udp_flow_src_port_kern.c
> >
> > First, for each review, backport and sync with libbpf repo, in the future,
> > could you break the patch to two patches?
> >    1. kernel changes (net/core/filter.c, include/uapi/linux/bpf.h)
> >    2. tools/include/uapi/linux/bpf.h
> >    3. tools/testing/ changes
>
> A lot of people get caught off by this, could explain why this is
> necessary?

We are using script [0] to sync libbpf sources from linux repo to
Github. It does a lot of things to make this happen, given that Github
structure is not a simple copy/move into subdirectory. Instead it does
a bunch of cherry-picking and tree rewrites, so when there are patches
that touched both libbpf sources (including those tools/include/...
files) and some sources that we don't sync (e.g., just include/...),
then script/git gets confused which breaks the flow and requires more
manual work. Which is why we are asking to split those changes. Hope
this helps to clarify.

  [0] https://github.com/libbpf/libbpf/blob/master/scripts/sync-kernel.sh

>
> git can deal with this scenario without missing a step, format-patch
> takes paths:
>
> $ git show --oneline -s
> 1002f3e955d7 (HEAD) bpf: introduce new helper udp_flow_src_port
>
> $ git format-patch HEAD~ -- tools/include/uapi/linux/bpf.h
> 0001-bpf-introduce-new-helper-udp_flow_src_port.patch
>
> $ grep -B1 changed 0001-bpf-introduce-new-helper-udp_flow_src_port.patch
>  tools/include/uapi/linux/bpf.h | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> $ cd ../libbpf
> $ git am -p2 ../linux/0001-bpf-introduce-new-helper-udp_flow_src_port.patch
> Applying: bpf: introduce new helper udp_flow_src_port
> error: patch failed: include/uapi/linux/bpf.h:2853
> error: include/uapi/linux/bpf.h: patch does not apply
> ...
>
> Well, the patch doesn't apply to libbpf right now, but git finds the
> right paths and all that.
>
> IMO it'd be good to not have this artificial process obstacle and all
> the "sync headers" commits in the tree.

It might be the case that script can be written in some different way
to bypass this limitation, but someone has to dedicate time to write
it and test it. Feel free to contribute.