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 function
> declaration, preferably doxygen-documented, at the beginning of ssl.c.
You're absolutely right about this function supposed to be static. I missed
that.
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?
>
> > +{
> > + /* if something goes wrong with stat(), we'll store 0 as mtime */
> > + platform_stat_t crl_stat = {0};
> > +
> > + /*
> > + * an inline CRL can't change at runtime, therefore there is no need to
> > + * reload it. It will be reloaded upon config change + SIGHUP.
> > + */
> > + if (crl_file_inline && reload)
> > + return;
> > +
> > + if (!crl_file_inline)
> > + platform_stat(crl_file, &crl_stat);
> > +
> > + /*
> > + * If this is not a reload, update the CRL right away.
> > + * Otherwise, update only if the CRL file was modified after the last
> > reload.
> > + * Note: Windows does not support tv_nsec.
> > + */
> > + if (reload &&
> > + (ssl_ctx->crl_last_mtime.tv_sec >= crl_stat.st_mtime))
> > + return;
> > +
> > + ssl_ctx->crl_last_mtime.tv_sec = crl_stat.st_mtime;
> > + backend_tls_ctx_reload_crl(ssl_ctx, crl_file, crl_file_inline);
> > +}
>
> This can be done without the 'reload' argument too, something like:
>
> /* if something goes wrong with stat(), we'll store 0 as mtime */
> platform_stat_t crl_stat = {0};
>
> if (crl_file_inline)
> {
> /*
> * an inline CRL can't change at runtime, therefore there is no
> need to
> * reload it. It will be reloaded upon config change + SIGHUP.
> */
> if (ssl_ctx->crl_last_mtime.tv_sec)
> return;
> else
> crl_stat.st_mtime = now; /* Set dummy mtime */
> }
> else
> {
> platform_stat (crl_file, &crl_stat);
> }
>
> /*
> * If this is not a reload, update the CRL right away.
> * Otherwise, update only if the CRL file was modified after the last
> reload.
> * Note: Windows does not support tv_nsec.
> */
> if (ssl_ctx->crl_last_mtime.tv_sec <= crl_stat.st_mtime)
> {
> ssl_ctx->crl_last_mtime.tv_sec = crl_stat.st_mtime;
> backend_tls_ctx_reload_crl (ssl_ctx, crl_file, crl_file_inline);
> }
>
> I slightly prefer this over adding the extra argument, but can live with
> your approach just fine too. Pick what you like best :)
I guess the assumption here is that crl_last_mtime.tv_sec is initialized with 0.
This would make your code work. And yeah, I also prefer to avoid the reload
argument ;)
Will make these changes and resend the patch.
Thanks for the comments!
Cheers,
--
Antonio Quartulli
------------------------------------------------------------------------------
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
