diff mbox series

[bpf-next] selftests/bpf: use ifname instead of ifindex in XDP compliance test tool

Message ID 5d11c9163490126fdc391dacb122480e4c059e62.1677863821.git.lorenzo@kernel.org (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf-next] selftests/bpf: use ifname instead of ifindex in XDP compliance test tool | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 14 maintainers not CCed: linux-kselftest@vger.kernel.org davem@davemloft.net jolsa@kernel.org john.fastabend@gmail.com mykolal@fb.com martin.lau@linux.dev yhs@fb.com kpsingh@kernel.org song@kernel.org haoluo@google.com kuba@kernel.org shuah@kernel.org hawk@kernel.org sdf@google.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 82 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32 on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for test_progs_parallel on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for test_progs_parallel on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 success Logs for test_verifier on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-38 success Logs for test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_progs_no_alu32_parallel on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for test_progs_parallel on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-7 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix

Commit Message

Lorenzo Bianconi March 3, 2023, 5:21 p.m. UTC 
Rely on interface name instead of interface index in error messages or logs
from XDP compliance test tool.
Improve XDP compliance test tool error messages.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 tools/testing/selftests/bpf/xdp_features.c | 92 ++++++++++++++--------
 1 file changed, 57 insertions(+), 35 deletions(-)

Comments

Daniel Borkmann March 6, 2023, 3:35 p.m. UTC | #1 
On 3/3/23 6:21 PM, Lorenzo Bianconi wrote:
> Rely on interface name instead of interface index in error messages or logs
> from XDP compliance test tool.
> Improve XDP compliance test tool error messages.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>   tools/testing/selftests/bpf/xdp_features.c | 92 ++++++++++++++--------
>   1 file changed, 57 insertions(+), 35 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/xdp_features.c b/tools/testing/selftests/bpf/xdp_features.c
> index fce12165213b..7414801cd7ec 100644
> --- a/tools/testing/selftests/bpf/xdp_features.c
> +++ b/tools/testing/selftests/bpf/xdp_features.c
> @@ -25,6 +25,7 @@
>   
>   static struct env {
>   	bool verbosity;
> +	char ifname[IF_NAMESIZE];
>   	int ifindex;
>   	bool is_tester;
>   	struct {
> @@ -109,25 +110,25 @@ static int get_xdp_feature(const char *arg)
>   	return 0;
>   }
>   
> -static char *get_xdp_feature_str(void)
> +static char *get_xdp_feature_str(bool color)
>   {
>   	switch (env.feature.action) {
>   	case XDP_PASS:
> -		return YELLOW("XDP_PASS");
> +		return color ? YELLOW("XDP_PASS") : "XDP_PASS";
>   	case XDP_DROP:
> -		return YELLOW("XDP_DROP");
> +		return color ? YELLOW("XDP_DROP") : "XDP_DROP";
>   	case XDP_ABORTED:
> -		return YELLOW("XDP_ABORTED");
> +		return color ? YELLOW("XDP_ABORTED") : "XDP_ABORTED";
>   	case XDP_TX:
> -		return YELLOW("XDP_TX");
> +		return color ? YELLOW("XDP_TX") : "XDP_TX";
>   	case XDP_REDIRECT:
> -		return YELLOW("XDP_REDIRECT");
> +		return color ? YELLOW("XDP_REDIRECT") : "XDP_REDIRECT";
>   	default:
>   		break;
>   	}
>   
>   	if (env.feature.drv_feature == NETDEV_XDP_ACT_NDO_XMIT)
> -		return YELLOW("XDP_NDO_XMIT");
> +		return color ? YELLOW("XDP_NDO_XMIT") : "XDP_NDO_XMIT";
>   
>   	return "";
>   }

Please split this into multiple patches, logically separated. This one is changing
multiple things at once and above has not much relation to relying on interface names.

Thanks,
Daniel
Lorenzo Bianconi March 6, 2023, 4:01 p.m. UTC | #2 
> On 3/3/23 6:21 PM, Lorenzo Bianconi wrote:
> > Rely on interface name instead of interface index in error messages or logs
> > from XDP compliance test tool.
> > Improve XDP compliance test tool error messages.
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >   tools/testing/selftests/bpf/xdp_features.c | 92 ++++++++++++++--------
> >   1 file changed, 57 insertions(+), 35 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/xdp_features.c b/tools/testing/selftests/bpf/xdp_features.c
> > index fce12165213b..7414801cd7ec 100644
> > --- a/tools/testing/selftests/bpf/xdp_features.c
> > +++ b/tools/testing/selftests/bpf/xdp_features.c
> > @@ -25,6 +25,7 @@
> >   static struct env {
> >   	bool verbosity;
> > +	char ifname[IF_NAMESIZE];
> >   	int ifindex;
> >   	bool is_tester;
> >   	struct {
> > @@ -109,25 +110,25 @@ static int get_xdp_feature(const char *arg)
> >   	return 0;
> >   }
> > -static char *get_xdp_feature_str(void)
> > +static char *get_xdp_feature_str(bool color)
> >   {
> >   	switch (env.feature.action) {
> >   	case XDP_PASS:
> > -		return YELLOW("XDP_PASS");
> > +		return color ? YELLOW("XDP_PASS") : "XDP_PASS";
> >   	case XDP_DROP:
> > -		return YELLOW("XDP_DROP");
> > +		return color ? YELLOW("XDP_DROP") : "XDP_DROP";
> >   	case XDP_ABORTED:
> > -		return YELLOW("XDP_ABORTED");
> > +		return color ? YELLOW("XDP_ABORTED") : "XDP_ABORTED";
> >   	case XDP_TX:
> > -		return YELLOW("XDP_TX");
> > +		return color ? YELLOW("XDP_TX") : "XDP_TX";
> >   	case XDP_REDIRECT:
> > -		return YELLOW("XDP_REDIRECT");
> > +		return color ? YELLOW("XDP_REDIRECT") : "XDP_REDIRECT";
> >   	default:
> >   		break;
> >   	}
> >   	if (env.feature.drv_feature == NETDEV_XDP_ACT_NDO_XMIT)
> > -		return YELLOW("XDP_NDO_XMIT");
> > +		return color ? YELLOW("XDP_NDO_XMIT") : "XDP_NDO_XMIT";
> >   	return "";
> >   }
> 
> Please split this into multiple patches, logically separated. This one is changing
> multiple things at once and above has not much relation to relying on interface names.

ack, I will do.

Regards,
Lorenzo

> 
> Thanks,
> Daniel
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/xdp_features.c b/tools/testing/selftests/bpf/xdp_features.c
index fce12165213b..7414801cd7ec 100644
--- a/tools/testing/selftests/bpf/xdp_features.c
+++ b/tools/testing/selftests/bpf/xdp_features.c
@@ -25,6 +25,7 @@ 
 
 static struct env {
 	bool verbosity;
+	char ifname[IF_NAMESIZE];
 	int ifindex;
 	bool is_tester;
 	struct {
@@ -109,25 +110,25 @@  static int get_xdp_feature(const char *arg)
 	return 0;
 }
 
-static char *get_xdp_feature_str(void)
+static char *get_xdp_feature_str(bool color)
 {
 	switch (env.feature.action) {
 	case XDP_PASS:
-		return YELLOW("XDP_PASS");
+		return color ? YELLOW("XDP_PASS") : "XDP_PASS";
 	case XDP_DROP:
-		return YELLOW("XDP_DROP");
+		return color ? YELLOW("XDP_DROP") : "XDP_DROP";
 	case XDP_ABORTED:
-		return YELLOW("XDP_ABORTED");
+		return color ? YELLOW("XDP_ABORTED") : "XDP_ABORTED";
 	case XDP_TX:
-		return YELLOW("XDP_TX");
+		return color ? YELLOW("XDP_TX") : "XDP_TX";
 	case XDP_REDIRECT:
-		return YELLOW("XDP_REDIRECT");
+		return color ? YELLOW("XDP_REDIRECT") : "XDP_REDIRECT";
 	default:
 		break;
 	}
 
 	if (env.feature.drv_feature == NETDEV_XDP_ACT_NDO_XMIT)
-		return YELLOW("XDP_NDO_XMIT");
+		return color ? YELLOW("XDP_NDO_XMIT") : "XDP_NDO_XMIT";
 
 	return "";
 }
@@ -151,20 +152,26 @@  static error_t parse_arg(int key, char *arg, struct argp_state *state)
 	case 'D':
 		if (make_sockaddr(AF_INET6, arg, DUT_ECHO_PORT,
 				  &env.dut_addr, NULL)) {
-			fprintf(stderr, "Invalid DUT address: %s\n", arg);
+			fprintf(stderr,
+				"Invalid address assigned to the Device Under Test: %s\n",
+				arg);
 			return ARGP_ERR_UNKNOWN;
 		}
 		break;
 	case 'C':
 		if (make_sockaddr(AF_INET6, arg, DUT_CTRL_PORT,
 				  &env.dut_ctrl_addr, NULL)) {
-			fprintf(stderr, "Invalid DUT CTRL address: %s\n", arg);
+			fprintf(stderr,
+				"Invalid address assigned to the Device Under Test: %s\n",
+				arg);
 			return ARGP_ERR_UNKNOWN;
 		}
 		break;
 	case 'T':
 		if (make_sockaddr(AF_INET6, arg, 0, &env.tester_addr, NULL)) {
-			fprintf(stderr, "Invalid Tester address: %s\n", arg);
+			fprintf(stderr,
+				"Invalid address assigned to the Tester device: %s\n",
+				arg);
 			return ARGP_ERR_UNKNOWN;
 		}
 		break;
@@ -179,7 +186,7 @@  static error_t parse_arg(int key, char *arg, struct argp_state *state)
 		env.ifindex = if_nametoindex(arg);
 		if (!env.ifindex)
 			env.ifindex = strtoul(arg, NULL, 0);
-		if (!env.ifindex) {
+		if (!env.ifindex || !if_indextoname(env.ifindex, env.ifname)) {
 			fprintf(stderr,
 				"Bad interface index or name (%d): %s\n",
 				errno, strerror(errno));
@@ -205,6 +212,7 @@  static void set_env_default(void)
 	env.feature.drv_feature = NETDEV_XDP_ACT_NDO_XMIT;
 	env.feature.action = -EINVAL;
 	env.ifindex = -ENODEV;
+	strcpy(env.ifname, "unknown");
 	make_sockaddr(AF_INET6, "::ffff:127.0.0.1", DUT_CTRL_PORT,
 		      &env.dut_ctrl_addr, NULL);
 	make_sockaddr(AF_INET6, "::ffff:127.0.0.1", DUT_ECHO_PORT,
@@ -248,15 +256,18 @@  static int dut_run_echo_thread(pthread_t *t, int *sockfd)
 	sockfd = start_reuseport_server(AF_INET6, SOCK_DGRAM, NULL,
 					DUT_ECHO_PORT, 0, 1);
 	if (!sockfd) {
-		fprintf(stderr, "Failed to create echo socket\n");
+		fprintf(stderr,
+			"Failed creating data UDP socket on device %s\n",
+			env.ifname);
 		return -errno;
 	}
 
 	/* start echo channel */
 	err = pthread_create(t, NULL, dut_echo_thread, sockfd);
 	if (err) {
-		fprintf(stderr, "Failed creating dut_echo thread: %s\n",
-			strerror(-err));
+		fprintf(stderr,
+			"Failed creating data UDP thread on device %s: %s\n",
+			env.ifname, strerror(-err));
 		free_fds(sockfd, 1);
 		return -EINVAL;
 	}
@@ -320,9 +331,8 @@  static int dut_attach_xdp_prog(struct xdp_features *skel, int flags)
 
 	err = bpf_xdp_attach(env.ifindex, bpf_program__fd(prog), flags, NULL);
 	if (err)
-		fprintf(stderr,
-			"Failed to attach XDP program to ifindex %d\n",
-			env.ifindex);
+		fprintf(stderr, "Failed attaching XDP program to device %s\n",
+			env.ifname);
 	return err;
 }
 
@@ -355,16 +365,22 @@  static int dut_run(struct xdp_features *skel)
 	pthread_t dut_thread;
 	socklen_t addrlen;
 
+	fprintf(stdout, "Starting test for %s capability on device %s\n",
+		get_xdp_feature_str(false), env.ifname);
+
 	sockfd = start_reuseport_server(AF_INET6, SOCK_STREAM, NULL,
 					DUT_CTRL_PORT, 0, 1);
 	if (!sockfd) {
-		fprintf(stderr, "Failed to create DUT socket\n");
+		fprintf(stderr,
+			"Failed creating control socket on device %s\n", env.ifname);
 		return -errno;
 	}
 
 	ctrl_sockfd = accept(*sockfd, (struct sockaddr *)&ctrl_addr, &addrlen);
 	if (ctrl_sockfd < 0) {
-		fprintf(stderr, "Failed to accept connection on DUT socket\n");
+		fprintf(stderr,
+			"Failed accepting connections on device %s control socket\n",
+			env.ifname);
 		free_fds(sockfd, 1);
 		return -errno;
 	}
@@ -422,8 +438,8 @@  static int dut_run(struct xdp_features *skel)
 					    &opts);
 			if (err) {
 				fprintf(stderr,
-					"Failed to query XDP cap for ifindex %d\n",
-					env.ifindex);
+					"Failed querying XDP cap for device %s\n",
+					env.ifname);
 				goto end_thread;
 			}
 
@@ -447,7 +463,8 @@  static int dut_run(struct xdp_features *skel)
 						   &key, sizeof(key),
 						   &val, sizeof(val), 0);
 			if (err) {
-				fprintf(stderr, "bpf_map_lookup_elem failed\n");
+				fprintf(stderr,
+					"bpf_map_lookup_elem failed (%d)\n", err);
 				goto end_thread;
 			}
 
@@ -489,7 +506,7 @@  static bool tester_collect_detected_cap(struct xdp_features *skel,
 	err = bpf_map__lookup_elem(skel->maps.stats, &key, sizeof(key),
 				   &val, sizeof(val), 0);
 	if (err) {
-		fprintf(stderr, "bpf_map_lookup_elem failed\n");
+		fprintf(stderr, "bpf_map_lookup_elem failed (%d)\n", err);
 		return false;
 	}
 
@@ -540,7 +557,9 @@  static int send_echo_msg(void)
 
 	sockfd = socket(AF_INET6, SOCK_DGRAM, 0);
 	if (sockfd < 0) {
-		fprintf(stderr, "Failed to create echo socket\n");
+		fprintf(stderr,
+			"Failed creating data UDP socket on device %s\n",
+			env.ifname);
 		return -errno;
 	}
 
@@ -563,9 +582,14 @@  static int tester_run(struct xdp_features *skel)
 	int i, err, sockfd;
 	bool detected_cap;
 
+	fprintf(stdout,
+		"Starting tester service for %s capability on device %s\n",
+		get_xdp_feature_str(false), env.ifname);
+
 	sockfd = socket(AF_INET6, SOCK_STREAM, 0);
 	if (sockfd < 0) {
-		fprintf(stderr, "Failed to create tester socket\n");
+		fprintf(stderr,
+			"Failed creating tester service control socket\n");
 		return -errno;
 	}
 
@@ -575,7 +599,8 @@  static int tester_run(struct xdp_features *skel)
 	err = connect(sockfd, (struct sockaddr *)&env.dut_ctrl_addr,
 		      sizeof(env.dut_ctrl_addr));
 	if (err) {
-		fprintf(stderr, "Failed to connect to the DUT\n");
+		fprintf(stderr,
+			"Failed connecting to the Device Under Test control socket\n");
 		return -errno;
 	}
 
@@ -596,8 +621,8 @@  static int tester_run(struct xdp_features *skel)
 
 	err = bpf_xdp_attach(env.ifindex, bpf_program__fd(prog), flags, NULL);
 	if (err) {
-		fprintf(stderr, "Failed to attach XDP program to ifindex %d\n",
-			env.ifindex);
+		fprintf(stderr, "Failed attaching XDP program to device %s\n",
+			env.ifname);
 		goto out;
 	}
 
@@ -624,7 +649,7 @@  static int tester_run(struct xdp_features *skel)
 
 	detected_cap = tester_collect_detected_cap(skel, ntohl(stats));
 
-	fprintf(stdout, "Feature %s: [%s][%s]\n", get_xdp_feature_str(),
+	fprintf(stdout, "Feature %s: [%s][%s]\n", get_xdp_feature_str(true),
 		detected_cap ? GREEN("DETECTED") : RED("NOT DETECTED"),
 		env.feature.drv_feature & advertised_feature ? GREEN("ADVERTISED")
 							     : RED("NOT ADVERTISED"));
@@ -653,7 +678,7 @@  int main(int argc, char **argv)
 		return err;
 
 	if (env.ifindex < 0) {
-		fprintf(stderr, "Invalid ifindex\n");
+		fprintf(stderr, "Invalid device name %s\n", env.ifname);
 		return -ENODEV;
 	}
 
@@ -682,15 +707,12 @@  int main(int argc, char **argv)
 		goto cleanup;
 	}
 
-	if (env.is_tester) {
+	if (env.is_tester)
 		/* Tester */
-		fprintf(stdout, "Starting tester on device %d\n", env.ifindex);
 		err = tester_run(skel);
-	} else {
+	else
 		/* DUT */
-		fprintf(stdout, "Starting DUT on device %d\n", env.ifindex);
 		err = dut_run(skel);
-	}
 
 cleanup:
 	xdp_features__destroy(skel);