to avoid manual calculation of min and max values and fix coccinelle warnings such WARNING opportunity for min()/max() adding one common definition that could be used in multiple files under selftests. there are also some defines for min/max scattered locally inside sources under selftests. this also prepares for cleaning up those redundant defines and include kselftest.h instead.
Signed-off-by: Mahmoud Maatuq mahmoudmatook.mm@gmail.com Suggested-by: David Laight David.Laight@aculab.com --- changes in v2: redefine min/max in a more strict way to avoid signedness mismatch and multiple evaluation. is_signed_type() moved from selftests/kselftest_harness.h to selftests/kselftest.h. --- tools/testing/selftests/kselftest.h | 24 +++++++++++++++++++++ tools/testing/selftests/kselftest_harness.h | 2 -- 2 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h index 829be379545a..93d029471cc9 100644 --- a/tools/testing/selftests/kselftest.h +++ b/tools/testing/selftests/kselftest.h @@ -55,6 +55,30 @@ #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0])) #endif
+#ifndef is_signed_type +#define is_signed_type(var) (!!(((__typeof__(var))(-1)) < (__typeof__(var))1)) +#endif + +#ifndef min +#define min(x, y) ({ \ + _Static_assert(is_signed_type(typeof(x)) == is_signed_type(typeof(y)), \ + "min: signedness mismatch"); \ + typeof(x) _x = (x); \ + typeof(y) _y = (y); \ + _x < _y ? _x : _y; \ +}) +#endif + +#ifndef max +#define max(x, y) ({ \ + _Static_assert(is_signed_type(typeof(x)) == is_signed_type(typeof(y)), \ + "max: signedness mismatch"); \ + typeof(x) _x = (x); \ + typeof(y) _y = (y); \ + _x > _y ? _x : _y; \ +}) +#endif + /* * gcc cpuid.h provides __cpuid_count() since v4.4. * Clang/LLVM cpuid.h provides __cpuid_count() since v3.4.0. diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index 5fd49ad0c696..e4e310815226 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -699,8 +699,6 @@ if (_metadata->passed && _metadata->step < 253) \ _metadata->step++;
-#define is_signed_type(var) (!!(((__typeof__(var))(-1)) < (__typeof__(var))1)) - #define __EXPECT(_expected, _expected_str, _seen, _seen_str, _t, _assert) do { \ /* Avoid multiple evaluation of the cases */ \ __typeof__(_expected) __exp = (_expected); \
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 --- 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../
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;
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.
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
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
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.
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
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
From: Mahmoud Maatuq
Sent: 24 August 2023 21:24
....
tdeliver = glob_tstart + ts->delay_us * 1000;
tdeliver_max = tdeliver_max > tdeliver ?tdeliver_max : tdeliver;
tdeliver_max = max(tdeliver_max, tdeliver);
That was spectacularly horrid... What is wrong with using: if (tdeliver > tdeliver_max) tdeliver_max = tdeliver; That is pretty obviously calculating the upper bound. Even the version with max() needs extra parsing by the human reader.
(The only issue is whether it reads better with the if condition reversed.)
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Fri, Aug 25, 2023, Mahmoud Maatuq wrote:
to avoid manual calculation of min and max values and fix coccinelle warnings such WARNING opportunity for min()/max() adding one common definition that could be used in multiple files under selftests. there are also some defines for min/max scattered locally inside sources under selftests. this also prepares for cleaning up those redundant defines and include kselftest.h instead.
Signed-off-by: Mahmoud Maatuq mahmoudmatook.mm@gmail.com Suggested-by: David Laight David.Laight@aculab.com
changes in v2: redefine min/max in a more strict way to avoid signedness mismatch and multiple evaluation. is_signed_type() moved from selftests/kselftest_harness.h to selftests/kselftest.h.
tools/testing/selftests/kselftest.h | 24 +++++++++++++++++++++
Heh, reminds me of https://xkcd.com/927.
All of these #defines are available in tools/include/linux/kernel.h, and it's trivially easy for selftests to add all of tools/include to their include path. I don't see any reason for the selftests framework to define yet another version, just fix the individual tests.
On 08/25, Sean Christopherson wrote:
On Fri, Aug 25, 2023, Mahmoud Maatuq wrote:
to avoid manual calculation of min and max values and fix coccinelle warnings such WARNING opportunity for min()/max() adding one common definition that could be used in multiple files under selftests. there are also some defines for min/max scattered locally inside sources under selftests. this also prepares for cleaning up those redundant defines and include kselftest.h instead.
Signed-off-by: Mahmoud Maatuq mahmoudmatook.mm@gmail.com Suggested-by: David Laight David.Laight@aculab.com
changes in v2: redefine min/max in a more strict way to avoid signedness mismatch and multiple evaluation. is_signed_type() moved from selftests/kselftest_harness.h to selftests/kselftest.h.
tools/testing/selftests/kselftest.h | 24 +++++++++++++++++++++
Heh, reminds me of https://xkcd.com/927.
All of these #defines are available in tools/include/linux/kernel.h, and it's trivially easy for selftests to add all of tools/include to their include path. I don't see any reason for the selftests framework to define yet another version, just fix the individual tests.
giving the reviews seems that patchset is useless. still a confusing point for me; after adding tools/include to the include path of selftes how we can differentaite between #include <linux/kernel.h> that under tools/include and one under usr/include.
On Sat, Aug 26, 2023, Mahmoud Matook wrote:
On 08/25, Sean Christopherson wrote:
On Fri, Aug 25, 2023, Mahmoud Maatuq wrote:
to avoid manual calculation of min and max values and fix coccinelle warnings such WARNING opportunity for min()/max() adding one common definition that could be used in multiple files under selftests. there are also some defines for min/max scattered locally inside sources under selftests. this also prepares for cleaning up those redundant defines and include kselftest.h instead.
Signed-off-by: Mahmoud Maatuq mahmoudmatook.mm@gmail.com Suggested-by: David Laight David.Laight@aculab.com
changes in v2: redefine min/max in a more strict way to avoid signedness mismatch and multiple evaluation. is_signed_type() moved from selftests/kselftest_harness.h to selftests/kselftest.h.
tools/testing/selftests/kselftest.h | 24 +++++++++++++++++++++
Heh, reminds me of https://xkcd.com/927.
All of these #defines are available in tools/include/linux/kernel.h, and it's trivially easy for selftests to add all of tools/include to their include path. I don't see any reason for the selftests framework to define yet another version, just fix the individual tests.
giving the reviews seems that patchset is useless. still a confusing point for me; after adding tools/include to the include path of selftes how we can differentaite between #include <linux/kernel.h> that under tools/include and one under usr/include.
AFAIK, it's up to the individual selftest (or it's "local" framework) to declare the tools/include path before usr/include, e.g. see tools/testing/selftests/kvm/Makefile.
The whole setup is definitely a bit kludgy, but IMO it's better than conditionally providing selftests specific fallbacks and potentially ending up with multiple definitions of min/max within a single test.
linux-kselftest-mirror@lists.linaro.org
-
David Laight -
Mahmoud Maatuq -
Mahmoud Matook
-
Sean Christopherson -
Willem de Bruijn