| Message ID | 20200302145348.559177-1-toke@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] selftests/bpf: Declare bpf_log_buf variables as static | expand |
Toke Høiland-Jørgensen <toke@redhat.com> [Mon, 2020-03-02 06:54 -0800]: > The cgroup selftests did not declare the bpf_log_buf variable as static, leading > to a linker error with GCC 10 (which defaults to -fno-common). Fix this by > adding the missing static declarations. > > Fixes: 257c88559f36 ("selftests/bpf: Convert test_cgroup_attach to prog_tests") Hi Toke, Thanks for the fix. My 257c88559f36 commit was just a split that simply moved this bpf_log_buf from tools/testing/selftests/bpf/test_cgroup_attach.c to all new three files as is among many other things. Before that it was moved as is from samples/ in ba0c0cc05dda ("selftests/bpf: convert test_cgrp2_attach2 example into kselftest") and before that it was introduced in d40fc181ebec ("samples/bpf: Make samples more libbpf-centric") Though since these are new files I guess having just 257c88559f36 in the tag should be fine(?) so: Acked-by: Andrey Ignatov <rdna@fb.com> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- > .../testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c | 2 +- > tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c | 2 +- > tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c > index 5b13f2c6c402..70e94e783070 100644 > --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c > @@ -6,7 +6,7 @@ > > #define PING_CMD "ping -q -c1 -w1 127.0.0.1 > /dev/null" > > -char bpf_log_buf[BPF_LOG_BUF_SIZE]; > +static char bpf_log_buf[BPF_LOG_BUF_SIZE]; > > static int prog_load(void) > { > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > index 2ff21dbce179..139f8e82c7c6 100644 > --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > @@ -6,7 +6,7 @@ > > #define PING_CMD "ping -q -c1 -w1 127.0.0.1 > /dev/null" > > -char bpf_log_buf[BPF_LOG_BUF_SIZE]; > +static char bpf_log_buf[BPF_LOG_BUF_SIZE]; > > static int map_fd = -1; > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c > index 9d8cb48b99de..9e96f8d87fea 100644 > --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c > @@ -8,7 +8,7 @@ > #define BAR "/foo/bar/" > #define PING_CMD "ping -q -c1 -w1 127.0.0.1 > /dev/null" > > -char bpf_log_buf[BPF_LOG_BUF_SIZE]; > +static char bpf_log_buf[BPF_LOG_BUF_SIZE]; > > static int prog_load(int verdict) > { > -- > 2.25.1 >
Andrey Ignatov <rdna@fb.com> writes: > Toke Høiland-Jørgensen <toke@redhat.com> [Mon, 2020-03-02 06:54 -0800]: >> The cgroup selftests did not declare the bpf_log_buf variable as static, leading >> to a linker error with GCC 10 (which defaults to -fno-common). Fix this by >> adding the missing static declarations. >> >> Fixes: 257c88559f36 ("selftests/bpf: Convert test_cgroup_attach to prog_tests") > > Hi Toke, > > Thanks for the fix. > > My 257c88559f36 commit was just a split that simply moved this > bpf_log_buf from tools/testing/selftests/bpf/test_cgroup_attach.c to all > new three files as is among many other things. Before that it was moved > as is from samples/ in > ba0c0cc05dda ("selftests/bpf: convert test_cgrp2_attach2 example into kselftest") > and before that it was introduced in > d40fc181ebec ("samples/bpf: Make samples more libbpf-centric") > > Though since these are new files I guess having just 257c88559f36 in the > tag should be fine(?) so: Yeah, I did realise you didn't write the original code, but this Fixes tag should at least make the patch be picked up by any stable trees after you moved things around, so I guess that's good enough :) -Toke
On Mon, Mar 02, 2020 at 03:53:48PM +0100, Toke Høiland-Jørgensen wrote: > The cgroup selftests did not declare the bpf_log_buf variable as static, leading > to a linker error with GCC 10 (which defaults to -fno-common). Fix this by > adding the missing static declarations. > > Fixes: 257c88559f36 ("selftests/bpf: Convert test_cgroup_attach to prog_tests") > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> Applied to bpf-next. It's hardly a fix. Fixes tag doesn't make it a fix in my mind. I really see no point rushing it into bpf->net->Linus's tree at this point.
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: > On Mon, Mar 02, 2020 at 03:53:48PM +0100, Toke Høiland-Jørgensen wrote: >> The cgroup selftests did not declare the bpf_log_buf variable as static, leading >> to a linker error with GCC 10 (which defaults to -fno-common). Fix this by >> adding the missing static declarations. >> >> Fixes: 257c88559f36 ("selftests/bpf: Convert test_cgroup_attach to prog_tests") >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > > Applied to bpf-next. > It's hardly a fix. Fixes tag doesn't make it a fix in my mind. It fixes a compile error of selftests with GCC 10; how is that not a fix? We found it while setting up a CI test compiling Linus' tree on Fedora rawhide, so it does happen in the wild. > I really see no point rushing it into bpf->net->Linus's tree at this point. Well if you're not pushing any other fixes then OK, sure, no reason to go through the whole process just for this. But if you end up pushing another round of fixes anyway, please include this as well. If not, I guess we can wait :) -Toke
On Tue, Mar 3, 2020 at 12:10 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: > > > On Mon, Mar 02, 2020 at 03:53:48PM +0100, Toke Høiland-Jørgensen wrote: > >> The cgroup selftests did not declare the bpf_log_buf variable as static, leading > >> to a linker error with GCC 10 (which defaults to -fno-common). Fix this by > >> adding the missing static declarations. > >> > >> Fixes: 257c88559f36 ("selftests/bpf: Convert test_cgroup_attach to prog_tests") > >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > > > > Applied to bpf-next. > > It's hardly a fix. Fixes tag doesn't make it a fix in my mind. > > It fixes a compile error of selftests with GCC 10; how is that not a > fix? We found it while setting up a CI test compiling Linus' tree on > Fedora rawhide, so it does happen in the wild. > > > I really see no point rushing it into bpf->net->Linus's tree at this point. > > Well if you're not pushing any other fixes then OK, sure, no reason to > go through the whole process just for this. But if you end up pushing > another round of fixes anyway, please include this as well. If not, I > guess we can wait :) CI stands for Continuous Integration == development. stable tree is not for development. If you want to develop anything or accommodate the tree for external development you need to use development tree. Which is bpf-next.
diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c index 5b13f2c6c402..70e94e783070 100644 --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c @@ -6,7 +6,7 @@ #define PING_CMD "ping -q -c1 -w1 127.0.0.1 > /dev/null" -char bpf_log_buf[BPF_LOG_BUF_SIZE]; +static char bpf_log_buf[BPF_LOG_BUF_SIZE]; static int prog_load(void) { diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c index 2ff21dbce179..139f8e82c7c6 100644 --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c @@ -6,7 +6,7 @@ #define PING_CMD "ping -q -c1 -w1 127.0.0.1 > /dev/null" -char bpf_log_buf[BPF_LOG_BUF_SIZE]; +static char bpf_log_buf[BPF_LOG_BUF_SIZE]; static int map_fd = -1; diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c index 9d8cb48b99de..9e96f8d87fea 100644 --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c @@ -8,7 +8,7 @@ #define BAR "/foo/bar/" #define PING_CMD "ping -q -c1 -w1 127.0.0.1 > /dev/null" -char bpf_log_buf[BPF_LOG_BUF_SIZE]; +static char bpf_log_buf[BPF_LOG_BUF_SIZE]; static int prog_load(int verdict) {
The cgroup selftests did not declare the bpf_log_buf variable as static, leading to a linker error with GCC 10 (which defaults to -fno-common). Fix this by adding the missing static declarations. Fixes: 257c88559f36 ("selftests/bpf: Convert test_cgroup_attach to prog_tests") Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- .../testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c | 2 +- tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c | 2 +- tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)