On 11/30/2016 06:52 AM, Michael Paquier wrote:
> On Mon, Nov 28, 2016 at 2:01 PM, Michael Paquier
>> Looking at the latest patch at code-level, there is some refactoring
>> to introduce initialize_context(). But it is actually not necessary
>> (perhaps this is the remnant of a past version?) as be_tls_init() is
>> its only caller. This patch makes hard to look at the diffs, and it
>> makes future back-patching more complicated, so I would suggest
>> simplifying the patch as much as possible. You could use for example a
>> goto block for the error code path to free the context with
>> SSL_CTX_free(), and set up ssl_loaded_verify_locations once the CA is
>> loaded. The same applies to initialize_ecdh().
>>
>> + if (secure_initialize() != 0)
>> + ereport(FATAL,
>> + (errmsg("could not load ssl context")));
>> + LoadedSSL = true;
>> In case of a failure, a LOG message would have been already generated,
>> so this duplicates the information. Worse, if log_min_messages is set
>> to a level higher than LOG, users *lose* information on what has
>> happened. I think that secure_initialize() should have an argument to
>> define elevel and that this routine should be in charge of generating
>> an error message. Having it return a status code is necessary, but you
>> could cast secure_initialize() with (void) to show that we don't care
>> about the status code in this case. There is no need to care about
>> freeing the new context when the FATAL code path is used as process
>> would just shut down.
Thanks, this is a really good suggestion which made the diff much
cleaner. I removed my new macro too now since I felt it mostly made the
code more cryptic just to gain a few lines of code.
>> config.sgml needs to be updated to reflect that the SSL parameters are
>> updated at server reload (mentioned already upthread, just
>> re-mentioning it to group all the comments in one place).
Thanks, fixed this.
> As this patch could be really simplified this way, I am marking is as
> returned with feedback.
I have attached a new version. The commitfest should technically have
been closed by now, so do what you like with the status. I can always
submit the patch to the next commitfest.
Andreas