Re: [Openvpn-devel] [PATCH] reload CRL only if file was modified

2016-11-30 Thread Antonio Quartulli
On Wed, Nov 30, 2016 at 05:26:30PM +0300, SviMik wrote: > 1) I would also check if the file size was changed, not only mtime. > this would work against 2 CRLs with the same mtime but different size: is this is a real case we have to worry about? Anyway, adding this check is easy. I'd do it if

Re: [Openvpn-devel] [PATCH] reload CRL only if file was modified

2016-11-30 Thread SviMik
1) I would also check if the file size was changed, not only mtime. 2) I wasn't digging the code deeply, but the > ssl_ctx->crl_last_mtime.tv_sec >= crl_stat.st_mtime makes me think it would fail if the file goes reverted to a previous version. Perhaps the check shall be != instead of >=. > In

Re: [Openvpn-devel] [PATCH] reload CRL only if file was modified

2016-11-30 Thread Steffan Karger
Hi, On 30-11-16 02:16, Antonio Quartulli wrote: > Do I really need to split declaration and definition? Or can I just > doxy-document the definition in the same place where it is now? Well, either add a declaration at the top of the file (before the first usage), or move the function up to

Re: [Openvpn-devel] [PATCH] reload CRL only if file was modified

2016-11-29 Thread Antonio Quartulli
On Tue, Nov 29, 2016 at 05:43:33PM +0100, Steffan Karger wrote: [CUT..] > > > > +void > > +tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file, > > + const char *crl_file_inline, bool reload) > > This is only used within ssl.c, so should be static and have a

Re: [Openvpn-devel] [PATCH] reload CRL only if file was modified

2016-11-29 Thread Steffan Karger
On 29-11-16 17:43, Steffan Karger wrote: > Will test more thoroughly tonight (hopefully on Windows too), but have a > lot of faith that those will succeed. Just did this, on linux and windows, and code works as expected. So once you've taken care of the (minor) remarks in my previous mails, I

Re: [Openvpn-devel] [PATCH] reload CRL only if file was modified

2016-11-29 Thread Steffan Karger
Hi, One more thing. On 29-11-16 07:38, Antonio Quartulli wrote: > + if (!crl_file_inline) > +platform_stat(crl_file, _stat); If platform_stat() fails, we now silently ignore the error and continue using the old CRL. I think using the old CRL is fine, but we should print a warning to tell

Re: [Openvpn-devel] [PATCH] reload CRL only if file was modified

2016-11-29 Thread Christian Hesse
Christian Hesse on Tue, 2016/11/29 20:16: > Oops, missed that in my logs (and did not find the code)... You are right, > cache is cleared. > > Either of both is just fine and it works as-is. So ignore my patch. Oops again... Looks like I answered a wrong mail. Please ignore...

Re: [Openvpn-devel] [PATCH] reload CRL only if file was modified

2016-11-29 Thread Christian Hesse
Steffan Karger on Tue, 2016/11/29 17:43: > Hi, > > Thanks for following up. I did some stare-at-code and trivial tests. > Will test more thoroughly tonight (hopefully on Windows too), but have a > lot of faith that those will succeed. I have some comments from

Re: [Openvpn-devel] [PATCH] reload CRL only if file was modified

2016-11-29 Thread Steffan Karger
Hi, Thanks for following up. I did some stare-at-code and trivial tests. Will test more thoroughly tonight (hopefully on Windows too), but have a lot of faith that those will succeed. I have some comments from staring at the code though, see below. On 29-11-16 07:38, Antonio Quartulli wrote: >

[Openvpn-devel] [PATCH] reload CRL only if file was modified

2016-11-28 Thread Antonio Quartulli
In order to prevent annoying delays upon client connection, reload the CRL file only if it was modified since the last reload operation. If not, keep on using the already stored CRL. This change will boost client connection time in instances where the CRL file is quite large (dropping from