Hello Shuah,
On Mon, Sep 16, 2019 at 10:05:17AM -0600, shuah wrote:
> On 9/16/19 7:57 AM, Eugeniu Rosca wrote:
> >Hi Shuah,
> >CC George
> >
> >On Mon, Sep 16, 2019 at 07:26:41AM -0600, shuah wrote:
> >[..]
> >>> 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]);
> >>>
> >>
> >>I would like to see these combined. Please don't add another argument.
> >>Combine patch and 1&2.
> >
> >With all my appreciation for your comment, why do you think it is better
> >to get rid of the new argument? I don't think it is user-friendly to
> >always report the watchdog_info to the user. Just look at outputs [1-2]
> >and imagine that the watchdog_info part would pop up unconditionally.
> >It looks too busy to me.
>
> Yes it does look busy. I am okay with adding a second options
> based on what you both said.
>
> I don't like the commit log.
Agreed, I didn't like it either - it was rather a draft.
>
> Unfortunately this thread no longer contains the commit log.
>
> I would like to see the commit log without any references to side
> effects. It make it rather confusing.
>
> "A side of affect of commit "selftests: watchdog: Add optional file
> argument" is that arbitrary files may be opened for watchdog testing, e.g.
> /dev/null. To prevent watchdog-test from operating on non-watchdog device
> files, commit "selftests: watchdog: Validate optional file argument" was
> added to validate that a file is indeed a watchdog device via an
> ioctl(WDIOC_GETSUPPORT) call. Since the watchdog_info is available as a
> result of the ioctl(WDIOC_GETSUPPORT) call, add a command line option to
> show the watchdog_info."
>
> I would drop all references to that.
How about the following commit message for the squash commit for [1] and [2]?:
"
selftests: watchdog: Validate optional file argument
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.
"
Thanks!
>
> thanks,
> -- Shuah
--
Regards,
George
