[Pki-devel] [PATCH] 862 TPS rewrite: provide connector service for JAVA-based TPS subsystem
Christina Fu
cfu at redhat.com
Thu Mar 6 00:12:10 UTC 2014
Hi,
First of all, thank you Endi for your through review.
Per irc discussions, I have filed the following tickets:
* https://fedorahosted.org/pki/ticket/892 Need better error handling for
HttpConnection send()
- this ticket will address the comments irt pre-existing code
* https://fedorahosted.org/pki/ticket/893 subsystems (ca, kra, tks, tps)
mixed-up their subsystem certs when sharing same Tomcat instance
- this ticket will provide investigation for an issue I found irt
shared tomcat instance causing mixed-up subsystem certs among subsystems
Attached please find the patch that addressed the remaining comments.
thanks,
Christina
On 03/04/2014 04:29 PM, Endi Sukma Dewata wrote:
> Some comments:
>
> 1. As discussed over IRC the common code HttpConnection shouldn't
> contain a subsystem-specific code, which in this case is setting the
> content type to application/x-www-form-urlencoded for TPS.
>
> 2. Also discussed over IRC, there's an existing issue with
> HttpConnection constructors:
> https://fedorahosted.org/pki/ticket/891
>
> Suppose that issue is fixed later, it should be possible to merge the
> constructors. Then instead of calling different constructors for
> different timeout values like this:
>
> if (timeout == 0) {
> connector =
> new HttpConnector(
> null, nickname, remauthority,
> resendInterval, conf);
> } else {
> connector =
> new HttpConnector(
> null, nickname, remauthority,
> resendInterval, conf, timeout);
> }
>
> it can be simplified like this:
>
> connector =
> new HttpConnector(
> null, nickname, remauthority,
> resendInterval, conf, timeout);
>
> There's also a similar code in HttpConnFactory.
>
> 4. In HttpConnection the existing constructors were changed to pass ""
> as the default value of op. I think we should just pass a null value.
> This way the code can simply check the op like this:
>
> this(dest, factory, null);
>
> ...
>
> if (op == null)
>
> If we really want to accept an empty string, then probably we should
> normalize the string by trimming the white spaces before checking for
> empty string.
>
> 5. The HttpConnection.send() checks if the parameter is null or empty
> string then returns a null value with a comment "throw later". This
> error will only be detected later if another code tries to use the
> null return value, but at that point we're looking at a secondary
> issue, which makes it harder to troubleshoot. I think in general it's
> better to throw an exception as soon as the error is detected.
>
> 6. The HttpConnection.send() checks for untrusted certificate by
> checking the exception message. This may work now but could become
> inaccurate later due to translation, text changes, or typo. It would
> be better if there's a specific exception type or error code that we
> can check for this specific case.
>
> 7. Similar to #5, HttpConnection.send() swallows
> NumberFormatException. I think we should not catch this to detect the
> error as early as possible and also NumberFormatException is already a
> RuntimeException so it doesn't need to be wrapped/declared.
>
> 8. Similar to #5, the HttpConnector.send() (not HttpConnection) would
> swallow the exception so some errors wouldn't be detected immediately.
> I think the code could be simplified as follows (the send() already
> throws EBaseException):
>
> try {
> curConn = mConnFactory.getConn(op);
> return curConn.send(msg);
>
> } finally {
> if (curConn != null) {
> mConnFactory.returnConn(curConn);
> }
> }
>
> 9. Similar to #5, the ConnectionManager.getXMLparser() would swallow
> the exceptions. I think it would be better to wrap the exception in a
> RuntimeException and rethrow it.
>
> try {
> ByteArrayInputStream bis =
> new ByteArrayInputStream(text.getBytes());
> return new XMLObject(bis);
>
> } catch (Exception e) {
> CMS.debug("TPSSubsystem: getXMLparser(): failed: "+
> e.toString());
> throw new RuntimeException(e);
> }
>
> We can change this code again in the future if necessary to use a more
> specific class or add the original exceptions into the throws list.
>
> 10. In ConnectionManager, the createConnector() will never return a
> null, so it's not necessary to check its return value in
> initConnectors(). It will simplify the code and some code analyzers
> like Coverity probably will report the part that handles the null case
> as dead code anyway.
>
> 11. Just some minor things:
> - The debug log in getXMLparser() should say "ConnectionManager"
> instead of "TPSSubsystem".
> - If we use camel-case consistently, the name should be getXMLParser()
> (with upper-case P).
> - Some toString() invocations are redundant because string
> concatenation will call it automatically.
>
> I'll leave it to others to review the functionality correctness.
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-trac-ticket-862-TPS-rewrite-provide-connector-servic.patch
Type: text/x-patch
Size: 23186 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20140305/d1db1a0f/attachment.bin>
More information about the Pki-devel
mailing list