Re: [PATCH v2] selftests: watchdog: Validate optional file argument

[Eugeniu Rosca]

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?

-- 
Best Regards,
Eugeniu