| Message ID | 1567169597-10330-1-git-send-email-george_davis@mentor.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] selftests: watchdog: Add optional file argument | expand |
Hi George, On Fri, Aug 30, 2019 at 08:53:16AM -0400, George G. Davis wrote: > Some systems have multiple watchdog devices where the first device > registered is assigned to the /dev/watchdog device file. In order > to test other watchdog devices, add an optional file argument for > selecting non-default watchdog devices for testing. > > Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com> > Signed-off-by: George G. Davis <george_davis@mentor.com> > --- > v1: > - https://lkml.org/lkml/2019/8/29/16 > v2: > - Update printf for ENOENT case based on report from Eugeniu Rosca Below interdiff [1] matches my expectations. Thanks! Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com> [1] interdiff <(git show patch_v1) <(git show patch_v2) diff -u b/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c --- b/tools/testing/selftests/watchdog/watchdog-test.c +++ b/tools/testing/selftests/watchdog/watchdog-test.c @@ -107,7 +107,7 @@ if (fd == -1) { if (errno == ENOENT) - printf("Watchdog device not enabled.\n"); + printf("Watchdog device (%s) not found.\n", file); else if (errno == EACCES) printf("Run watchdog as root.\n"); else
Hello Eugeniu, On Fri, Aug 30, 2019 at 03:38:13PM +0200, Eugeniu Rosca wrote: > Hi George, > > On Fri, Aug 30, 2019 at 08:53:16AM -0400, George G. Davis wrote: > > v2: > > - Update printf for ENOENT case based on report from Eugeniu Rosca > > Below interdiff [1] matches my expectations. Thanks! Thanks for confirming.
On 8/30/19 6:53 AM, George G. Davis wrote: > Some systems have multiple watchdog devices where the first device > registered is assigned to the /dev/watchdog device file. In order > to test other watchdog devices, add an optional file argument for > selecting non-default watchdog devices for testing. > > Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com> > Signed-off-by: George G. Davis <george_davis@mentor.com> > --- > v1: > - https://lkml.org/lkml/2019/8/29/16 > v2: > - Update printf for ENOENT case based on report from Eugeniu Rosca > --- > tools/testing/selftests/watchdog/watchdog-test.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c > index c2333c78cf04..9f17cae61007 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:Tn:NL"; > +static const char sopts[] = "bdehp:t:Tn:NLf:"; > static const struct option lopts[] = { > {"bootstatus", no_argument, NULL, 'b'}, > {"disable", no_argument, NULL, 'd'}, > @@ -31,6 +31,7 @@ static const struct option lopts[] = { > {"pretimeout", required_argument, NULL, 'n'}, > {"getpretimeout", no_argument, NULL, 'N'}, > {"gettimeleft", no_argument, NULL, 'L'}, > + {"file", required_argument, NULL, 'f'}, > {NULL, no_argument, NULL, 0x0} > }; > > @@ -69,6 +70,7 @@ static void term(int sig) > static void usage(char *progname) > { > printf("Usage: %s [options]\n", progname); > + printf(" -f, --file Open watchdog device file (default is /dev/watchdog)\n"); Can you split this line into two printf's. Checkpatch doesn't like it. printf(" -f, --file Open watchdog device file\n"); A second one below for default. On a separate note, I wish this usage block uses \t instead of spacing things out. thanks, -- Shuah
On Fri, Aug 30, 2019 at 09:12:10AM -0600, shuah wrote: > On 8/30/19 6:53 AM, George G. Davis wrote: > >diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c > >@@ -69,6 +70,7 @@ static void term(int sig) > > static void usage(char *progname) > > { > > printf("Usage: %s [options]\n", progname); > >+ printf(" -f, --file Open watchdog device file (default is /dev/watchdog)\n"); > > Can you split this line into two printf's. Checkpatch doesn't like > it. > > printf(" -f, --file Open watchdog device file\n"); > A second one below for default. Sure, I'll add the following interdiff in v3: diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c index 9f17cae61007..6a68b486dd61 100644 --- a/tools/testing/selftests/watchdog/watchdog-test.c +++ b/tools/testing/selftests/watchdog/watchdog-test.c @@ -70,7 +70,8 @@ static void term(int sig) static void usage(char *progname) { printf("Usage: %s [options]\n", progname); - printf(" -f, --file Open watchdog device file (default is /dev/watchdog)\n"); + printf(" -f, --file Open watchdog device file\n"); + printf(" Default is /dev/watchdog\n"); printf(" -b, --bootstatus Get last boot status (Watchdog/POR)\n"); printf(" -d, --disable Turn off the watchdog timer\n"); printf(" -e, --enable Turn on the watchdog timer\n"); > On a separate note, I wish this usage block uses \t instead of spacing > things out. I noticed that most of those lines are hard spaced with only one using tabs. To remain consistent with existing CodingStyle, I used hard spaces. Thanks!
On 8/30/19 10:13 AM, George G. Davis wrote: > On Fri, Aug 30, 2019 at 09:12:10AM -0600, shuah wrote: >> On 8/30/19 6:53 AM, George G. Davis wrote: >>> diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c >>> @@ -69,6 +70,7 @@ static void term(int sig) >>> static void usage(char *progname) >>> { >>> printf("Usage: %s [options]\n", progname); >>> + printf(" -f, --file Open watchdog device file (default is /dev/watchdog)\n"); >> >> Can you split this line into two printf's. Checkpatch doesn't like >> it. >> >> printf(" -f, --file Open watchdog device file\n"); >> A second one below for default. > > Sure, I'll add the following interdiff in v3: > > diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c > index 9f17cae61007..6a68b486dd61 100644 > --- a/tools/testing/selftests/watchdog/watchdog-test.c > +++ b/tools/testing/selftests/watchdog/watchdog-test.c > @@ -70,7 +70,8 @@ static void term(int sig) > static void usage(char *progname) > { > printf("Usage: %s [options]\n", progname); > - printf(" -f, --file Open watchdog device file (default is /dev/watchdog)\n"); > + printf(" -f, --file Open watchdog device file\n"); > + printf(" Default is /dev/watchdog\n"); Thanks. This is what I am looking for. Please send v3 with thsi change. > printf(" -b, --bootstatus Get last boot status (Watchdog/POR)\n"); > printf(" -d, --disable Turn off the watchdog timer\n"); > printf(" -e, --enable Turn on the watchdog timer\n"); > >> On a separate note, I wish this usage block uses \t instead of spacing >> things out. > > I noticed that most of those lines are hard spaced with only one using tabs. > To remain consistent with existing CodingStyle, I used hard spaces. > yes. My comment about using "\t" can be done later and if you would like to send for that patch, I will be happy to take it. thanks, -- Shuah
On Fri, Aug 30, 2019 at 10:20:23AM -0600, shuah wrote: > On 8/30/19 10:13 AM, George G. Davis wrote: > >On Fri, Aug 30, 2019 at 09:12:10AM -0600, shuah wrote: > >>Can you split this line into two printf's. Checkpatch doesn't like > >>it. > >> > >Sure, I'll add the following interdiff in v3: > > Thanks. This is what I am looking for. Please send v3 with thsi change. Done. > >>On a separate note, I wish this usage block uses \t instead of spacing > >>things out. > > > >I noticed that most of those lines are hard spaced with only one using tabs. > >To remain consistent with existing CodingStyle, I used hard spaces. > > > > yes. My comment about using "\t" can be done later and if you would like > to send for that patch, I will be happy to take it. I'll send that along in a separate thread. Thanks!
diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c index c2333c78cf04..9f17cae61007 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:Tn:NL"; +static const char sopts[] = "bdehp:t:Tn:NLf:"; static const struct option lopts[] = { {"bootstatus", no_argument, NULL, 'b'}, {"disable", no_argument, NULL, 'd'}, @@ -31,6 +31,7 @@ static const struct option lopts[] = { {"pretimeout", required_argument, NULL, 'n'}, {"getpretimeout", no_argument, NULL, 'N'}, {"gettimeleft", no_argument, NULL, 'L'}, + {"file", required_argument, NULL, 'f'}, {NULL, no_argument, NULL, 0x0} }; @@ -69,6 +70,7 @@ static void term(int sig) static void usage(char *progname) { printf("Usage: %s [options]\n", progname); + printf(" -f, --file Open watchdog device file (default is /dev/watchdog)\n"); printf(" -b, --bootstatus Get last boot status (Watchdog/POR)\n"); printf(" -d, --disable Turn off the watchdog timer\n"); printf(" -e, --enable Turn on the watchdog timer\n"); @@ -92,14 +94,20 @@ int main(int argc, char *argv[]) int ret; int c; int oneshot = 0; + char *file = "/dev/watchdog"; setbuf(stdout, NULL); - fd = open("/dev/watchdog", O_WRONLY); + while ((c = getopt_long(argc, argv, sopts, lopts, NULL)) != -1) { + if (c == 'f') + file = optarg; + } + + fd = open(file, O_WRONLY); if (fd == -1) { if (errno == ENOENT) - printf("Watchdog device not enabled.\n"); + printf("Watchdog device (%s) not found.\n", file); else if (errno == EACCES) printf("Run watchdog as root.\n"); else @@ -108,6 +116,8 @@ int main(int argc, char *argv[]) exit(-1); } + optind = 0; + while ((c = getopt_long(argc, argv, sopts, lopts, NULL)) != -1) { switch (c) { case 'b': @@ -190,6 +200,9 @@ int main(int argc, char *argv[]) else printf("WDIOC_GETTIMELEFT error '%s'\n", strerror(errno)); break; + case 'f': + /* Handled above */ + break; default: usage(argv[0]);