Giving in to Coverity (was: cvs commit: src/sys/netgraph/bluetooth/l2cap ng_l2cap_cmds.c)

[Wes Peters]

On Apr 2, 2007, at 5:21 AM, John Baldwin wrote:

> On Monday 02 April 2007 03:32:38 am Alexander Leidinger wrote:
>> Quoting Greg 'groggy' Lehey <grog at FreeBSD.org> (from Mon, 2 Apr 2007
>> 13:56:00 +0930):
>>
>>> On Thursday, 29 March 2007 at 13:36:31 +0200, Alexander Leidinger  
>>> wrote:
>>>> Quoting Andrew Thompson <thompsa at freebsd.org> (from Thu, 29 Mar  
>>>> 2007
>>>> 13:52:12 +1200):
>>>>
>>>>> On Thu, Mar 29, 2007 at 10:58:34AM +0930, Greg 'groggy' Lehey  
>>>>> wrote:
>>>>>> On Wednesday, 28 March 2007 at 21:25:56 +0000, Maksim  
>>>>>> Yevmenkin wrote:
>>>>>>> emax        2007-03-28 21:25:56 UTC
>>>>>>>
>>>>>>>   FreeBSD src repository
>>>>>>>
>>>>>>>   Modified files:
>>>>>>>     sys/netgraph/bluetooth/l2cap ng_l2cap_cmds.c
>>>>>>>   Log:
>>>>>>>   Try to silence Coverity by adding (void) in front of  
>>>>>>> function call.
>>>>>>>   Also add a comment, explaining why return value is not being
> checked.
>>>>>>
>>>>>> I hope Coverity isn't going to force us to add unnecessary  
>>>>>> casts to
>>>>>> function calls.
>>>>>
>>>>> Well no, you can always silence Coverity by just marking it as  
>>>>> a false
>>>>> bug.
>>>>
>>>> Maxim and me discussed this briefly before this commit.
>>>>
>>>> ...
>>>>
>>>> The cast does not obfuscate the code, doesn't make it harder to  
>>>> read ...
>>>
>>> I've dropped the rest of your argumentation, because I don't  
>>> disagree
>>> with it, but I do think that unnecessary casts cause (minor)
>>> obfuscation and make it (fractionally) more difficult to read.
>>>
>>> My concern is that we shouldn't compromise our style because of bugs
>>> in program checkers.  I understand that there are alternatives, like
>>> flagging it for Coverity as "OK", and I'd expect that to be the
>>> preferable solution.  But I'm not the guardian of style, so I'll let
>>> others decide on this if they care.
>>
>> There are several cases where Coverity gets something wrong (e.g. the
>> use of TAILQ). I did mark those as invalid in Coverity (until either
>> we get a new version of Coverity which understands this, or someone
>> writes a model of the TAILQ stuff for Coverity, or until someone  
>> tells
>> me to mark them as false positives). I did this because I don't know
>> how to fix this in our code _and_ I see no benefit in fixing this in
>> our code just to make Coverity not moan. For the void cast we are
>> talking about I see a benefit. Coverity can count this as "the return
>> value of this function is checked". As such a report is only  
>> generated
>> if a specific percentage of the use of a function is handled this  
>> way,
>> it is important if we want to get reports for this. And we want to  
>> get
>> reports for functions where the return value typically has to be
>> checked.
>
> There is previous history of casting a function's return value to  
> (void) to
> please lint(1).  Just look for '(void)printf' :)  Coverity at least is
> smarter than lint as it doesn't warn about printf not being checked.

Void casts are a valuable tool, they tell the next poor dumb  
programmer who comes along "I know this returns a value but I'm too  
lazy or stupid to check it."  I find I often get bitten by these, in  
my own code and in others.

I mean, it's not like printf can overflow your buffer or anything, is  
it?

--
            Where am I, and what am I doing in this handbasket?
Wes Peters                                                      
wes at softweyr.com