You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mina.apache.org by ng...@apache.org on 2011/06/18 23:40:01 UTC
svn commit: r1137252 - in /mina/ftpserver/branches/1.0.x/core/src:
main/java/org/apache/ftpserver/ main/java/org/apache/ftpserver/impl/
test/java/org/apache/ftpserver/clienttests/
test/java/org/apache/ftpserver/impl/
Author: ngn
Date: Sat Jun 18 21:40:01 2011
New Revision: 1137252
URL: http://svn.apache.org/viewvc?rev=1137252&view=rev
Log:
New implementation of passive ports, uses random assignments of ports and has more efficient handling of wide passive port ranges. Thanks to Allen Firstenberg for the implementation! (FTPSERVER-420, FTPSERVER-419)
Modified:
mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java
mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java
mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java
mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java
Modified: mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java
URL: http://svn.apache.org/viewvc/mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java?rev=1137252&r1=1137251&r2=1137252&view=diff
==============================================================================
--- mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java (original)
+++ mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java Sat Jun 18 21:40:01 2011
@@ -21,6 +21,7 @@ package org.apache.ftpserver;
import java.net.InetAddress;
import java.net.UnknownHostException;
+import java.util.Collections;
import org.apache.ftpserver.impl.DefaultDataConnectionConfiguration;
import org.apache.ftpserver.impl.PassivePorts;
@@ -48,7 +49,7 @@ public class DataConnectionConfiguration
private String passiveAddress;
private String passiveExternalAddress;
- private PassivePorts passivePorts = new PassivePorts(new int[] { 0 }, true);
+ private PassivePorts passivePorts = new PassivePorts(Collections.<Integer>emptySet(), true);
private boolean implicitSsl;
/**
Modified: mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java
URL: http://svn.apache.org/viewvc/mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java?rev=1137252&r1=1137251&r2=1137252&view=diff
==============================================================================
--- mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java (original)
+++ mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java Sat Jun 18 21:40:01 2011
@@ -22,10 +22,15 @@ package org.apache.ftpserver.impl;
import java.io.IOException;
import java.net.ServerSocket;
import java.util.ArrayList;
-import java.util.Iterator;
+import java.util.HashSet;
import java.util.List;
+import java.util.Random;
+import java.util.Set;
import java.util.StringTokenizer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
/**
* <strong>Internal class, do not use directly.</strong>
*
@@ -36,16 +41,22 @@ import java.util.StringTokenizer;
*/
public class PassivePorts {
+ private Logger log = LoggerFactory.getLogger(PassivePorts.class);
+
private static final int MAX_PORT = 65535;
- private int[] passivePorts;
+ private static final Integer MAX_PORT_INTEGER = Integer.valueOf(MAX_PORT);
+
+ private List<Integer> freeList;
- private boolean[] reservedPorts;
+ private Set<Integer> usedList;
+
+ private Random r = new Random();
private String passivePortsString;
private boolean checkIfBound;
-
+
/**
* Parse a string containing passive ports
*
@@ -55,27 +66,27 @@ public class PassivePorts {
* 123,124,125) or ranges of ports, including open ended ranges
* (e.g. 123-125, 30000-, -1023). Combinations for single ports
* and ranges is also supported.
- * @return An instance of {@link PassivePorts} based on the parsed string
+ * @return A list of Integer objects, based on the parsed string
* @throws IllegalArgumentException
* If any of of the ports in the string is invalid (e.g. not an
* integer or too large for a port number)
*/
- private static int[] parse(final String portsString) {
- List<Integer> passivePortsList = new ArrayList<Integer>();
+ private static Set<Integer> parse(final String portsString) {
+ Set<Integer> passivePortsList = new HashSet<Integer>();
boolean inRange = false;
- Integer lastPort = 1;
+ Integer lastPort = Integer.valueOf(1);
StringTokenizer st = new StringTokenizer(portsString, ",;-", true);
while (st.hasMoreTokens()) {
String token = st.nextToken().trim();
if (",".equals(token) || ";".equals(token)) {
if (inRange) {
- fillRange(passivePortsList, lastPort, MAX_PORT);
+ fillRange(passivePortsList, lastPort, MAX_PORT_INTEGER);
}
// reset state
- lastPort = 1;
+ lastPort = Integer.valueOf(1);
inRange = false;
} else if ("-".equals(token)) {
inRange = true;
@@ -84,7 +95,7 @@ public class PassivePorts {
} else {
Integer port = Integer.valueOf(token);
- verifyPort(port.intValue());
+ verifyPort(port);
if (inRange) {
// add all numbers from last int
@@ -100,41 +111,26 @@ public class PassivePorts {
}
if (inRange) {
- fillRange(passivePortsList, lastPort, MAX_PORT);
+ fillRange(passivePortsList, lastPort, MAX_PORT_INTEGER);
}
- int[] passivePorts = new int[passivePortsList.size()];
-
- Iterator<Integer> iter = passivePortsList.iterator();
-
- int counter = 0;
- while (iter.hasNext()) {
- Integer port = iter.next();
- passivePorts[counter] = port.intValue();
- counter++;
- }
-
- return passivePorts;
+ return passivePortsList;
}
/**
* Fill a range of ports
*/
- private static void fillRange(final List<Integer> passivePortsList,
- final Integer beginPort, final Integer endPort) {
- for (int i = beginPort.intValue(); i <= endPort.intValue(); i++) {
- addPort(passivePortsList, i);
+ private static void fillRange(final Set<Integer> passivePortsList, final Integer beginPort, final Integer endPort) {
+ for (int i = beginPort; i <= endPort; i++) {
+ addPort(passivePortsList, Integer.valueOf(i));
}
}
/**
* Add a single port if not already in list
*/
- private static void addPort(final List<Integer> passivePortsList,
- final Integer rangePort) {
- if (!passivePortsList.contains(rangePort)) {
- passivePortsList.add(rangePort);
- }
+ private static void addPort(final Set<Integer> passivePortsList, final Integer port) {
+ passivePortsList.add(port);
}
/**
@@ -142,8 +138,7 @@ public class PassivePorts {
*/
private static void verifyPort(final int port) {
if (port < 0) {
- throw new IllegalArgumentException("Port can not be negative: "
- + port);
+ throw new IllegalArgumentException("Port can not be negative: " + port);
} else if (port > MAX_PORT) {
throw new IllegalArgumentException("Port too large: " + port);
}
@@ -155,14 +150,17 @@ public class PassivePorts {
this.passivePortsString = passivePorts;
}
- public PassivePorts(final int[] passivePorts, boolean checkIfBound) {
- if(passivePorts == null) {
- throw new NullPointerException("passivePorts can not be null");
- }
-
- this.passivePorts = passivePorts.clone();
+ public PassivePorts(Set<Integer> passivePorts, boolean checkIfBound) {
+ if (passivePorts == null) {
+ throw new NullPointerException("passivePorts can not be null");
+ } else if(passivePorts.isEmpty()) {
+ passivePorts = new HashSet<Integer>();
+ passivePorts.add(0);
+ }
+
+ this.freeList = new ArrayList<Integer>(passivePorts);
+ this.usedList = new HashSet<Integer>(passivePorts.size());
- reservedPorts = new boolean[passivePorts.length];
this.checkIfBound = checkIfBound;
}
@@ -171,15 +169,15 @@ public class PassivePorts {
*/
private boolean checkPortUnbound(int port) {
// is this check disabled?
- if(!checkIfBound) {
+ if (!checkIfBound) {
return true;
}
-
+
// if using 0 port, it will always be available
- if(port == 0) {
+ if (port == 0) {
return true;
}
-
+
ServerSocket ss = null;
try {
ss = new ServerSocket(port);
@@ -189,7 +187,7 @@ public class PassivePorts {
// port probably in use, check next
return false;
} finally {
- if(ss != null) {
+ if (ss != null) {
try {
ss.close();
} catch (IOException e) {
@@ -199,27 +197,49 @@ public class PassivePorts {
}
}
}
-
- public int reserveNextPort() {
- // search for a free port
- for (int i = 0; i < passivePorts.length; i++) {
- if (!reservedPorts[i] && checkPortUnbound(passivePorts[i])) {
- if (passivePorts[i] != 0) {
- reservedPorts[i] = true;
- }
- return passivePorts[i];
+
+ public synchronized int reserveNextPort() {
+ // create a copy of the free ports, so that we can keep track of the tested ports
+ List<Integer> freeCopy = new ArrayList<Integer>(freeList);
+
+ // Loop until we have found a port, or exhausted all available ports
+ while (freeCopy.size() > 0) {
+ // Otherwise, pick one at random
+ int i = r.nextInt(freeCopy.size());
+ Integer ret = freeCopy.get(i);
+
+ if (ret == 0) {
+ // "Any" port should not be removed from our free list,
+ // nor added to the used list
+ return 0;
+
+ } else if (checkPortUnbound(ret)) {
+ // Not used by someone else, so lets reserve it and return it
+ freeList.remove(i);
+ usedList.add(ret);
+ return ret;
+
+ } else {
+ freeCopy.remove(i);
+ // log port unavailable, but left in pool
+ log.warn("Passive port in use by another process: " + ret);
}
}
return -1;
}
- public void releasePort(final int port) {
- for (int i = 0; i < passivePorts.length; i++) {
- if (passivePorts[i] == port) {
- reservedPorts[i] = false;
- break;
- }
+ public synchronized void releasePort(final int port) {
+ if (port == 0) {
+ // Ignore port 0 being released,
+ // since its not put on the used list
+
+ } else if (usedList.remove(port)) {
+ freeList.add(port);
+
+ } else {
+ // log attempt to release unused port
+ log.warn("Releasing unreserved passive port: " + port);
}
}
@@ -231,7 +251,7 @@ public class PassivePorts {
StringBuilder sb = new StringBuilder();
- for (int port : passivePorts) {
+ for (Integer port : freeList) {
sb.append(port);
sb.append(",");
}
Modified: mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java
URL: http://svn.apache.org/viewvc/mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java?rev=1137252&r1=1137251&r2=1137252&view=diff
==============================================================================
--- mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java (original)
+++ mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java Sat Jun 18 21:40:01 2011
@@ -68,10 +68,5 @@ public class PasvUsedPortTest extends Cl
// close blocking socket
ss.close();
-
- client.connect("localhost", getListenerPort());
- client.login(ADMIN_USERNAME, ADMIN_PASSWORD);
- client.pasv();
- assertEquals("227 Entering Passive Mode (127,0,0,1,48,156)", client.getReplyString().trim());
}
}
Modified: mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java
URL: http://svn.apache.org/viewvc/mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java?rev=1137252&r1=1137251&r2=1137252&view=diff
==============================================================================
--- mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java (original)
+++ mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java Sat Jun 18 21:40:01 2011
@@ -19,11 +19,18 @@
package org.apache.ftpserver.impl;
+import java.io.IOException;
+import java.net.ServerSocket;
+import java.util.ArrayList;
+import java.util.List;
+
+import junit.framework.AssertionFailedError;
import junit.framework.TestCase;
/**
*
-* @author <a href="http://mina.apache.org">Apache MINA Project</a>*
+* @author <a href="http://mina.apache.org">Apache MINA Project</a>
+*
*/
public class PassivePortsTest extends TestCase {
@@ -68,146 +75,109 @@ public class PassivePortsTest extends Te
// ok
}
}
-
- public void testParseListOfValues() {
- PassivePorts ports = new PassivePorts("123, 456,\t\n789", false);
-
- assertEquals(123, ports.reserveNextPort());
- assertEquals(456, ports.reserveNextPort());
- assertEquals(789, ports.reserveNextPort());
- assertEquals(-1, ports.reserveNextPort());
+
+ private void assertContains( List<Integer> valid, Integer testVal ){
+ if( !valid.remove(testVal) ){
+ throw new AssertionFailedError( "Did not find "+testVal+" in valid list "+valid );
+ }
}
+
+ private void assertReserveAll(String portString, int... validPorts) {
+ PassivePorts ports = new PassivePorts(portString, false);
+
+ List<Integer> valid = valid(validPorts);
+
+ int len = valid.size();
+ for(int i = 0; i<len; i++) {
+ assertContains(valid, ports.reserveNextPort());
+ }
+ assertEquals(-1, ports.reserveNextPort());
+ assertTrue(valid.isEmpty());
- public void testParseListOfValuesOrder() {
- PassivePorts ports = new PassivePorts("123, 789, 456", false);
+ }
+
+ private List<Integer> valid(int... ints) {
+ List<Integer> valid = new ArrayList<Integer>();
+ for(int i : ints) {
+ valid.add(i);
+ }
+ return valid;
+ }
- assertEquals(123, ports.reserveNextPort());
- assertEquals(789, ports.reserveNextPort());
- assertEquals(456, ports.reserveNextPort());
- assertEquals(-1, ports.reserveNextPort());
+ public void testParseListOfValues() {
+ assertReserveAll("123, 456,\t\n789", 123, 456, 789);
}
public void testParseListOfValuesDuplicate() {
- PassivePorts ports = new PassivePorts("123, 789, 456, 789", false);
-
- assertEquals(123, ports.reserveNextPort());
- assertEquals(789, ports.reserveNextPort());
- assertEquals(456, ports.reserveNextPort());
- assertEquals(-1, ports.reserveNextPort());
+ assertReserveAll("123, 789, 456, 789", 123, 456, 789);
}
public void testParseSimpleRange() {
- PassivePorts ports = new PassivePorts("123-125", false);
-
- assertEquals(123, ports.reserveNextPort());
- assertEquals(124, ports.reserveNextPort());
- assertEquals(125, ports.reserveNextPort());
- assertEquals(-1, ports.reserveNextPort());
+ assertReserveAll("123-125", 123, 124, 125);
}
public void testParseMultipleRanges() {
- PassivePorts ports = new PassivePorts("123-125, 127-128, 130-132", false);
-
- assertEquals(123, ports.reserveNextPort());
- assertEquals(124, ports.reserveNextPort());
- assertEquals(125, ports.reserveNextPort());
- assertEquals(127, ports.reserveNextPort());
- assertEquals(128, ports.reserveNextPort());
- assertEquals(130, ports.reserveNextPort());
- assertEquals(131, ports.reserveNextPort());
- assertEquals(132, ports.reserveNextPort());
- assertEquals(-1, ports.reserveNextPort());
+ assertReserveAll("123-125, 127-128, 130-132", 123, 124, 125, 127, 128, 130, 131, 132);
}
public void testParseMixedRangeAndSingle() {
- PassivePorts ports = new PassivePorts("123-125, 126, 128-129", false);
-
- assertEquals(123, ports.reserveNextPort());
- assertEquals(124, ports.reserveNextPort());
- assertEquals(125, ports.reserveNextPort());
- assertEquals(126, ports.reserveNextPort());
- assertEquals(128, ports.reserveNextPort());
- assertEquals(129, ports.reserveNextPort());
- assertEquals(-1, ports.reserveNextPort());
+ assertReserveAll("123-125, 126, 128-129", 123, 124, 125, 126, 128, 129);
}
public void testParseOverlapingRanges() {
- PassivePorts ports = new PassivePorts("123-125, 124-126", false);
-
- assertEquals(123, ports.reserveNextPort());
- assertEquals(124, ports.reserveNextPort());
- assertEquals(125, ports.reserveNextPort());
- assertEquals(126, ports.reserveNextPort());
- assertEquals(-1, ports.reserveNextPort());
+ assertReserveAll("123-125, 124-126", 123, 124, 125, 126);
}
public void testParseOverlapingRangesorder() {
- PassivePorts ports = new PassivePorts("124-126, 123-125", false);
-
- assertEquals(124, ports.reserveNextPort());
- assertEquals(125, ports.reserveNextPort());
- assertEquals(126, ports.reserveNextPort());
- assertEquals(123, ports.reserveNextPort());
- assertEquals(-1, ports.reserveNextPort());
+ assertReserveAll("124-126, 123-125", 123, 124, 125, 126);
}
public void testParseOpenLowerRange() {
- PassivePorts ports = new PassivePorts("9, -3", false);
-
- assertEquals(9, ports.reserveNextPort());
- assertEquals(1, ports.reserveNextPort());
- assertEquals(2, ports.reserveNextPort());
- assertEquals(3, ports.reserveNextPort());
- assertEquals(-1, ports.reserveNextPort());
+ assertReserveAll("9, -3", 1, 2, 3, 9);
}
public void testParseOpenUpperRange() {
- PassivePorts ports = new PassivePorts("65533-", false);
-
- assertEquals(65533, ports.reserveNextPort());
- assertEquals(65534, ports.reserveNextPort());
- assertEquals(65535, ports.reserveNextPort());
- assertEquals(-1, ports.reserveNextPort());
+ assertReserveAll("65533-", 65533, 65534, 65535);
}
public void testParseOpenUpperRange3() {
- PassivePorts ports = new PassivePorts("65533-, 65532-", false);
-
- assertEquals(65533, ports.reserveNextPort());
- assertEquals(65534, ports.reserveNextPort());
- assertEquals(65535, ports.reserveNextPort());
- assertEquals(65532, ports.reserveNextPort());
- assertEquals(-1, ports.reserveNextPort());
+ assertReserveAll("65533-, 65532-", 65532, 65533, 65534, 65535);
}
public void testParseOpenUpperRange2() {
- PassivePorts ports = new PassivePorts("65533-, 1", false);
+ assertReserveAll("65533-, 1", 1, 65533, 65534, 65535);
+ }
+
+ public void testReserveNextPortBound() throws IOException {
+ ServerSocket ss = new ServerSocket(0);
+
+ PassivePorts ports = new PassivePorts(Integer.toString(ss.getLocalPort()), true);
- assertEquals(65533, ports.reserveNextPort());
- assertEquals(65534, ports.reserveNextPort());
- assertEquals(65535, ports.reserveNextPort());
- assertEquals(1, ports.reserveNextPort());
assertEquals(-1, ports.reserveNextPort());
+
+ ss.close();
+
+ assertEquals(ss.getLocalPort(), ports.reserveNextPort());
}
+
public void testParseRelease() {
PassivePorts ports = new PassivePorts("123, 456,789", false);
- assertEquals(123, ports.reserveNextPort());
- assertEquals(456, ports.reserveNextPort());
- ports.releasePort(456);
- assertEquals(456, ports.reserveNextPort());
+ List<Integer> valid = valid(123, 456, 789);
+
+ assertContains(valid, ports.reserveNextPort());
+
+ int port = ports.reserveNextPort();
+ assertContains(valid, port);
+ ports.releasePort(port);
+ valid.add(port);
+ assertContains(valid, ports.reserveNextPort());
+
+ assertContains(valid, ports.reserveNextPort());
- assertEquals(789, ports.reserveNextPort());
assertEquals(-1, ports.reserveNextPort());
+ assertEquals(0, valid.size());
}
- public void testNullPorts() {
- try {
- new PassivePorts((int[])null, false);
- fail("Must throw NPE");
- } catch(NullPointerException e) {
- // ok
- }
- }
-}
+}
\ No newline at end of file
Re: svn commit: r1137252 - in /mina/ftpserver/branches/1.0.x/core/src:
main/java/org/apache/ftpserver/ main/java/org/apache/ftpserver/impl/
test/java/org/apache/ftpserver/clienttests/ test/java/org/apache/ftpserver/impl/
Posted by Niklas Gustavsson <ni...@protocol7.com>.
On Sun, Jun 19, 2011 at 8:04 AM, Emmanuel Lecharny <el...@apache.org> wrote:
> wouldn't it be more efficient to use new ServerSocket(0).getLocalport() to
> find a random available port ?
Yes, if any port would be okay. But in most cases, user configure a
tight range (or even multiple ranges) that are allowed. If the user
sets this range to 0, then the above will happen.
/niklas
Re: svn commit: r1137252 - in /mina/ftpserver/branches/1.0.x/core/src:
main/java/org/apache/ftpserver/ main/java/org/apache/ftpserver/impl/
test/java/org/apache/ftpserver/clienttests/ test/java/org/apache/ftpserver/impl/
Posted by Emmanuel Lecharny <el...@apache.org>.
Jus wondering,
wouldn't it be more efficient to use new ServerSocket(0).getLocalport() to
find a random available port ?
On Sat, Jun 18, 2011 at 11:40 PM, <ng...@apache.org> wrote:
> Author: ngn
> Date: Sat Jun 18 21:40:01 2011
> New Revision: 1137252
>
> URL: http://svn.apache.org/viewvc?rev=1137252&view=rev
> Log:
> New implementation of passive ports, uses random assignments of ports and
> has more efficient handling of wide passive port ranges. Thanks to Allen
> Firstenberg for the implementation! (FTPSERVER-420, FTPSERVER-419)
>
> Modified:
>
> mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java
>
> mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java
>
> mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java
>
> mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java
>
> Modified:
> mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java
> URL:
> http://svn.apache.org/viewvc/mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java?rev=1137252&r1=1137251&r2=1137252&view=diff
>
> ==============================================================================
> ---
> mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java
> (original)
> +++
> mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java
> Sat Jun 18 21:40:01 2011
> @@ -21,6 +21,7 @@ package org.apache.ftpserver;
>
> import java.net.InetAddress;
> import java.net.UnknownHostException;
> +import java.util.Collections;
>
> import org.apache.ftpserver.impl.DefaultDataConnectionConfiguration;
> import org.apache.ftpserver.impl.PassivePorts;
> @@ -48,7 +49,7 @@ public class DataConnectionConfiguration
>
> private String passiveAddress;
> private String passiveExternalAddress;
> - private PassivePorts passivePorts = new PassivePorts(new int[] { 0 },
> true);
> + private PassivePorts passivePorts = new
> PassivePorts(Collections.<Integer>emptySet(), true);
> private boolean implicitSsl;
>
> /**
>
> Modified:
> mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java
> URL:
> http://svn.apache.org/viewvc/mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java?rev=1137252&r1=1137251&r2=1137252&view=diff
>
> ==============================================================================
> ---
> mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java
> (original)
> +++
> mina/ftpserver/branches/1.0.x/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java
> Sat Jun 18 21:40:01 2011
> @@ -22,10 +22,15 @@ package org.apache.ftpserver.impl;
> import java.io.IOException;
> import java.net.ServerSocket;
> import java.util.ArrayList;
> -import java.util.Iterator;
> +import java.util.HashSet;
> import java.util.List;
> +import java.util.Random;
> +import java.util.Set;
> import java.util.StringTokenizer;
>
> +import org.slf4j.Logger;
> +import org.slf4j.LoggerFactory;
> +
> /**
> * <strong>Internal class, do not use directly.</strong>
> *
> @@ -36,16 +41,22 @@ import java.util.StringTokenizer;
> */
> public class PassivePorts {
>
> + private Logger log = LoggerFactory.getLogger(PassivePorts.class);
> +
> private static final int MAX_PORT = 65535;
>
> - private int[] passivePorts;
> + private static final Integer MAX_PORT_INTEGER =
> Integer.valueOf(MAX_PORT);
> +
> + private List<Integer> freeList;
>
> - private boolean[] reservedPorts;
> + private Set<Integer> usedList;
> +
> + private Random r = new Random();
>
> private String passivePortsString;
>
> private boolean checkIfBound;
> -
> +
> /**
> * Parse a string containing passive ports
> *
> @@ -55,27 +66,27 @@ public class PassivePorts {
> * 123,124,125) or ranges of ports, including open ended
> ranges
> * (e.g. 123-125, 30000-, -1023). Combinations for single
> ports
> * and ranges is also supported.
> - * @return An instance of {@link PassivePorts} based on the parsed
> string
> + * @return A list of Integer objects, based on the parsed string
> * @throws IllegalArgumentException
> * If any of of the ports in the string is invalid (e.g.
> not an
> * integer or too large for a port number)
> */
> - private static int[] parse(final String portsString) {
> - List<Integer> passivePortsList = new ArrayList<Integer>();
> + private static Set<Integer> parse(final String portsString) {
> + Set<Integer> passivePortsList = new HashSet<Integer>();
>
> boolean inRange = false;
> - Integer lastPort = 1;
> + Integer lastPort = Integer.valueOf(1);
> StringTokenizer st = new StringTokenizer(portsString, ",;-", true);
> while (st.hasMoreTokens()) {
> String token = st.nextToken().trim();
>
> if (",".equals(token) || ";".equals(token)) {
> if (inRange) {
> - fillRange(passivePortsList, lastPort, MAX_PORT);
> + fillRange(passivePortsList, lastPort,
> MAX_PORT_INTEGER);
> }
>
> // reset state
> - lastPort = 1;
> + lastPort = Integer.valueOf(1);
> inRange = false;
> } else if ("-".equals(token)) {
> inRange = true;
> @@ -84,7 +95,7 @@ public class PassivePorts {
> } else {
> Integer port = Integer.valueOf(token);
>
> - verifyPort(port.intValue());
> + verifyPort(port);
>
> if (inRange) {
> // add all numbers from last int
> @@ -100,41 +111,26 @@ public class PassivePorts {
> }
>
> if (inRange) {
> - fillRange(passivePortsList, lastPort, MAX_PORT);
> + fillRange(passivePortsList, lastPort, MAX_PORT_INTEGER);
> }
>
> - int[] passivePorts = new int[passivePortsList.size()];
> -
> - Iterator<Integer> iter = passivePortsList.iterator();
> -
> - int counter = 0;
> - while (iter.hasNext()) {
> - Integer port = iter.next();
> - passivePorts[counter] = port.intValue();
> - counter++;
> - }
> -
> - return passivePorts;
> + return passivePortsList;
> }
>
> /**
> * Fill a range of ports
> */
> - private static void fillRange(final List<Integer> passivePortsList,
> - final Integer beginPort, final Integer endPort) {
> - for (int i = beginPort.intValue(); i <= endPort.intValue(); i++) {
> - addPort(passivePortsList, i);
> + private static void fillRange(final Set<Integer> passivePortsList,
> final Integer beginPort, final Integer endPort) {
> + for (int i = beginPort; i <= endPort; i++) {
> + addPort(passivePortsList, Integer.valueOf(i));
> }
> }
>
> /**
> * Add a single port if not already in list
> */
> - private static void addPort(final List<Integer> passivePortsList,
> - final Integer rangePort) {
> - if (!passivePortsList.contains(rangePort)) {
> - passivePortsList.add(rangePort);
> - }
> + private static void addPort(final Set<Integer> passivePortsList, final
> Integer port) {
> + passivePortsList.add(port);
> }
>
> /**
> @@ -142,8 +138,7 @@ public class PassivePorts {
> */
> private static void verifyPort(final int port) {
> if (port < 0) {
> - throw new IllegalArgumentException("Port can not be negative:
> "
> - + port);
> + throw new IllegalArgumentException("Port can not be negative:
> " + port);
> } else if (port > MAX_PORT) {
> throw new IllegalArgumentException("Port too large: " + port);
> }
> @@ -155,14 +150,17 @@ public class PassivePorts {
> this.passivePortsString = passivePorts;
> }
>
> - public PassivePorts(final int[] passivePorts, boolean checkIfBound) {
> - if(passivePorts == null) {
> - throw new NullPointerException("passivePorts can not be
> null");
> - }
> -
> - this.passivePorts = passivePorts.clone();
> + public PassivePorts(Set<Integer> passivePorts, boolean checkIfBound) {
> + if (passivePorts == null) {
> + throw new NullPointerException("passivePorts can not be
> null");
> + } else if(passivePorts.isEmpty()) {
> + passivePorts = new HashSet<Integer>();
> + passivePorts.add(0);
> + }
> +
> + this.freeList = new ArrayList<Integer>(passivePorts);
> + this.usedList = new HashSet<Integer>(passivePorts.size());
>
> - reservedPorts = new boolean[passivePorts.length];
> this.checkIfBound = checkIfBound;
> }
>
> @@ -171,15 +169,15 @@ public class PassivePorts {
> */
> private boolean checkPortUnbound(int port) {
> // is this check disabled?
> - if(!checkIfBound) {
> + if (!checkIfBound) {
> return true;
> }
> -
> +
> // if using 0 port, it will always be available
> - if(port == 0) {
> + if (port == 0) {
> return true;
> }
> -
> +
> ServerSocket ss = null;
> try {
> ss = new ServerSocket(port);
> @@ -189,7 +187,7 @@ public class PassivePorts {
> // port probably in use, check next
> return false;
> } finally {
> - if(ss != null) {
> + if (ss != null) {
> try {
> ss.close();
> } catch (IOException e) {
> @@ -199,27 +197,49 @@ public class PassivePorts {
> }
> }
> }
> -
> - public int reserveNextPort() {
> - // search for a free port
> - for (int i = 0; i < passivePorts.length; i++) {
> - if (!reservedPorts[i] && checkPortUnbound(passivePorts[i])) {
> - if (passivePorts[i] != 0) {
> - reservedPorts[i] = true;
> - }
> - return passivePorts[i];
> +
> + public synchronized int reserveNextPort() {
> + // create a copy of the free ports, so that we can keep track of
> the tested ports
> + List<Integer> freeCopy = new ArrayList<Integer>(freeList);
> +
> + // Loop until we have found a port, or exhausted all available
> ports
> + while (freeCopy.size() > 0) {
> + // Otherwise, pick one at random
> + int i = r.nextInt(freeCopy.size());
> + Integer ret = freeCopy.get(i);
> +
> + if (ret == 0) {
> + // "Any" port should not be removed from our free list,
> + // nor added to the used list
> + return 0;
> +
> + } else if (checkPortUnbound(ret)) {
> + // Not used by someone else, so lets reserve it and return
> it
> + freeList.remove(i);
> + usedList.add(ret);
> + return ret;
> +
> + } else {
> + freeCopy.remove(i);
> + // log port unavailable, but left in pool
> + log.warn("Passive port in use by another process: " +
> ret);
> }
> }
>
> return -1;
> }
>
> - public void releasePort(final int port) {
> - for (int i = 0; i < passivePorts.length; i++) {
> - if (passivePorts[i] == port) {
> - reservedPorts[i] = false;
> - break;
> - }
> + public synchronized void releasePort(final int port) {
> + if (port == 0) {
> + // Ignore port 0 being released,
> + // since its not put on the used list
> +
> + } else if (usedList.remove(port)) {
> + freeList.add(port);
> +
> + } else {
> + // log attempt to release unused port
> + log.warn("Releasing unreserved passive port: " + port);
> }
> }
>
> @@ -231,7 +251,7 @@ public class PassivePorts {
>
> StringBuilder sb = new StringBuilder();
>
> - for (int port : passivePorts) {
> + for (Integer port : freeList) {
> sb.append(port);
> sb.append(",");
> }
>
> Modified:
> mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java
> URL:
> http://svn.apache.org/viewvc/mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java?rev=1137252&r1=1137251&r2=1137252&view=diff
>
> ==============================================================================
> ---
> mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java
> (original)
> +++
> mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java
> Sat Jun 18 21:40:01 2011
> @@ -68,10 +68,5 @@ public class PasvUsedPortTest extends Cl
>
> // close blocking socket
> ss.close();
> -
> - client.connect("localhost", getListenerPort());
> - client.login(ADMIN_USERNAME, ADMIN_PASSWORD);
> - client.pasv();
> - assertEquals("227 Entering Passive Mode (127,0,0,1,48,156)",
> client.getReplyString().trim());
> }
> }
>
> Modified:
> mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java
> URL:
> http://svn.apache.org/viewvc/mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java?rev=1137252&r1=1137251&r2=1137252&view=diff
>
> ==============================================================================
> ---
> mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java
> (original)
> +++
> mina/ftpserver/branches/1.0.x/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java
> Sat Jun 18 21:40:01 2011
> @@ -19,11 +19,18 @@
>
> package org.apache.ftpserver.impl;
>
> +import java.io.IOException;
> +import java.net.ServerSocket;
> +import java.util.ArrayList;
> +import java.util.List;
> +
> +import junit.framework.AssertionFailedError;
> import junit.framework.TestCase;
>
> /**
> *
> -* @author <a href="http://mina.apache.org">Apache MINA Project</a>*
> +* @author <a href="http://mina.apache.org">Apache MINA Project</a>
> +*
> */
> public class PassivePortsTest extends TestCase {
>
> @@ -68,146 +75,109 @@ public class PassivePortsTest extends Te
> // ok
> }
> }
> -
> - public void testParseListOfValues() {
> - PassivePorts ports = new PassivePorts("123, 456,\t\n789", false);
> -
> - assertEquals(123, ports.reserveNextPort());
> - assertEquals(456, ports.reserveNextPort());
> - assertEquals(789, ports.reserveNextPort());
> - assertEquals(-1, ports.reserveNextPort());
> +
> + private void assertContains( List<Integer> valid, Integer testVal ){
> + if( !valid.remove(testVal) ){
> + throw new AssertionFailedError( "Did not find "+testVal+" in
> valid list "+valid );
> + }
> }
> +
> + private void assertReserveAll(String portString, int... validPorts) {
> + PassivePorts ports = new PassivePorts(portString, false);
> +
> + List<Integer> valid = valid(validPorts);
> +
> + int len = valid.size();
> + for(int i = 0; i<len; i++) {
> + assertContains(valid, ports.reserveNextPort());
> + }
> + assertEquals(-1, ports.reserveNextPort());
> + assertTrue(valid.isEmpty());
>
> - public void testParseListOfValuesOrder() {
> - PassivePorts ports = new PassivePorts("123, 789, 456", false);
> + }
> +
> + private List<Integer> valid(int... ints) {
> + List<Integer> valid = new ArrayList<Integer>();
> + for(int i : ints) {
> + valid.add(i);
> + }
> + return valid;
> + }
>
> - assertEquals(123, ports.reserveNextPort());
> - assertEquals(789, ports.reserveNextPort());
> - assertEquals(456, ports.reserveNextPort());
> - assertEquals(-1, ports.reserveNextPort());
> + public void testParseListOfValues() {
> + assertReserveAll("123, 456,\t\n789", 123, 456, 789);
> }
>
> public void testParseListOfValuesDuplicate() {
> - PassivePorts ports = new PassivePorts("123, 789, 456, 789",
> false);
> -
> - assertEquals(123, ports.reserveNextPort());
> - assertEquals(789, ports.reserveNextPort());
> - assertEquals(456, ports.reserveNextPort());
> - assertEquals(-1, ports.reserveNextPort());
> + assertReserveAll("123, 789, 456, 789", 123, 456, 789);
> }
>
> public void testParseSimpleRange() {
> - PassivePorts ports = new PassivePorts("123-125", false);
> -
> - assertEquals(123, ports.reserveNextPort());
> - assertEquals(124, ports.reserveNextPort());
> - assertEquals(125, ports.reserveNextPort());
> - assertEquals(-1, ports.reserveNextPort());
> + assertReserveAll("123-125", 123, 124, 125);
> }
>
> public void testParseMultipleRanges() {
> - PassivePorts ports = new PassivePorts("123-125, 127-128, 130-132",
> false);
> -
> - assertEquals(123, ports.reserveNextPort());
> - assertEquals(124, ports.reserveNextPort());
> - assertEquals(125, ports.reserveNextPort());
> - assertEquals(127, ports.reserveNextPort());
> - assertEquals(128, ports.reserveNextPort());
> - assertEquals(130, ports.reserveNextPort());
> - assertEquals(131, ports.reserveNextPort());
> - assertEquals(132, ports.reserveNextPort());
> - assertEquals(-1, ports.reserveNextPort());
> + assertReserveAll("123-125, 127-128, 130-132", 123, 124, 125, 127,
> 128, 130, 131, 132);
> }
>
> public void testParseMixedRangeAndSingle() {
> - PassivePorts ports = new PassivePorts("123-125, 126, 128-129",
> false);
> -
> - assertEquals(123, ports.reserveNextPort());
> - assertEquals(124, ports.reserveNextPort());
> - assertEquals(125, ports.reserveNextPort());
> - assertEquals(126, ports.reserveNextPort());
> - assertEquals(128, ports.reserveNextPort());
> - assertEquals(129, ports.reserveNextPort());
> - assertEquals(-1, ports.reserveNextPort());
> + assertReserveAll("123-125, 126, 128-129", 123, 124, 125, 126, 128,
> 129);
> }
>
> public void testParseOverlapingRanges() {
> - PassivePorts ports = new PassivePorts("123-125, 124-126", false);
> -
> - assertEquals(123, ports.reserveNextPort());
> - assertEquals(124, ports.reserveNextPort());
> - assertEquals(125, ports.reserveNextPort());
> - assertEquals(126, ports.reserveNextPort());
> - assertEquals(-1, ports.reserveNextPort());
> + assertReserveAll("123-125, 124-126", 123, 124, 125, 126);
> }
>
> public void testParseOverlapingRangesorder() {
> - PassivePorts ports = new PassivePorts("124-126, 123-125", false);
> -
> - assertEquals(124, ports.reserveNextPort());
> - assertEquals(125, ports.reserveNextPort());
> - assertEquals(126, ports.reserveNextPort());
> - assertEquals(123, ports.reserveNextPort());
> - assertEquals(-1, ports.reserveNextPort());
> + assertReserveAll("124-126, 123-125", 123, 124, 125, 126);
> }
>
> public void testParseOpenLowerRange() {
> - PassivePorts ports = new PassivePorts("9, -3", false);
> -
> - assertEquals(9, ports.reserveNextPort());
> - assertEquals(1, ports.reserveNextPort());
> - assertEquals(2, ports.reserveNextPort());
> - assertEquals(3, ports.reserveNextPort());
> - assertEquals(-1, ports.reserveNextPort());
> + assertReserveAll("9, -3", 1, 2, 3, 9);
> }
>
> public void testParseOpenUpperRange() {
> - PassivePorts ports = new PassivePorts("65533-", false);
> -
> - assertEquals(65533, ports.reserveNextPort());
> - assertEquals(65534, ports.reserveNextPort());
> - assertEquals(65535, ports.reserveNextPort());
> - assertEquals(-1, ports.reserveNextPort());
> + assertReserveAll("65533-", 65533, 65534, 65535);
> }
>
> public void testParseOpenUpperRange3() {
> - PassivePorts ports = new PassivePorts("65533-, 65532-", false);
> -
> - assertEquals(65533, ports.reserveNextPort());
> - assertEquals(65534, ports.reserveNextPort());
> - assertEquals(65535, ports.reserveNextPort());
> - assertEquals(65532, ports.reserveNextPort());
> - assertEquals(-1, ports.reserveNextPort());
> + assertReserveAll("65533-, 65532-", 65532, 65533, 65534, 65535);
> }
>
> public void testParseOpenUpperRange2() {
> - PassivePorts ports = new PassivePorts("65533-, 1", false);
> + assertReserveAll("65533-, 1", 1, 65533, 65534, 65535);
> + }
> +
> + public void testReserveNextPortBound() throws IOException {
> + ServerSocket ss = new ServerSocket(0);
> +
> + PassivePorts ports = new
> PassivePorts(Integer.toString(ss.getLocalPort()), true);
>
> - assertEquals(65533, ports.reserveNextPort());
> - assertEquals(65534, ports.reserveNextPort());
> - assertEquals(65535, ports.reserveNextPort());
> - assertEquals(1, ports.reserveNextPort());
> assertEquals(-1, ports.reserveNextPort());
> +
> + ss.close();
> +
> + assertEquals(ss.getLocalPort(), ports.reserveNextPort());
> }
>
> +
> public void testParseRelease() {
> PassivePorts ports = new PassivePorts("123, 456,789", false);
>
> - assertEquals(123, ports.reserveNextPort());
> - assertEquals(456, ports.reserveNextPort());
> - ports.releasePort(456);
> - assertEquals(456, ports.reserveNextPort());
> + List<Integer> valid = valid(123, 456, 789);
> +
> + assertContains(valid, ports.reserveNextPort());
> +
> + int port = ports.reserveNextPort();
> + assertContains(valid, port);
> + ports.releasePort(port);
> + valid.add(port);
> + assertContains(valid, ports.reserveNextPort());
> +
> + assertContains(valid, ports.reserveNextPort());
>
> - assertEquals(789, ports.reserveNextPort());
> assertEquals(-1, ports.reserveNextPort());
> + assertEquals(0, valid.size());
> }
>
> - public void testNullPorts() {
> - try {
> - new PassivePorts((int[])null, false);
> - fail("Must throw NPE");
> - } catch(NullPointerException e) {
> - // ok
> - }
> - }
> -}
> +}
> \ No newline at end of file
>
>
>
--
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com