|From:||Andreas Karlsson <andreas(at)proxel(dot)se>|
|To:||Michael Paquier <michael(dot)paquier(at)gmail(dot)com>|
|Cc:||Michael Banck <michael(dot)banck(at)credativ(dot)de>, Peter Geoghegan <pg(at)heroku(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Peter Eisentraut <peter_e(at)gmx(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: [PATCH] Reload SSL certificates on SIGHUP|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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.
|Next Message||Michael Paquier||2016-12-02 02:31:51||Re: Broken SSL tests in master|
|Previous Message||Haribabu Kommi||2016-12-02 02:25:29||Re: Stopping logical replication protocol|