Thread: [PATCH] libpq improvements and fixes
Hi,
Coverity detected a dead code in the src / interfaces / libpq / fe-auth.c file, to correct it, a simplification was made and the oom_error goto was removed, since it is clearly redundant and its presence can be confusing.
The second part of the patch refers to the file src / interfaces / libpq / fe-exec.c.
First, a correction was made to the return types of some functions that clearly return bool, but are defined as int.
The second part of the patch refers to the file src / interfaces / libpq / fe-exec.c.
First, a correction was made to the return types of some functions that clearly return bool, but are defined as int.
According to some functions, they do a basic check and if they fail, they return immediately, so it does not make sense to start communication and then return.
It makes more sense to do the basic checks, only to start communicating with the server afterwards.
These changes are passing the regression tests and are in use in libpq.dll, used in production by my customers.
It makes more sense to do the basic checks, only to start communicating with the server afterwards.
These changes are passing the regression tests and are in use in libpq.dll, used in production by my customers.
regards,
Ranier Vilela
Attachment
Ranier Vilela <ranier.vf@gmail.com> writes:
> Coverity detected a dead code in the src / interfaces / libpq / fe-auth.c
> file, to correct it, a simplification was made and the oom_error goto was
> removed, since it is clearly redundant and its presence can be confusing.
I'm kind of disinclined to let Coverity dictate our coding style here.
We've dismissed many hundreds of its reports as false positives, and
this seems like one that could get (probably already has gotten) the
same treatment. I also don't feel like duplicating error messages
as you propose is an improvement.
If we did want to adjust the code in pg_SASL_init, my thought would
be to reduce not increase the code duplication, by making the error
exits look like
...
return STATUS_OK;
oom_error:
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("out of memory\n"));
/* FALL THRU */
error:
termPQExpBuffer(&mechanism_buf);
if (initialresponse)
free(initialresponse);
return STATUS_ERROR;
}
It's only marginally worth the trouble though.
> First, a correction was made to the return types of some functions that
> clearly return bool, but are defined as int.
This is ancient history that doesn't seem worth revisiting. There is
certainly exactly zero chance of us changing libpq's external API
as you propose, because of the ensuing ABI breakage. Maybe we could
change the static functions, but I'm not very excited about it.
I can't get excited about the other code rearrangements you're proposing
here either. They seem to make the code more intellectually complex for
little benefit.
regards, tom lane
Em qua., 12 de fev. de 2020 às 22:25, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Ranier Vilela <ranier.vf@gmail.com> writes:
> Coverity detected a dead code in the src / interfaces / libpq / fe-auth.c
> file, to correct it, a simplification was made and the oom_error goto was
> removed, since it is clearly redundant and its presence can be confusing.
I'm kind of disinclined to let Coverity dictate our coding style here.
We've dismissed many hundreds of its reports as false positives, and
this seems like one that could get (probably already has gotten) the
same treatment. I also don't feel like duplicating error messages
as you propose is an improvement.
If you look closely at the code in the source, you will see that the style there is:
if (test)
if (test)
{
msg;
goto;
}
I just kept it, even if I duplicated the error message, the style was kept and in my opinion it is much more coherent and readable.
But your solution is also good, and yes, it is worth it, because even with small benefits, the change improves the code and prevents Coverity or another tool from continuing to report false positives or not.
goto;
}
I just kept it, even if I duplicated the error message, the style was kept and in my opinion it is much more coherent and readable.
But your solution is also good, and yes, it is worth it, because even with small benefits, the change improves the code and prevents Coverity or another tool from continuing to report false positives or not.
If we did want to adjust the code in pg_SASL_init, my thought would
be to reduce not increase the code duplication, by making the error
exits look like
...
return STATUS_OK;
oom_error:
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("out of memory\n"));
/* FALL THRU */
error:
termPQExpBuffer(&mechanism_buf);
if (initialresponse)
free(initialresponse);
return STATUS_ERROR;
}
It's only marginally worth the trouble though.
Sounds good to me.
> First, a correction was made to the return types of some functions that
> clearly return bool, but are defined as int.
This is ancient history that doesn't seem worth revisiting. There is
certainly exactly zero chance of us changing libpq's external API
as you propose, because of the ensuing ABI breakage. Maybe we could
change the static functions, but I'm not very excited about it.
Virtually no code will break for the change, since bool and int are internally the same types.
I believe that no code will have either adjusted to work with corrected functions, even if they use compiled libraries.
I believe that no code will have either adjusted to work with corrected functions, even if they use compiled libraries.
And again, it is worth correcting at least the static ones, because the goal here, too, is to improve readability.
I can't get excited about the other code rearrangements you're proposing
here either. They seem to make the code more intellectually complex for
little benefit.
I cannot agree with you that these changes add complexity.
It was using the principle enshrined in programming that I proposed these changes.
"Get out quick."
For 99% of calls to these functions, there won't be any changes, since all parameters are ok, tests will be done and the PQsendQueryStart function will be called anyway.
If it were possible, it would be better to eliminate the basic tests, but this is not possible, so better to do them first and get out of there soon, without doing anything else.
It was using the principle enshrined in programming that I proposed these changes.
"Get out quick."
For 99% of calls to these functions, there won't be any changes, since all parameters are ok, tests will be done and the PQsendQueryStart function will be called anyway.
If it were possible, it would be better to eliminate the basic tests, but this is not possible, so better to do them first and get out of there soon, without doing anything else.
regards,
Ranier Villela
On Thu, Feb 13, 2020 at 02:22:36PM -0300, Ranier Vilela wrote: > I just kept it, even if I duplicated the error message, the style was kept > and in my opinion it is much more coherent and readable. > But your solution is also good, and yes, it is worth it, because even with > small benefits, the change improves the code and prevents Coverity or > another tool from continuing to report false positives or not. Complaints from static analyzers need to be taken with a pinch of salt, and I agree with Tom here. > Virtually no code will break for the change, since bool and int are > internally the same types. > I believe that no code will have either adjusted to work with corrected > functions, even if they use compiled libraries. > And again, it is worth correcting at least the static ones, because the > goal here, too, is to improve readability. FWIW, looking at the patch from upthread, I think that it is not that wise to blindly break the error compatibility handling of all PQsend* routines by switching the error handling of the connection to be after the compatibility checks, and all the other changes don't justify a breakage making back-patching more complicated nor do they improve readability at great lengths. -- Michael
Attachment
Em sex., 14 de fev. de 2020 às 03:13, Michael Paquier <michael@paquier.xyz> escreveu:
On Thu, Feb 13, 2020 at 02:22:36PM -0300, Ranier Vilela wrote:
> I just kept it, even if I duplicated the error message, the style was kept
> and in my opinion it is much more coherent and readable.
> But your solution is also good, and yes, it is worth it, because even with
> small benefits, the change improves the code and prevents Coverity or
> another tool from continuing to report false positives or not.
Complaints from static analyzers need to be taken with a pinch of
salt, and I agree with Tom here.
That's right, I will try avoid sending patches that only satisfy static analysis tools.
> Virtually no code will break for the change, since bool and int are
> internally the same types.
> I believe that no code will have either adjusted to work with corrected
> functions, even if they use compiled libraries.
> And again, it is worth correcting at least the static ones, because the
> goal here, too, is to improve readability.
FWIW, looking at the patch from upthread, I think that it is not that
wise to blindly break the error compatibility handling of all PQsend*
routines by switching the error handling of the connection to be after
the compatibility checks, and all the other changes don't justify a
breakage making back-patching more complicated nor do they improve
readability at great lengths.
Another programming principle I follow is to remove anything static from loops that can be executed outside the loop.
In this specific case, from the loop modified in fe-exec, two branches were removed, is this an improvement for you or not?
See patch attached.
regards,
Ranier Vilela