diff mbox series

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

Message ID 20240506111359.224579-3-laura.nao@collabora.com (mailing list archive)
State Changes Requested
Headers show
Series Modify the watchdog selftest for execution with kselftest runner | expand

Commit Message

Laura Nao May 6, 2024, 11:13 a.m. UTC 
Modify the script output to conform to the KTAP format standard. The
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.

Signed-off-by: Laura Nao <laura.nao@collabora.com>
---
 .../selftests/watchdog/watchdog-test.c        | 154 ++++++++++--------
 1 file changed, 89 insertions(+), 65 deletions(-)

Comments

Shuah Khan June 27, 2024, 6:41 p.m. UTC | #1 
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?

> 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?

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

thanks,
-- Shuah
Laura Nao July 3, 2024, 2:49 p.m. UTC | #2 
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
diff mbox series

Patch

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;
 }
 
 /*
@@ -73,36 +77,36 @@  static void term(int sig)
 
 	close(fd);
 	if (ret < 0)
-		printf("\nStopping watchdog ticks failed (%d)...\n", errno);
+		ksft_print_msg("\nStopping watchdog ticks failed (%d)...\n", errno);
 	else
-		printf("\nStopping watchdog ticks...\n");
+		ksft_print_msg("\nStopping watchdog ticks...\n");
 	exit(0);
 }
 
 static void usage(char *progname)
 {
-	printf("Usage: %s [options]\n", progname);
-	printf(" -f, --file\t\tOpen watchdog device file\n");
-	printf("\t\t\tDefault is /dev/watchdog\n");
-	printf(" -i, --info\t\tShow watchdog_info\n");
-	printf(" -s, --status\t\tGet status & supported features\n");
-	printf(" -b, --bootstatus\tGet last boot status (Watchdog/POR)\n");
-	printf(" -d, --disable\t\tTurn off the watchdog timer\n");
-	printf(" -e, --enable\t\tTurn on the watchdog timer\n");
-	printf(" -h, --help\t\tPrint the help message\n");
-	printf(" -p, --pingrate=P\tSet ping rate to P seconds (default %d)\n",
+	ksft_print_msg("Usage: %s [options]\n", progname);
+	ksft_print_msg(" -f, --file\t\tOpen watchdog device file\n");
+	ksft_print_msg("\t\t\tDefault is /dev/watchdog\n");
+	ksft_print_msg(" -i, --info\t\tShow watchdog_info\n");
+	ksft_print_msg(" -s, --status\t\tGet status & supported features\n");
+	ksft_print_msg(" -b, --bootstatus\tGet last boot status (Watchdog/POR)\n");
+	ksft_print_msg(" -d, --disable\t\tTurn off the watchdog timer\n");
+	ksft_print_msg(" -e, --enable\t\tTurn on the watchdog timer\n");
+	ksft_print_msg(" -h, --help\t\tPrint the help message\n");
+	ksft_print_msg(" -p, --pingrate=P\tSet ping rate to P seconds (default %d)\n",
 	       DEFAULT_PING_RATE);
-	printf(" -c, --pingcount=C\tSet number of pings to C (default %d)\n",
+	ksft_print_msg(" -c, --pingcount=C\tSet number of pings to C (default %d)\n",
 	       DEFAULT_PING_COUNT);
-	printf(" -t, --timeout=T\tSet timeout to T seconds\n");
-	printf(" -T, --gettimeout\tGet the timeout\n");
-	printf(" -n, --pretimeout=T\tSet the pretimeout to T seconds\n");
-	printf(" -N, --getpretimeout\tGet the pretimeout\n");
-	printf(" -L, --gettimeleft\tGet the time left until timer expires\n");
-	printf("\n");
-	printf("Parameters are parsed left-to-right in real-time.\n");
-	printf("Example: %s -d -t 10 -p 5 -e\n", progname);
-	printf("Example: %s -t 12 -T -n 7 -N\n", progname);
+	ksft_print_msg(" -t, --timeout=T\tSet timeout to T seconds\n");
+	ksft_print_msg(" -T, --gettimeout\tGet the timeout\n");
+	ksft_print_msg(" -n, --pretimeout=T\tSet the pretimeout to T seconds\n");
+	ksft_print_msg(" -N, --getpretimeout\tGet the pretimeout\n");
+	ksft_print_msg(" -L, --gettimeleft\tGet the time left until timer expires\n");
+	ksft_print_msg("\n");
+	ksft_print_msg("Parameters are parsed left-to-right in real-time.\n");
+	ksft_print_msg("Example: %s -d -t 10 -p 5 -e\n", progname);
+	ksft_print_msg("Example: %s -t 12 -T -n 7 -N\n", progname);
 }
 
 struct wdiof_status {
@@ -128,13 +132,13 @@  static void print_status(int flags)
 	int wdiof = 0;
 
 	if (flags == WDIOS_UNKNOWN) {
-		printf("Unknown status error from WDIOC_GETSTATUS\n");
+		ksft_print_msg("Unknown status error from WDIOC_GETSTATUS\n");
 		return;
 	}
 
 	for (wdiof = 0; wdiof < WDIOF_NUM_STATUS; wdiof++) {
 		if (flags & wdiof_status[wdiof].flag)
-			printf("Support/Status: %s\n",
+			ksft_print_msg("Support/Status: %s\n",
 				wdiof_status[wdiof].status_str);
 	}
 }
@@ -156,18 +160,18 @@  static void print_boot_status(int flags)
 	int wdiof = 0;
 
 	if (flags == WDIOF_UNKNOWN) {
-		printf("Unknown flag error from WDIOC_GETBOOTSTATUS\n");
+		ksft_print_msg("Unknown flag error from WDIOC_GETBOOTSTATUS\n");
 		return;
 	}
 
 	if (flags == 0) {
-		printf("Last boot is caused by: Power-On-Reset\n");
+		ksft_print_msg("Last boot is caused by: Power-On-Reset\n");
 		return;
 	}
 
 	for (wdiof = 0; wdiof < WDIOF_NUM_BOOTSTATUS; wdiof++) {
 		if (flags & wdiof_bootstatus[wdiof].flag)
-			printf("Last boot is caused by: %s\n",
+			ksft_print_msg("Last boot is caused by: %s\n",
 				wdiof_bootstatus[wdiof].status_str);
 	}
 }
@@ -183,25 +187,28 @@  int main(int argc, char *argv[])
 	char *file = "/dev/watchdog";
 	struct watchdog_info info;
 	int temperature;
+	/* run WDIOC_KEEPALIVE test by default */
+	int test_num = 1;
 
 	setbuf(stdout, NULL);
 
 	while ((c = getopt_long(argc, argv, sopts, lopts, NULL)) != -1) {
 		if (c == 'f')
 			file = optarg;
+
+		if (strchr(topts, c))
+			test_num++;
 	}
 
 	fd = open(file, O_WRONLY);
 
 	if (fd == -1) {
 		if (errno == ENOENT)
-			printf("Watchdog device (%s) not found.\n", file);
+			ksft_exit_skip("Watchdog device (%s) not found.\n", file);
 		else if (errno == EACCES)
-			printf("Run watchdog as root.\n");
+			ksft_exit_skip("Run watchdog as root.\n");
 		else
-			printf("Watchdog device open failed %s\n",
-				strerror(errno));
-		exit(-1);
+			ksft_exit_skip("Watchdog device open failed %s\n", strerror(errno));
 	}
 
 	/*
@@ -209,13 +216,15 @@  int main(int argc, char *argv[])
 	 */
 	ret = ioctl(fd, WDIOC_GETSUPPORT, &info);
 	if (ret) {
-		printf("WDIOC_GETSUPPORT error '%s'\n", strerror(errno));
 		close(fd);
-		exit(ret);
+		ksft_exit_skip("WDIOC_GETSUPPORT error '%s'\n", strerror(errno));
 	}
 
 	optind = 0;
 
+	ksft_print_header();
+	ksft_set_plan(test_num);
+
 	while ((c = getopt_long(argc, argv, sopts, lopts, NULL)) != -1) {
 		switch (c) {
 		case 'b':
@@ -225,39 +234,42 @@  int main(int argc, char *argv[])
 			if (!ret)
 				print_boot_status(flags);
 			else
-				printf("WDIOC_GETBOOTSTATUS error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOC_GETBOOTSTATUS error '%s'\n", strerror(errno));
+			ksft_test_result(!ret, "WDIOC_GETBOOTSTATUS\n");
 			break;
 		case 'd':
 			flags = WDIOS_DISABLECARD;
 			ret = ioctl(fd, WDIOC_SETOPTIONS, &flags);
 			if (!ret)
-				printf("Watchdog card disabled.\n");
+				ksft_print_msg("Watchdog card disabled.\n");
 			else {
-				printf("WDIOS_DISABLECARD error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOS_DISABLECARD error '%s'\n", strerror(errno));
 				oneshot = 1;
 			}
+			ksft_test_result(!ret, "WDIOC_SETOPTIONS_WDIOS_DISABLECARD\n");
 			break;
 		case 'e':
 			flags = WDIOS_ENABLECARD;
 			ret = ioctl(fd, WDIOC_SETOPTIONS, &flags);
 			if (!ret)
-				printf("Watchdog card enabled.\n");
+				ksft_print_msg("Watchdog card enabled.\n");
 			else {
-				printf("WDIOS_ENABLECARD error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOS_ENABLECARD error '%s'\n", strerror(errno));
 				oneshot = 1;
 			}
+			ksft_test_result(!ret, "WDIOC_SETOPTIONS_WDIOS_ENABLECARD\n");
 			break;
 		case 'p':
 			ping_rate = strtoul(optarg, NULL, 0);
 			if (!ping_rate)
 				ping_rate = DEFAULT_PING_RATE;
-			printf("Watchdog ping rate set to %u seconds.\n", ping_rate);
+			ksft_print_msg("Watchdog ping rate set to %u seconds.\n", ping_rate);
 			break;
 		case 'c':
 			ping_count = strtoul(optarg, NULL, 0);
 			if (!ping_count)
 				ping_count = DEFAULT_PING_COUNT;
-			printf("Number of pings set to %u.\n", ping_count);
+			ksft_print_msg("Number of pings set to %u.\n", ping_count);
 			break;
 		case 's':
 			flags = 0;
@@ -266,57 +278,62 @@  int main(int argc, char *argv[])
 			if (!ret)
 				print_status(flags);
 			else
-				printf("WDIOC_GETSTATUS error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOC_GETSTATUS error '%s'\n", strerror(errno));
+			ksft_test_result(!ret, "WDIOC_GETSTATUS\n");
 			ret = ioctl(fd, WDIOC_GETTEMP, &temperature);
 			if (ret)
-				printf("WDIOC_GETTEMP: '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOC_GETTEMP: '%s'\n", strerror(errno));
 			else
-				printf("Temperature %d\n", temperature);
-
+				ksft_print_msg("Temperature %d\n", temperature);
 			break;
 		case 't':
 			flags = strtoul(optarg, NULL, 0);
 			ret = ioctl(fd, WDIOC_SETTIMEOUT, &flags);
 			if (!ret)
-				printf("Watchdog timeout set to %u seconds.\n", flags);
+				ksft_print_msg("Watchdog timeout set to %u seconds.\n", flags);
 			else {
-				printf("WDIOC_SETTIMEOUT error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOC_SETTIMEOUT error '%s'\n", strerror(errno));
 				oneshot = 1;
 			}
+			ksft_test_result(!ret, "WDIOC_SETTIMEOUT\n");
 			break;
 		case 'T':
 			oneshot = 1;
 			ret = ioctl(fd, WDIOC_GETTIMEOUT, &flags);
 			if (!ret)
-				printf("WDIOC_GETTIMEOUT returns %u seconds.\n", flags);
+				ksft_print_msg("WDIOC_GETTIMEOUT returns %u seconds.\n", flags);
 			else
-				printf("WDIOC_GETTIMEOUT error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOC_GETTIMEOUT error '%s'\n", strerror(errno));
+			ksft_test_result(!ret, "WDIOC_GETTIMEOUT\n");
 			break;
 		case 'n':
 			flags = strtoul(optarg, NULL, 0);
 			ret = ioctl(fd, WDIOC_SETPRETIMEOUT, &flags);
 			if (!ret)
-				printf("Watchdog pretimeout set to %u seconds.\n", flags);
+				ksft_print_msg("Watchdog pretimeout set to %u seconds.\n", flags);
 			else {
-				printf("WDIOC_SETPRETIMEOUT error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOC_SETPRETIMEOUT error '%s'\n", strerror(errno));
 				oneshot = 1;
 			}
+			ksft_test_result(!ret, "WDIOC_SETPRETIMEOUT\n");
 			break;
 		case 'N':
 			oneshot = 1;
 			ret = ioctl(fd, WDIOC_GETPRETIMEOUT, &flags);
 			if (!ret)
-				printf("WDIOC_GETPRETIMEOUT returns %u seconds.\n", flags);
+				ksft_print_msg("WDIOC_GETPRETIMEOUT returns %u seconds.\n", flags);
 			else
-				printf("WDIOC_GETPRETIMEOUT error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOC_GETPRETIMEOUT error '%s'\n", strerror(errno));
+			ksft_test_result(!ret, "WDIOC_GETPRETIMEOUT\n");
 			break;
 		case 'L':
 			oneshot = 1;
 			ret = ioctl(fd, WDIOC_GETTIMELEFT, &flags);
 			if (!ret)
-				printf("WDIOC_GETTIMELEFT returns %u seconds.\n", flags);
+				ksft_print_msg("WDIOC_GETTIMELEFT returns %u seconds.\n", flags);
 			else
-				printf("WDIOC_GETTIMELEFT error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOC_GETTIMELEFT error '%s'\n", strerror(errno));
+			ksft_test_result(!ret, "WDIOC_GETTIMELEFT\n");
 			break;
 		case 'f':
 			/* Handled above */
@@ -327,31 +344,38 @@  int main(int argc, char *argv[])
 			 * validation. So we just show it here.
 			 */
 			oneshot = 1;
-			printf("watchdog_info:\n");
-			printf(" identity:\t\t%s\n", info.identity);
-			printf(" firmware_version:\t%u\n",
-			       info.firmware_version);
+			ksft_print_msg("watchdog_info:\n");
+			ksft_print_msg(" identity:\t\t%s\n", info.identity);
+			ksft_print_msg(" firmware_version:\t%u\n", info.firmware_version);
 			print_status(info.options);
 			break;
 
 		default:
 			usage(argv[0]);
+			ksft_test_result_skip("WDIOC_KEEPALIVE\n");
 			goto end;
 		}
 	}
 
-	if (oneshot)
+	if (oneshot) {
+		ksft_test_result_skip("WDIOC_KEEPALIVE\n");
 		goto end;
+	}
 
-	printf("Watchdog Ticking Away!\n");
+	ksft_print_msg("Watchdog Ticking Away!\n");
+	ksft_print_msg("");
 
 	signal(SIGINT, term);
 
 	while (ping_count > 0) {
-		keep_alive();
+		ret = keep_alive();
+		if (ret)
+			break;
 		sleep(ping_rate);
 		ping_count--;
 	}
+	printf("\n");
+	ksft_test_result(!ret, "WDIOC_KEEPALIVE\n");
 end:
 	/*
 	 * Send specific magic character 'V' just in case Magic Close is
@@ -359,7 +383,7 @@  int main(int argc, char *argv[])
 	 */
 	ret = write(fd, &v, 1);
 	if (ret < 0)
-		printf("Stopping watchdog ticks failed (%d)...\n", errno);
+		ksft_print_msg("Stopping watchdog ticks failed (%d)...\n", errno);
 	close(fd);
-	return 0;
+	ksft_finished();
 }