Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 2/2] selftests: watchdog: Add command line option to show
 watchdog_info
To:     Eugeniu Rosca <erosca@de.adit-jv.com>
Cc:     Eugeniu Rosca <roscaeugeniu@gmail.com>,
        "George G. Davis" <george_davis@mentor.com>,
        Jerry Hoemann <jerry.hoemann@hpe.com>,
        Colin Ian King <colin.king@canonical.com>,
        linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
        shuah <shuah@kernel.org>
References: <20190907085833.21167-1-erosca@de.adit-jv.com>
 <20190907085833.21167-2-erosca@de.adit-jv.com>
 <2b08fa83-185b-4eb7-1217-37ed31d810b5@kernel.org>
 <20190916135756.GA24319@vmlxhi-102.adit-jv.com>
From:   shuah <shuah@kernel.org>
Message-ID: <24cc0dde-6df3-3ea2-3e1d-0bc166bb48ac@kernel.org>
Date:   Mon, 16 Sep 2019 10:05:17 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20190916135756.GA24319@vmlxhi-102.adit-jv.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

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.

thanks,
-- Shuah