Received-SPF: pass (google.com: domain of linux-kernel+bounces-120748-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <59a92e7f-321c-40e1-8e6e-1dcc0035cbed@collabora.com>
Date: Wed, 27 Mar 2024 16:25:25 +0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc: Muhammad Usama Anjum <usama.anjum@collabora.com>, kernel@collabora.com,
 linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 1/2] selftests/dmabuf-heap: conform test to TAP format
 output
Content-Language: en-US
To: Shuah Khan <skhan@linuxfoundation.org>, Shuah Khan <shuah@kernel.org>,
 "T.J. Mercier" <tjmercier@google.com>
References: <20240305060848.2478806-1-usama.anjum@collabora.com>
 <387a04c3-356e-4248-9cd3-7aa1a617c221@linuxfoundation.org>
From: Muhammad Usama Anjum <usama.anjum@collabora.com>
In-Reply-To: <387a04c3-356e-4248-9cd3-7aa1a617c221@linuxfoundation.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Hi Shuah,

Thanks for reviewing.

On 3/27/24 2:05 AM, Shuah Khan wrote:
> 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:
I'll update and send the this patch as v6.

> 
>> ---
>>   .../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
> 

-- 
BR,
Muhammad Usama Anjum