Received-SPF: pass (google.com: domain of linux-kernel+bounces-119975-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <387a04c3-356e-4248-9cd3-7aa1a617c221@linuxfoundation.org>
Date: Tue, 26 Mar 2024 15:05:47 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 1/2] selftests/dmabuf-heap: conform test to TAP format
 output
Content-Language: en-US
To: Muhammad Usama Anjum <usama.anjum@collabora.com>,
 Shuah Khan <shuah@kernel.org>, "T.J. Mercier" <tjmercier@google.com>
Cc: kernel@collabora.com, linux-kselftest@vger.kernel.org,
 linux-kernel@vger.kernel.org, Shuah Khan <skhan@linuxfoundation.org>
References: <20240305060848.2478806-1-usama.anjum@collabora.com>
From: Shuah Khan <skhan@linuxfoundation.org>
In-Reply-To: <20240305060848.2478806-1-usama.anjum@collabora.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 3/4/24 23:08, Muhammad Usama Anjum wrote:
> Conform the layout, informational and status messages to TAP. No
> functional change is intended other than the layout of output messages.
> 
> Reviewed-by: T.J. Mercier <tjmercier@google.com>
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---
> Changes since v4:
> - close fds correctly with code changes added in v3
> 
> Chanages since v3:
> - abort test-case instead of exiting if heap/mem allocation fails
> - Correct test_alloc_zeroed() test case in case of failure
> 
> Changes since v2:
> - Minor improvements in test_alloc_zeroed() results
> 
> Changes since v1:
> - Update some more error handling code

t would be nice to improve the error messages in addition to
changing them over to TAP format. Please see below:

> ---
>   .../selftests/dmabuf-heaps/dmabuf-heap.c      | 246 +++++++-----------
>   1 file changed, 101 insertions(+), 145 deletions(-)
> 
> diff --git a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
> index 890a8236a8ba7..e7bd03e0af2ea 100644
> --- a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
> +++ b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
> @@ -15,6 +15,7 @@
>   #include <linux/dma-buf.h>
>   #include <linux/dma-heap.h>
>   #include <drm/drm.h>
> +#include "../kselftest.h"
>   
>   #define DEVPATH "/dev/dma_heap"
>   
> @@ -90,14 +91,13 @@ static int dmabuf_heap_open(char *name)
>   	char buf[256];
>   
>   	ret = snprintf(buf, 256, "%s/%s", DEVPATH, name);
> -	if (ret < 0) {
> -		printf("snprintf failed!\n");
> -		return ret;
> -	}
> +	if (ret < 0)
> +		ksft_exit_fail_msg("snprintf failed!\n");

Why not include the return value in the message?

>   
>   	fd = open(buf, O_RDWR);
>   	if (fd < 0)
> -		printf("open %s failed!\n", buf);
> +		ksft_exit_fail_msg("open %s failed: %s\n", buf, strerror(errno));
> +
>   	return fd;
>   }
>   
> @@ -140,7 +140,7 @@ static int dmabuf_sync(int fd, int start_stop)
>   
>   #define ONE_MEG (1024 * 1024)
>   
> -static int test_alloc_and_import(char *heap_name)
> +static void test_alloc_and_import(char *heap_name)
>   {
>   	int heap_fd = -1, dmabuf_fd = -1, importer_fd = -1;
>   	uint32_t handle = 0;
> @@ -148,27 +148,19 @@ static int test_alloc_and_import(char *heap_name)
>   	int ret;
>   
>   	heap_fd = dmabuf_heap_open(heap_name);
> -	if (heap_fd < 0)
> -		return -1;
>   
> -	printf("  Testing allocation and importing:  ");
> +	ksft_print_msg("Testing allocation and importing:\n");
>   	ret = dmabuf_heap_alloc(heap_fd, ONE_MEG, 0, &dmabuf_fd);
>   	if (ret) {
> -		printf("FAIL (Allocation Failed!)\n");
> -		ret = -1;
> -		goto out;
> +		ksft_test_result_fail("FAIL (Allocation Failed!)\n");

Same here/

> +		return;
>   	}
> +
>   	/* mmap and write a simple pattern */
> -	p = mmap(NULL,
> -		 ONE_MEG,
> -		 PROT_READ | PROT_WRITE,
> -		 MAP_SHARED,
> -		 dmabuf_fd,
> -		 0);
> +	p = mmap(NULL, ONE_MEG, PROT_READ | PROT_WRITE, MAP_SHARED, dmabuf_fd, 0);
>   	if (p == MAP_FAILED) {
> -		printf("FAIL (mmap() failed)\n");
> -		ret = -1;
> -		goto out;
> +		ksft_test_result_fail("FAIL (mmap() failed)\n");

Same here and the rest of the changes in this patch.

thanks,
-- Shuah