| Message ID | 20240824074033.2134514-1-lihongbo22@huawei.com |
|---|---|
| Headers | show |
| Series | Use max/min to simplify the code | expand |
On Sat, 24 Aug 2024 15:40:25 +0800 Hongbo Li wrote: > Many Coccinelle/coccicheck warning reported by minmax.cocci > in net module, such as: > WARNING opportunity for max() > WARNING opportunity for min() > > Let's use max/min to simplify the code and fix these warnings. > These patch have passed compilation test. This set does not build.
On 2024/8/27 5:44, Jakub Kicinski wrote: > On Sat, 24 Aug 2024 15:40:25 +0800 Hongbo Li wrote: >> Many Coccinelle/coccicheck warning reported by minmax.cocci >> in net module, such as: >> WARNING opportunity for max() >> WARNING opportunity for min() >> >> Let's use max/min to simplify the code and fix these warnings. >> These patch have passed compilation test. > > This set does not build. > Do you mean some patches will go to other branches (such as mac80211)? Thanks, Hongbo >
Hongbo Li <lihongbo22@huawei.com> writes: > On 2024/8/27 5:44, Jakub Kicinski wrote: >> On Sat, 24 Aug 2024 15:40:25 +0800 Hongbo Li wrote: >>> Many Coccinelle/coccicheck warning reported by minmax.cocci >>> in net module, such as: >>> WARNING opportunity for max() >>> WARNING opportunity for min() >>> >>> Let's use max/min to simplify the code and fix these warnings. >>> These patch have passed compilation test. >> This set does not build. >> > Do you mean some patches will go to other branches (such as mac80211)? Jakub means that your patchset had compilation errors, see the red on patchwork: https://patchwork.kernel.org/project/netdevbpf/list/?series=882901&state=*&order=date
On Tue, 27 Aug 2024 07:45:02 +0300 Kalle Valo wrote: > > Do you mean some patches will go to other branches (such as mac80211)? > > Jakub means that your patchset had compilation errors, see the red on > patchwork: > > https://patchwork.kernel.org/project/netdevbpf/list/?series=882901&state=*&order=date FWIW I prefer not to point noobs to the patchwork checks, lest they think it's a public CI and they can fling broken code at the list :( But yes, in case "code doesn't build" needs a further explanation: net/core/pktgen.c: In function ‘pktgen_finalize_skb’: ./../include/linux/compiler_types.h:510:45: error: call to ‘__compiletime_assert_928’ declared with attribute error: min(datalen/frags, ((1UL) << 12)) signedness error 510 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^ ./../include/linux/compiler_types.h:491:25: note: in definition of macro ‘__compiletime_assert’ 491 | prefix ## suffix(); \ | ^~~~~~ ./../include/linux/compiler_types.h:510:9: note: in expansion of macro ‘_compiletime_assert’ 510 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ ../include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’ 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) | ^~~~~~~~~~~~~~~~~~ ../include/linux/minmax.h:100:9: note: in expansion of macro ‘BUILD_BUG_ON_MSG’ 100 | BUILD_BUG_ON_MSG(!__types_ok(x,y,ux,uy), \ | ^~~~~~~~~~~~~~~~ ../include/linux/minmax.h:105:9: note: in expansion of macro ‘__careful_cmp_once’ 105 | __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_)) | ^~~~~~~~~~~~~~~~~~ ../include/linux/minmax.h:129:25: note: in expansion of macro ‘__careful_cmp’ 129 | #define min(x, y) __careful_cmp(min, x, y) | ^~~~~~~~~~~~~ ../net/core/pktgen.c:2796:28: note: in expansion of macro ‘min’ 2796 | frag_len = min(datalen/frags, PAGE_SIZE); | ^~~ make[5]: *** [../scripts/Makefile.build:244: net/core/pktgen.o] Error 1
Jakub Kicinski <kuba@kernel.org> writes: > On Tue, 27 Aug 2024 07:45:02 +0300 Kalle Valo wrote: >> > Do you mean some patches will go to other branches (such as mac80211)? >> >> Jakub means that your patchset had compilation errors, see the red on >> patchwork: >> >> https://patchwork.kernel.org/project/netdevbpf/list/?series=882901&state=*&order=date > > FWIW I prefer not to point noobs to the patchwork checks, lest they > think it's a public CI and they can fling broken code at the list :( Good point, that's definitely what we do not want. I'll keep this in mind.
From: Jakub Kicinski > Sent: 27 August 2024 15:04 > > On Tue, 27 Aug 2024 07:45:02 +0300 Kalle Valo wrote: > > > Do you mean some patches will go to other branches (such as mac80211)? > > > > Jakub means that your patchset had compilation errors, see the red on > > patchwork: > > > > https://patchwork.kernel.org/project/netdevbpf/list/?series=882901&state=*&order=date > > FWIW I prefer not to point noobs to the patchwork checks, lest they > think it's a public CI and they can fling broken code at the list :( > But yes, in case "code doesn't build" needs a further explanation: > > net/core/pktgen.c: In function ‘pktgen_finalize_skb’: > ./../include/linux/compiler_types.h:510:45: error: call to ‘__compiletime_assert_928’ declared with > attribute error: min(datalen/frags, ((1UL) << 12)) signedness error ... > ../net/core/pktgen.c:2796:28: note: in expansion of macro ‘min’ > 2796 | frag_len = min(datalen/frags, PAGE_SIZE); > | ^~~ I can't help feeling that a signed divide isn't intended here. Which rather implies that both datalen and frags are signed types. Whereas neither can be sensibly negative. Perhaps that is the real bug? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Many Coccinelle/coccicheck warning reported by minmax.cocci in net module, such as: WARNING opportunity for max() WARNING opportunity for min() Let's use max/min to simplify the code and fix these warnings. These patch have passed compilation test. Hongbo Li (8): net/mac80211: use max to simplify the code net/rds: Use max() to simplify the code net/ipv4: Use min() to simplify the code net/core: Use min()/max() to simplify the code net/dccp: Use min()/max() to simplify the code net/openvswitch: Use max() to simplify the code net/rxrpc: Use min() to simplify the code net/ceph: Use min() to simplify the code net/ceph/osd_client.c | 2 +- net/core/pktgen.c | 6 ++---- net/core/sock.c | 2 +- net/dccp/ackvec.c | 2 +- net/dccp/dccp.h | 2 +- net/ipv4/ip_sockglue.c | 2 +- net/mac80211/driver-ops.h | 2 +- net/mac80211/mlme.c | 2 +- net/mac80211/scan.c | 6 ++---- net/mac80211/tdls.c | 2 +- net/openvswitch/vport.c | 2 +- net/rds/info.c | 5 +---- net/rxrpc/input.c | 3 +-- 13 files changed, 15 insertions(+), 23 deletions(-)