Re: [HACKERS] Nitpicking: unnecessary NULL-pointer check in pg_upgrade's controldata.c
On Fri, Jun 26, 2015 at 7:21 AM, Tom Lane wrote: > I agree that the correct handling of this particular case is to mark it > as not-a-bug. We have better things to do. +1 -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Nitpicking: unnecessary NULL-pointer check in pg_upgrade's controldata.c
On Fri, Jun 26, 2015 at 10:54 AM, Tom Lane wrote: > Well, if you find this to be good code cleanup on its own merits, > you have a commit bit, you can go commit it. I'm just saying that > Coverity is not a good judge of code readability and even less of > a judge of likely future changes. So we should not let it determine > whether we approve of "unnecessary" tests. Yes, it might not be right in every case, but this one seems like a good change to me, so committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Nitpicking: unnecessary NULL-pointer check in pg_upgrade's controldata.c
Robert Haas writes: > On Fri, Jun 26, 2015 at 10:21 AM, Tom Lane wrote: >> I agree that the correct handling of this particular case is to mark it >> as not-a-bug. We have better things to do. > Well, I find that a disappointing conclusion, but I'm not going to > spend a lot of time arguing against both of you. But, for what it's > worth: it's not as if somebody is going to modify the code in that > function to make output == NULL a plausible option, so I think the > change could easily be justified on code clean-up grounds if nothing > else. There's not much point calling fgets on a FILE unconditionally > and then immediately thereafter allowing for the possibility that > output might be NULL. That's not easing the work of anyone who might > want to modify that code in the future; it just makes the code more > confusing. Well, if you find this to be good code cleanup on its own merits, you have a commit bit, you can go commit it. I'm just saying that Coverity is not a good judge of code readability and even less of a judge of likely future changes. So we should not let it determine whether we approve of "unnecessary" tests. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Nitpicking: unnecessary NULL-pointer check in pg_upgrade's controldata.c
On Fri, Jun 26, 2015 at 10:21 AM, Tom Lane wrote: > Robert Haas writes: >> On Fri, Jun 26, 2015 at 9:49 AM, Andres Freund wrote: >>> It takes about three seconds to mark it as ignored which will hide it >>> going forward. > >> So what? That doesn't help if someone *else* sets up a Coverity run >> on this code base, or if say Salesforce sets up such a run on their >> fork of the code base. It's much better to fix the problem at the >> root. > > The problem with that is allowing Coverity, which in the end is not magic > but just another piece of software with many faults, to define what is a > "problem". In this particular case, the only effect of the change that > I can see is to make the code less flexible, and less robust against a > fairly obvious type of future change. So I'm not on board with removing > if-guards just because Coverity thinks they are unnecessary. > > I agree that the correct handling of this particular case is to mark it > as not-a-bug. We have better things to do. Well, I find that a disappointing conclusion, but I'm not going to spend a lot of time arguing against both of you. But, for what it's worth: it's not as if somebody is going to modify the code in that function to make output == NULL a plausible option, so I think the change could easily be justified on code clean-up grounds if nothing else. There's not much point calling fgets on a FILE unconditionally and then immediately thereafter allowing for the possibility that output might be NULL. That's not easing the work of anyone who might want to modify that code in the future; it just makes the code more confusing. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Nitpicking: unnecessary NULL-pointer check in pg_upgrade's controldata.c
Robert Haas writes: > On Fri, Jun 26, 2015 at 9:49 AM, Andres Freund wrote: >> It takes about three seconds to mark it as ignored which will hide it >> going forward. > So what? That doesn't help if someone *else* sets up a Coverity run > on this code base, or if say Salesforce sets up such a run on their > fork of the code base. It's much better to fix the problem at the > root. The problem with that is allowing Coverity, which in the end is not magic but just another piece of software with many faults, to define what is a "problem". In this particular case, the only effect of the change that I can see is to make the code less flexible, and less robust against a fairly obvious type of future change. So I'm not on board with removing if-guards just because Coverity thinks they are unnecessary. I agree that the correct handling of this particular case is to mark it as not-a-bug. We have better things to do. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Nitpicking: unnecessary NULL-pointer check in pg_upgrade's controldata.c
On Fri, Jun 26, 2015 at 9:49 AM, Andres Freund wrote: > On 2015-06-26 09:44:14 -0400, Robert Haas wrote: >> I don't mind committing patches for this kind of thing if it makes the >> Coverity reports easier to deal with, which I gather that it does. > > It takes about three seconds to mark it as ignored which will hide it > going forward. So what? That doesn't help if someone *else* sets up a Coverity run on this code base, or if say Salesforce sets up such a run on their fork of the code base. It's much better to fix the problem at the root. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Nitpicking: unnecessary NULL-pointer check in pg_upgrade's controldata.c
On 2015-06-26 09:44:14 -0400, Robert Haas wrote: > I don't mind committing patches for this kind of thing if it makes the > Coverity reports easier to deal with, which I gather that it does. It takes about three seconds to mark it as ignored which will hide it going forward. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Nitpicking: unnecessary NULL-pointer check in pg_upgrade's controldata.c
On Fri, Jun 26, 2015 at 9:06 AM, Andres Freund wrote: > On 2015-06-26 22:03:05 +0900, Michael Paquier wrote: >> Hi, >> >> Coverity is nitpickingly pointing out the following thing: >> --- a/src/bin/pg_upgrade/controldata.c >> +++ b/src/bin/pg_upgrade/controldata.c >> @@ -402,8 +402,7 @@ get_control_data(ClusterInfo *cluster, bool live_check) >> } >> } >> >> - if (output) >> - pclose(output); >> + pclose(output); >> The thing is that output can never be NULL, pg_upgrade leaving with >> pg_fatal before coming to this code path. Hence this NULL check could >> be simply removed. > > FWIW, I think these type of coverity items should just be discarded with > prejudice. Fixing them doesn't seem to buy anything and there's enough > actually worthwhile stuff going on. I don't mind committing patches for this kind of thing if it makes the Coverity reports easier to deal with, which I gather that it does. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Nitpicking: unnecessary NULL-pointer check in pg_upgrade's controldata.c
Hi, On 2015-06-26 22:03:05 +0900, Michael Paquier wrote: > Hi, > > Coverity is nitpickingly pointing out the following thing: > --- a/src/bin/pg_upgrade/controldata.c > +++ b/src/bin/pg_upgrade/controldata.c > @@ -402,8 +402,7 @@ get_control_data(ClusterInfo *cluster, bool live_check) > } > } > > - if (output) > - pclose(output); > + pclose(output); > The thing is that output can never be NULL, pg_upgrade leaving with > pg_fatal before coming to this code path. Hence this NULL check could > be simply removed. FWIW, I think these type of coverity items should just be discarded with prejudice. Fixing them doesn't seem to buy anything and there's enough actually worthwhile stuff going on. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
