A quick guide to why stand-alone checkpatch patches suck...

[Greg Freemyer]

On September 17, 2014 8:09:36 AM EDT, nick <xerofoify at gmail.com> wrote:
>
>
>On 14-09-17 08:05 AM, Greg Freemyer wrote:
>> 
>> 
>> On September 17, 2014 7:53:24 AM EDT, nick <xerofoify at gmail.com>
>wrote:
>>>
>>>
>>> On 14-09-17 07:51 AM, Sudip Mukherjee wrote:
>>>> On Wed, Sep 17, 2014 at 5:08 PM, nick <xerofoify at gmail.com> wrote:
>>>>>
>>>>>
>>>>> On 14-09-17 07:20 AM, Robert P. J. Day wrote:
>>>> <snip>
>>>>>>   anyway, it's time for coffee.
>>>>>>
>>>>>> rday
>>>>>>
>>>>> Rday and others,
>>>>> That's not what I wanted I was trying to improve my rep after
>>> getting banned from vger.org and now it seems
>>>>> I can't even get a patch right. In addition I was trying to do
>check
>>> patch because  it was easier for me
>>>>> due to not understanding some parts of the code.
>>>>> Nick
>>>>>
>>>>
>>>> try to understand the code first. if you do not understand the code
>>>> how do you know that your patch will not break any part of the
>logic
>>> .
>>>> ok , by adding blank lines you will not break the logic.
>>>> but yesterday in your other patch you removed an error message .
>may
>>> i
>>>> ask why did you think that error message is not required ?
>>>>
>>>> thanks
>>>> sudip
>>>>
>>>>> _______________________________________________
>>>>> Kernelnewbies mailing list
>>>>> Kernelnewbies at kernelnewbies.org
>>>>> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
>>> I thought that the return statement of NULL to a caller was enough.
>>> Nick 
>> 
>> Uh...
>> 
>> I don't know that chunk of code, but error messages that go to the
>kernel log exist for a specific reason.  Taking them out requires a
>specific reason.
>> 
>> Ie. This would make a good commit message "At this point the
>condition is well understood and the code that handles it is well
>tested and has been stable for 3 years, thus removing the error
>message."
>> 
>> Greg
>> 
>Thanks Greg Again,
>This is what I meant with my patch, why have a unneeded error message
>if the code is already tested and only uses
>the return value in that function.
>Cheers Nick 

Because in general we don't use asserts in the kernel. I'm sure I've used 10,000s of asserts in user space over the decades.  Zero in the kernel.

Specifically, in user space when writing code we can put asserts throughout the code that will cause an immediate code explosion if unexpected things happen.  In the kernel, the better choice is printing an error message then have the code do it's best to handle it.

That still begs the question of why it happened in the first place.  As long as the event itself us unexpected (ie. not routine) then the error message should remain.  Re-read the sample commit message I wrote.  The first thing I said is the "condition is well understood".  Never remove an error message unless you can explain with clarity why the "error" is happening.   Obviously in that case you should be replacing the error message with a comment that explains the condition.

Greg
-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.