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

2016-12-01 Thread Steffan Karger 
On 01-12-16 09:13, Steffan Karger wrote:
>   else if (0 != platform_stat(crl_file, &crl_stat))
> {
>   msg (M_WARN, "WARNING: Failed to stat CRL file, using cached CRL.");
> }

Ahum, as Gert noted on IRC, this missed a return statement to actually
*not* load the CRL.  So, better suggestion:

  else if (platform_stat(crl_file, &crl_stat) < 0)
{
  msg (M_WARN, "WARNING: Failed to stat CRL file, using (re)loading
CRL.");
  return;
}

-Steffan

--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

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

2016-12-01 Thread Antonio Quartulli 
On Thu, Dec 01, 2016 at 09:13:36AM +0100, Steffan Karger wrote:
> Hi,
> 
> Tested on linux and windows, works as expected, except for one thing:
> 
> On 01-12-16 07:55, Antonio Quartulli wrote:
> > +  /*
> > +   * an inline CRL can't change at runtime, therefore there is no need to
> > +   * reload it. It will be reloaded upon config change + SIGHUP.
> > +   * Use always '1' as dummy timestamp in this case: it will trigger the
> > +   * first load, but will prevent any future reload.
> > +   */
> > +  if (crl_file_inline)
> > +crl_stat.st_mtime = 1;
> > +  else
> > +platform_stat(crl_file, &crl_stat);
> 
> I still think this should issue a warning when we do not reload the CRL
> because platform_stat() fails, e.g. due to the CRL file missing.  So, if
> we replace this with:
> 
>   if (crl_file_inline)
> {
>   crl_stat.st_mtime = 1;
> }
>   else if (0 != platform_stat(crl_file, &crl_stat))
> {
>   msg (M_WARN, "WARNING: Failed to stat CRL file, using cached CRL.");
> }
> 
> I can ACK this.
> 
> Since the deadline for 2.4_rc1 is this afternoon (CET), and Antonio
> seems to be awake at quite different times than the Europeans doing the
> tagging, maybe one of the committers can make this change on the file if
> he agrees?  Of course, if Antonio does read this in time, a v3 would be
> even better!

v3 is coming ;)

> 
> Thanks,
> -Steffan
> 
> --
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
> 

-- 
Antonio Quartulli

--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

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

2016-12-01 Thread Steffan Karger 
Hi,

Tested on linux and windows, works as expected, except for one thing:

On 01-12-16 07:55, Antonio Quartulli wrote:
> +  /*
> +   * an inline CRL can't change at runtime, therefore there is no need to
> +   * reload it. It will be reloaded upon config change + SIGHUP.
> +   * Use always '1' as dummy timestamp in this case: it will trigger the
> +   * first load, but will prevent any future reload.
> +   */
> +  if (crl_file_inline)
> +crl_stat.st_mtime = 1;
> +  else
> +platform_stat(crl_file, &crl_stat);

I still think this should issue a warning when we do not reload the CRL
because platform_stat() fails, e.g. due to the CRL file missing.  So, if
we replace this with:

  if (crl_file_inline)
{
  crl_stat.st_mtime = 1;
}
  else if (0 != platform_stat(crl_file, &crl_stat))
{
  msg (M_WARN, "WARNING: Failed to stat CRL file, using cached CRL.");
}

I can ACK this.

Since the deadline for 2.4_rc1 is this afternoon (CET), and Antonio
seems to be awake at quite different times than the Europeans doing the
tagging, maybe one of the committers can make this change on the file if
he agrees?  Of course, if Antonio does read this in time, a v3 would be
even better!

Thanks,
-Steffan

--
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel