<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
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. <br>
<br>
Attached please find the small patch.<br>
<br>
thanks,<br>
Christina<br>
<br>
<div class="moz-cite-prefix">On 03/05/2014 04:12 PM, Christina Fu
wrote:<br>
</div>
<blockquote cite="mid:5317BD5A.8050804@redhat.com" type="cite">Hi,
<br>
First of all, thank you Endi for your through review.
<br>
<br>
Per irc discussions, I have filed the following tickets:
<br>
* <a class="moz-txt-link-freetext" href="https://fedorahosted.org/pki/ticket/892">https://fedorahosted.org/pki/ticket/892</a> Need better error
handling for HttpConnection send()
<br>
- this ticket will address the comments irt pre-existing code
<br>
* <a class="moz-txt-link-freetext" href="https://fedorahosted.org/pki/ticket/893">https://fedorahosted.org/pki/ticket/893</a> subsystems (ca, kra,
tks, tps) mixed-up their subsystem certs when sharing same Tomcat
instance
<br>
- this ticket will provide investigation for an issue I found
irt shared tomcat instance causing mixed-up subsystem certs among
subsystems
<br>
<br>
Attached please find the patch that addressed the remaining
comments.
<br>
<br>
thanks,
<br>
Christina
<br>
<br>
On 03/04/2014 04:29 PM, Endi Sukma Dewata wrote:
<br>
<blockquote type="cite">Some comments:
<br>
<br>
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.
<br>
<br>
2. Also discussed over IRC, there's an existing issue with
HttpConnection constructors:
<br>
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/pki/ticket/891">https://fedorahosted.org/pki/ticket/891</a>
<br>
<br>
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:
<br>
<br>
if (timeout == 0) {
<br>
connector =
<br>
new HttpConnector(
<br>
null, nickname, remauthority,
<br>
resendInterval, conf);
<br>
} else {
<br>
connector =
<br>
new HttpConnector(
<br>
null, nickname, remauthority,
<br>
resendInterval, conf, timeout);
<br>
}
<br>
<br>
it can be simplified like this:
<br>
<br>
connector =
<br>
new HttpConnector(
<br>
null, nickname, remauthority,
<br>
resendInterval, conf, timeout);
<br>
<br>
There's also a similar code in HttpConnFactory.
<br>
<br>
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:
<br>
<br>
this(dest, factory, null);
<br>
<br>
...
<br>
<br>
if (op == null)
<br>
<br>
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.
<br>
<br>
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.
<br>
<br>
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.
<br>
<br>
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.
<br>
<br>
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):
<br>
<br>
try {
<br>
curConn = mConnFactory.getConn(op);
<br>
return curConn.send(msg);
<br>
<br>
} finally {
<br>
if (curConn != null) {
<br>
mConnFactory.returnConn(curConn);
<br>
}
<br>
}
<br>
<br>
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.
<br>
<br>
try {
<br>
ByteArrayInputStream bis =
<br>
new ByteArrayInputStream(text.getBytes());
<br>
return new XMLObject(bis);
<br>
<br>
} catch (Exception e) {
<br>
CMS.debug("TPSSubsystem: getXMLparser(): failed: "+
e.toString());
<br>
throw new RuntimeException(e);
<br>
}
<br>
<br>
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.
<br>
<br>
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.
<br>
<br>
11. Just some minor things:
<br>
- The debug log in getXMLparser() should say "ConnectionManager"
instead of "TPSSubsystem".
<br>
- If we use camel-case consistently, the name should be
getXMLParser() (with upper-case P).
<br>
- Some toString() invocations are redundant because string
concatenation will call it automatically.
<br>
<br>
I'll leave it to others to review the functionality correctness.
<br>
<br>
</blockquote>
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
Pki-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:Pki-devel@redhat.com">Pki-devel@redhat.com</a>
<a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/pki-devel">https://www.redhat.com/mailman/listinfo/pki-devel</a></pre>
</blockquote>
<br>
</body>
</html>