Re: [PATCH 2/2] selftests: watchdog: Add command line option to show watchdog_info

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