Date: Mon, 2 Apr 2007 11:20:43 -0600 From: "Coleman Kane" <zombyfork@gmail.com> To: "Wes Peters" <wes@opensail.org> Cc: src-committers@freebsd.org, John Baldwin <jhb@freebsd.org>, cvs-src@freebsd.org, cvs-all@freebsd.org, Greg 'groggy' Lehey <grog@freebsd.org>, Alexander Leidinger <Alexander@leidinger.net> Subject: Re: Giving in to Coverity (was: cvs commit: src/sys/netgraph/bluetooth/l2cap ng_l2cap_cmds.c) Message-ID: <346a80220704021020o1fe14c78se8ffaad10c2b50bd@mail.gmail.com> In-Reply-To: <3B10268D-C554-4EC2-8F16-388A49982AF3@opensail.org> References: <200703282125.l2SLPuR9058727@repoman.freebsd.org> <20070402042600.GB19923@wantadilla.lemis.com> <20070402093238.dmw2rypu40sksc0o@webmail.leidinger.net> <200704020821.15298.jhb@freebsd.org> <3B10268D-C554-4EC2-8F16-388A49982AF3@opensail.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 4/2/07, Wes Peters <wes@opensail.org> wrote: > > > 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@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@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@softweyr.com It's not so much that as it is common for a programmer to "forget" that useful return values are produced by many functions that are typically called in a fashion consistent with a void return type. They help to inform the others that are working on it that the called function does, in fact, return a value, and that this value may be helpful in debugging problems with the call or even it might be applicable to code modification being done by a second (or third) person on the project. While printf, fprintf, etc... are all simple examples to many of us, there are likely many cases where a vendor-specific function may return a value, but be called as a void. Later on, after some turn-over in staffing, a new developer inherits the code and must "learn" from it. Forcing hints like these make that job just that much easier. In addition, I have found myself in many cases where I should have checked a return value, but due to laziness I decided to add "that part" later. Of course, later came along and something else came up which caused me to forget this.... when said function call did not get the data it was expecting, errors were silently ignored and "magical" stuff began to happen. -- Coleman Kane
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?346a80220704021020o1fe14c78se8ffaad10c2b50bd>