Thread: Coverity Open Source Defect Scan of PostgreSQL
Hello PostgreSQL Developers, I'm the CTO of Coverity, Inc., a company that does static source code analysis to look for defects in code. You may have heard of us or of our technology from its days at Stanford (the "Stanford Checker"). The reason I'm writing is because we have set up a framework internally to continually scan open source projects and provide the results of our analysis back to the developers of those projects. PostgreSQL is one of the 32 projects currently scanned at: http://scan.coverity.com My belief is that we (Coverity) must reach out to the developers of these packages (you) in order to make progress in actually fixing the defects that we happen to find, so this is my first step in that mission. Of course, I think Coverity technology is great, but I want to hear what you think and that's why I worked with folks at Coverity to put this infrastructure in place. The process is simple -- it checks out your code each night from your repository and scans it so you can always see the latest results. Right now, we're guarding access to the actual defects that we report for a couple of reasons: (1) We think that you, as developers of PostgreSQL, should have the chance to look at the defects we find to patch them before random other folks get to see what we found and (2) From a support perspective, we want to make sure thatwe have the appropriate time to engage with those who want to use the results to fix the code. Because of this second point, I'd ask that if you are interested in really digging into the results a bit further for your project, please have a couple of core maintainers (or group nominated individuals) reach out to me to request access. As this is a new process for us and still involves a small number of packages, I want to make sure that I personally can be involved with the activity that is generated from this effort. So I'm basically asking for people who want to play around with some cool new technology to help make source code better. If this interests you, please feel free to reach out to me directly. And of course, if there are other packages you care about that aren't currently on the list, I want to know about those too. If this is the wrong list, my sincerest apologies and please let me know where would be a more appropriate forum for this type of message. Many thanks for reading this far... -ben Ben Chelf Chief Technology Officer Coverity, Inc.
Ben Chelf wrote: > Hello PostgreSQL Developers, > > I'm the CTO of Coverity, Inc., a company that does static source code > analysis to look for defects in code. You may have heard of us or of our > technology from its days at Stanford (the "Stanford Checker"). The > reason I'm writing is because we have set up a framework internally to > continually scan open source projects and provide the results of our > analysis back to the developers of those projects. PostgreSQL is one of > the 32 projects currently scanned at: > > http://scan.coverity.com Hm, interestingly and in contrast to some announcements, MySQL is not included in this list. Did it blast the defects column ? :-) Regards, Andreas
Andreas Pflug wrote: > Ben Chelf wrote: > > Hello PostgreSQL Developers, > > > > I'm the CTO of Coverity, Inc., a company that does static source code > > analysis to look for defects in code. You may have heard of us or of our > > technology from its days at Stanford (the "Stanford Checker"). The > > reason I'm writing is because we have set up a framework internally to > > continually scan open source projects and provide the results of our > > analysis back to the developers of those projects. PostgreSQL is one of > > the 32 projects currently scanned at: > > > > http://scan.coverity.com > > Hm, interestingly and in contrast to some announcements, MySQL is not > included in this list. Did it blast the defects column ? :-) I thought we ran the Converity analysis a year ago and cleaned up the warnings, so I am surprised at our high number, but I assume they are mostly noise. -- Bruce Momjian http://candle.pha.pa.us SRA OSS, Inc. http://www.sraoss.com + If your life is a hard drive, Christ can be your backup. +
Andreas Pflug wrote: > Ben Chelf wrote: > >Hello PostgreSQL Developers, > > > > I'm the CTO of Coverity, Inc., a company that does static source code > >analysis to look for defects in code. You may have heard of us or of our > >technology from its days at Stanford (the "Stanford Checker"). The > >reason I'm writing is because we have set up a framework internally to > >continually scan open source projects and provide the results of our > >analysis back to the developers of those projects. PostgreSQL is one of > >the 32 projects currently scanned at: > > > >http://scan.coverity.com > > Hm, interestingly and in contrast to some announcements, MySQL is not > included in this list. Did it blast the defects column ? :-) AFAIR they got a private scan done and they fixed the reported defects. After that they issued a press release telling how little defects they got, or something ... OTOH neither JBoss, BerkeleyDB, Qt are listed. Is there a pattern here? -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote: > AFAIR they got a private scan done and they fixed the reported defects. > After that they issued a press release telling how little defects they > got, or something ... > > OTOH neither JBoss, BerkeleyDB, Qt are listed. Is there a pattern here? I guess the pattern is obvious indeed: Coverity probably wants a shot at selling their services to these companies and rightly so imho. regards, Lukas
> >OTOH neither JBoss, BerkeleyDB, Qt are listed. Is there a pattern here? > > > http://www.coverity.com/news/news_02_15_05_story_6.html
On Mon, 6 Mar 2006, Bruce Momjian wrote: > Andreas Pflug wrote: >> Ben Chelf wrote: >>> Hello PostgreSQL Developers, >>> >>> I'm the CTO of Coverity, Inc., a company that does static source code >>> analysis to look for defects in code. You may have heard of us or of our >>> technology from its days at Stanford (the "Stanford Checker"). The >>> reason I'm writing is because we have set up a framework internally to >>> continually scan open source projects and provide the results of our >>> analysis back to the developers of those projects. PostgreSQL is one of >>> the 32 projects currently scanned at: >>> >>> http://scan.coverity.com >> >> Hm, interestingly and in contrast to some announcements, MySQL is not >> included in this list. Did it blast the defects column ? :-) > > I thought we ran the Converity analysis a year ago and cleaned up the > warnings, so I am surprised at our high number, but I assume they are > mostly noise. Got an account and will take a look at the details this evening ... :) ---- Marc G. Fournier Hub.Org Networking Services (http://www.hub.org) Email: scrappy@hub.org Yahoo!: yscrappy ICQ: 7615664
On Mon, 2006-03-06 at 11:55 -0300, Alvaro Herrera wrote: > AFAIR they got a private scan done and they fixed the reported defects. Indeed: EnterpriseDB paid for a license for the Coverity static analysis tool, and then ran that tool on the open-source Postgres tree. One of their engineers then worked with me to get a bunch of patches committed to fix the issues the tool identified -- e.g. http://archives.postgresql.org/pgsql-committers/2005-06/msg00428.php http://archives.postgresql.org/pgsql-committers/2005-06/msg00314.php http://archives.postgresql.org/pgsql-committers/2005-06/msg00315.php http://archives.postgresql.org/pgsql-committers/2005-06/msg00298.php The tool found a few significant bugs, but most of the fixes were somewhat cosmetic. (Perhaps one reason for this is that the Stanford checker was run on an earlier version of PostgreSQL by some grad students at Stanford, who submitted patches / bug reports for the more serious issues they found.) I'm a bit surprised to see that there are ~300 unfixed defects: AFAIR I fixed all the issues the EDB guys passed on to me, with the exception of some false positives and a handful of minor issues in ECPG that I couldn't be bothered fixing (frankly I would rather not touch the ECPG code). I've requested access to the Coverity results -- I'll be curious to see if we can get any more useful fixes from the tool. -Neil
Neil Conway wrote: >On Mon, 2006-03-06 at 11:55 -0300, Alvaro Herrera wrote: > > >>AFAIR they got a private scan done and they fixed the reported defects. >> >> > >Indeed: EnterpriseDB paid for a license for the Coverity static analysis >tool, and then ran that tool on the open-source Postgres tree. One of >their engineers then worked with me to get a bunch of patches committed >to fix the issues the tool identified -- e.g. > >http://archives.postgresql.org/pgsql-committers/2005-06/msg00428.php >http://archives.postgresql.org/pgsql-committers/2005-06/msg00314.php >http://archives.postgresql.org/pgsql-committers/2005-06/msg00315.php >http://archives.postgresql.org/pgsql-committers/2005-06/msg00298.php > >The tool found a few significant bugs, but most of the fixes were >somewhat cosmetic. (Perhaps one reason for this is that the Stanford >checker was run on an earlier version of PostgreSQL by some grad >students at Stanford, who submitted patches / bug reports for the more >serious issues they found.) > >I'm a bit surprised to see that there are ~300 unfixed defects: AFAIR I >fixed all the issues the EDB guys passed on to me, with the exception of >some false positives and a handful of minor issues in ECPG that I >couldn't be bothered fixing (frankly I would rather not touch the ECPG >code). I've requested access to the Coverity results -- I'll be curious >to see if we can get any more useful fixes from the tool. > > > For a short while EDB were pushing their Coverity results up to the buildfarm server, too. But it didn't last long. cheers andrew
Ben, > I'm the CTO of Coverity, Inc., a company that does static source code > analysis to look for defects in code. You may have heard of us or of our > technology from its days at Stanford (the "Stanford Checker"). The > reason I'm writing is because we have set up a framework internally to > continually scan open source projects and provide the results of our > analysis back to the developers of those projects. PostgreSQL is one of > the 32 projects currently scanned at: Nice to hear from you! Coverity has previously worked with Sean Chittenden, EnterpriseDB and Neil Conway. So we're glad to be continuing our relationship with you. -- --Josh Josh Berkus Aglio Database Solutions San Francisco
On Mon, Mar 06, 2006 at 12:50:15PM -0400, Marc G. Fournier wrote: > >I thought we ran the Converity analysis a year ago and cleaned up the > >warnings, so I am surprised at our high number, but I assume they are > >mostly noise. > > Got an account and will take a look at the details this evening ... :) Are the people who look at the results allowed to tell the rest of us what kind of bugs are involved (ie trivial or otherwise)? I mean, presumably they can't cut and paste the results to the mailing list but are we going to see anything other than a list of bug-fixes for (apparently) unreported bugs? Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
Neil Conway wrote:
> I'm a bit surprised to see that there are ~300 unfixed defects: AFAIR I
> fixed all the issues the EDB guys passed on to me, with the exception of
> some false positives and a handful of minor issues in ECPG that I
> couldn't be bothered fixing (frankly I would rather not touch the ECPG
> code). I've requested access to the Coverity results -- I'll be curious
> to see if we can get any more useful fixes from the tool.
It's not unlikely that the checker got improved.
FWIW I don't see any report related to the MemoryContext stuff, which I
was expecting we would be flooded with. I haven't seen the entire list
yet, but they do make the mistake of not noticing that ereport(ERROR)
does not continue execution. For example:
-------->-------->-------->-------->-------->-------->-------->-------->
pgsql/src/backend/optimizer/plan/createplan.c
638 foreach(l, uniq_exprs)
639 {
640 Node *uniqexpr = lfirst(l);
641 TargetEntry *tle;
642
643 tle = tlist_member(uniqexpr, newtlist);
Event var_compare_op: Added "tle" due to comparison "tle == 0"
Also see events: [var_deref_op]
At conditional (1): "tle == 0" taking true path
644 if (!tle) /* shouldn't happen */
645 elog(ERROR, "failed to find unique expression in subplan tlist");
Event var_deref_op: Variable "tle" tracked as NULL was dereferenced.
Also see events: [var_compare_op]
646 groupColIdx[groupColPos++] = tle->resno;
647 }
-------->-------->-------->-------->-------->-------->-------->-------->
We all know that this is not a bug. There are lots of these, as you can
imagine.
There are lots of other "not a bugs". For example in initdb, there is a
lot of noise about how we don't free the resources.
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes: > but they do make the mistake of not noticing that ereport(ERROR) > does not continue execution. There is a way in gcc to indicate that a function never returns. But in Postgres it's a bit weird since elog()/ereport() sometimes return and sometimes don't. Perhaps it would be better to make the forms that don't return separate functions. Then hopefully they can be marked to not trigger these kinds of warnings. > We all know that this is not a bug. There are lots of these, as you can > imagine. > > There are lots of other "not a bugs". For example in initdb, there is a > lot of noise about how we don't free the resources. Presumably the reason they don't immediately release the reports is for fear of security holes revealed. Once somebody has checked the errors and checked for any exploitable holes would it be possible to just open access to anyone so mere mortals can clean up the mundane bugs? -- greg
On Tue, Mar 07, 2006 at 05:10:44PM -0500, Greg Stark wrote: > > Alvaro Herrera <alvherre@commandprompt.com> writes: > > > but they do make the mistake of not noticing that ereport(ERROR) > > does not continue execution. > > There is a way in gcc to indicate that a function never returns. But in > Postgres it's a bit weird since elog()/ereport() sometimes return and > sometimes don't. Perhaps it would be better to make the forms that don't > return separate functions. Then hopefully they can be marked to not trigger > these kinds of warnings. I think the problem is that both of those are macros that expand into calls to errstart and errfinish. The error level is passed to errstart but the actual exception is thrown in errfinish using the value stored on the exception stack. For a static analysis tool to pick that up would be quite a trick. For gcc I wouldn't bet on it. One possibility would be to add code to the elog/ereport macros that is only used when using one of these tools. For example: #ifdef STATIC_ANALYSIS #define ereport(elevel, rest) \ (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \ (errfinish rest): (void) 0), (elevel >= ERROR ? exit(0) : 0) #else /* Normal def */ #endif The actual code never gets executed but it would give gcc and any other tools the info they need to handle this situation. Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
Martijn van Oosterhout <kleptog@svana.org> writes:
> #ifdef STATIC_ANALYSIS
> #define ereport(elevel, rest) \
> (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \
> (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0)
> #else
> /* Normal def */
> #endif
Hmm, neat idea ... though I wonder whether either gcc or Coverity's tool
is smart enough to draw the right conclusions from a conditional exit()
call ...
regards, tom lane
On Tue, Mar 07, 2006 at 05:39:18PM -0500, Tom Lane wrote: > Martijn van Oosterhout <kleptog@svana.org> writes: > > #ifdef STATIC_ANALYSIS > > #define ereport(elevel, rest) \ > > (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \ > > (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0) > > #else > > /* Normal def */ > > #endif > > Hmm, neat idea ... though I wonder whether either gcc or Coverity's tool > is smart enough to draw the right conclusions from a conditional exit() > call ... Well, remember this is a macro so the conditional is known at compile time and the optimiser should see that the exit is unconditional. A quick test with the attached program shows that gcc does correctly determine that the last few lines are unreachable and are optimised out entirely (with -Wunreachable-code which is not the default). I tried to create an empty static inline function with attribute((noreturn)) to optimise out the call to exit(), but gcc merely points out the function does actually return and proceeds to assume that the rest of main() is also reachable. Another possibility would be to create two versions of errfinish, one marked (noreturn), and use a conditional on elevel to decide which to use. However, then you get issues with multiple evaluation of macro arguments... gcc 3.3.5 -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
Attachment
Martijn van Oosterhout wrote: > On Tue, Mar 07, 2006 at 05:39:18PM -0500, Tom Lane wrote: > >>Martijn van Oosterhout <kleptog@svana.org> writes: >> >>>#ifdef STATIC_ANALYSIS >>>#define ereport(elevel, rest) \ >>> (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \ >>> (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0) >>>#else >>>/* Normal def */ >>>#endif >> >>Hmm, neat idea ... though I wonder whether either gcc or Coverity's tool >>is smart enough to draw the right conclusions from a conditional exit() >>call ... > As for Coverity, if the elevel that's passed to the ereport is really a constant, the above #ifdef should absolutely do the trick for us so we know to stop analyzing on that path...Let me know if it doesn't actually do that ;) -ben
Folks, > As for Coverity, if the elevel that's passed to the ereport is really a > constant, the above #ifdef should absolutely do the trick for us so we > know to stop analyzing on that path...Let me know if it doesn't actually > do that ;) Um, I think the answer is to train Coverity, not change our code to avoid the false-positives. I know that Coverity is sophisticated enough to, for example, be programed to understand that elog(ERROR) does not continue. Actually, I thougth that Neil/eDB did this with their copy. Is there any way to get a copy of that "training configuration"? -- --Josh Josh Berkus Aglio Database Solutions San Francisco
On 3/8/06, Josh Berkus <josh@agliodbs.com> wrote:
I think we have a backup of it somewhere. I'll look into it.
--
Jonah H. Harris, Database Internals Architect
EnterpriseDB Corporation
732.331.1324
Actually, I thougth that Neil/eDB did this with their copy. Is there any
way to get a copy of that "training configuration"?
I think we have a backup of it somewhere. I'll look into it.
--
Jonah H. Harris, Database Internals Architect
EnterpriseDB Corporation
732.331.1324
Ben Chelf <ben@coverity.com> writes: > >>>#ifdef STATIC_ANALYSIS > >>>#define ereport(elevel, rest) \ > >>> (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \ > >>> (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0) > >>>#else > >>>/* Normal def */ > >>>#endif > > As for Coverity, if the elevel that's passed to the ereport is really a > constant, the above #ifdef should absolutely do the trick for us so we know to > stop analyzing on that path...Let me know if it doesn't actually do that ;) If you're willing to require elevel to always be a constant then why not just tack on the (elevel >= ERROR ? exit(0) : 0) onto the end of the regular definition of ereport instead of having an ifdef? Incidentally, if it's not guaranteed to be a constant then the definition above is wrong because it's missing parentheses around elevel at both occurrences. -- greg
On Wed, Mar 08, 2006 at 06:42:45PM -0500, Greg Stark wrote: > Ben Chelf <ben@coverity.com> writes: > > > >>>#ifdef STATIC_ANALYSIS > > >>>#define ereport(elevel, rest) \ > > >>> (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \ > > >>> (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0) > > >>>#else > > >>>/* Normal def */ > > >>>#endif > > > > As for Coverity, if the elevel that's passed to the ereport is really a > > constant, the above #ifdef should absolutely do the trick for us so we know to > > stop analyzing on that path...Let me know if it doesn't actually do that ;) > > If you're willing to require elevel to always be a constant then why not just > tack on the (elevel >= ERROR ? exit(0) : 0) onto the end of the regular > definition of ereport instead of having an ifdef? Well, the only cost would be a useless call to exit() for each elog/ereport with an elevel >= ERROR. It bloats the binary a bit. Not sure whether people care enough about that. Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
Martijn van Oosterhout wrote: -- Start of PGP signed section. > On Wed, Mar 08, 2006 at 06:42:45PM -0500, Greg Stark wrote: > > Ben Chelf <ben@coverity.com> writes: > > > > > >>>#ifdef STATIC_ANALYSIS > > > >>>#define ereport(elevel, rest) \ > > > >>> (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \ > > > >>> (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0) > > > >>>#else > > > >>>/* Normal def */ > > > >>>#endif > > > > > > As for Coverity, if the elevel that's passed to the ereport is really a > > > constant, the above #ifdef should absolutely do the trick for us so we know to > > > stop analyzing on that path...Let me know if it doesn't actually do that ;) > > > > If you're willing to require elevel to always be a constant then why not just > > tack on the (elevel >= ERROR ? exit(0) : 0) onto the end of the regular > > definition of ereport instead of having an ifdef? > > Well, the only cost would be a useless call to exit() for each > elog/ereport with an elevel >= ERROR. It bloats the binary a bit. Not > sure whether people care enough about that. We care. :-) -- Bruce Momjian http://candle.pha.pa.us SRA OSS, Inc. http://www.sraoss.com + If your life is a hard drive, Christ can be your backup. +
On Thu, 9 Mar 2006, Bruce Momjian wrote: > Martijn van Oosterhout wrote: > -- Start of PGP signed section. >> On Wed, Mar 08, 2006 at 06:42:45PM -0500, Greg Stark wrote: >>> Ben Chelf <ben@coverity.com> writes: >>> >>>>>>> #ifdef STATIC_ANALYSIS >>>>>>> #define ereport(elevel, rest) \ >>>>>>> (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \ >>>>>>> (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0) >>>>>>> #else >>>>>>> /* Normal def */ >>>>>>> #endif >>>> >>>> As for Coverity, if the elevel that's passed to the ereport is really a >>>> constant, the above #ifdef should absolutely do the trick for us so we know to >>>> stop analyzing on that path...Let me know if it doesn't actually do that ;) >>> >>> If you're willing to require elevel to always be a constant then why not just >>> tack on the (elevel >= ERROR ? exit(0) : 0) onto the end of the regular >>> definition of ereport instead of having an ifdef? >> >> Well, the only cost would be a useless call to exit() for each >> elog/ereport with an elevel >= ERROR. It bloats the binary a bit. Not >> sure whether people care enough about that. > > We care. :-) 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? Martijn, could you do a build with/without it and compare sizes? ---- Marc G. Fournier Hub.Org Networking Services (http://www.hub.org) Email: scrappy@hub.org Yahoo!: yscrappy ICQ: 7615664
"Marc G. Fournier" <scrappy@postgresql.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.
regards, tom lane
Tom Lane wrote: > "Marc G. Fournier" <scrappy@postgresql.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. Agreed. The idea of modifying our binary in any way to help a sanity tool not complain is totally backwards. -- Bruce Momjian http://candle.pha.pa.us SRA OSS, Inc. http://www.sraoss.com + If your life is a hard drive, Christ can be your backup. +
* Bruce Momjian (pgman@candle.pha.pa.us) wrote:
> > 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.
>
> Agreed. The idea of modifying our binary in any way to help a sanity
> tool not complain is totally backwards.
This is very amusing. I have to agree w/ Tom in general, the code in
this case does the right thing and the Coverity tool should be able to
be told about that. However, for areas where the tool is actually right
and there's some bug in Postgres, well, I'd hope we'd modify Postgres to
fix the bug... ;)
Enjoy,
Stephen
On Thu, 9 Mar 2006, Tom Lane wrote: > "Marc G. Fournier" <scrappy@postgresql.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@hub.org Yahoo!: yscrappy ICQ: 7615664
On 3/8/06, Josh Berkus <josh ( at ) agliodbs ( dot ) com> wrote: > Actually, I thougth that Neil/eDB did this with their copy. Is> there any way to get a copy of that "training configuration"? Just to jump in on this thread, we can absolutely configure elog -- if you have the config already, great. If not, if you can just send me the prototype/macro expansion for 'elog' and the constant values that are passed in the case where it exits, I'll add that config. Thanks! -ben
On Wed, Mar 08, 2006 at 10:34:38PM -0800, Ben Chelf wrote: > On 3/8/06, Josh Berkus <josh ( at ) agliodbs ( dot ) com> wrote: > > > Actually, I thougth that Neil/eDB did this with their copy. Is > > there any way to get a copy of that "training configuration"? > > > Just to jump in on this thread, we can absolutely configure elog -- if > you have the config already, great. If not, if you can just send me the > prototype/macro expansion for 'elog' and the constant values that are > passed in the case where it exits, I'll add that config. Thanks! I don't know it anyone has responded to this but it works as follows. the actual source can be seen here: http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/utils/elog.h?rev=1.82 elog expands to elog_finish, which doesn't return if the first argument is >= ERROR (which is the number 20). ereport(X) is more complex. You want the first argument of that but it expands to something similar to: errstart(X), errfinish() if X >= ERROR in the call to errstart, errfinish doesn't return. Hope this helps, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.