| Message ID | 1568659751-1845-1-git-send-email-george_davis@mentor.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] selftests: watchdog: Validate optional file argument | expand |
On 9/16/19 12:49 PM, George G. Davis wrote: > As reported by Eugeniu Rosca, a side of affect of commit c3f2490d6e92 > ("selftests: watchdog: Add optional file argument") is that arbitrary files > may be opened for watchdog testing, e.g. > You don't need to say this here since you are already have a Reported-by tag. You are missing the Fixes tag. > ./watchdog-test -f /dev/zero > Watchdog Ticking Away! > > To prevent watchdog-test from operating on non-watchdog device files, > validate that a file is indeed a watchdog device via an > ioctl(WDIOC_GETSUPPORT) call. > > While we're at it, since the watchdog_info is available as a result of the > ioctl(WDIOC_GETSUPPORT) call, add a command line option to optionally show > the watchdog_info. > Let's try this again. I want two patches. The first one with Fixes tag. The first patch might be candidate for going into stables. The -i (info) should be a separate patch. This won't go into stables. Please write a clear commit log. The following will help: https://chris.beams.io/posts/git-commit/ > Reported-by: Eugeniu Rosca <erosca@de.adit-jv.com> > Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com> > Signed-off-by: George G. Davis <george_davis@mentor.com> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > --- > v1: Applied/tested on commit ce54eab71e210f ("kunit: fix failure to build without printk") of > https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/log/?h=next > v2: Squashed [1] and [2], and update commit description as discussed in [3]. > [1] https://patchwork.kernel.org/patch/11136283/ > [2] https://patchwork.kernel.org/patch/11136285/ > [3] https://patchwork.kernel.org/patch/11136285/#22883573 > --- > tools/testing/selftests/watchdog/watchdog-test.c | 27 +++++++++++++++++++++++- > 1 file changed, 26 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c > index afff120c7be6..f45e510500c0 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:NLf:"; > +static const char sopts[] = "bdehp:t:Tn:NLf:i"; > static const struct option lopts[] = { > {"bootstatus", no_argument, NULL, 'b'}, > {"disable", no_argument, NULL, 'd'}, > @@ -32,6 +32,7 @@ static const struct option lopts[] = { > {"getpretimeout", no_argument, NULL, 'N'}, > {"gettimeleft", no_argument, NULL, 'L'}, > {"file", required_argument, NULL, 'f'}, > + {"info", no_argument, NULL, 'i'}, > {NULL, no_argument, NULL, 0x0} > }; > > @@ -72,6 +73,7 @@ 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(" -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"); > @@ -97,6 +99,7 @@ int main(int argc, char *argv[]) > int c; > int oneshot = 0; > char *file = "/dev/watchdog"; > + struct watchdog_info info; > > setbuf(stdout, NULL); > > @@ -118,6 +121,16 @@ int main(int argc, char *argv[]) > exit(-1); > } > > + /* > + * Validate that `file` is a watchdog device > + */ > + ret = ioctl(fd, WDIOC_GETSUPPORT, &info); > + if (ret) { > + printf("WDIOC_GETSUPPORT error '%s'\n", strerror(errno)); > + close(fd); > + exit(ret); > + } > + > optind = 0; > > while ((c = getopt_long(argc, argv, sopts, lopts, NULL)) != -1) { > @@ -205,6 +218,18 @@ int main(int argc, char *argv[]) > case 'f': > /* Handled above */ > break; > + case 'i': > + /* > + * watchdog_info was obtained as part of file open > + * 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); > + printf(" options:\t\t%08x\n", info.options); > + break; > > default: > usage(argv[0]); > thanks, -- Shuah
Shuah, On Mon, Sep 16, 2019 at 07:19:35PM -0600, shuah wrote: > On 9/16/19 12:49 PM, George G. Davis wrote: > > As reported by Eugeniu Rosca, a side of affect of commit c3f2490d6e92 > > ("selftests: watchdog: Add optional file argument") is that arbitrary files > > may be opened for watchdog testing, e.g. > > > > You don't need to say this here since you are already have a > Reported-by tag. This looks like asking people to stick to your personal taste which BTW doesn't really match the patterns established in Linux community. With a bit of scripting, I am able to find around 4600 vanilla commits which happen to mention the name of the reporter in addition to Reported-by: https://paste.ubuntu.com/p/wNXfdGCJbX/ . I really don't care if my name is mentioned once or twice, but I do believe that requesting a new patch revision just based on this criteria is nonsense. Can you please revise your review criteria? > You are missing the Fixes tag. The _fixed_ commit didn't land in vanilla as of v5.3-2061-gad062195731. It is still undergoing the linux-next testing, where it can be found as https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=c3f2490d6e92 ("selftests: watchdog: Add optional file argument"). Since the _fixed_ commit is not yet in mainline, there is no Fixes tag included in this patch. > > ./watchdog-test -f /dev/zero > > Watchdog Ticking Away! > > > > To prevent watchdog-test from operating on non-watchdog device files, > > validate that a file is indeed a watchdog device via an > > ioctl(WDIOC_GETSUPPORT) call. > > > > While we're at it, since the watchdog_info is available as a result of the > > ioctl(WDIOC_GETSUPPORT) call, add a command line option to optionally show > > the watchdog_info. > > > > Let's try this again. I want two patches. The first one with Fixes tag. > The first patch might be candidate for going into stables. This makes me wonder if you figured out the relationship and timeline of this and the fixed patches. Since both are going to be part of the same kernel release (v5.4), why do you worry about the stable updates? > > The -i (info) should be a separate patch. This won't go into stables. Please, see the above. I don't think this patch, or any of its parts, are candidate for linux-stable. > > Please write a clear commit log. The following will help: > > https://chris.beams.io/posts/git-commit/ With all my appreciation for https://chris.beams.io/, I see no contributions from him in Linux whatsoever. Given that Linux commit descriptions and summary lines obey specific/unique rules, I doubt this is the best guide to provide to your contributors :) With the above inputs, can you please outline your expectations precisely which changes are expected in the next patch revision, if at all needed?
On 9/17/19 8:54 AM, Eugeniu Rosca wrote: > Shuah, > > On Mon, Sep 16, 2019 at 07:19:35PM -0600, shuah wrote: >> On 9/16/19 12:49 PM, George G. Davis wrote: >>> As reported by Eugeniu Rosca, a side of affect of commit c3f2490d6e92 >>> ("selftests: watchdog: Add optional file argument") is that arbitrary files >>> may be opened for watchdog testing, e.g. >>> >> >> You don't need to say this here since you are already have a >> Reported-by tag. > > This looks like asking people to stick to your personal taste which > BTW doesn't really match the patterns established in Linux community. > > With a bit of scripting, I am able to find around 4600 vanilla commits > which happen to mention the name of the reporter in addition to > Reported-by: https://paste.ubuntu.com/p/wNXfdGCJbX/ . > > I really don't care if my name is mentioned once or twice, but I do > believe that requesting a new patch revision just based on this criteria > is nonsense. Can you please revise your review criteria? I already said what I want. I want two patches and the first one with Fixes tag. The reason for that is that the first patch fixes a problem in patch that is already in my tree which is fixes a problem. I am going to mark the patch for stables and the first patch in this series. I would like the commit log written clearly. Having a clear commit log is a critical review comment. It is important for any change to have clear commit logs for clarity and maintainability. So please send me two patches one with Fixes tag and second that has the -i support. thanks, -- Shuah
Shuah, On Tue, Sep 17, 2019 at 09:25:31AM -0600, shuah wrote: [..] > I want two patches and the first one with > Fixes tag. I am not sure we are on the same page and you don't seem to be receptive to what I say. > The reason for that is that the first patch fixes a problem > in patch that is already in my tree which is fixes a problem. Here is my understanding of your request: +--------------+ +--------------+ |1/2 this patch| |1/2 this patch| | (fix) +----+ (feature) | +------+-------+ +--------------+ | Fixes +------v-------+ | [A] | +------+-------+ | Fixes +------v-------+ | [B] | +--------------+ So, you ask to decompose this v2 patch into two parts (fix and feature), __exactly like it was in v1__, with the reasoning that the bugfix related part of this patch fixes [A] (which is true), while [A] fixes another commit [B]. But given that [A] is a feature commit, adding brand new functionality, there can't be any [B] commit being fixed by [A]. > I am going to mark the patch for stables and the first patch in this > series. I do not understand your request. Both current patch and [A] are scheduled for v5.4. I do not see any relevant patches for linux-stable. I hope either a clarification or a third opinion will shed more light onto this totally unproductive dialogue. [A] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=c3f2490d6e92 ("selftests: watchdog: Add optional file argument") https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=next&id=c3f2490d6e92 ("selftests: watchdog: Add optional file argument") [B] ???
On 9/17/19 10:54 AM, Eugeniu Rosca wrote: > Shuah, > > On Tue, Sep 17, 2019 at 09:25:31AM -0600, shuah wrote: > > [..] > >> I want two patches and the first one with >> Fixes tag. > These two patches need to be separate. The first one is a fix and the second is an enhancement. Please send two patches - the first one with Fixes tag. thanks, -- Shuah
On Tue, Sep 17, 2019 at 11:44:45AM -0600, shuah wrote: > On 9/17/19 10:54 AM, Eugeniu Rosca wrote: > > Shuah, > > > > On Tue, Sep 17, 2019 at 09:25:31AM -0600, shuah wrote: > > > > [..] > > > > > I want two patches and the first one with > > > Fixes tag. > > > > These two patches need to be separate. The first one is a fix and > the second is an enhancement. That was exactly the idea of v1. > > Please send two patches - the first one with Fixes tag. Can you please pick [v1] series from below: - https://patchwork.kernel.org/patch/11136283/ - https://patchwork.kernel.org/patch/11136285/ with adding a one-liner Fixes tag to the first patch?
On 9/17/19 11:50 AM, Eugeniu Rosca wrote: > On Tue, Sep 17, 2019 at 11:44:45AM -0600, shuah wrote: >> On 9/17/19 10:54 AM, Eugeniu Rosca wrote: >>> Shuah, >>> >>> On Tue, Sep 17, 2019 at 09:25:31AM -0600, shuah wrote: >>> >>> [..] >>> >>>> I want two patches and the first one with >>>> Fixes tag. >>> >> >> These two patches need to be separate. The first one is a fix and >> the second is an enhancement. > > That was exactly the idea of v1. > >> >> Please send two patches - the first one with Fixes tag. > > Can you please pick [v1] series from below: > - https://patchwork.kernel.org/patch/11136283/ > - https://patchwork.kernel.org/patch/11136285/ > > with adding a one-liner Fixes tag to the first patch? > Please send v2 with Fixes tag on the first one and making the changes I suggested on that v1 series. thanks, -- Shuah
Hello Shuah, On Tue, Sep 17, 2019 at 12:05:06PM -0600, shuah wrote: > On 9/17/19 11:50 AM, Eugeniu Rosca wrote: > >On Tue, Sep 17, 2019 at 11:44:45AM -0600, shuah wrote: > >>On 9/17/19 10:54 AM, Eugeniu Rosca wrote: > >>>Shuah, > >>> > >>>On Tue, Sep 17, 2019 at 09:25:31AM -0600, shuah wrote: > >>> > >>>[..] > >>> > >>>>I want two patches and the first one with > >>>>Fixes tag. > >>> > >> > >>These two patches need to be separate. The first one is a fix and > >>the second is an enhancement. > > > >That was exactly the idea of v1. > > > >> > >>Please send two patches - the first one with Fixes tag. > > > >Can you please pick [v1] series from below: > > - https://patchwork.kernel.org/patch/11136283/ > > - https://patchwork.kernel.org/patch/11136285/ > > > >with adding a one-liner Fixes tag to the first patch? > > > > Please send v2 with Fixes tag on the first one and making the changes > I suggested on that v1 series. I'll submit v3 series with the requested changes. Thanks! > > thanks, > -- Shuah
On Tue, Sep 17, 2019 at 02:32:47PM -0400, George G. Davis wrote: > Hello Shuah, > > On Tue, Sep 17, 2019 at 12:05:06PM -0600, shuah wrote: > > On 9/17/19 11:50 AM, Eugeniu Rosca wrote: > > >On Tue, Sep 17, 2019 at 11:44:45AM -0600, shuah wrote: > > >>On 9/17/19 10:54 AM, Eugeniu Rosca wrote: > > >>>Shuah, > > >>> > > >>>On Tue, Sep 17, 2019 at 09:25:31AM -0600, shuah wrote: > > >>> > > >>>[..] > > >>> > > >>>>I want two patches and the first one with > > >>>>Fixes tag. > > >>> > > >> > > >>These two patches need to be separate. The first one is a fix and > > >>the second is an enhancement. > > > > > >That was exactly the idea of v1. > > > > > >> > > >>Please send two patches - the first one with Fixes tag. > > > > > >Can you please pick [v1] series from below: > > > - https://patchwork.kernel.org/patch/11136283/ > > > - https://patchwork.kernel.org/patch/11136285/ > > > > > >with adding a one-liner Fixes tag to the first patch? > > > > > > > Please send v2 with Fixes tag on the first one and making the changes > > I suggested on that v1 series. > > I'll submit v3 series with the requested changes. Eugeniu has submitted v3 with requested changes. Thanks! > > Thanks! > > > > > thanks, > > -- Shuah > > -- > Regards, > George
(For LKML readability) Superseded by:
- https://patchwork.kernel.org/patch/11149287/
("[v3,1/2] selftests: watchdog: Validate optional file argument")
- https://patchwork.kernel.org/patch/11149289/
("[v3,2/2] selftests: watchdog: Add command line option to show watchdog_info")
diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c index afff120c7be6..f45e510500c0 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:NLf:"; +static const char sopts[] = "bdehp:t:Tn:NLf:i"; static const struct option lopts[] = { {"bootstatus", no_argument, NULL, 'b'}, {"disable", no_argument, NULL, 'd'}, @@ -32,6 +32,7 @@ static const struct option lopts[] = { {"getpretimeout", no_argument, NULL, 'N'}, {"gettimeleft", no_argument, NULL, 'L'}, {"file", required_argument, NULL, 'f'}, + {"info", no_argument, NULL, 'i'}, {NULL, no_argument, NULL, 0x0} }; @@ -72,6 +73,7 @@ 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(" -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"); @@ -97,6 +99,7 @@ int main(int argc, char *argv[]) int c; int oneshot = 0; char *file = "/dev/watchdog"; + struct watchdog_info info; setbuf(stdout, NULL); @@ -118,6 +121,16 @@ int main(int argc, char *argv[]) exit(-1); } + /* + * Validate that `file` is a watchdog device + */ + ret = ioctl(fd, WDIOC_GETSUPPORT, &info); + if (ret) { + printf("WDIOC_GETSUPPORT error '%s'\n", strerror(errno)); + close(fd); + exit(ret); + } + optind = 0; while ((c = getopt_long(argc, argv, sopts, lopts, NULL)) != -1) { @@ -205,6 +218,18 @@ int main(int argc, char *argv[]) case 'f': /* Handled above */ break; + case 'i': + /* + * watchdog_info was obtained as part of file open + * 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); + printf(" options:\t\t%08x\n", info.options); + break; default: usage(argv[0]);
As reported by Eugeniu Rosca, a side of affect of commit c3f2490d6e92 ("selftests: watchdog: Add optional file argument") is that arbitrary files may be opened for watchdog testing, e.g. ./watchdog-test -f /dev/zero Watchdog Ticking Away! To prevent watchdog-test from operating on non-watchdog device files, validate that a file is indeed a watchdog device via an ioctl(WDIOC_GETSUPPORT) call. While we're at it, since the watchdog_info is available as a result of the ioctl(WDIOC_GETSUPPORT) call, add a command line option to optionally show the watchdog_info. Reported-by: Eugeniu Rosca <erosca@de.adit-jv.com> Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com> Signed-off-by: George G. Davis <george_davis@mentor.com> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> --- v1: Applied/tested on commit ce54eab71e210f ("kunit: fix failure to build without printk") of https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/log/?h=next v2: Squashed [1] and [2], and update commit description as discussed in [3]. [1] https://patchwork.kernel.org/patch/11136283/ [2] https://patchwork.kernel.org/patch/11136285/ [3] https://patchwork.kernel.org/patch/11136285/#22883573 --- tools/testing/selftests/watchdog/watchdog-test.c | 27 +++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)