[Pki-devel] [PATCH] 303-306 Various issues

[Ade Lee]

Patches 303, 305 and 306 have been modified as discussed and checked
in.

Patch 304 has been revised as discussed on IRC.  Please review.

Ade

On Fri, 2016-05-20 at 17:00 -0500, Endi Sukma Dewata wrote:
> On 5/20/2016 2:20 PM, Ade Lee wrote:
> > Please review:
> > 
> > Patches listed in reverse order (306 -> 303)
> > 
> > Ade
> 
> Some comments/questions:
> 
> Patch #303:
> 1. Instead of using underscores (i.e. ca.publishing.cert_enable and 
> ca.publishing.crl_enable) it would be more consistent to use dots
> (i.e. 
> ca.publishing.cert.enable and ca.publishing.crl.enable) in the
> parameter 
> names.
> 
> 2. The PublisherProcessor.isCertPublishingEnabled() and 
> isCRLPublishingEnabled() currently swallow the exception thrown by 
> getBoolean() and interpret it as disabled. I think since "this should
> never happen" the exception should (if not too much additional work)
> be 
> allowed to bubble up.
> 
> Patch #304:
> 1. I think the default maxAge and maxFiles should not be unlimited 
> because most people probably will use the default values until
> something 
> goes wrong (e.g. disk full), and we want to avoid that. It would be 
> better to pick something reasonable, for example 1 year and 100
> files, 
> respectively.
> 
> 2. Currently the unit for maxAge is hour. How long do people usually 
> want to retain old files? Should we use day instead?
> 
> 3. In purgeExcessFiles() the files are sorted by last modified 
> timestamp. It's kind of risky since someone could accidentally do 
> something that updates the timestamp, then code will be deleting a
> file 
> that's not supposed to be purged yet. Can the files be sorted by
> their 
> names? In Tomcat the log files can be sorted by their names since
> they 
> contain a timestamp or sequence number.
> 
> 4. Also in purgeExcessFiles() the last loop calls dir.listFiles() in 
> each iteration. For efficiency it might be better to use a counter
> since 
> the number of excess files can be computed before the loop.
> 
> 5. The exception thrown by getInteger() should not be swallowed
> either. 
> If there's a configuration problem we want to know that.
> 
> Patch #305:
> 1. It might be better to check for invalid revocation reason in the 
> RevocationReason.valueOf() itself so any code using it is guaranteed
> to 
> get a valid value.
> 
> Patch #306 will follow later.
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0304-Add-parameters-to-purge-old-published-files.patch
Type: text/x-patch
Size: 12142 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20160524/f8cb1d0f/attachment.bin>