[Pki-devel] [PATCH] 862 TPS rewrite: provide connector service for JAVA-based TPS subsystem
Christina Fu
cfu at redhat.com
Thu Mar 6 17:28:54 UTC 2014
I decided to change the default for connector enable's to false instead
of true so that the server can start even if no connectors are enabled.
Attached please find the small patch.
thanks,
Christina
On 03/05/2014 04:12 PM, Christina Fu wrote:
> 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.
>>
>
>
>
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20140306/9daaa8d5/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-trac-ticket-862-TPS-rewrite-provide-connector-servic.patch
Type: text/x-patch
Size: 2886 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20140306/9daaa8d5/attachment.bin>
More information about the Pki-devel
mailing list