Re: [PATCH 2/2] selftests/watchdog: convert the test output to KTAP format

[Laura Nao <laura.nao@collabora.com>]

On 6/27/24 20:41, Shuah Khan wrote:
> On 5/6/24 05:13, Laura Nao wrote:
>> Modify the script output to conform to the KTAP format standard. The
> 
> What is script here?
> 

I was referring to the watchdog-test.c file addressed in this patch. I 
understand this could be confusing, I will rephrase the commit message 
to avoid ambiguity.

>> number of tests executed is determined by the script arguments, and
>> options such as -c, -f, -h, -i, and -p do not impact the total test
>> count.
>>
>> No functional change is intended.
> 
> There are functional changes - keep_alive() coupled with changes
> tailored by a script that isn't in the kernel code which isn't
> ideal.
> 
> Why not inlcude the script in this patch series to make it part
> of the kernel?
> 

Right, I'll remove the 'no functional change is intended' sentence from 
the commit message.

Apart from the patches already in this series, no other script is 
required to run the test in a CI environment.

>>
>> Signed-off-by: Laura Nao <laura.nao@collabora.com>
>> ---
>>   .../selftests/watchdog/watchdog-test.c        | 154 ++++++++++--------
>>   1 file changed, 89 insertions(+), 65 deletions(-)
>>
>> diff --git a/tools/testing/selftests/watchdog/watchdog-test.c 
>> b/tools/testing/selftests/watchdog/watchdog-test.c
>> index 786cc5a26206..90f32de9e194 100644
>> --- a/tools/testing/selftests/watchdog/watchdog-test.c
>> +++ b/tools/testing/selftests/watchdog/watchdog-test.c
>> @@ -22,6 +22,7 @@
>>   #include <sys/ioctl.h>
>>   #include <linux/types.h>
>>   #include <linux/watchdog.h>
>> +#include "../kselftest.h"
>>   #define DEFAULT_PING_RATE    1
>>   #define DEFAULT_PING_COUNT    5
>> @@ -29,6 +30,7 @@
>>   int fd;
>>   const char v = 'V';
>>   static const char sopts[] = "bdehp:c:st:Tn:NLf:i";
>> +static const char topts[] = "bdeLn:Nst:T";
>>   static const struct option lopts[] = {
>>       {"bootstatus",          no_argument, NULL, 'b'},
>>       {"disable",             no_argument, NULL, 'd'},
>> @@ -52,7 +54,7 @@ static const struct option lopts[] = {
>>    * the PC Watchdog card to reset its internal timer so it doesn't 
>> trigger
>>    * a computer reset.
>>    */
>> -static void keep_alive(void)
>> +static int keep_alive(void)
>>   {
>>       int dummy;
>>       int ret;
>> @@ -60,6 +62,8 @@ static void keep_alive(void)
>>       ret = ioctl(fd, WDIOC_KEEPALIVE, &dummy);
>>       if (!ret)
>>           printf(".");
>> +
>> +    return ret;
>>   }
> 
> Are these changes driven by the script that isn't in the kernel code?
> I don't want to see changes to keep_alive() bevator.
> 

These changes are not driven by any external script; the aim of this
patch is just to conform the output to KTAP for easier parsing of the
results in CI environments.

Returning ret from keep_alive() allows to track the result for
the last WDIOC_KEEPALIVE ioctl and report it to the user through 
ksft_test_result, analogously to other ioctls tested in this
same file.

Thanks,

Laura