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

[Endi Sukma Dewata]

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.

-- 
Endi S. Dewata