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

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(-)

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

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