Thread: Nitpicking: unnecessary NULL-pointer check in pg_upgrade's controldata.c

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.
Regards,
-- 
Michael

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

On Fri, Jun 26, 2015 at 9:06 AM, Andres Freund <andres@anarazel.de> 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

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.

On Fri, Jun 26, 2015 at 9:49 AM, Andres Freund <andres@anarazel.de> 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

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Jun 26, 2015 at 9:49 AM, Andres Freund <andres@anarazel.de> 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

On Fri, Jun 26, 2015 at 10:21 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Fri, Jun 26, 2015 at 9:49 AM, Andres Freund <andres@anarazel.de> 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

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Jun 26, 2015 at 10:21 AM, Tom Lane <tgl@sss.pgh.pa.us> 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

On Fri, Jun 26, 2015 at 10:54 AM, Tom Lane <tgl@sss.pgh.pa.us> 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

On Fri, Jun 26, 2015 at 7:21 AM, Tom Lane <tgl@sss.pgh.pa.us> 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