On Thu, Aug 24, 2023 at 5:13 PM Mahmoud Matook mahmoudmatook.mm@gmail.com wrote:
On 08/24, Willem de Bruijn wrote:
On Thu, Aug 24, 2023 at 4:25 PM Mahmoud Maatuq mahmoudmatook.mm@gmail.com wrote:
Fix the following coccicheck warning: tools/testing/selftests/net/udpgso_bench_tx.c:297:18-19: WARNING opportunity for min() tools/testing/selftests/net/udpgso_bench_tx.c:354:27-28: WARNING opportunity for min() tools/testing/selftests/net/so_txtime.c:129:24-26: WARNING opportunity for max() tools/testing/selftests/net/so_txtime.c:96:30-31: WARNING opportunity for max()
Signed-off-by: Mahmoud Maatuq mahmoudmatook.mm@gmail.com Suggested-by: Willem de Bruijn willemdebruijn.kernel@gmail.com
I did not suggest this change.
the suggestion i meant here is about the following part "Note that a more strict version that includes __typecheck would warn on the type difference between total_len and cfg_mss. Fine with changing the type of cfg_mss in the follow-on patch to address that." as mentioned here https://lore.kernel.org/all/CAF=yD-+6cWTiDgpsu=hUV+OvzDFRaT2ZUmtQo9qTrCB9i-+... I tried to change the type of cfg_mss but it creates a different warn at udpgso_bench_tx.c:354 between cfg_mss and left that's way i casted instead. apology if i misunderstood your point.
Thanks. It's fine to avoid changing the type or cast if it does not set of any alarms.
I think Suggested-by is for when the main idea of the patch is someone's suggestion. Not a big deal, but please drop in v3. It's your hard work and yours only. I'll add my Reviewed-by.
changes in v2: cast var cfg_mss to int to avoid static assertion after providing a stricter version of min() that does signedness checking.
tools/testing/selftests/net/Makefile | 2 ++ tools/testing/selftests/net/so_txtime.c | 7 ++++--- tools/testing/selftests/net/udpgso_bench_tx.c | 6 +++--- 3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile index 7f3ab2a93ed6..a06cc25489f9 100644 --- a/tools/testing/selftests/net/Makefile +++ b/tools/testing/selftests/net/Makefile @@ -3,6 +3,8 @@
CFLAGS = -Wall -Wl,--no-as-needed -O2 -g CFLAGS += -I../../../../usr/include/ $(KHDR_INCLUDES) +# Additional include paths needed by kselftest.h +CFLAGS += -I../
Why does kselftest.h need this? It does not include anything else?
I'd just add #include "../kselftests.h" to so_txtime.c and remove the path change from udpgso_bench_tx.c
Fine with this approach. Just don't understand the comment
the comment here is wrong and it should be changed to include path needed to include kselftest.h or to be deleted
TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh \ rtnetlink.sh xfrm_policy.sh test_blackhole_dev.sh diff --git a/tools/testing/selftests/net/so_txtime.c b/tools/testing/selftests/net/so_txtime.c index 2672ac0b6d1f..2936174e7de4 100644 --- a/tools/testing/selftests/net/so_txtime.c +++ b/tools/testing/selftests/net/so_txtime.c @@ -33,6 +33,8 @@ #include <unistd.h> #include <poll.h>
+#include "kselftest.h"
static int cfg_clockid = CLOCK_TAI; static uint16_t cfg_port = 8000; static int cfg_variance_us = 4000; @@ -93,8 +95,7 @@ static void do_send_one(int fdt, struct timed_send *ts) msg.msg_controllen = sizeof(control);
tdeliver = glob_tstart + ts->delay_us * 1000;
tdeliver_max = tdeliver_max > tdeliver ?tdeliver_max : tdeliver;
tdeliver_max = max(tdeliver_max, tdeliver); cm = CMSG_FIRSTHDR(&msg); cm->cmsg_level = SOL_SOCKET;@@ -126,7 +127,7 @@ static void do_recv_one(int fdr, struct timed_send *ts) error(1, 0, "read: %dB", ret);
tstop = (gettime_ns(cfg_clockid) - glob_tstart) / 1000;
texpect = ts->delay_us >= 0 ? ts->delay_us : 0;
texpect = max(ts->delay_us, 0); fprintf(stderr, "payload:%c delay:%lld expected:%lld (us)\n", rbuf[0], (long long)tstop, (long long)texpect);diff --git a/tools/testing/selftests/net/udpgso_bench_tx.c b/tools/testing/selftests/net/udpgso_bench_tx.c index 477392715a9a..6bd32a312901 100644 --- a/tools/testing/selftests/net/udpgso_bench_tx.c +++ b/tools/testing/selftests/net/udpgso_bench_tx.c @@ -25,7 +25,7 @@ #include <sys/types.h> #include <unistd.h>
-#include "../kselftest.h" +#include "kselftest.h"
#ifndef ETH_MAX_MTU #define ETH_MAX_MTU 0xFFFFU @@ -294,7 +294,7 @@ static int send_udp(int fd, char *data) total_len = cfg_payload_len;
while (total_len) {
len = total_len < cfg_mss ? total_len : cfg_mss;
len = min(total_len, (int)cfg_mss); ret = sendto(fd, data, len, cfg_zerocopy ? MSG_ZEROCOPY : 0, cfg_connected ? NULL : (void *)&cfg_dst_addr,@@ -351,7 +351,7 @@ static int send_udp_sendmmsg(int fd, char *data) error(1, 0, "sendmmsg: exceeds max_nr_msg");
iov[i].iov_base = data + off;
iov[i].iov_len = cfg_mss < left ? cfg_mss : left;
iov[i].iov_len = min(cfg_mss, left); mmsgs[i].msg_hdr.msg_iov = iov + i; mmsgs[i].msg_hdr.msg_iovlen = 1;-- 2.34.1