Re: Patch: Don't set LoadedSSL unless secure_initialize succeeds

On Thu, May 26, 2022 at 12:16 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Gurjeet Singh <gurjeet(at)singh(dot)im> writes:
> > On Mon, May 23, 2022 at 8:51 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> The comments for secure_initialize() and be_tls_init() both explain
> >> this already.
>
> > The comments above secure_initialize() do, but there are no comments
> > above be_tls_init(), and nothing in there attempts to explain the
> > FATAL vs. LOG difference.
>
> I was looking at the comments in libpq-be.h:
>
> /*
> * Initialize global SSL context.
> *
> * If isServerStart is true, report any errors as FATAL (so we don't return).
> * Otherwise, log errors at LOG level and return -1 to indicate trouble,
> * preserving the old SSL state if any. Returns 0 if OK.
> */
> extern int be_tls_init(bool isServerStart);
>
> It isn't our usual practice to put such API comments with the extern
> rather than the function definition,

Yep, and I didn't notice these comments, or even bothered to look at
the extern declaration, precisely because my knowledge of Postgres
coding convention told me the comments are supposed to be on the
function definition.

> so maybe those comments in libpq-be.h
> should be moved to their respective functions? In any case, I'm not
> excited about having three separate comments covering the same point.

By 3 locations, I suppose you're referring to the definition of
secure_initialize(), extern declaration of be_tls_init(), and the
definition of be_tls_init().

The comment on the extern declaration does not belong there, so that
one definitely needs go. The other two locations need descriptive
comments, even if they might sound duplicate. Because the one on
secure_initialize() describes the abstraction's expectations, and the
one on be_tls_init() should refer to it, and additionally mention any
implementation details.

Best regards,
Gurjeet
http://Gurje.et