diff mbox series

[net] selftests/net: report etf errors correctly

Message ID 20200618145549.37937-1-willemdebruijn.kernel@gmail.com
State Awaiting Upstream
Delegated to: David Miller
Headers show
Series [net] selftests/net: report etf errors correctly | expand

Commit Message

Willem de Bruijn June 18, 2020, 2:55 p.m. UTC 
From: Willem de Bruijn <willemb@google.com>

The ETF qdisc can queue skbs that it could not pace on the errqueue.

Address a few issues in the selftest

- recv buffer size was too small, and incorrectly calculated
- compared errno to ee_code instead of ee_errno
- missed error type invalid request

Fixes: ea6a547669b3 ("selftests/net: make so_txtime more robust to timer variance")
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 tools/testing/selftests/net/so_txtime.c | 33 +++++++++++++++++++------
 1 file changed, 26 insertions(+), 7 deletions(-)

Comments

Jakub Kicinski June 18, 2020, 3:54 p.m. UTC | #1 
On Thu, 18 Jun 2020 10:55:49 -0400 Willem de Bruijn wrote:
> +		switch (err->ee_errno) {
> +		case ECANCELED:
> +			if (err->ee_code != SO_EE_CODE_TXTIME_MISSED)
> +				error(1, 0, "errqueue: unknown ECANCELED %u\n",
> +					    err->ee_code);
> +			reason = "missed txtime";
> +		break;
> +		case EINVAL:
> +			if (err->ee_code != SO_EE_CODE_TXTIME_INVALID_PARAM)
> +				error(1, 0, "errqueue: unknown EINVAL %u\n",
> +					    err->ee_code);
> +			reason = "invalid txtime";
> +		break;
> +		default:
> +			error(1, 0, "errqueue: errno %u code %u\n",
> +			      err->ee_errno, err->ee_code);
> +		};
>  
>  		tstamp = ((int64_t) err->ee_data) << 32 | err->ee_info;
>  		tstamp -= (int64_t) glob_tstart;
>  		tstamp /= 1000 * 1000;
> -		fprintf(stderr, "send: pkt %c at %" PRId64 "ms dropped\n",
> -				data[ret - 1], tstamp);
> +		fprintf(stderr, "send: pkt %c at %" PRId64 "ms dropped: %s\n",
> +				data[ret - 1], tstamp, reason);

Hi Willem! Checkpatch is grumpy about some misalignment here:

CHECK: Alignment should match open parenthesis
#67: FILE: tools/testing/selftests/net/so_txtime.c:187:
+				error(1, 0, "errqueue: unknown ECANCELED %u\n",
+					    err->ee_code);

CHECK: Alignment should match open parenthesis
#73: FILE: tools/testing/selftests/net/so_txtime.c:193:
+				error(1, 0, "errqueue: unknown EINVAL %u\n",
+					    err->ee_code);

CHECK: Alignment should match open parenthesis
#87: FILE: tools/testing/selftests/net/so_txtime.c:205:
+		fprintf(stderr, "send: pkt %c at %" PRId64 "ms dropped: %s\n",
+				data[ret - 1], tstamp, reason);
Willem de Bruijn June 18, 2020, 4:18 p.m. UTC | #2 
On Thu, Jun 18, 2020 at 11:54 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 18 Jun 2020 10:55:49 -0400 Willem de Bruijn wrote:
> > +             switch (err->ee_errno) {
> > +             case ECANCELED:
> > +                     if (err->ee_code != SO_EE_CODE_TXTIME_MISSED)
> > +                             error(1, 0, "errqueue: unknown ECANCELED %u\n",
> > +                                         err->ee_code);
> > +                     reason = "missed txtime";
> > +             break;
> > +             case EINVAL:
> > +                     if (err->ee_code != SO_EE_CODE_TXTIME_INVALID_PARAM)
> > +                             error(1, 0, "errqueue: unknown EINVAL %u\n",
> > +                                         err->ee_code);
> > +                     reason = "invalid txtime";
> > +             break;
> > +             default:
> > +                     error(1, 0, "errqueue: errno %u code %u\n",
> > +                           err->ee_errno, err->ee_code);
> > +             };
> >
> >               tstamp = ((int64_t) err->ee_data) << 32 | err->ee_info;
> >               tstamp -= (int64_t) glob_tstart;
> >               tstamp /= 1000 * 1000;
> > -             fprintf(stderr, "send: pkt %c at %" PRId64 "ms dropped\n",
> > -                             data[ret - 1], tstamp);
> > +             fprintf(stderr, "send: pkt %c at %" PRId64 "ms dropped: %s\n",
> > +                             data[ret - 1], tstamp, reason);
>
> Hi Willem! Checkpatch is grumpy about some misalignment here:
>
> CHECK: Alignment should match open parenthesis
> #67: FILE: tools/testing/selftests/net/so_txtime.c:187:
> +                               error(1, 0, "errqueue: unknown ECANCELED %u\n",
> +                                           err->ee_code);
>
> CHECK: Alignment should match open parenthesis
> #73: FILE: tools/testing/selftests/net/so_txtime.c:193:
> +                               error(1, 0, "errqueue: unknown EINVAL %u\n",
> +                                           err->ee_code);
>
> CHECK: Alignment should match open parenthesis
> #87: FILE: tools/testing/selftests/net/so_txtime.c:205:
> +               fprintf(stderr, "send: pkt %c at %" PRId64 "ms dropped: %s\n",
> +                               data[ret - 1], tstamp, reason);

Thanks for the heads-up, Jakub.

I decided to follow the convention in the file, which is to align with
the start of the string.

Given that, do you want me to resubmit with the revised offset? I'm
fine either way, of course.

Also, which incantation of checkpatch do you use? I did run
checkpatch, without extra args, and it did not warn me about this.
Jakub Kicinski June 18, 2020, 4:36 p.m. UTC | #3 
On Thu, 18 Jun 2020 12:18:01 -0400 Willem de Bruijn wrote:
> On Thu, Jun 18, 2020 at 11:54 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Thu, 18 Jun 2020 10:55:49 -0400 Willem de Bruijn wrote:  
> > > +             switch (err->ee_errno) {
> > > +             case ECANCELED:
> > > +                     if (err->ee_code != SO_EE_CODE_TXTIME_MISSED)
> > > +                             error(1, 0, "errqueue: unknown ECANCELED %u\n",
> > > +                                         err->ee_code);
> > > +                     reason = "missed txtime";
> > > +             break;
> > > +             case EINVAL:
> > > +                     if (err->ee_code != SO_EE_CODE_TXTIME_INVALID_PARAM)
> > > +                             error(1, 0, "errqueue: unknown EINVAL %u\n",
> > > +                                         err->ee_code);
> > > +                     reason = "invalid txtime";
> > > +             break;
> > > +             default:
> > > +                     error(1, 0, "errqueue: errno %u code %u\n",
> > > +                           err->ee_errno, err->ee_code);
> > > +             };
> > >
> > >               tstamp = ((int64_t) err->ee_data) << 32 | err->ee_info;
> > >               tstamp -= (int64_t) glob_tstart;
> > >               tstamp /= 1000 * 1000;
> > > -             fprintf(stderr, "send: pkt %c at %" PRId64 "ms dropped\n",
> > > -                             data[ret - 1], tstamp);
> > > +             fprintf(stderr, "send: pkt %c at %" PRId64 "ms dropped: %s\n",
> > > +                             data[ret - 1], tstamp, reason);  
> >
> > Hi Willem! Checkpatch is grumpy about some misalignment here:
> >
> > CHECK: Alignment should match open parenthesis
> > #67: FILE: tools/testing/selftests/net/so_txtime.c:187:
> > +                               error(1, 0, "errqueue: unknown ECANCELED %u\n",
> > +                                           err->ee_code);
> >
> > CHECK: Alignment should match open parenthesis
> > #73: FILE: tools/testing/selftests/net/so_txtime.c:193:
> > +                               error(1, 0, "errqueue: unknown EINVAL %u\n",
> > +                                           err->ee_code);
> >
> > CHECK: Alignment should match open parenthesis
> > #87: FILE: tools/testing/selftests/net/so_txtime.c:205:
> > +               fprintf(stderr, "send: pkt %c at %" PRId64 "ms dropped: %s\n",
> > +                               data[ret - 1], tstamp, reason);  
> 
> Thanks for the heads-up, Jakub.
> 
> I decided to follow the convention in the file, which is to align with
> the start of the string.

Ack, I remember the selftest was added with a larger series so I didn't
want to complain about minutia :)

> Given that, do you want me to resubmit with the revised offset? I'm
> fine either way, of course.

No strong feelings, perhaps it's fine if the rest of the file is
like that already.

> Also, which incantation of checkpatch do you use? I did run
> checkpatch, without extra args, and it did not warn me about this.

I run with --strict, and pick the warnings which make sense.
Willem de Bruijn June 18, 2020, 4:43 p.m. UTC | #4 
On Thu, Jun 18, 2020 at 12:36 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 18 Jun 2020 12:18:01 -0400 Willem de Bruijn wrote:
> > On Thu, Jun 18, 2020 at 11:54 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > On Thu, 18 Jun 2020 10:55:49 -0400 Willem de Bruijn wrote:
> > > > +             switch (err->ee_errno) {
> > > > +             case ECANCELED:
> > > > +                     if (err->ee_code != SO_EE_CODE_TXTIME_MISSED)
> > > > +                             error(1, 0, "errqueue: unknown ECANCELED %u\n",
> > > > +                                         err->ee_code);
> > > > +                     reason = "missed txtime";
> > > > +             break;
> > > > +             case EINVAL:
> > > > +                     if (err->ee_code != SO_EE_CODE_TXTIME_INVALID_PARAM)
> > > > +                             error(1, 0, "errqueue: unknown EINVAL %u\n",
> > > > +                                         err->ee_code);
> > > > +                     reason = "invalid txtime";
> > > > +             break;
> > > > +             default:
> > > > +                     error(1, 0, "errqueue: errno %u code %u\n",
> > > > +                           err->ee_errno, err->ee_code);
> > > > +             };
> > > >
> > > >               tstamp = ((int64_t) err->ee_data) << 32 | err->ee_info;
> > > >               tstamp -= (int64_t) glob_tstart;
> > > >               tstamp /= 1000 * 1000;
> > > > -             fprintf(stderr, "send: pkt %c at %" PRId64 "ms dropped\n",
> > > > -                             data[ret - 1], tstamp);
> > > > +             fprintf(stderr, "send: pkt %c at %" PRId64 "ms dropped: %s\n",
> > > > +                             data[ret - 1], tstamp, reason);
> > >
> > > Hi Willem! Checkpatch is grumpy about some misalignment here:
> > >
> > > CHECK: Alignment should match open parenthesis
> > > #67: FILE: tools/testing/selftests/net/so_txtime.c:187:
> > > +                               error(1, 0, "errqueue: unknown ECANCELED %u\n",
> > > +                                           err->ee_code);
> > >
> > > CHECK: Alignment should match open parenthesis
> > > #73: FILE: tools/testing/selftests/net/so_txtime.c:193:
> > > +                               error(1, 0, "errqueue: unknown EINVAL %u\n",
> > > +                                           err->ee_code);
> > >
> > > CHECK: Alignment should match open parenthesis
> > > #87: FILE: tools/testing/selftests/net/so_txtime.c:205:
> > > +               fprintf(stderr, "send: pkt %c at %" PRId64 "ms dropped: %s\n",
> > > +                               data[ret - 1], tstamp, reason);
> >
> > Thanks for the heads-up, Jakub.
> >
> > I decided to follow the convention in the file, which is to align with
> > the start of the string.
>
> Ack, I remember the selftest was added with a larger series so I didn't
> want to complain about minutia :)
>
> > Given that, do you want me to resubmit with the revised offset? I'm
> > fine either way, of course.
>
> No strong feelings, perhaps it's fine if the rest of the file is
> like that already.

We'll have to standardize at some point anyway. Sent a v2.

>
> > Also, which incantation of checkpatch do you use? I did run
> > checkpatch, without extra args, and it did not warn me about this.
>
> I run with --strict, and pick the warnings which make sense.

Ah, thanks. I've updated my bash alias to do the same from now on. The
PRId64 CamelCase warning is a false positive I'll have to leave as is.
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/so_txtime.c b/tools/testing/selftests/net/so_txtime.c
index 383bac05ac32..3400afff0c3c 100644
--- a/tools/testing/selftests/net/so_txtime.c
+++ b/tools/testing/selftests/net/so_txtime.c
@@ -15,8 +15,9 @@ 
 #include <inttypes.h>
 #include <linux/net_tstamp.h>
 #include <linux/errqueue.h>
+#include <linux/if_ether.h>
 #include <linux/ipv6.h>
-#include <linux/tcp.h>
+#include <linux/udp.h>
 #include <stdbool.h>
 #include <stdlib.h>
 #include <stdio.h>
@@ -140,8 +141,8 @@  static void do_recv_errqueue_timeout(int fdt)
 {
 	char control[CMSG_SPACE(sizeof(struct sock_extended_err)) +
 		     CMSG_SPACE(sizeof(struct sockaddr_in6))] = {0};
-	char data[sizeof(struct ipv6hdr) +
-		  sizeof(struct tcphdr) + 1];
+	char data[sizeof(struct ethhdr) + sizeof(struct ipv6hdr) +
+		  sizeof(struct udphdr) + 1];
 	struct sock_extended_err *err;
 	struct msghdr msg = {0};
 	struct iovec iov = {0};
@@ -159,6 +160,8 @@  static void do_recv_errqueue_timeout(int fdt)
 	msg.msg_controllen = sizeof(control);
 
 	while (1) {
+		const char *reason;
+
 		ret = recvmsg(fdt, &msg, MSG_ERRQUEUE);
 		if (ret == -1 && errno == EAGAIN)
 			break;
@@ -176,14 +179,30 @@  static void do_recv_errqueue_timeout(int fdt)
 		err = (struct sock_extended_err *)CMSG_DATA(cm);
 		if (err->ee_origin != SO_EE_ORIGIN_TXTIME)
 			error(1, 0, "errqueue: origin 0x%x\n", err->ee_origin);
-		if (err->ee_code != ECANCELED)
-			error(1, 0, "errqueue: code 0x%x\n", err->ee_code);
+
+		switch (err->ee_errno) {
+		case ECANCELED:
+			if (err->ee_code != SO_EE_CODE_TXTIME_MISSED)
+				error(1, 0, "errqueue: unknown ECANCELED %u\n",
+					    err->ee_code);
+			reason = "missed txtime";
+		break;
+		case EINVAL:
+			if (err->ee_code != SO_EE_CODE_TXTIME_INVALID_PARAM)
+				error(1, 0, "errqueue: unknown EINVAL %u\n",
+					    err->ee_code);
+			reason = "invalid txtime";
+		break;
+		default:
+			error(1, 0, "errqueue: errno %u code %u\n",
+			      err->ee_errno, err->ee_code);
+		};
 
 		tstamp = ((int64_t) err->ee_data) << 32 | err->ee_info;
 		tstamp -= (int64_t) glob_tstart;
 		tstamp /= 1000 * 1000;
-		fprintf(stderr, "send: pkt %c at %" PRId64 "ms dropped\n",
-				data[ret - 1], tstamp);
+		fprintf(stderr, "send: pkt %c at %" PRId64 "ms dropped: %s\n",
+				data[ret - 1], tstamp, reason);
 
 		msg.msg_flags = 0;
 		msg.msg_controllen = sizeof(control);