On Sun, May 22, 2022 at 12:17 AM Daniel Gustafsson <dan...@yesql.se> wrote: > > On 22 May 2022, at 08:41, Gurjeet Singh <gurj...@singh.im> wrote: > > > The initialization in PostmasterMain() blindly turns on LoadedSSL, > > irrespective of the outcome of secure_initialize(). > > This call is invoked with isServerStart set to true so any error in > secure_initialize should error out with ereport FATAL (in be_tls_init()). > That > could be explained in a comment though, which is currently isn't.
That makes sense. I have attached a patch that adds a couple of lines of comments explaining this at call-site. > Did you manage to get LoadedSSL set to true without SSL having been properly > initialized? Fortunately, no. I'm trying to add a new network protocol, and caught this inconsistency while reading/adapting the code. If a committer sees some value in it, in the attached patch I have also attempted to improve the readability of the code a little bit in startup-packet handling, and in SSL functions. These are purely cosmetic changes, but I think they reduce repetition, and improve readability, by quite a bit. For example, every ereport call evaluates the same 'isServerStart ? FATAL : LOG', over and over again; replacing this with variable 'logLevel' reduces cognitive load for the reader. And I've replace one 'goto retry1' with a 'while' loop, like the GASSAPI does it below that occurrence. There's an symmetry, almost a diametric opposition, between how SSL initialization error is treated when it occurs during server startup, versus when the error occurs during a reload/SIGHUP. During startup an error in SSL initialization leads to FATAL, whereas during a SIGHUP it's merely a LOG message. I found this difference in treatment of SSL initialization errors quite bothersome, and there is no ready explanation for this. Either a properly initialized SSL stack is important for server operation, or it is not. What do we gain by letting the server operate normally after a reload that failed to initialize SSL stack. Conversely, why do we kill the server during startup on SSL initialization error, when it's okay to operate normally after a reload that is unable to initialize SSL. I have added a comment to be_tls_init(), which I hope explains this difference in treatment of errors. I have also added comments to be_tls_init(), explaining why we don't destroy/free the global SSL_context variable in case of an error in re-initialization of SSL; it's not just an optimization, it's essential to normal server operation. Best regards, Gurjeet http://Gurje.et
readability_improvements-ssl-startup_packet.patch
Description: Binary data
