2018-09-24 19:36:48

by Jerry Hoemann

[permalink] [raw]
Subject: [V2 PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout


Add command line arguments to call ioctl WDIOC_GETTIMEOUT,
WDIOC_GETPRETIMEOUT and WDIOC_SETPRETIMEOUT.

Changes v2
1) Update usage to include argument
2) Update usage to give example.
3) Made printf of WDIOC_GETTIMEOUT distinct from WDIOC_SETTIMEOUT
4) Made WDIOC_GETTIMEOUT a "one shot"
5) Made printf of WDIOC_GETPRETIMEOUT disnct from WDIOC_SETPRETIMEOUT
6) Made WDIOC_GETPRETIMEOUT a "one shot"


Jerry Hoemann (1):
selftests: watchdog: Add gettimeout and get|set pretimeout

tools/testing/selftests/watchdog/watchdog-test.c | 33 +++++++++++++++++++++++-
1 file changed, 32 insertions(+), 1 deletion(-)

--
1.8.3.1



2018-09-24 19:38:07

by Jerry Hoemann

[permalink] [raw]
Subject: [V2 PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout

Add command line arguments to call ioctl WDIOC_GETTIMEOUT,
WDIOC_GETPRETIMEOUT and WDIOC_SETPRETIMEOUT.

Signed-off-by: Jerry Hoemann <[email protected]>
---
tools/testing/selftests/watchdog/watchdog-test.c | 33 +++++++++++++++++++++++-
1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c
index 6e29087..3e8e393 100644
--- a/tools/testing/selftests/watchdog/watchdog-test.c
+++ b/tools/testing/selftests/watchdog/watchdog-test.c
@@ -19,7 +19,7 @@

int fd;
const char v = 'V';
-static const char sopts[] = "bdehp:t:";
+static const char sopts[] = "bdehp:t:Tn:N";
static const struct option lopts[] = {
{"bootstatus", no_argument, NULL, 'b'},
{"disable", no_argument, NULL, 'd'},
@@ -27,6 +27,9 @@
{"help", no_argument, NULL, 'h'},
{"pingrate", required_argument, NULL, 'p'},
{"timeout", required_argument, NULL, 't'},
+ {"gettimeout", no_argument, NULL, 'T'},
+ {"pretimeout", required_argument, NULL, 'n'},
+ {"getpretimeout", no_argument, NULL, 'N'},
{NULL, no_argument, NULL, 0x0}
};

@@ -71,9 +74,13 @@ static void usage(char *progname)
printf(" -h, --help Print the help message\n");
printf(" -p, --pingrate=P Set ping rate to P seconds (default %d)\n", DEFAULT_PING_RATE);
printf(" -t, --timeout=T Set timeout to T seconds\n");
+ printf(" -T, --gettimeout Get the timeout\n");
+ printf(" -n, --pretimeout=T Set the pretimeout to T seconds\n");
+ printf(" -N, --getpretimeout Get the pretimeout\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);
}

int main(int argc, char *argv[])
@@ -135,6 +142,30 @@ int main(int argc, char *argv[])
else
printf("WDIOC_SETTIMEOUT errno '%s'\n", strerror(errno));
break;
+ case 'T':
+ oneshot = 1;
+ ret = ioctl(fd, WDIOC_GETTIMEOUT, &flags);
+ if (!ret)
+ printf("WDIOC_GETTIMEOUT returns %u seconds.\n", flags);
+ else
+ printf("WDIOC_GETTIMEOUT errno '%s'\n", strerror(errno));
+ 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);
+ else
+ printf("WDIOC_SETPRETIMEOUT errno '%s'\n", strerror(errno));
+ break;
+ case 'N':
+ oneshot = 1;
+ ret = ioctl(fd, WDIOC_GETPRETIMEOUT, &flags);
+ if (!ret)
+ printf("WDIOC_GETPRETIMEOUT returns %u seconds.\n", flags);
+ else
+ printf("WDIOC_GETPRETIMEOUT errno '%s'\n", strerror(errno));
+ break;
default:
usage(argv[0]);
goto end;
--
1.8.3.1


2018-09-24 19:58:27

by Shuah Khan

[permalink] [raw]
Subject: Re: [V2 PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout

On 09/24/2018 01:36 PM, Jerry Hoemann wrote:
> Add command line arguments to call ioctl WDIOC_GETTIMEOUT,
> WDIOC_GETPRETIMEOUT and WDIOC_SETPRETIMEOUT.
>
> Changes v2
> 1) Update usage to include argument
> 2) Update usage to give example.
> 3) Made printf of WDIOC_GETTIMEOUT distinct from WDIOC_SETTIMEOUT
> 4) Made WDIOC_GETTIMEOUT a "one shot"
> 5) Made printf of WDIOC_GETPRETIMEOUT disnct from WDIOC_SETPRETIMEOUT
> 6) Made WDIOC_GETPRETIMEOUT a "one shot"
>
>
> Jerry Hoemann (1):
> selftests: watchdog: Add gettimeout and get|set pretimeout
>
> tools/testing/selftests/watchdog/watchdog-test.c | 33 +++++++++++++++++++++++-
> 1 file changed, 32 insertions(+), 1 deletion(-)
>


You are a bit too quick with your v2 :) I am just about to respond to the v1. Please
look at my comments on v1. You will have to do another revision anyway to address
my comments.

thanks,
-- Shuah

2018-09-25 20:53:06

by Shuah Khan

[permalink] [raw]
Subject: Re: [V2 PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout

Hi Jerry,

On 09/24/2018 01:36 PM, Jerry Hoemann wrote:
> Add command line arguments to call ioctl WDIOC_GETTIMEOUT,
> WDIOC_GETPRETIMEOUT and WDIOC_SETPRETIMEOUT.
>
> Signed-off-by: Jerry Hoemann <[email protected]>
> ---
> tools/testing/selftests/watchdog/watchdog-test.c | 33 +++++++++++++++++++++++-
> 1 file changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c
> index 6e29087..3e8e393 100644
> --- a/tools/testing/selftests/watchdog/watchdog-test.c
> +++ b/tools/testing/selftests/watchdog/watchdog-test.c
> @@ -19,7 +19,7 @@
>
> int fd;
> const char v = 'V';
> -static const char sopts[] = "bdehp:t:";
> +static const char sopts[] = "bdehp:t:Tn:N";
> static const struct option lopts[] = {
> {"bootstatus", no_argument, NULL, 'b'},
> {"disable", no_argument, NULL, 'd'},
> @@ -27,6 +27,9 @@
> {"help", no_argument, NULL, 'h'},
> {"pingrate", required_argument, NULL, 'p'},
> {"timeout", required_argument, NULL, 't'},
> + {"gettimeout", no_argument, NULL, 'T'},
> + {"pretimeout", required_argument, NULL, 'n'},
> + {"getpretimeout", no_argument, NULL, 'N'},
> {NULL, no_argument, NULL, 0x0}
> };
>
> @@ -71,9 +74,13 @@ static void usage(char *progname)
> printf(" -h, --help Print the help message\n");
> printf(" -p, --pingrate=P Set ping rate to P seconds (default %d)\n", DEFAULT_PING_RATE);
> printf(" -t, --timeout=T Set timeout to T seconds\n");
> + printf(" -T, --gettimeout Get the timeout\n");
> + printf(" -n, --pretimeout=T Set the pretimeout to T seconds\n");
> + printf(" -N, --getpretimeout Get the pretimeout\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);

Would this work the way you want it though, since -N now is oneshot?
Should this be just "printf("Example: %s -t 12 -T -n 7\n", progname); ??

The rest looks good to me.

> }
>
> int main(int argc, char *argv[])
> @@ -135,6 +142,30 @@ int main(int argc, char *argv[])
> else
> printf("WDIOC_SETTIMEOUT errno '%s'\n", strerror(errno));
> break;
> + case 'T':
> + oneshot = 1;
> + ret = ioctl(fd, WDIOC_GETTIMEOUT, &flags);
> + if (!ret)
> + printf("WDIOC_GETTIMEOUT returns %u seconds.\n", flags);
> + else
> + printf("WDIOC_GETTIMEOUT errno '%s'\n", strerror(errno));
> + 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);
> + else
> + printf("WDIOC_SETPRETIMEOUT errno '%s'\n", strerror(errno));
> + break;
> + case 'N':
> + oneshot = 1;
> + ret = ioctl(fd, WDIOC_GETPRETIMEOUT, &flags);
> + if (!ret)
> + printf("WDIOC_GETPRETIMEOUT returns %u seconds.\n", flags);
> + else
> + printf("WDIOC_GETPRETIMEOUT errno '%s'\n", strerror(errno));
> + break;
> default:
> usage(argv[0]);
> goto end;
>

thanks,
-- Shuah

2018-09-26 16:29:52

by Jerry Hoemann

[permalink] [raw]
Subject: Re: [V2 PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout

On Tue, Sep 25, 2018 at 02:51:15PM -0600, Shuah Khan wrote:
> Hi Jerry,
>
> On 09/24/2018 01:36 PM, Jerry Hoemann wrote:
> > {"disable", no_argument, NULL, 'd'},
> > @@ -27,6 +27,9 @@
> > {"help", no_argument, NULL, 'h'},
> > {"pingrate", required_argument, NULL, 'p'},
> > {"timeout", required_argument, NULL, 't'},
> > + {"gettimeout", no_argument, NULL, 'T'},
> > + {"pretimeout", required_argument, NULL, 'n'},
> > + {"getpretimeout", no_argument, NULL, 'N'},
> > {NULL, no_argument, NULL, 0x0}
> > };
> >
> > @@ -71,9 +74,13 @@ static void usage(char *progname)
> > printf(" -h, --help Print the help message\n");
> > printf(" -p, --pingrate=P Set ping rate to P seconds (default %d)\n", DEFAULT_PING_RATE);
> > printf(" -t, --timeout=T Set timeout to T seconds\n");
> > + printf(" -T, --gettimeout Get the timeout\n");
> > + printf(" -n, --pretimeout=T Set the pretimeout to T seconds\n");
> > + printf(" -N, --getpretimeout Get the pretimeout\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);
>
> Would this work the way you want it though, since -N now is oneshot?
> Should this be just "printf("Example: %s -t 12 -T -n 7\n", progname); ??


Example shows how to set/query the timers to make sure value set was what
was intended.

Note: "oneshot" is a bit of a misnomer as it causes clean exit before
going into the keep alive loop, but one can still specify multiple
options.


>
> The rest looks good to me.
>
> > }
> >
> > int main(int argc, char *argv[])
> > @@ -135,6 +142,30 @@ int main(int argc, char *argv[])
> > else
> > printf("WDIOC_SETTIMEOUT errno '%s'\n", strerror(errno));
> > break;
> > + case 'T':
> > + oneshot = 1;
> > + ret = ioctl(fd, WDIOC_GETTIMEOUT, &flags);
> > + if (!ret)
> > + printf("WDIOC_GETTIMEOUT returns %u seconds.\n", flags);
> > + else
> > + printf("WDIOC_GETTIMEOUT errno '%s'\n", strerror(errno));
> > + 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);
> > + else
> > + printf("WDIOC_SETPRETIMEOUT errno '%s'\n", strerror(errno));
> > + break;
> > + case 'N':
> > + oneshot = 1;
> > + ret = ioctl(fd, WDIOC_GETPRETIMEOUT, &flags);
> > + if (!ret)
> > + printf("WDIOC_GETPRETIMEOUT returns %u seconds.\n", flags);
> > + else
> > + printf("WDIOC_GETPRETIMEOUT errno '%s'\n", strerror(errno));
> > + break;
> > default:
> > usage(argv[0]);
> > goto end;
> >
>
> thanks,
> -- Shuah

--

-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------

2018-09-26 19:49:21

by Shuah Khan

[permalink] [raw]
Subject: Re: [V2 PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout

On 09/26/2018 10:29 AM, Jerry Hoemann wrote:
> On Tue, Sep 25, 2018 at 02:51:15PM -0600, Shuah Khan wrote:
>> Hi Jerry,
>>
>> On 09/24/2018 01:36 PM, Jerry Hoemann wrote:
>>> {"disable", no_argument, NULL, 'd'},
>>> @@ -27,6 +27,9 @@
>>> {"help", no_argument, NULL, 'h'},
>>> {"pingrate", required_argument, NULL, 'p'},
>>> {"timeout", required_argument, NULL, 't'},
>>> + {"gettimeout", no_argument, NULL, 'T'},
>>> + {"pretimeout", required_argument, NULL, 'n'},
>>> + {"getpretimeout", no_argument, NULL, 'N'},
>>> {NULL, no_argument, NULL, 0x0}
>>> };
>>>
>>> @@ -71,9 +74,13 @@ static void usage(char *progname)
>>> printf(" -h, --help Print the help message\n");
>>> printf(" -p, --pingrate=P Set ping rate to P seconds (default %d)\n", DEFAULT_PING_RATE);
>>> printf(" -t, --timeout=T Set timeout to T seconds\n");
>>> + printf(" -T, --gettimeout Get the timeout\n");
>>> + printf(" -n, --pretimeout=T Set the pretimeout to T seconds\n");
>>> + printf(" -N, --getpretimeout Get the pretimeout\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);
>>
>> Would this work the way you want it though, since -N now is oneshot?
>> Should this be just "printf("Example: %s -t 12 -T -n 7\n", progname); ??
>
>
> Example shows how to set/query the timers to make sure value set was what
> was intended.
>
> Note: "oneshot" is a bit of a misnomer as it causes clean exit before
> going into the keep alive loop, but one can still specify multiple
> options.
>
>
>>
>> The rest looks good to me.

I spoke too soon. I ran your patch on softdog and error messages in unsupported
cases could you refinement. Please see below:

Sorry for not catching this earlier.

>>
>>> }
>>>
>>> int main(int argc, char *argv[])
>>> @@ -135,6 +142,30 @@ int main(int argc, char *argv[])
>>> else
>>> printf("WDIOC_SETTIMEOUT errno '%s'\n", strerror(errno));
>>> break;
>>> + case 'T':
>>> + oneshot = 1;
>>> + ret = ioctl(fd, WDIOC_GETTIMEOUT, &flags);
>>> + if (!ret)
>>> + printf("WDIOC_GETTIMEOUT returns %u seconds.\n", flags);
>>> + else
>>> + printf("WDIOC_GETTIMEOUT errno '%s'\n", strerror(errno));

Either remove "errno" or change it to "error '%s'"

>>> + 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);
>>> + else
>>> + printf("WDIOC_SETPRETIMEOUT errno '%s'\n", strerror(errno));

Same as above.

>>> + break;
>>> + case 'N':
>>> + oneshot = 1;
>>> + ret = ioctl(fd, WDIOC_GETPRETIMEOUT, &flags);
>>> + if (!ret)
>>> + printf("WDIOC_GETPRETIMEOUT returns %u seconds.\n", flags);
>>> + else
>>> + printf("WDIOC_GETPRETIMEOUT errno '%s'\n", strerror(errno));

Same as above.

>>> + break;
>>> default:
>>> usage(argv[0]);
>>> goto end;
>>>
>>

thanks,
-- Shuah


2018-09-26 20:03:42

by Jerry Hoemann

[permalink] [raw]
Subject: Re: [V2 PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout

On Wed, Sep 26, 2018 at 01:47:25PM -0600, Shuah Khan wrote:
> On 09/26/2018 10:29 AM, Jerry Hoemann wrote:
> > On Tue, Sep 25, 2018 at 02:51:15PM -0600, Shuah Khan wrote:
> >> Hi Jerry,
> >>
> >>
> >> The rest looks good to me.
>
> I spoke too soon. I ran your patch on softdog and error messages in unsupported
> cases could you refinement. Please see below:
>
> Sorry for not catching this earlier.
>
> >>
> >>> }
> >>>
> >>> int main(int argc, char *argv[])
> >>> @@ -135,6 +142,30 @@ int main(int argc, char *argv[])
> >>> else
> >>> printf("WDIOC_SETTIMEOUT errno '%s'\n", strerror(errno));
> >>> break;
> >>> + case 'T':
> >>> + oneshot = 1;
> >>> + ret = ioctl(fd, WDIOC_GETTIMEOUT, &flags);
> >>> + if (!ret)
> >>> + printf("WDIOC_GETTIMEOUT returns %u seconds.\n", flags);
> >>> + else
> >>> + printf("WDIOC_GETTIMEOUT errno '%s'\n", strerror(errno));
>
> Either remove "errno" or change it to "error '%s'"

Oh, I see. I did a cut/paste from prior printf in file which have same issue.
I'll fix those while I'm at it.



--

-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------

2018-09-26 21:06:05

by Shuah Khan

[permalink] [raw]
Subject: Re: [V2 PATCH] selftests: watchdog: Add gettimeout and get|set pretimeout

On 09/26/2018 02:03 PM, Jerry Hoemann wrote:
> On Wed, Sep 26, 2018 at 01:47:25PM -0600, Shuah Khan wrote:
>> On 09/26/2018 10:29 AM, Jerry Hoemann wrote:
>>> On Tue, Sep 25, 2018 at 02:51:15PM -0600, Shuah Khan wrote:
>>>> Hi Jerry,
>>>>
>>>>
>>>> The rest looks good to me.
>>
>> I spoke too soon. I ran your patch on softdog and error messages in unsupported
>> cases could you refinement. Please see below:
>>
>> Sorry for not catching this earlier.
>>
>>>>
>>>>> }
>>>>>
>>>>> int main(int argc, char *argv[])
>>>>> @@ -135,6 +142,30 @@ int main(int argc, char *argv[])
>>>>> else
>>>>> printf("WDIOC_SETTIMEOUT errno '%s'\n", strerror(errno));
>>>>> break;
>>>>> + case 'T':
>>>>> + oneshot = 1;
>>>>> + ret = ioctl(fd, WDIOC_GETTIMEOUT, &flags);
>>>>> + if (!ret)
>>>>> + printf("WDIOC_GETTIMEOUT returns %u seconds.\n", flags);
>>>>> + else
>>>>> + printf("WDIOC_GETTIMEOUT errno '%s'\n", strerror(errno));
>>
>> Either remove "errno" or change it to "error '%s'"
>
> Oh, I see. I did a cut/paste from prior printf in file which have same issue.
> I'll fix those while I'm at it.
>
>
>

Thanks. That will be awesome.

-- Shuah