Message-ID: <CABPSWR7_w3mxr74wCDEF=MYYuG2F_vMJeD-dqotc8MDmaS_FpQ@mail.gmail.com>
Date: Thu, 25 Sep 2025 21:48:22 +0530
From: vivek yadav <vivekyadav1207731111@...il.com>
To: Mehdi Ben Hadj Khelifa <mehdi.benhadjkhelifa@...il.com>
Cc: Daniel Borkmann <daniel@...earbox.net>, andrii@...nel.org, eddyz87@...il.com, 
	ast@...nel.org, martin.lau@...ux.dev, song@...nel.org, 
	yonghong.song@...ux.dev, john.fastabend@...il.com, kpsingh@...nel.org, 
	sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org, shuah@...nel.org, 
	matttbe@...nel.org, martineau@...nel.org, geliang@...nel.org, 
	davem@...emloft.net, kuba@...nel.org, hawk@...nel.org, linux@...danrome.com, 
	ameryhung@...il.com, toke@...hat.com, houtao1@...wei.com, 
	emil@...alapatis.com, yatsenko@...a.com, isolodrai@...a.com, 
	a.s.protopopov@...il.com, dxu@...uu.xyz, memxor@...il.com, vmalik@...hat.com, 
	bigeasy@...utronix.de, tj@...nel.org, gregkh@...uxfoundation.org, 
	paul@...l-moore.com, bboscaccy@...ux.microsoft.com, 
	James.Bottomley@...senpartnership.com, mrpre@....com, jakub@...udflare.com, 
	bpf@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, netdev@...r.kernel.org, 
	mptcp@...ts.linux.dev, linux-kernel-mentees@...ts.linuxfoundation.org, 
	skhan@...uxfoundation.org, david.hunter.linux@...il.com
Subject: Re: [PATCH v3 3/3] selftests/bpf: Prepare to add -Wsign-compare for
 bpf tests

Hi Mehdi,
You are trying to do much with the patch series. I don't think it will
help much as reviewers will give comments and you will revise the
patches. This loop will continue forever.

I totally agree with Daniel. Please write a proper commit message.

While writing a commit message or creating a patch.Please try to give
the answers of the following questions.
1) What is the issue which your patch resolves?
2) Are you trying to do more than one thing at a time? Please don't.
3) if a patch modifies more than one file then either these changes
inter link with each other or they have similar kind of work.

~~Vivek

On Thu, Sep 25, 2025 at 9:04 PM Mehdi Ben Hadj Khelifa
<mehdi.benhadjkhelifa@...il.com> wrote:
>
> On 9/25/25 4:04 PM, Daniel Borkmann wrote:
> > On 9/25/25 12:35 PM, Mehdi Ben Hadj Khelifa wrote:
> >> -Change only variable types for correct sign comparisons
> >>
> >> Signed-off-by: Mehdi Ben Hadj Khelifa <mehdi.benhadjkhelifa@...il.com>
> >
> > Pls write some better commit messages and not just copy/paste the same
> > $subj/
> > message every time; proper sentences w/o the weird " -" indent.
>
> Understood, though the changes are very similar / are the same with the
> same goal that's why it made sense to me to do that and I will remove
> the - in future commits.> Also say
> > why
> > this is needed in the commit message, and add a reference to the commit
> > which
> > initially added this as a TODO, i.e. 495d2d8133fd ("selftests/bpf:
> > Attempt to
> > build BPF programs with -Wsign-compare").
> I will do that in the upcoming version.
>
> > If you group these, then maybe
> > also
> > include the parts of the compiler-emitted warnings during build which are
> > relevant to the code changes you do here.
>
> Okay. I will do that. Should i send a v4 with the recommended changes
> but not including the rest of the files meaning the ones that I haven't
> uploaded in this patch series which contain type casting or should i
> just make changes for these files in this series?
> Also will it be better if dropped these versions and made a new patch
> with v1?
>
> Thank you for your review and time Daniel.
> Regards,
> Mehdi
> >> ---
> >>   tools/testing/selftests/bpf/progs/test_xdp_dynptr.c          | 2 +-
> >>   tools/testing/selftests/bpf/progs/test_xdp_loop.c            | 2 +-
> >>   tools/testing/selftests/bpf/progs/test_xdp_noinline.c        | 4 ++--
> >>   tools/testing/selftests/bpf/progs/uprobe_multi.c             | 4 ++--
> >>   .../selftests/bpf/progs/uprobe_multi_session_recursive.c     | 5 +++--
> >>   .../selftests/bpf/progs/verifier_iterating_callbacks.c       | 2 +-
> >>   6 files changed, 10 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_dynptr.c b/
> >> tools/testing/selftests/bpf/progs/test_xdp_dynptr.c
> >> index 67a77944ef29..12ad0ec91021 100644
> >> --- a/tools/testing/selftests/bpf/progs/test_xdp_dynptr.c
> >> +++ b/tools/testing/selftests/bpf/progs/test_xdp_dynptr.c
> >> @@ -89,7 +89,7 @@ static __always_inline int handle_ipv4(struct xdp_md
> >> *xdp, struct bpf_dynptr *xd
> >>       struct vip vip = {};
> >>       int dport;
> >>       __u32 csum = 0;
> >> -    int i;
> >> +    size_t i;
> >>       __builtin_memset(eth_buffer, 0, sizeof(eth_buffer));
> >>       __builtin_memset(iph_buffer_tcp, 0, sizeof(iph_buffer_tcp));
> >> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_loop.c b/
> >> tools/testing/selftests/bpf/progs/test_xdp_loop.c
> >> index 93267a68825b..e9b7bbff5c23 100644
> >> --- a/tools/testing/selftests/bpf/progs/test_xdp_loop.c
> >> +++ b/tools/testing/selftests/bpf/progs/test_xdp_loop.c
> >> @@ -85,7 +85,7 @@ static __always_inline int handle_ipv4(struct xdp_md
> >> *xdp)
> >>       struct vip vip = {};
> >>       int dport;
> >>       __u32 csum = 0;
> >> -    int i;
> >> +    size_t i;
> >>       if (iph + 1 > data_end)
> >>           return XDP_DROP;
> >> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_noinline.c b/
> >> tools/testing/selftests/bpf/progs/test_xdp_noinline.c
> >> index fad94e41cef9..85ef3c0a3e20 100644
> >> --- a/tools/testing/selftests/bpf/progs/test_xdp_noinline.c
> >> +++ b/tools/testing/selftests/bpf/progs/test_xdp_noinline.c
> >> @@ -372,7 +372,7 @@ bool encap_v4(struct xdp_md *xdp, struct ctl_value
> >> *cval,
> >>       next_iph_u16 = (__u16 *) iph;
> >>       __pragma_loop_unroll_full
> >> -    for (int i = 0; i < sizeof(struct iphdr) >> 1; i++)
> >> +    for (size_t i = 0; i < sizeof(struct iphdr) >> 1; i++)
> >>           csum += *next_iph_u16++;
> >>       iph->check = ~((csum & 0xffff) + (csum >> 16));
> >>       if (bpf_xdp_adjust_head(xdp, (int)sizeof(struct iphdr)))
> >> @@ -423,7 +423,7 @@ int send_icmp_reply(void *data, void *data_end)
> >>       iph->check = 0;
> >>       next_iph_u16 = (__u16 *) iph;
> >>       __pragma_loop_unroll_full
> >> -    for (int i = 0; i < sizeof(struct iphdr) >> 1; i++)
> >> +    for (size_t i = 0; i < sizeof(struct iphdr) >> 1; i++)
> >>           csum += *next_iph_u16++;
> >>       iph->check = ~((csum & 0xffff) + (csum >> 16));
> >>       return swap_mac_and_send(data, data_end);
> >> diff --git a/tools/testing/selftests/bpf/progs/uprobe_multi.c b/tools/
> >> testing/selftests/bpf/progs/uprobe_multi.c
> >> index 44190efcdba2..f99957773c3a 100644
> >> --- a/tools/testing/selftests/bpf/progs/uprobe_multi.c
> >> +++ b/tools/testing/selftests/bpf/progs/uprobe_multi.c
> >> @@ -20,13 +20,13 @@ __u64 uretprobe_multi_func_3_result = 0;
> >>   __u64 uprobe_multi_sleep_result = 0;
> >> -int pid = 0;
> >> +__u32 pid = 0;
> >>   int child_pid = 0;
> >>   int child_tid = 0;
> >>   int child_pid_usdt = 0;
> >>   int child_tid_usdt = 0;
> >> -int expect_pid = 0;
> >> +__u32 expect_pid = 0;
> >>   bool bad_pid_seen = false;
> >>   bool bad_pid_seen_usdt = false;
> >> diff --git a/tools/testing/selftests/bpf/progs/
> >> uprobe_multi_session_recursive.c b/tools/testing/selftests/bpf/progs/
> >> uprobe_multi_session_recursive.c
> >> index 8fbcd69fae22..017f1859ebe8 100644
> >> --- a/tools/testing/selftests/bpf/progs/uprobe_multi_session_recursive.c
> >> +++ b/tools/testing/selftests/bpf/progs/uprobe_multi_session_recursive.c
> >> @@ -3,6 +3,7 @@
> >>   #include <bpf/bpf_helpers.h>
> >>   #include <bpf/bpf_tracing.h>
> >>   #include <stdbool.h>
> >> +#include <stddef.h>
> >>   #include "bpf_kfuncs.h"
> >>   #include "bpf_misc.h"
> >> @@ -10,8 +11,8 @@ char _license[] SEC("license") = "GPL";
> >>   int pid = 0;
> >> -int idx_entry = 0;
> >> -int idx_return = 0;
> >> +size_t idx_entry = 0;
> >> +size_t idx_return = 0;
> >>   __u64 test_uprobe_cookie_entry[6];
> >>   __u64 test_uprobe_cookie_return[3];
> >> diff --git a/tools/testing/selftests/bpf/progs/
> >> verifier_iterating_callbacks.c b/tools/testing/selftests/bpf/progs/
> >> verifier_iterating_callbacks.c
> >> index 75dd922e4e9f..72f9f8c23c93 100644
> >> --- a/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c
> >> +++ b/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c
> >> @@ -593,7 +593,7 @@ int loop_inside_iter_volatile_limit(const void *ctx)
> >>   {
> >>       struct bpf_iter_num it;
> >>       int *v, sum = 0;
> >> -    __u64 i = 0;
> >> +    __s32 i = 0;
> >>       bpf_iter_num_new(&it, 0, ARR2_SZ);
> >>       while ((v = bpf_iter_num_next(&it))) {
> >
>
>

Powered by blists - more mailing lists