- UPnP defaults to on - I didn't do all this for nothing

- Set Status to OK for local public addresses or UPnP port open
- Allow UDP address changes after we transition to firewalled
- Have NTCP start reporting reachability status, this will
  get OK on the console more often and mask UDP problems,
  which might be good or bad...
- Fix UDP port configuration
- Reword and rearrange configuration options again
- Rearrange configuration help
- More right-alignment on config
- Prevent Concurrent modification exception in UPnP
- UPnP HTML output tweaks - remove "plugin" references
- Move UDP message failed log from WARN to INFO
- Short-circuit message history call in UDP

apps/routerconsole/java/src/net/i2p/router/web/ConfigNetHelper.java

@@ -132,8 +132,11 @@ public class ConfigNetHelper extends HelperBase {
         return "";
     }
 
+    /** default true */
     public String getUpnpChecked() {
-        return getChecked(TransportManager.PROP_ENABLE_UPNP);
+        if (Boolean.valueOf(_context.getProperty(TransportManager.PROP_ENABLE_UPNP, "true")).booleanValue())
+            return CHECKED;
+        return "";
     }
 
     public String getRequireIntroductionsChecked() {
@@ -229,7 +232,7 @@ public class ConfigNetHelper extends HelperBase {
     public String getSharePercentageBox() {
         int pct = (int) (100 * _context.router().getSharePercentage());
         StringBuffer buf = new StringBuffer(256);
-        buf.append("<select name=\"sharePercentage\">\n");
+        buf.append("<select style=\"text-align: right;\" name=\"sharePercentage\">\n");
         boolean found = false;
         for (int i = 30; i <= 110; i += 10) {
             int val = i;
@@ -239,7 +242,7 @@ public class ConfigNetHelper extends HelperBase {
                 else
                     val = pct;
             }
-            buf.append("<option value=\"").append(val).append("\" ");
+            buf.append("<option style=\"text-align: right;\" value=\"").append(val).append("\" ");
             if (pct == val) {
                 buf.append("selected=\"true\" ");
                 found = true;

apps/routerconsole/jsp/config.jsp

@@ -83,13 +83,13 @@
  <b>IP Configuration:</b><br />
  Externally reachable hostname or IP address:<br />
     <input type="radio" name="udpAutoIP" value="local,upnp,ssu" <%=nethelper.getUdpAutoIPChecked(3) %> />
-    Use local public address if available, then UPnP detection, then SSU detection<br />
+    Use all auto-detect methods<br />
     <input type="radio" name="udpAutoIP" value="local,ssu" <%=nethelper.getUdpAutoIPChecked(4) %> />
-    Use local public address if available, then SSU detection<br />
+    Disable UPnP IP address detection<br />
     <input type="radio" name="udpAutoIP" value="upnp,ssu" <%=nethelper.getUdpAutoIPChecked(5) %> />
-    Use UPnP detection if available, then SSU detection<br />
+    Ignore local interface IP address<br />
     <input type="radio" name="udpAutoIP" value="ssu" <%=nethelper.getUdpAutoIPChecked(0) %> />
-    Use SSU detection only<br />
+    Use SSU IP address detection only<br />
     <input type="radio" name="udpAutoIP" value="fixed" <%=nethelper.getUdpAutoIPChecked(1) %> />
     Specify hostname or IP:
     <input name ="udpHost1" type="text" size="16" value="<jsp:getProperty name="nethelper" property="udphostname" />" />
@@ -112,34 +112,27 @@
  </p><p>
  <b>UDP Configuration:</b><br />
  Internal UDP port:
- <input name ="udpPort" type="text" size="5" maxlength="5" vvalue="<jsp:getProperty name="nethelper" property="configuredUdpPort" />" /><br />
+ <input name ="udpPort" type="text" size="5" maxlength="5" value="<jsp:getProperty name="nethelper" property="configuredUdpPort" />" /><br />
 <input type="checkbox" name="requireIntroductions" value="true" <jsp:getProperty name="nethelper" property="requireIntroductionsChecked" /> />
  Require SSU introductions
  <i>(Enable if you cannot open your firewall)</i>
  </p><p>
  Current External UDP address: <i><jsp:getProperty name="nethelper" property="udpAddress" /></i><br />
- </p><p>If you can, please poke a hole in your NAT or firewall to allow unsolicited UDP packets to reach
-    you on your external UDP address.  If you can't, I2P now includes supports UDP hole punching
-    with "SSU introductions" - peers who will relay a request from someone you don't know to your
-    router for your router so that you can make an outbound connection to them.  I2P will use these
-    introductions automatically if it detects that the port is not forwarded (as shown by
-    the <i>Reachability: Firewalled</i> line), or you can manually require them here.  
-    Users behind symmetric NATs, such as OpenBSD's pf, are not currently supported.</p>
-<input type="submit" name="recheckReachability" value="Check network reachability..." />
  </p><p>
  <b>Inbound TCP Configuration:</b><br />
  Externally reachable hostname or IP address:<br />
+    <input type="radio" name="ntcpAutoIP" value="true" <%=nethelper.getTcpAutoIPChecked(2) %> />
+    Use auto-detected IP address
+    <i>(currently <jsp:getProperty name="nethelper" property="udpIP" />)</i>
+    if we are not firewalled<br />
+    <input type="radio" name="ntcpAutoIP" value="always" <%=nethelper.getTcpAutoIPChecked(3) %> />
+    Always use auto-detected IP address (Not firewalled)<br />
     <input type="radio" name="ntcpAutoIP" value="false" <%=nethelper.getTcpAutoIPChecked(0) %> />
     Disable (Firewalled)<br />
-    <input type="radio" name="ntcpAutoIP" value="always" <%=nethelper.getTcpAutoIPChecked(3) %> />
-    Use IP address detected by SSU (Not firewalled)
-    <i>(currently <jsp:getProperty name="nethelper" property="udpIP" />)</i><br />
-    <input type="radio" name="ntcpAutoIP" value="true" <%=nethelper.getTcpAutoIPChecked(2) %> />
-    Use IP address detected by SSU, only if we do not appear to be firewalled<br />
     <input type="radio" name="ntcpAutoIP" value="false" <%=nethelper.getTcpAutoIPChecked(1) %> />
     Specify hostname or IP:
     <input name ="ntcphost" type="text" size="16" value="<jsp:getProperty name="nethelper" property="ntcphostname" />" />
-    <i>(dyndns and the like are fine)</i><br />
+    <br />
  </p><p>
  Externally reachable TCP port:<br />
     <input type="radio" name="ntcpAutoPort" value="2" <%=nethelper.getTcpAutoPortChecked(2) %> />
@@ -148,7 +141,21 @@
     <input type="radio" name="ntcpAutoPort" value="1" <%=nethelper.getTcpAutoPortChecked(1) %> />
     Specify Port:
     <input name ="ntcpport" type="text" size="5" maxlength="5" value="<jsp:getProperty name="nethelper" property="ntcpport" />" /><br />
- </p><p>Hostnames entered here will be published in the network database.
+ </p><p><b>Note: changing any of these settings will terminate all of your connections and effectively
+    restart your router.</b>
+ </p>
+ <input type="submit" name="save" value="Save changes" /> <input type="reset" value="Cancel" /><br />
+ <hr />
+ <b><a name="chelp">Configuration Help:</a></b>
+ <p>If you can, please poke a hole in your NAT or firewall to allow unsolicited UDP packets to reach
+    you on your external UDP address.  If you can't, I2P now includes supports UDP hole punching
+    with "SSU introductions" - peers who will relay a request from someone you don't know to your
+    router for your router so that you can make an outbound connection to them.  I2P will use these
+    introductions automatically if it detects that the port is not forwarded (as shown by
+    the <i>Reachability: Firewalled</i> line), or you can manually require them here.  
+    Users behind symmetric NATs, such as OpenBSD's pf, are not currently supported.</p>
+<input type="submit" name="recheckReachability" value="Check network reachability..." />
+ <p>Hostnames entered here will be published in the network database.
     They are <b>not private</b>.
     Also, <b>do not enter a private IP address</b> like 127.0.0.1 or 192.168.1.1.
  </p>
@@ -157,10 +164,6 @@
     in your NAT or firewall for unsolicited TCP connections.  If you specify the wrong IP address or
     hostname, or do not properly configure your NAT or firewall, your network performance will degrade
     substantially.  When in doubt, leave the hostname and port number blank.</p>
- <p><b>Note: changing any of these settings will terminate all of your connections and effectively
-    restart your router.</b>
- </p>
- <input type="submit" name="save" value="Save changes" /> <input type="reset" value="Cancel" /><br />
  <hr />
  <b><a name="help">Reachability Help:</a></b>
  <p>

router/java/src/net/i2p/router/MessageHistory.java

@@ -61,7 +61,7 @@ public class MessageHistory {
     }
     
     void setDoLog(boolean log) { _doLog = log; }
-    boolean getDoLog() { return _doLog; }
+    public boolean getDoLog() { return _doLog; }
     
     void setPauseFlushes(boolean doPause) { _doPause = doPause; }
     String getFilename() { return _historyFile; }

router/java/src/net/i2p/router/transport/TransportManager.java

@@ -44,6 +44,7 @@ public class TransportManager implements TransportEventListener {
     private final static String PROP_ENABLE_NTCP = "i2np.ntcp.enable";
     private final static String DEFAULT_ENABLE_NTCP = "true";
     private final static String DEFAULT_ENABLE_UDP = "true";
+    /** default true */
     public final static String PROP_ENABLE_UPNP = "i2np.upnp.enable";
     
     public TransportManager(RouterContext context) {
@@ -56,7 +57,7 @@ public class TransportManager implements TransportEventListener {
         _context.statManager().createRateStat("transport.bidFailNoTransports", "Could not attempt to bid on message, as none of the transports could attempt it", "Transport", new long[] { 60*1000, 10*60*1000, 60*60*1000 });
         _context.statManager().createRateStat("transport.bidFailAllTransports", "Could not attempt to bid on message, as all of the transports had failed", "Transport", new long[] { 60*1000, 10*60*1000, 60*60*1000 });
         _transports = new ArrayList();
-        if (Boolean.valueOf(_context.getProperty(PROP_ENABLE_UPNP)).booleanValue())
+        if (Boolean.valueOf(_context.getProperty(PROP_ENABLE_UPNP, "true")).booleanValue())
             _upnpManager = new UPnPManager(context, this);
     }
     
@@ -227,15 +228,15 @@ public class TransportManager implements TransportEventListener {
         return skews;
     }
     
+    /** @return the best status of any transport */
     public short getReachabilityStatus() { 
-        if (_transports.size() <= 0) return CommSystemFacade.STATUS_UNKNOWN;
+        short rv = CommSystemFacade.STATUS_UNKNOWN;
-        short status[] = new short[_transports.size()];
+        for (Transport t : _transports) {
-        for (int i = 0; i < _transports.size(); i++) {
+            short s = t.getReachabilityStatus();
-            status[i] = ((Transport)_transports.get(i)).getReachabilityStatus();
+            if (s < rv)
+                rv = s;
         }
-        // the values for the statuses are increasing for their 'badness'
+        return rv;
-        Arrays.sort(status);
-        return status[0];
     }
 
     public void recheckReachability() { 

router/java/src/net/i2p/router/transport/UPnP.java

@@ -232,7 +232,7 @@ public class UPnP extends ControlPoint implements DeviceChangeListener {
 	public void unregisterPortMappings() {
 		Set ports;
 		synchronized(lock) {
-			ports = portsForwarded;
+			ports = new HashSet(portsForwarded);
 		}
 		this.unregisterPorts(ports);
 	}
@@ -406,10 +406,10 @@ public class UPnP extends ControlPoint implements DeviceChangeListener {
 		sb.append("<a name=\"upnp\"><b>UPnP Status:</b><br />");
 		
 		if(isDisabled) {
-			sb.append("The plugin has been disabled; Do you have more than one UPnP Internet Gateway Device on your LAN ?");
+			sb.append("UPnP has been disabled; Do you have more than one UPnP Internet Gateway Device on your LAN ?");
 			return sb.toString();
 		} else if(!isNATPresent()) {
-			sb.append("The plugin hasn't found any UPnP aware, compatible device on your LAN.");
+			sb.append("UPnP has not found any UPnP-aware, compatible device on your LAN.");
 			return sb.toString();
 		}
 		

router/java/src/net/i2p/router/transport/ntcp/NTCPTransport.java

@@ -20,6 +20,7 @@ import net.i2p.data.Hash;
 import net.i2p.data.RouterAddress;
 import net.i2p.data.RouterIdentity;
 import net.i2p.data.RouterInfo;
+import net.i2p.router.CommSystemFacade;
 import net.i2p.router.OutNetMessage;
 import net.i2p.router.RouterContext;
 import net.i2p.router.transport.CommSystemFacadeImpl;
@@ -37,7 +38,7 @@ public class NTCPTransport extends TransportImpl {
     private SharedBid _slowBid;
     private SharedBid _transientFail;
     private final Object _conLock;
-    private Map _conByIdent;
+    private Map<Hash, NTCPConnection> _conByIdent;
     private NTCPAddress _myAddress;
     private EventPumper _pumper;
     private Reader _reader;
@@ -566,6 +567,27 @@ public class NTCPTransport extends TransportImpl {
         return _context.getProperty(CommSystemFacadeImpl.PROP_I2NP_NTCP_PORT, -1);
     }
 
+    /**
+     * Maybe we should trust UPnP here and report OK if it opened the port, but
+     * for now we don't. Just go through and if we have one inbound connection,
+     * we must be good. As we drop idle connections pretty quickly, this will
+     * be fairly accurate.
+     *
+     * We have to be careful here because much of the router console code assumes
+     * that the reachability status is really just the UDP status.
+     */
+    public short getReachabilityStatus() { 
+        if (isAlive() && _myAddress != null) {
+            synchronized (_conLock) {
+                for (NTCPConnection con : _conByIdent.values()) {
+                    if (con.isInbound())
+                        return CommSystemFacade.STATUS_OK;
+                }
+            }
+        }
+        return CommSystemFacade.STATUS_UNKNOWN;
+    }
+
     /**
      *  This doesn't (completely) block, caller should check isAlive()
      *  before calling startListening() or restartListening()

router/java/src/net/i2p/router/transport/udp/UDPTransport.java

@@ -346,7 +346,28 @@ public class UDPTransport extends TransportImpl implements TimedWeightedPriority
         String sources = _context.getProperty(PROP_SOURCES, DEFAULT_SOURCES);
         if (!sources.contains(source))
             return;
-        changeAddress(ip, port);
+        boolean changed = changeAddress(ip, port);
+        // Assume if we have an interface with a public IP that we aren't firewalled.
+        // If this is wrong, the peer test will figure it out and change the status.
+        if (changed && source.equals(Transport.SOURCE_INTERFACE))
+            setReachabilityStatus(CommSystemFacade.STATUS_OK);
+    }
+
+    /**
+     *  Callback from UPnP.
+     *  If we we have an IP address and UPnP claims success, believe it.
+     *  If this is wrong, the peer test will figure it out and change the status.
+     *  Don't do anything if UPnP claims failure.
+     */
+    public void forwardPortStatus(int port, boolean success, String reason) {
+        if (_log.shouldLog(Log.WARN)) {
+            if (success)
+                _log.warn("UPnP has opened the SSU port: " + port);
+            else
+                _log.warn("UPnP has failed to open the SSU port: " + port + " reason: " + reason);
+        }
+        if (success && _externalListenHost != null)
+            setReachabilityStatus(CommSystemFacade.STATUS_OK);
     }
 
     /**
@@ -397,7 +418,7 @@ public class UDPTransport extends TransportImpl implements TimedWeightedPriority
     /**
      * @param ourPort >= 1024 or 0 for no change
      */
-    private void changeAddress(byte ourIP[], int ourPort) {
+    private boolean changeAddress(byte ourIP[], int ourPort) {
         boolean fixedPort = getIsPortFixed();
         boolean updated = false;
         boolean fireTest = false;
@@ -405,7 +426,7 @@ public class UDPTransport extends TransportImpl implements TimedWeightedPriority
             synchronized (this) {
                 if ( (_externalListenHost == null) ||
                      (!eq(_externalListenHost.getAddress(), _externalListenPort, ourIP, ourPort)) ) {
-                    if ( (_reachabilityStatus == CommSystemFacade.STATUS_UNKNOWN) ||
+                    if ( (_reachabilityStatus != CommSystemFacade.STATUS_OK) ||
                          (_externalListenHost == null) || (_externalListenPort <= 0) ||
                          (_context.clock().now() - _reachabilityStatusLastUpdated > 2*TEST_FREQUENCY) ) {
                         // they told us something different and our tests are either old or failing
@@ -452,6 +473,7 @@ public class UDPTransport extends TransportImpl implements TimedWeightedPriority
             _testEvent.forceRun();
             SimpleTimer.getInstance().addEvent(_testEvent, 5*1000);
         }
+        return updated;
     }
 
     private static final boolean eq(byte laddr[], int lport, byte raddr[], int rport) {
@@ -1199,8 +1221,8 @@ public class UDPTransport extends TransportImpl implements TimedWeightedPriority
             //                + " lastSentFully: " + sendDelay
             //                + " expired? " + msg.isExpired());
             consecutive = msg.getPeer().incrementConsecutiveFailedSends();
-            if (_log.shouldLog(Log.WARN))
+            if (_log.shouldLog(Log.INFO))
-                _log.warn("Consecutive failure #" + consecutive 
+                _log.info("Consecutive failure #" + consecutive 
                           + " on " + msg.toString()
                           + " to " + msg.getPeer());
             if ( (_context.clock().now() - msg.getPeer().getLastSendFullyTime() <= 60*1000) || (consecutive < MAX_CONSECUTIVE_FAILED) ) {
@@ -1220,6 +1242,9 @@ public class UDPTransport extends TransportImpl implements TimedWeightedPriority
     }
     
     private void noteSend(OutboundMessageState msg, boolean successful) {
+        // bail before we do all the work
+        if (!_context.messageHistory().getDoLog())
+            return;
         int pushCount = msg.getPushCount();
         int sends = msg.getMaxSends();
         boolean expired = msg.isExpired();
@@ -1256,10 +1281,11 @@ public class UDPTransport extends TransportImpl implements TimedWeightedPriority
     
     public void failed(OutNetMessage msg, String reason) {
         if (msg == null) return;
-        if (_log.shouldLog(Log.WARN))
+        if (_log.shouldLog(Log.INFO))
-            _log.warn("Sending message failed: " + msg, new Exception("failed from"));
+            _log.info("Sending message failed: " + msg, new Exception("failed from"));
         
-        _context.messageHistory().sendMessage(msg.getMessageType(), msg.getMessageId(), msg.getExpiration(), 
+        if (!_context.messageHistory().getDoLog())
+            _context.messageHistory().sendMessage(msg.getMessageType(), msg.getMessageId(), msg.getExpiration(), 
                                               msg.getTarget().getIdentity().calculateHash(), false, reason);
         super.afterSend(msg, false);
     }