Re: [PATCH] libpq improvements and fixes

Em qua., 12 de fev. de 2020 às 22:25, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> escreveu:

> Ranier Vilela <ranier(dot)vf(at)gmail(dot)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)
{
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.

> 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.
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.

regards,
Ranier Villela