[
https://issues.apache.org/jira/browse/QPID-4334?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13465491#comment-13465491
]
Keith Wall commented on QPID-4334:
----------------------------------
Hi Phil
Looks good, I just have a couple of comments, I think most of which are
actually against the original implementation.
AclRulePredicates.java
64 - Guard debug logging
Action.java
77 - public setOperation, setObjectType, setProperties could be private and
members final
107 - comment re compareTo seems spurious
112 - the refactoring means we now always evaluate operation, objectType and
properties rather than potentially short circuiting earlier. Also could
properties ever end up being null?
PlainConfiguration.jav
75 - load should be closing the underlying reader
HostnameFirewallRule.java
68 - Add the IP that failed to lookup to the text of the
AccessControlFirewallException
100 - I think we should be logging at warn the case where rDNS lookups are
failing/timing out. I worry that a sporadic DNS failure might lead to
mysterious Java Broker defect reports.
{code}
public String call()
{
boolean success = false;
try
{
String hn = remote.getCanonicalHostName();
success = true;
return hn;
}
finally
{
if (!success)
{
log.warn("Failed to get canonical for " + ip + ", DNS timeout or
error.");.
}
}
}
{code}
InetAddress.java
100 - confusing braces
177 - seemingly unnecessary use of reflection. InetAddress.getByAddress has
been part of the API since 1.4
AccessControlFirewallException.java/InetNetwork.java
code style (brace positions)
> [Java broker] move the Firewall functionality into the ACL plugin
> -----------------------------------------------------------------
>
> Key: QPID-4334
> URL: https://issues.apache.org/jira/browse/QPID-4334
> Project: Qpid
> Issue Type: Improvement
> Components: Java Broker
> Reporter: Robbie Gemmell
> Assignee: Keith Wall
> Fix For: 0.19
>
> Attachments:
> 0001-QPID-4334-removed-the-firewall-plugin-and-moved-its-.patch,
> 0001-QPID-4334-updated-the-Java-ACL-docbook-to-mention-it.patch
>
>
> Firewall rules are currently expressed separately from ACls, but this plugin
> is effectively an access control plugin and it is felt that the functionality
> would thus be better expressed using ACL rules. Such request has been made in
> the past, including a patch for the C++ broker as in use by a user.
> The firewall functionality will be moved into the ACL plugin, and the
> firewall plugin removed.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
