You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by no...@apache.org on 2006/07/30 04:10:34 UTC
svn commit: r426858 -
/james/server/branches/v2.3/src/java/org/apache/james/smtpserver/DNSRBLHandler.java
Author: noel
Date: Sat Jul 29 19:10:34 2006
New Revision: 426858
URL: http://svn.apache.org/viewvc?rev=426858&view=rev
Log:
Add a DEBUG log entry, and correct a INFO log entry that didn't include enough information.
Modified:
james/server/branches/v2.3/src/java/org/apache/james/smtpserver/DNSRBLHandler.java
Modified: james/server/branches/v2.3/src/java/org/apache/james/smtpserver/DNSRBLHandler.java
URL: http://svn.apache.org/viewvc/james/server/branches/v2.3/src/java/org/apache/james/smtpserver/DNSRBLHandler.java?rev=426858&r1=426857&r2=426858&view=diff
==============================================================================
--- james/server/branches/v2.3/src/java/org/apache/james/smtpserver/DNSRBLHandler.java (original)
+++ james/server/branches/v2.3/src/java/org/apache/james/smtpserver/DNSRBLHandler.java Sat Jul 29 19:10:34 2006
@@ -119,14 +119,27 @@
if (whitelist != null) {
String[] rblList = whitelist;
for (int i = 0 ; i < rblList.length ; i++) try {
- org.apache.james.dnsserver.DNSServer.getByName(reversedOctets + rblList[i]);
+ java.net.InetAddress addr = org.apache.james.dnsserver.DNSServer.getByName(reversedOctets + rblList[i]);
if (getLogger().isInfoEnabled()) {
getLogger().info("Connection from " + ipAddress + " whitelisted by " + rblList[i]);
}
+
+ /* Ihis code may be helpful if admins need to debug why they are getting weird
+ behavior from the blocklists. Also, it might help them to know what IP is
+ returned, since zones often use that to indicate interesting information.
+
+ At some point, we may wish to do something more interesting with both the A
+ and TXT records from block lists, at which point this code can probably be
+ removed.
+ */
+ if (getLogger().isDebugEnabled()) {
+ getLogger().debug("Whitelist addr = " + addr.toString());
+ }
+
return false;
} catch (java.net.UnknownHostException uhe) {
if (getLogger().isDebugEnabled()) {
- getLogger().debug("unknown host exception thrown:" + rblList[i]);
+ getLogger().debug("unknown host exception thrown:" + reversedOctets + rblList[i]);
}
}
}
@@ -134,15 +147,28 @@
if (blacklist != null) {
String[] rblList = blacklist;
for (int i = 0 ; i < rblList.length ; i++) try {
- org.apache.james.dnsserver.DNSServer.getByName(reversedOctets + rblList[i]);
+ java.net.InetAddress addr = org.apache.james.dnsserver.DNSServer.getByName(reversedOctets + rblList[i]);
if (getLogger().isInfoEnabled()) {
getLogger().info("Connection from " + ipAddress + " restricted by " + rblList[i] + " to SMTP AUTH/postmaster/abuse.");
}
+
+ /* Ihis code may be helpful if admins need to debug why they are getting weird
+ behavior from the blocklists. Also, it might help them to know what IP is
+ returned, since zones often use that to indicate interesting information.
+
+ At some point, we may wish to do something more interesting with both the A
+ and TXT records from block lists, at which point this code can probably be
+ removed.
+ */
+ if (getLogger().isDebugEnabled()) {
+ getLogger().debug("Blacklist addr = " + addr.toString());
+ }
+
return true;
} catch (java.net.UnknownHostException uhe) {
// if it is unknown, it isn't blocked
if (getLogger().isDebugEnabled()) {
- getLogger().debug("unknown host exception thrown:" + rblList[i]);
+ getLogger().debug("unknown host exception thrown:" + reversedOctets + rblList[i]);
}
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: svn commit: r426858 - /james/server/branches/v2.3/src/java/org/apache/james/smtpserver/DNSRBLHandler.java
Posted by Stefano Bagnara <ap...@bago.org>.
Noel J. Bergman wrote:
> Stefano Bagnara wrote:
>
>> As an example you added an addr.toString(): This could raise a
>> NullPointerException.
>
> LOL. Amusingly, I actually had (addr == null) ? "null?" : addr.toString() in the original code, but removed it. If that throws an NPE, there are other problems.
You know I already said that I reviewed the code and that I verified
that addr could never be null, but this is not the point. The point is
that this is a manual review and, like I already said, more dangerous
than removing unused code.
Unfortunately NPE are not handled by the java compilers. It would be
different if we were using Nice (http://nice.sourceforge.net/).
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
RE: svn commit: r426858 - /james/server/branches/v2.3/src/java/org/apache/james/smtpserver/DNSRBLHandler.java
Posted by "Noel J. Bergman" <no...@devtech.com>.
Stefano Bagnara wrote:
> As an example you added an addr.toString(): This could raise a
> NullPointerException.
LOL. Amusingly, I actually had (addr == null) ? "null?" : addr.toString() in the original code, but removed it. If that throws an NPE, there are other problems.
--- Noel
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: svn commit: r426858 - /james/server/branches/v2.3/src/java/org/apache/james/smtpserver/DNSRBLHandler.java
Posted by Stefano Bagnara <ap...@bago.org>.
As an example you added an addr.toString(): This could raise a
NullPointerException.
I reviewed the code that return the addr variable and I now know that it
never return null but this is *manual* review, *error* *prone*.
Instead the jar have been removed in trunk since weeks and we run that
code without any problem. I'm still really convinced that removing an
unused jar is MUCH LESS dungerous that changing a single line of code
(even a comment).
Btw this is personal opinion. I just wanted to put emphasys on the fact
that I don't like this and if I have to accept vetoes to removal of jars
I should be free to cast at least a -0 about this changes without too
much discussions.
Again, and for the last time: I think that all of this stuff belong to
the RC process, but I simply aligned myself to the fact that other
committers seems to require a more strict approach to changes while in
rc. So this deserve my -0 and my dissappoint. About 2.4 I'll talk after
2.3.0 final. Stop.
Stefano
Noel J. Bergman wrote:
> Stefano Bagnara wrote:
>
>> I still don't get how all of this changes can be less dungerous than
>> removing an unused jar (JAMES-515).
>
> OK, let's compare. I made the following kinds of changes:
>
> - added a few lines of clear code to generate an optional log entry,
> which was helpful in diagnosing a problem
> - changed an incorrect log level from .info() to .debug()
> - corrected the contents of a static text string
>
> Now, while these are changes that should be reviewed, they are also things that the compiler can do at least some sanity checking on, and are rather different from removing JAR files where even you had some questions to make sure that something wasn't being missed, and which someone else might be using. The level of visibility to the impact of the change is not the same.
>
> Although JAMES-515 does not really meet the LOW-RISK, HIGH-VALUE criteria, I would be OK to make that change for v2.4.
>
> --- Noel
>
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: svn commit: r426858 -
/james/server/branches/v2.3/src/java/org/apache/james/smtpserver/DNSRBLHandler.java
Posted by Norman Maurer <nm...@byteaction.de>.
Hi peoples,
its time for me to say something too ;-)
Well, i understand Stefano.. I think his problem is that we change it in
RC and not in beta.. Cause it's a "small" step from RC to final its some
kind of "dangerous". Even if its just a small change. This kind of
thinkin safe us to indreduce new bugs..
I think he whouldn't have problems with the change if it was made i beta
stage.
For me the jar removal was ok in beta stage ( like you can see from my
vote about this), for others not.. Anyway we should try to not change
anything in the code if not needed in RC stage.
Anyhow, i don'T think the change will cause any problems. But we should
try to not break the (unwritten) rule of not change any code in RC if no
critical BUG will be fixed with the change.
Maybe the best way to solve such "problems" whould be to try to test
earlier the james releases.. So we can do such changes before we reach
RC stage..
Don't get me wrong. Its not a personal critic .. its just my thinkin.
bye
Norman
Am Sonntag, den 30.07.2006, 20:40 +0200 schrieb Stefano Bagnara:
> As an example you added an addr.toString(): This could raise a
> NullPointerException.
> I reviewed the code that return the addr variable and I now know that it
> never return null but this is *manual* review, *error* *prone*.
>
> Instead the jar have been removed in trunk since weeks and we run that
> code without any problem. I'm still really convinced that removing an
> unused jar is MUCH LESS dungerous that changing a single line of code
> (even a comment).
>
> Btw this is personal opinion. I just wanted to put emphasys on the fact
> that I don't like this and if I have to accept vetoes to removal of jars
> I should be free to cast at least a -0 about this changes without too
> much discussions.
>
> Again, and for the last time: I think that all of this stuff belong to
> the RC process, but I simply aligned myself to the fact that other
> committers seems to require a more strict approach to changes while in
> rc. So this deserve my -0 and my dissappoint. About 2.4 I'll talk after
> 2.3.0 final. Stop.
>
> Stefano
>
> Noel J. Bergman wrote:
> > Stefano Bagnara wrote:
> >
> >> I still don't get how all of this changes can be less dungerous than
> >> removing an unused jar (JAMES-515).
> >
> > OK, let's compare. I made the following kinds of changes:
> >
> > - added a few lines of clear code to generate an optional log entry,
> > which was helpful in diagnosing a problem
> > - changed an incorrect log level from .info() to .debug()
> > - corrected the contents of a static text string
> >
> > Now, while these are changes that should be reviewed, they are also things that the compiler can do at least some sanity checking on, and are rather different from removing JAR files where even you had some questions to make sure that something wasn't being missed, and which someone else might be using. The level of visibility to the impact of the change is not the same.
> >
> > Although JAMES-515 does not really meet the LOW-RISK, HIGH-VALUE criteria, I would be OK to make that change for v2.4.
> >
> > --- Noel
> >
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
> For additional commands, e-mail: server-dev-help@james.apache.org
>
> !EXCUBATOR:1,44ccfd6443381146759681!
RE: svn commit: r426858 - /james/server/branches/v2.3/src/java/org/apache/james/smtpserver/DNSRBLHandler.java
Posted by "Noel J. Bergman" <no...@devtech.com>.
Stefano Bagnara wrote:
> I still don't get how all of this changes can be less dungerous than
> removing an unused jar (JAMES-515).
OK, let's compare. I made the following kinds of changes:
- added a few lines of clear code to generate an optional log entry,
which was helpful in diagnosing a problem
- changed an incorrect log level from .info() to .debug()
- corrected the contents of a static text string
Now, while these are changes that should be reviewed, they are also things that the compiler can do at least some sanity checking on, and are rather different from removing JAR files where even you had some questions to make sure that something wasn't being missed, and which someone else might be using. The level of visibility to the impact of the change is not the same.
Although JAMES-515 does not really meet the LOW-RISK, HIGH-VALUE criteria, I would be OK to make that change for v2.4.
--- Noel
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: svn commit: r426858 - /james/server/branches/v2.3/src/java/org/apache/james/smtpserver/DNSRBLHandler.java
Posted by Stefano Bagnara <ap...@bago.org>.
I still don't get how all of this changes can be less dungerous than
removing an unused jar (JAMES-515).
I will not veto this, but I'm not fine with the rules (what are?) about
what we can commit in 2.3 while in rc and what we can't.
Stefano
noel@apache.org wrote:
> Author: noel
> Date: Sat Jul 29 19:10:34 2006
> New Revision: 426858
>
> URL: http://svn.apache.org/viewvc?rev=426858&view=rev
> Log:
> Add a DEBUG log entry, and correct a INFO log entry that didn't include enough information.
>
> Modified:
> james/server/branches/v2.3/src/java/org/apache/james/smtpserver/DNSRBLHandler.java
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org