Re: [Ksummit-discuss] bug-introducing patches

[Sasha Levin]

On Thu, May 03, 2018 at 10:17:57AM -0700, Randy Dunlap wrote:
>On 05/03/2018 08:43 AM, Sasha Levin wrote:
>> On Thu, May 03, 2018 at 08:27:48AM -0700, James Bottomley wrote:
>>> On Thu, 2018-05-03 at 15:06 +0000, Sasha Levin via Ksummit-discuss
>>> wrote:
>>>> On Thu, May 03, 2018 at 04:48:50PM +0200, Willy Tarreau wrote:
>>>>> On Thu, May 03, 2018 at 07:33:04AM -0700, James Bottomley wrote:
>>>>>> They're definitely for bug fixes, but there's a spectrum: obvious
>>>>>> bug fixes with no side effects are easy to justify.  More complex
>>>>>> bug fixes run the risk of having side effects which introduce
>>>>>> other bugs, so could potentially destabilize the -rc process.  In
>>>>>> SCSI we tend to look at what the user visible effects of the bug
>>>>>> are in the post -rc5 region and if they're slight or wouldn't be
>>>>>> visible to most users, we'll hold them over.  If the fix looks
>>>>>> complex and we're not sure we caught the ramifications, we often
>>>>>> add it to the merge window tree with a cc to stable and a note
>>>>>> saying to wait X weeks before actually adding to the
>>>>>> stable tree just to make sure no side effects show up with wider
>>>>>> testing.  So, as with most things, it's a judgment call for the
>>>>>> maintainer.
>>>>>
>>>>> For me this is the right, and responsible way to deal with bug
>>>>> fixes. Self-control is much more efficient than random rejection
>>>>> and favors a good analysis.
>>>>
>>>> I think that the ideal outcome of this discussion, at least for me,
>>>> is a tool to go under scripts/ that would allow maintainers to get
>>>> some sort of (quantifiable) data that will indicate how well the
>>>> patch was tested via the regular channels.
>>>>
>>>> At which point it's the maintainer's judgement call on whether he
>>>> wants to grab the patch or wait for more tests or reviews.
>>>>
>>>> This is very similar to what James has described, it just needs to
>>>> leave his brain and turn into code :)
>>>
>>> I appreciate the sentiment, but if we could script taste, we'd have
>>> replaced Linus with something far less cantankerous a long time ago ...
>>
>> Linus, IMO, is getting replaced. Look at how many functions he used to
>> do 10 years ago he's no longer responsible for.
>
>Agree.
>
>> One of the most obvious examples is -next, where most integration issues
>> are resolved before they even reach to Linus.
>>
>> This is good for the community, as it allows us make the process better
>> and scale out. It is also good for Linus, as I'm not sure how long he'd
>> last if he still had to edit patches by hand too often. Instead, he gets
>> to play with things that interest him more where his is irreplaceable.
>>
>>> It's also a sad fact that a lot of things which look like obvious fixes
>>> actually turn out not to be so with later testing.  This is why the
>>> user visibility test is paramount.  If a bug fix has no real user
>>> visible effects, it's often better to defer it no matter how obvious it
>>> looks, which is why the static code checkers often get short shrift
>>> before a merge window.
>>>
>>> A script measuring user visibility would be nice, but looks a bit
>>> complex ...
>>
>> It is, but I think it's worthwhile. Would something that'll show you
>> things like:
>>
>>  - How long a patch has been in -next?
>>  - How many replies/reviews/comments it got on a mailing list?
>>  - Did the 0day bot test it?
>>  - Did syzbot fuzz it? for how long?
>>  - If it references a bugzilla of some sort, how many
>>    comments/reviews/etc it got there?
>>  - Is it -stable material, or does it fix a regression in the current
>>    merge window?
>>  - If subsystem has custom testing rig, results from those tests
>>
>> be a step in the right way? is it something you'd use to make decisions
>> on whether you'd take a patch in?
>>
>
>Reminds me (too much) of checkpatch.  Sure checkpatch has its uses,
>as long as its not seen as the only true voice. (some beginners don't
>know about that yet)
>
>So with this new script, human evaluation would still be needed.
>It's just a tool.  I could be used or misused or abused.
>$maintainer still has a job to do, but having a tool could help.
>
>But be careful what you wish for.  Having such a tool could help get
>patches merged even quicker.

While checkpatch is a tool for both authors and maintainers, I'm hoping
that this tool will only be useful for maintainers, who are less likely
to abuse it. I hope.

Maintainers are still needed. I started this discussion because right
now maintainers don't scale enough, and that in turn causes both delays
and mistakes in the process. We have a bunch of tools to help patch
authors, but not as many for maintainers.

To some extent, I do wish that this will help patches get merged
earlier. If a maintainer sees that the patch spent a while in -next,
passed all his subsystem's internal testing, got a few reviews, he could
just go ahead and merge it in faster without starting to dig through his
mail client and git tree.