[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