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@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel