| Message ID | 4f4702e9f6115b7f84fea01b2326ca24c6df7ba8.1721713597.git.tony.ambardar@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 730561d3c08d4a327cceaabf11365958a1c00cec |
| Headers | show |
| Series | selftests/bpf: Improve libc portability / musl support (part 1) | expand |
On Mon, Jul 22, 2024 at 10:56 PM Tony Ambardar <tony.ambardar@gmail.com> wrote: > > Remove a redundant include of '<asm/types.h>', whose needed definitions are > already included (via '<linux/types.h>') in cg_storage_multi_egress_only.c, > cg_storage_multi_isolated.c, and cg_storage_multi_shared.c. This avoids > redefinition errors seen compiling for mips64el/musl-libc like: > > In file included from progs/cg_storage_multi_egress_only.c:13: > In file included from progs/cg_storage_multi.h:6: > In file included from /usr/mips64el-linux-gnuabi64/include/asm/types.h:23: > /usr/include/asm-generic/int-l64.h:29:25: error: typedef redefinition with different types ('long' vs 'long long') > 29 | typedef __signed__ long __s64; > | ^ > /usr/include/asm-generic/int-ll64.h:30:44: note: previous definition is here > 30 | __extension__ typedef __signed__ long long __s64; > | ^ > > Fixes: 9e5bd1f7633b ("selftests/bpf: Test CGROUP_STORAGE map can't be used by multiple progs") > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com> > --- > tools/testing/selftests/bpf/progs/cg_storage_multi.h | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/tools/testing/selftests/bpf/progs/cg_storage_multi.h b/tools/testing/selftests/bpf/progs/cg_storage_multi.h > index a0778fe7857a..41d59f0ee606 100644 > --- a/tools/testing/selftests/bpf/progs/cg_storage_multi.h > +++ b/tools/testing/selftests/bpf/progs/cg_storage_multi.h > @@ -3,8 +3,6 @@ > #ifndef __PROGS_CG_STORAGE_MULTI_H > #define __PROGS_CG_STORAGE_MULTI_H > > -#include <asm/types.h> > - > struct cgroup_value { > __u32 egress_pkts; > __u32 ingress_pkts; > -- > 2.34.1 > Hmm, some linter checks prefer headers themselves include everything they use. This header uses __u32 and after this patch it would include no headers. Would it be okay to include <linux/types.h> or we don't care? YiFei Zhu
On Tue, Jul 23, 2024 at 3:35 PM YiFei Zhu <zhuyifei@google.com> wrote: > > On Mon, Jul 22, 2024 at 10:56 PM Tony Ambardar <tony.ambardar@gmail.com> wrote: > > > > Remove a redundant include of '<asm/types.h>', whose needed definitions are > > already included (via '<linux/types.h>') in cg_storage_multi_egress_only.c, > > cg_storage_multi_isolated.c, and cg_storage_multi_shared.c. This avoids > > redefinition errors seen compiling for mips64el/musl-libc like: > > > > In file included from progs/cg_storage_multi_egress_only.c:13: > > In file included from progs/cg_storage_multi.h:6: > > In file included from /usr/mips64el-linux-gnuabi64/include/asm/types.h:23: > > /usr/include/asm-generic/int-l64.h:29:25: error: typedef redefinition with different types ('long' vs 'long long') > > 29 | typedef __signed__ long __s64; > > | ^ > > /usr/include/asm-generic/int-ll64.h:30:44: note: previous definition is here > > 30 | __extension__ typedef __signed__ long long __s64; > > | ^ > > > > Fixes: 9e5bd1f7633b ("selftests/bpf: Test CGROUP_STORAGE map can't be used by multiple progs") > > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com> > > --- > > tools/testing/selftests/bpf/progs/cg_storage_multi.h | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/progs/cg_storage_multi.h b/tools/testing/selftests/bpf/progs/cg_storage_multi.h > > index a0778fe7857a..41d59f0ee606 100644 > > --- a/tools/testing/selftests/bpf/progs/cg_storage_multi.h > > +++ b/tools/testing/selftests/bpf/progs/cg_storage_multi.h > > @@ -3,8 +3,6 @@ > > #ifndef __PROGS_CG_STORAGE_MULTI_H > > #define __PROGS_CG_STORAGE_MULTI_H > > > > -#include <asm/types.h> > > - > > struct cgroup_value { > > __u32 egress_pkts; > > __u32 ingress_pkts; > > -- > > 2.34.1 > > > > Hmm, some linter checks prefer headers themselves include everything > they use. This header uses __u32 and after this patch it would include > no headers. Would it be okay to include <linux/types.h> or we don't > care? > This header seems to be included both from user space code and BPF code, so it probably is for the better to not include specific headers and rely on respective code to provide "basic" type definitions like __u32. (For BPF side they might be coming from vmlinux.h, for example, which won't be compatible with anything included in user space code). > YiFei Zhu
On Tue, Jul 23, 2024 at 03:35:22PM -0700, YiFei Zhu wrote: > On Mon, Jul 22, 2024 at 10:56 PM Tony Ambardar <tony.ambardar@gmail.com> wrote: > > > > Remove a redundant include of '<asm/types.h>', whose needed definitions are > > already included (via '<linux/types.h>') in cg_storage_multi_egress_only.c, > > cg_storage_multi_isolated.c, and cg_storage_multi_shared.c. This avoids > > redefinition errors seen compiling for mips64el/musl-libc like: > > > > In file included from progs/cg_storage_multi_egress_only.c:13: > > In file included from progs/cg_storage_multi.h:6: > > In file included from /usr/mips64el-linux-gnuabi64/include/asm/types.h:23: > > /usr/include/asm-generic/int-l64.h:29:25: error: typedef redefinition with different types ('long' vs 'long long') > > 29 | typedef __signed__ long __s64; > > | ^ > > /usr/include/asm-generic/int-ll64.h:30:44: note: previous definition is here > > 30 | __extension__ typedef __signed__ long long __s64; > > | ^ > > > > Fixes: 9e5bd1f7633b ("selftests/bpf: Test CGROUP_STORAGE map can't be used by multiple progs") > > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com> > > --- > > tools/testing/selftests/bpf/progs/cg_storage_multi.h | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/progs/cg_storage_multi.h b/tools/testing/selftests/bpf/progs/cg_storage_multi.h > > index a0778fe7857a..41d59f0ee606 100644 > > --- a/tools/testing/selftests/bpf/progs/cg_storage_multi.h > > +++ b/tools/testing/selftests/bpf/progs/cg_storage_multi.h > > @@ -3,8 +3,6 @@ > > #ifndef __PROGS_CG_STORAGE_MULTI_H > > #define __PROGS_CG_STORAGE_MULTI_H > > > > -#include <asm/types.h> > > - > > struct cgroup_value { > > __u32 egress_pkts; > > __u32 ingress_pkts; > > -- > > 2.34.1 > > > > Hmm, some linter checks prefer headers themselves include everything > they use. This header uses __u32 and after this patch it would include > no headers. Would it be okay to include <linux/types.h> or we don't > care? > > YiFei Zhu Good point, I agree even the readability suffers without headers. Replacing <asm/types.h> with <linux/types.h> makes more sense, and guards in the latter avoid the conflicts noted above. I'll queue this change for a v2. Appreciate the feedback! Cheers, Tony Ambardar
diff --git a/tools/testing/selftests/bpf/progs/cg_storage_multi.h b/tools/testing/selftests/bpf/progs/cg_storage_multi.h index a0778fe7857a..41d59f0ee606 100644 --- a/tools/testing/selftests/bpf/progs/cg_storage_multi.h +++ b/tools/testing/selftests/bpf/progs/cg_storage_multi.h @@ -3,8 +3,6 @@ #ifndef __PROGS_CG_STORAGE_MULTI_H #define __PROGS_CG_STORAGE_MULTI_H -#include <asm/types.h> - struct cgroup_value { __u32 egress_pkts; __u32 ingress_pkts;
Remove a redundant include of '<asm/types.h>', whose needed definitions are already included (via '<linux/types.h>') in cg_storage_multi_egress_only.c, cg_storage_multi_isolated.c, and cg_storage_multi_shared.c. This avoids redefinition errors seen compiling for mips64el/musl-libc like: In file included from progs/cg_storage_multi_egress_only.c:13: In file included from progs/cg_storage_multi.h:6: In file included from /usr/mips64el-linux-gnuabi64/include/asm/types.h:23: /usr/include/asm-generic/int-l64.h:29:25: error: typedef redefinition with different types ('long' vs 'long long') 29 | typedef __signed__ long __s64; | ^ /usr/include/asm-generic/int-ll64.h:30:44: note: previous definition is here 30 | __extension__ typedef __signed__ long long __s64; | ^ Fixes: 9e5bd1f7633b ("selftests/bpf: Test CGROUP_STORAGE map can't be used by multiple progs") Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com> --- tools/testing/selftests/bpf/progs/cg_storage_multi.h | 2 -- 1 file changed, 2 deletions(-)