Re: [PATCH net-next 1/4] selftests: drv-net: ncdevmem: remove use of error()

On 08/22, Jakub Kicinski wrote:

Using error() makes it impossible for callers to unwind their changes. Replace error() calls with proper error handling.

Signed-off-by: Jakub Kicinski kuba@kernel.org

Acked-by: Stanislav Fomichev sdf@fomichev.me

---

.../selftests/drivers/net/hw/ncdevmem.c | 528 ++++++++++++------ 1 file changed, 364 insertions(+), 164 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/hw/ncdevmem.c b/tools/testing/selftests/drivers/net/hw/ncdevmem.c index 71961a7688e6..e75adfed33ac 100644 --- a/tools/testing/selftests/drivers/net/hw/ncdevmem.c +++ b/tools/testing/selftests/drivers/net/hw/ncdevmem.c @@ -115,6 +115,21 @@ struct memory_provider { size_t off, int n); }; +static void pr_err(const char *fmt, ...) +{

• va_list args;
• fprintf(stderr, "%s: ", TEST_PREFIX);
• va_start(args, fmt);
• vfprintf(stderr, fmt, args);
• va_end(args);
• if (errno != 0)

• fprintf(stderr, ": %s", strerror(errno));
  

• fprintf(stderr, "\n");

+}

static struct memory_buffer *udmabuf_alloc(size_t size) { struct udmabuf_create create; @@ -123,27 +138,33 @@ static struct memory_buffer *udmabuf_alloc(size_t size) ctx = malloc(sizeof(*ctx)); if (!ctx)

• error(1, ENOMEM, "malloc failed");
  

• return NULL;
  

ctx->size = size; ctx->devfd = open("/dev/udmabuf", O_RDWR);

• if (ctx->devfd < 0)

• error(1, errno,
  

•       "%s: [skip,no-udmabuf: Unable to access DMA buffer device file]\n",
  

•       TEST_PREFIX);
  

• if (ctx->devfd < 0) {

• pr_err("[skip,no-udmabuf: Unable to access DMA buffer device file]");
  

• goto err_free_ctx;
  

• }

ctx->memfd = memfd_create("udmabuf-test", MFD_ALLOW_SEALING);

• if (ctx->memfd < 0)

• error(1, errno, "%s: [skip,no-memfd]\n", TEST_PREFIX);
  

• if (ctx->memfd < 0) {

• pr_err("[skip,no-memfd]");
  

• goto err_close_dev;
  

• }

ret = fcntl(ctx->memfd, F_ADD_SEALS, F_SEAL_SHRINK);

• if (ret < 0)

• error(1, errno, "%s: [skip,fcntl-add-seals]\n", TEST_PREFIX);
  

• if (ret < 0) {

• pr_err("[skip,fcntl-add-seals]");
  

• goto err_close_memfd;
  

• }

ret = ftruncate(ctx->memfd, size);

• if (ret == -1)

• error(1, errno, "%s: [FAIL,memfd-truncate]\n", TEST_PREFIX);
  

• if (ret == -1) {

• pr_err("[FAIL,memfd-truncate]");
  

• goto err_close_memfd;
  

• }

memset(&create, 0, sizeof(create)); @@ -151,15 +172,29 @@ static struct memory_buffer *udmabuf_alloc(size_t size) create.offset = 0; create.size = size; ctx->fd = ioctl(ctx->devfd, UDMABUF_CREATE, &create);

• if (ctx->fd < 0)

• error(1, errno, "%s: [FAIL, create udmabuf]\n", TEST_PREFIX);
  

• if (ctx->fd < 0) {

• pr_err("[FAIL, create udmabuf]");
  

• goto err_close_fd;
  

• }

ctx->buf_mem = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, ctx->fd, 0);

• if (ctx->buf_mem == MAP_FAILED)

• error(1, errno, "%s: [FAIL, map udmabuf]\n", TEST_PREFIX);
  

• if (ctx->buf_mem == MAP_FAILED) {

• pr_err("[FAIL, map udmabuf]");
  

• goto err_close_fd;
  

• }

return ctx;

+err_close_fd:

• close(ctx->fd);

+err_close_memfd:

• close(ctx->memfd);

+err_close_dev:

• close(ctx->devfd);

+err_free_ctx:

• free(ctx);
• return NULL;

} static void udmabuf_free(struct memory_buffer *ctx) @@ -217,7 +252,7 @@ static void print_nonzero_bytes(void *ptr, size_t size) putchar(p[i]); } -void validate_buffer(void *line, size_t size) +int validate_buffer(void *line, size_t size) { static unsigned char seed = 1; unsigned char *ptr = line; @@ -232,8 +267,10 @@ void validate_buffer(void *line, size_t size) "Failed validation: expected=%u, actual=%u, index=%lu\n", expected, ptr[i], i); errors++;

• 	if (errors > 20)
  

• 		error(1, 0, "validation failed.");
  

• 	if (errors > 20) {
  

• 		pr_err("validation failed");
  

• 		return -1;
  

• 	}
  

  } seed++; if (seed == do_validation)

@@ -241,6 +278,7 @@ void validate_buffer(void *line, size_t size) } fprintf(stdout, "Validated buffer\n");

• return 0;

} static int rxq_num(int ifindex) @@ -279,7 +317,7 @@ static int rxq_num(int ifindex) system(command); \ }) -static int reset_flow_steering(void) +static void reset_flow_steering(void) { /* Depending on the NIC, toggling ntuple off and on might not * be allowed. Additionally, attempting to delete existing filters @@ -292,7 +330,6 @@ static int reset_flow_steering(void) run_command( "ethtool -n %s | grep 'Filter:' | awk '{print $2}' | xargs -n1 ethtool -N %s delete >&2", ifname, ifname);

• return 0;

} static const char *tcp_data_split_str(int val) @@ -354,6 +391,11 @@ static int configure_rss(void) return run_command("ethtool -X %s equal %d >&2", ifname, start_queue); } +static void reset_rss(void) +{

• run_command("ethtool -X %s default >&2", ifname, start_queue);

+}

static int configure_channels(unsigned int rx, unsigned int tx) { struct ethtool_channels_get_req *gchan; @@ -479,6 +521,7 @@ static int bind_rx_queue(unsigned int ifindex, unsigned int dmabuf_fd, *ys = ynl_sock_create(&ynl_netdev_family, &yerr); if (!*ys) {

• netdev_queue_id_free(queues);
  

Funny how you spotted this.. Ownership of these is complicated with ynl :-(