Re: Coverity Open Source Defect Scan of PostgreSQL

On Thu, 9 Mar 2006, Tom Lane wrote:

> "Marc G. Fournier" <scrappy(at)postgresql(dot)org> writes:
>> Why? I don't think we are able to run 'embedded' now as it is, so its not
>> like we're dealign with system with small disk spaces :) how much bigger
>> would adding that exit() make the binary?
>
> It's not only the exit(), as the elevel parameter isn't always a
> constant. The proposed patch would at a minimum expose us to
> double-evaluation risks. I kinda doubt there are any cases where an
> elevel parameter expression has side-effects, so that objection may be
> mostly hypothetical, but nonetheless we are talking about more than just
> wasting a few bytes. It's not impossible that the patch would introduce
> outright bugs. Consider something like
>
> /* ENOENT is expected, anything else is not */
> elog(errno == ENOENT ? DEBUG : ERROR, ...)
>
> By the time control comes back from elog, errno would likely be
> different, and so this would result in an unexpected exit() call
> if the patch is in place. I'd be the first to call the above poor
> coding, but it wouldn't be a bug ... unless the errno is rechecked.
>
> It's been asserted that Coverity can be taught to understand about
> elog/ereport without this sort of hack, so I'd rather take that tack.

I realize that this might sound 'odd' ... but, would it maybe make sense
to document the code around stuff like this as to why we do it the way we
do? Basically, we're debating how we could change the code to clean
things up for Coverity's analysis, and by the fact that we're getting both
sides of the discussion, there are ppl that think that the code
could/should be changed ... the arguments against make sense, but instead
of coming back to revisit this some time in the future, documenting it in
the code as to why we are doing it this way in the first place might save
time?

----
Marc G. Fournier Hub.Org Networking Services (http://www.hub.org)
Email: scrappy(at)hub(dot)org Yahoo!: yscrappy ICQ: 7615664