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

[Stanislav Fomichev <stfomichev@gmail.com>]

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