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

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