[v2,2/2] selftests/net: replace ternary operator with min()/max()

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(-)

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 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-+7ng@mail.gmail.com/
> 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)