[PATCH v2 0/2] Modify the watchdog selftest for execution in non-interactive environments

[PATCH v2 0/2] Modify the watchdog selftest for execution in non-interactive environments

[Laura Nao]

This series is a follow-up to v1[1], aimed at making the watchdog selftest
more suitable for CI environments. Currently, in non-interactive setups,
the watchdog kselftest can only run with oneshot parameters, preventing the
testing of the WDIOC_KEEPALIVE ioctl since the ping loop is only
interrupted by SIGINT.

The first patch adds a new -c option to limit the number of watchdog pings,
allowing the test to be optionally finite. The second patch updates the
test output to conform to KTAP.

The default behavior remains unchanged: without the -c option, the
keep_alive() loop continues indefinitely until interrupted by SIGINT.

[1] https://lore.kernel.org/all/20240506111359.224579-1-laura.nao@collabora.com/

Changes in v2:
- The keep_alive() loop remains infinite by default
- Introduced keep_alive_res variable to track the WDIOC_KEEPALIVE ioctl return code for user reporting

Laura Nao (2):
  selftests/watchdog: add -c option to limit the ping loop
  selftests/watchdog: convert the test output to KTAP format

 .../selftests/watchdog/watchdog-test.c        | 169 +++++++++++-------
 1 file changed, 103 insertions(+), 66 deletions(-)

-- 
2.30.2

[PATCH v2 1/2] selftests/watchdog: add -c option to limit the ping loop

[Laura Nao]

In order to run the watchdog selftest in a non-interactive environment,
the loop responsible for pinging the watchdog should be finite.
Introduce a new '-c' option to adjust the number of pings as needed.

Signed-off-by: Laura Nao <laura.nao@collabora.com>
---
 tools/testing/selftests/watchdog/watchdog-test.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c
index bc71cbca0dde..58c25015d5e7 100644
--- a/tools/testing/selftests/watchdog/watchdog-test.c
+++ b/tools/testing/selftests/watchdog/watchdog-test.c
@@ -27,13 +27,14 @@
 
 int fd;
 const char v = 'V';
-static const char sopts[] = "bdehp:st:Tn:NLf:i";
+static const char sopts[] = "bdehp:c:st:Tn:NLf:i";
 static const struct option lopts[] = {
 	{"bootstatus",          no_argument, NULL, 'b'},
 	{"disable",             no_argument, NULL, 'd'},
 	{"enable",              no_argument, NULL, 'e'},
 	{"help",                no_argument, NULL, 'h'},
 	{"pingrate",      required_argument, NULL, 'p'},
+	{"pingcount",     required_argument, NULL, 'c'},
 	{"status",              no_argument, NULL, 's'},
 	{"timeout",       required_argument, NULL, 't'},
 	{"gettimeout",          no_argument, NULL, 'T'},
@@ -90,6 +91,7 @@ static void usage(char *progname)
 	printf(" -h, --help\t\tPrint the help message\n");
 	printf(" -p, --pingrate=P\tSet ping rate to P seconds (default %d)\n",
 	       DEFAULT_PING_RATE);
+	printf(" -c, --pingcount=C\tLimit the number of pings to C (default infinite)\n");
 	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");
@@ -172,6 +174,7 @@ int main(int argc, char *argv[])
 {
 	int flags;
 	unsigned int ping_rate = DEFAULT_PING_RATE;
+	unsigned int ping_count = -1;
 	int ret;
 	int c;
 	int oneshot = 0;
@@ -248,6 +251,12 @@ int main(int argc, char *argv[])
 				ping_rate = DEFAULT_PING_RATE;
 			printf("Watchdog ping rate set to %u seconds.\n", ping_rate);
 			break;
+		case 'c':
+			ping_count = strtoul(optarg, NULL, 0);
+			if (!ping_count)
+				oneshot = 1;
+			printf("Number of pings set to %u.\n", ping_count);
+			break;
 		case 's':
 			flags = 0;
 			oneshot = 1;
@@ -336,9 +345,11 @@ int main(int argc, char *argv[])
 
 	signal(SIGINT, term);
 
-	while (1) {
+	while (ping_count != 0) {
 		keep_alive();
 		sleep(ping_rate);
+		if (ping_count > 0)
+			ping_count--;
 	}
 end:
 	/*
-- 
2.30.2

Re: [PATCH v2 1/2] selftests/watchdog: add -c option to limit the ping loop

[Shuah Khan]

On 11/19/24 08:01, Laura Nao wrote:
> In order to run the watchdog selftest in a non-interactive environment,
> the loop responsible for pinging the watchdog should be finite.
> Introduce a new '-c' option to adjust the number of pings as needed.
> 
> Signed-off-by: Laura Nao <laura.nao@collabora.com>
> ---
>   tools/testing/selftests/watchdog/watchdog-test.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c
> index bc71cbca0dde..58c25015d5e7 100644
> --- a/tools/testing/selftests/watchdog/watchdog-test.c
> +++ b/tools/testing/selftests/watchdog/watchdog-test.c
> @@ -27,13 +27,14 @@
>   
>   int fd;
>   const char v = 'V';
> -static const char sopts[] = "bdehp:st:Tn:NLf:i";
> +static const char sopts[] = "bdehp:c:st:Tn:NLf:i";
>   static const struct option lopts[] = {
>   	{"bootstatus",          no_argument, NULL, 'b'},
>   	{"disable",             no_argument, NULL, 'd'},
>   	{"enable",              no_argument, NULL, 'e'},
>   	{"help",                no_argument, NULL, 'h'},
>   	{"pingrate",      required_argument, NULL, 'p'},
> +	{"pingcount",     required_argument, NULL, 'c'},
>   	{"status",              no_argument, NULL, 's'},
>   	{"timeout",       required_argument, NULL, 't'},
>   	{"gettimeout",          no_argument, NULL, 'T'},
> @@ -90,6 +91,7 @@ static void usage(char *progname)
>   	printf(" -h, --help\t\tPrint the help message\n");
>   	printf(" -p, --pingrate=P\tSet ping rate to P seconds (default %d)\n",
>   	       DEFAULT_PING_RATE);
> +	printf(" -c, --pingcount=C\tLimit the number of pings to C (default infinite)\n");
>   	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");
> @@ -172,6 +174,7 @@ int main(int argc, char *argv[])
>   {
>   	int flags;
>   	unsigned int ping_rate = DEFAULT_PING_RATE;
> +	unsigned int ping_count = -1;

Assigning -1 to unsigned?

>   	int ret;
>   	int c;
>   	int oneshot = 0;
> @@ -248,6 +251,12 @@ int main(int argc, char *argv[])
>   				ping_rate = DEFAULT_PING_RATE;
>   			printf("Watchdog ping rate set to %u seconds.\n", ping_rate);
>   			break;
> +		case 'c':
> +			ping_count = strtoul(optarg, NULL, 0);

strtoul() returns ULONG_MAX if there are errors.Don't you have
to handle those cases? Also ping_count in unsigned int? Do you
see compile warns?


> +			if (!ping_count)
> +				oneshot = 1;

Why not just "goto end" at this point?

> +			printf("Number of pings set to %u.\n", ping_count);> +			break;
>   		case 's':
>   			flags = 0;
>   			oneshot = 1;
> @@ -336,9 +345,11 @@ int main(int argc, char *argv[])
>   
>   	signal(SIGINT, term);
>   
> -	while (1) {
> +	while (ping_count != 0) {
>   		keep_alive();
>   		sleep(ping_rate);
> +		if (ping_count > 0)
> +			ping_count--;

Did you test this with strtoul() failed case when the return
could be ULONG_MAX?

>   	}
>   end:
>   	/*

thanks,
-- Shuah

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

[Laura Nao]

Conform the test output 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.

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

diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c
index 58c25015d5e7..4781736070e3 100644
--- a/tools/testing/selftests/watchdog/watchdog-test.c
+++ b/tools/testing/selftests/watchdog/watchdog-test.c
@@ -22,12 +22,15 @@
 #include <sys/ioctl.h>
 #include <linux/types.h>
 #include <linux/watchdog.h>
+#include "../kselftest.h"
 
 #define DEFAULT_PING_RATE	1
 
 int fd;
+int keep_alive_res;
 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'},
@@ -51,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;
@@ -59,6 +62,8 @@ static void keep_alive(void)
 	ret = ioctl(fd, WDIOC_KEEPALIVE, &dummy);
 	if (!ret)
 		printf(".");
+
+	return ret;
 }
 
 /*
@@ -72,35 +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");
-	exit(0);
+		ksft_print_msg("\nStopping watchdog ticks...\n");
+	ksft_test_result(!keep_alive_res, "WDIOC_KEEPALIVE\n");
+	ksft_finished();
 }
 
 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\tLimit the number of pings to C (default infinite)\n");
-	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(" -c, --pingcount=C\tSet number of pings to C (default infinite)\n");
+	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 {
@@ -126,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);
 	}
 }
@@ -154,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);
 	}
 }
@@ -181,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));
 	}
 
 	/*
@@ -207,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':
@@ -223,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_perror("WDIOC_GETBOOTSTATUS error");
+			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_perror("WDIOS_DISABLECARD error");
 				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_perror("WDIOS_ENABLECARD error");
 				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)
 				oneshot = 1;
-			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;
@@ -264,57 +278,62 @@ int main(int argc, char *argv[])
 			if (!ret)
 				print_status(flags);
 			else
-				printf("WDIOC_GETSTATUS error '%s'\n", strerror(errno));
+				ksft_perror("WDIOC_GETSTATUS error");
+			ksft_test_result(!ret, "WDIOC_GETSTATUS\n");
 			ret = ioctl(fd, WDIOC_GETTEMP, &temperature);
 			if (ret)
-				printf("WDIOC_GETTEMP: '%s'\n", strerror(errno));
+				ksft_perror("WDIOC_GETTEMP");
 			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_perror("WDIOC_SETTIMEOUT error");
 				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_perror("WDIOC_GETTIMEOUT error");
+			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_perror("WDIOC_SETPRETIMEOUT error");
 				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_perror("WDIOC_GETPRETIMEOUT error");
+			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_perror("WDIOC_GETTIMELEFT error");
+			ksft_test_result(!ret, "WDIOC_GETTIMELEFT\n");
 			break;
 		case 'f':
 			/* Handled above */
@@ -325,32 +344,39 @@ 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();
+		if (keep_alive())
+			keep_alive_res = -1;
+
 		sleep(ping_rate);
 		if (ping_count > 0)
 			ping_count--;
 	}
+	printf("\n");
+	ksft_test_result(!keep_alive_res, "WDIOC_KEEPALIVE\n");
 end:
 	/*
 	 * Send specific magic character 'V' just in case Magic Close is
@@ -358,7 +384,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();
 }
-- 
2.30.2

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

[Shuah Khan]

On 11/19/24 08:01, Laura Nao wrote:
> Conform the test output 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.

Didn't I review this patche before and say I don't see value
in converting the output to ktap.

This conevts all messages including usage to ktap which isn';t
necessary. It also introduces skips for oneshot cases which is
miesleading - if user asks for oneshot, there is not value in
marking the test skipped.

I am not going to take this patch.

> 
> Signed-off-by: Laura Nao <laura.nao@collabora.com>
> ---
>   .../selftests/watchdog/watchdog-test.c        | 158 ++++++++++--------
>   1 file changed, 92 insertions(+), 66 deletions(-)
> 
> diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c
> index 58c25015d5e7..4781736070e3 100644
> --- a/tools/testing/selftests/watchdog/watchdog-test.c
> +++ b/tools/testing/selftests/watchdog/watchdog-test.c
> @@ -22,12 +22,15 @@
>   #include <sys/ioctl.h>
>   #include <linux/types.h>
>   #include <linux/watchdog.h>
> +#include "../kselftest.h"
>   
>   #define DEFAULT_PING_RATE	1
>   
>   int fd;
> +int keep_alive_res;
>   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'},
> @@ -51,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;
> @@ -59,6 +62,8 @@ static void keep_alive(void)
>   	ret = ioctl(fd, WDIOC_KEEPALIVE, &dummy);
>   	if (!ret)
>   		printf(".");
> +
> +	return ret;
>   }
>   
>   /*
> @@ -72,35 +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");
> -	exit(0);
> +		ksft_print_msg("\nStopping watchdog ticks...\n");
> +	ksft_test_result(!keep_alive_res, "WDIOC_KEEPALIVE\n");
> +	ksft_finished();
>   }
>   
>   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\tLimit the number of pings to C (default infinite)\n");
> -	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(" -c, --pingcount=C\tSet number of pings to C (default infinite)\n");
> +	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);
>   }
>   

No - we don't need ksft_ outout for usage.

>   struct wdiof_status {
> @@ -126,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);
>   	}
>   }
> @@ -154,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);
>   	}
>   }
> @@ -181,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));
>   	}
>   
>   	/*
> @@ -207,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':
> @@ -223,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_perror("WDIOC_GETBOOTSTATUS error");
> +			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_perror("WDIOS_DISABLECARD error");
>   				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_perror("WDIOS_ENABLECARD error");
>   				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)
>   				oneshot = 1;
> -			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;
> @@ -264,57 +278,62 @@ int main(int argc, char *argv[])
>   			if (!ret)
>   				print_status(flags);
>   			else
> -				printf("WDIOC_GETSTATUS error '%s'\n", strerror(errno));
> +				ksft_perror("WDIOC_GETSTATUS error");
> +			ksft_test_result(!ret, "WDIOC_GETSTATUS\n");
>   			ret = ioctl(fd, WDIOC_GETTEMP, &temperature);
>   			if (ret)
> -				printf("WDIOC_GETTEMP: '%s'\n", strerror(errno));
> +				ksft_perror("WDIOC_GETTEMP");
>   			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_perror("WDIOC_SETTIMEOUT error");
>   				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_perror("WDIOC_GETTIMEOUT error");
> +			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_perror("WDIOC_SETPRETIMEOUT error");
>   				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_perror("WDIOC_GETPRETIMEOUT error");
> +			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_perror("WDIOC_GETTIMELEFT error");
> +			ksft_test_result(!ret, "WDIOC_GETTIMELEFT\n");
>   			break;
>   		case 'f':
>   			/* Handled above */
> @@ -325,32 +344,39 @@ 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");

Why skip here - user asked for oneshot. This doesn't look right.
>   		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();
> +		if (keep_alive())
> +			keep_alive_res = -1;
> +
>   		sleep(ping_rate);
>   		if (ping_count > 0)
>   			ping_count--;
>   	}
> +	printf("\n");
> +	ksft_test_result(!keep_alive_res, "WDIOC_KEEPALIVE\n");
>   end:
>   	/*
>   	 * Send specific magic character 'V' just in case Magic Close is
> @@ -358,7 +384,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();
>   }

thanks,
-- Shuah

Re: [PATCH v2 0/2] Modify the watchdog selftest for execution in non-interactive environments

[Shuah Khan]

On 11/19/24 08:01, Laura Nao wrote:
> This series is a follow-up to v1[1], aimed at making the watchdog selftest
> more suitable for CI environments. Currently, in non-interactive setups,
> the watchdog kselftest can only run with oneshot parameters, preventing the
> testing of the WDIOC_KEEPALIVE ioctl since the ping loop is only
> interrupted by SIGINT.
> 
> The first patch adds a new -c option to limit the number of watchdog pings,
> allowing the test to be optionally finite. The second patch updates the
> test output to conform to KTAP.
> 
> The default behavior remains unchanged: without the -c option, the
> keep_alive() loop continues indefinitely until interrupted by SIGINT.
> 
> [1] https://lore.kernel.org/all/20240506111359.224579-1-laura.nao@collabora.com/
> 
> Changes in v2:
> - The keep_alive() loop remains infinite by default
> - Introduced keep_alive_res variable to track the WDIOC_KEEPALIVE ioctl return code for user reporting
> 
> Laura Nao (2):
>    selftests/watchdog: add -c option to limit the ping loop
>    selftests/watchdog: convert the test output to KTAP format

I think we discussed this conversion before and I still don't see
the need for this especially this patch converts every single
print() into ktap which isn't necessary.

> 
>   .../selftests/watchdog/watchdog-test.c        | 169 +++++++++++-------
>   1 file changed, 103 insertions(+), 66 deletions(-)
> 

These don't apply on 6.13 or 6.12 - rebase and resend after
making changes to address comments on individual patches.

thanks,
-- Shuah