2007-12-25 Tatsuhiro Tsujikawa <tujikawa at rednoah dot com>

	Removed incomingPeer. Set 0 to peer's port if it is not a listening
	port.
	* src/DefaultPeerStorage.{h, cc}
	* test/DefaultPeerStorageTest.cc
	* src/HandshakeExtensionMessage.cc
	* test/HandshakeExtensionMessageTest.cc
	* src/Peer.{h, cc}: Added ipaddr and port to identity comparison.
	* src/PeerStorage.h
	* test/MockPeerStorage.h
	* src/PeerListenCommand.cc

11 files changed, 50 insertions, 164 deletions

diff --git a/ChangeLog b/ChangeLog
index a81ed5b9..13b303b4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2007-12-25  Tatsuhiro Tsujikawa  <tujikawa at rednoah dot com>
+
+	Removed incomingPeer. Set 0 to peer's port if it is not a listening
+	port.
+	* src/DefaultPeerStorage.{h, cc}
+	* test/DefaultPeerStorageTest.cc
+	* src/HandshakeExtensionMessage.cc
+	* test/HandshakeExtensionMessageTest.cc
+	* src/Peer.{h, cc}: Added ipaddr and port to identity comparison.
+	* src/PeerStorage.h
+	* test/MockPeerStorage.h
+	* src/PeerListenCommand.cc
+
 2007-12-22  Tatsuhiro Tsujikawa  <tujikawa at rednoah dot com>
 
 	Added --metalink-enable-unique-protocol option.
diff --git a/src/DefaultPeerStorage.cc b/src/DefaultPeerStorage.cc
index a9e714db..059cf0b6 100644
--- a/src/DefaultPeerStorage.cc
+++ b/src/DefaultPeerStorage.cc
@@ -54,18 +54,13 @@ DefaultPeerStorage::~DefaultPeerStorage() {}
 
 bool DefaultPeerStorage::isPeerAlreadyAdded(const PeerHandle& peer)
 {
-  return find(peers.begin(), peers.end(), peer) != peers.end() ||
-    find(incomingPeers.begin(), incomingPeers.end(), peer) != incomingPeers.end();
+  return find(peers.begin(), peers.end(), peer) != peers.end();
 }
 
 bool DefaultPeerStorage::addPeer(const PeerHandle& peer) {
-  {
-    Peers::iterator i = find(incomingPeers.begin(), incomingPeers.end(), peer);
-    if(i != incomingPeers.end() && (*i).get() != peer.get() ||
-       find(peers.begin(), peers.end(), peer) != peers.end()) {
-      logger->debug("Adding %s:%u is rejected because it is already in PeerStorage.", peer->ipaddr.c_str(), peer->port);
-      return false;
-    }
+  if(isPeerAlreadyAdded(peer)) {
+    logger->debug("Adding %s:%u is rejected because it has been already added.", peer->ipaddr.c_str(), peer->port);
+    return false;
   }
   if(peers.size() >= (size_t)maxPeerListSize) {
     deleteUnusedPeer(peers.size()-maxPeerListSize+1);
@@ -74,17 +69,6 @@ bool DefaultPeerStorage::addPeer(const PeerHandle& peer) {
   return true;
 }
 
-bool DefaultPeerStorage::addIncomingPeer(const PeerHandle& peer)
-{
-  if(isPeerAlreadyAdded(peer)) {
-    logger->debug("Adding %s:%u is rejected because it is already in PeerStorage.", peer->ipaddr.c_str(), peer->port);
-    return false;
-  } else {
-    incomingPeers.push_back(peer);
-    return true;
-  }
-}
-
 void DefaultPeerStorage::addPeer(const Peers& peers) {
   for(Peers::const_iterator itr = peers.begin();
       itr != peers.end(); itr++) {
@@ -163,10 +147,7 @@ public:
 };
 
 Peers DefaultPeerStorage::getActivePeers() {
-  CollectActivePeer funcObj;
-  funcObj = for_each(peers.begin(), peers.end(), funcObj);
-  funcObj = for_each(incomingPeers.begin(), incomingPeers.end(), funcObj);
-  return funcObj.getActivePeers();
+  return for_each(peers.begin(), peers.end(), CollectActivePeer()).getActivePeers();
 }
 
 class CalculateStat {
@@ -193,11 +174,7 @@ public:
 };
 
 TransferStat DefaultPeerStorage::calculateStat() {
-  CalculateStat calStat;
-  calStat = for_each(peers.begin(), peers.end(), calStat);
-  calStat = for_each(incomingPeers.begin(), incomingPeers.end(), calStat);
-
-  TransferStat stat = calStat.getTransferStat();
+  TransferStat stat = for_each(peers.begin(), peers.end(), CalculateStat()).getTransferStat();
   stat.sessionDownloadLength += removedPeerSessionDownloadLength;
   stat.sessionUploadLength += removedPeerSessionUploadLength;
   stat.setAllTimeUploadLength(btRuntime->getUploadLengthAtStartup()+
@@ -231,19 +208,16 @@ void DefaultPeerStorage::returnPeer(const PeerHandle& peer)
 {
   Peers::iterator itr = find(peers.begin(), peers.end(), peer);
   if(itr == peers.end()) {
-    itr = find(incomingPeers.begin(), incomingPeers.end(), peer);
-    if(itr == peers.end()) {
-      // do nothing
-    } else {
-      // erase incoming peer because we cannot connect to it with port number
-      // (*itr)->port. It is not the listening port.
+    logger->debug("Cannot find peer %s:%u in PeerStorage.", peer->ipaddr.c_str(), peer->port);
+  } else {
+    if((*itr)->port == 0) {
       onErasingPeer(*itr);
-      incomingPeers.erase(itr);
+      peers.erase(itr);
+    } else {
+      peer->startBadCondition();
+      peer->resetStatus();
+      peers.erase(itr);
+      peers.push_back(peer);
     }
-  } else {
-    peer->startBadCondition();
-    peer->resetStatus();
-    peers.erase(itr);
-    peers.push_back(peer);
   }
 }
diff --git a/src/DefaultPeerStorage.h b/src/DefaultPeerStorage.h
index 5e889768..c2b5d49f 100644
--- a/src/DefaultPeerStorage.h
+++ b/src/DefaultPeerStorage.h
@@ -49,7 +49,6 @@ private:
   BtContextHandle btContext;
   const Option* option;
   Peers peers;
-  Peers incomingPeers;
   int32_t maxPeerListSize;
   Logger* logger;
   BtRuntimeHandle btRuntime;
@@ -68,8 +67,6 @@ public:
 
   virtual bool addPeer(const PeerHandle& peer);
 
-  virtual bool addIncomingPeer(const PeerHandle& peer);
-
   int32_t countPeer() const;
 
   virtual PeerHandle getUnusedPeer();
@@ -95,8 +92,6 @@ public:
   void deleteUnusedPeer(int32_t delSize);
   
   void onErasingPeer(const PeerHandle& peer);
-
-  const Peers& getIncomingPeers() const { return incomingPeers; }
 };
 
 #endif // _D_DEFAULT_PEER_STORAGE_H_
diff --git a/src/HandshakeExtensionMessage.cc b/src/HandshakeExtensionMessage.cc
index df003b5b..e58a70e3 100644
--- a/src/HandshakeExtensionMessage.cc
+++ b/src/HandshakeExtensionMessage.cc
@@ -39,7 +39,6 @@
 #include "Data.h"
 #include "Util.h"
 #include "BencodeVisitor.h"
-#include "BtRegistry.h"
 #include "MetaFileUtil.h"
 #include "DlAbortEx.h"
 #include "LogFactory.h"
@@ -107,14 +106,6 @@ void HandshakeExtensionMessage::doReceivedAction()
     const map<string, uint8_t>::value_type& vt = *itr;
     _peer->setExtension(vt.first, vt.second);
   }
-  if(_peer->port > 0) {
-    // This is needed when _peer is a connection initiator, listen port of
-    // _peer is now available, which is initially unknown.
-    // If _peer is a receiver or already its port is known, _peer has to be
-    // already added to PeerStorage using addPeer() and call
-    // PeerStorage::addPeer() here does nothing and just returns false.
-    PEER_STORAGE(_btContext)->addPeer(_peer);
-  }
 }
 
 void HandshakeExtensionMessage::setPeer(const PeerHandle& peer)
diff --git a/src/Peer.cc b/src/Peer.cc
index f7637792..10d1a27d 100644
--- a/src/Peer.cc
+++ b/src/Peer.cc
@@ -44,7 +44,6 @@ Peer::Peer(string ipaddr, uint16_t port, int32_t pieceLength, int64_t totalLengt
   port(port),
   sessionUploadLength(0),
   sessionDownloadLength(0),
-  pieceLength(pieceLength),
   active(false),
   _badConditionStartTime(0),
   _badConditionInterval(10)
@@ -63,20 +62,10 @@ Peer::Peer(string ipaddr, uint16_t port, int32_t pieceLength, int64_t totalLengt
 void Peer::reconfigure(int32_t pieceLength, int64_t totalLength)
 {
   delete bitfield;
-  this->pieceLength = pieceLength;
   this->bitfield = BitfieldManFactory::getFactoryInstance()->
-    createBitfieldMan(this->pieceLength, totalLength);  
+    createBitfieldMan(pieceLength, totalLength);  
 }
 
-/*
-Peer::Peer():entryId(0), ipaddr(""), port(0), bitfield(0),
-       sessionUploadLength(0), sessionDownloadLength(0),
-       pieceLength(0)
-{
-  resetStatus();
-}
-*/
-
 void Peer::updateBitfield(int32_t index, int32_t operation) {
   if(operation == 1) {
     bitfield->setBit(index);
diff --git a/src/Peer.h b/src/Peer.h
index 04ae33af..44dd68c9 100644
--- a/src/Peer.h
+++ b/src/Peer.h
@@ -46,10 +46,11 @@
 #define DEFAULT_LATENCY 1500
 
 class Peer {
-  friend bool operator==(const Peer& p1, const Peer& p2);
-  friend bool operator!=(const Peer& p1, const Peer& p2);
 public:
   string ipaddr;
+  // TCP port which this peer is listening for incoming connections.
+  // If it is unknown, for example, localhost accepted the incoming connection
+  // from this peer, set port to 0.
   uint16_t port;
   bool amChoking;
   bool amInterested;
@@ -73,7 +74,6 @@ private:
   PeerStat peerStat;
   int64_t sessionUploadLength;
   int64_t sessionDownloadLength;
-  int32_t pieceLength;
   int32_t latency;
   bool active;
   string id;
@@ -88,8 +88,7 @@ public:
   }
 
   bool operator==(const Peer& p) {
-    //return ipaddr == p.ipaddr && port == p.port;
-    return id == p.id;
+    return id == p.id || ipaddr == p.ipaddr && port == p.port;
   }
   
   bool operator!=(const Peer& p) {
diff --git a/src/PeerListenCommand.cc b/src/PeerListenCommand.cc
index b27fd759..c4b61003 100644
--- a/src/PeerListenCommand.cc
+++ b/src/PeerListenCommand.cc
@@ -93,12 +93,12 @@ bool PeerListenCommand::execute() {
       if(peerInfo.first == localInfo.first) {
 	continue;
       }
-      PeerHandle peer = new Peer(peerInfo.first, peerInfo.second, 0, 0);
+      PeerHandle peer = new Peer(peerInfo.first, 0, 0, 0);
       PeerReceiveHandshakeCommand* command =
 	new PeerReceiveHandshakeCommand(CUIDCounterSingletonHolder::instance()->newID(),
 					peer, e, peerSocket);
       e->commands.push_back(command);
-      logger->debug("Accepted the connection from %s:%d.",
+      logger->debug("Accepted the connection from %s:%u.",
 		    peer->ipaddr.c_str(),
 		    peer->port);
       logger->debug("Added CUID#%d to receive Bt handshake.",
diff --git a/src/PeerStorage.h b/src/PeerStorage.h
index a7e4accd..2c76c514 100644
--- a/src/PeerStorage.h
+++ b/src/PeerStorage.h
@@ -50,12 +50,6 @@ public:
   virtual bool addPeer(const PeerHandle& peer) = 0;
 
   /**
-   * Adds new incoming peer to the internal peer list.
-   * If the peer is added successfully, returns true. Otherwise returns false.
-   */
-  virtual bool addIncomingPeer(const PeerHandle& peer) = 0;
-
-  /**
    * Adds all peers in peers to internal peer list.
    */
   virtual void addPeer(const Peers& peers) = 0;
diff --git a/test/DefaultPeerStorageTest.cc b/test/DefaultPeerStorageTest.cc
index f353f959..fd4f4576 100644
--- a/test/DefaultPeerStorageTest.cc
+++ b/test/DefaultPeerStorageTest.cc
@@ -12,12 +12,10 @@ class DefaultPeerStorageTest:public CppUnit::TestFixture {
   CPPUNIT_TEST(testCountPeer);
   CPPUNIT_TEST(testDeleteUnusedPeer);
   CPPUNIT_TEST(testAddPeer);
-  CPPUNIT_TEST(testAddPeer_incomingPeer);
   CPPUNIT_TEST(testGetPeer);
   CPPUNIT_TEST(testIsPeerAvailable);
   CPPUNIT_TEST(testActivatePeer);
   CPPUNIT_TEST(testCalculateStat);
-  CPPUNIT_TEST(testAddIncomingPeer);
   CPPUNIT_TEST(testReturnPeer);
   CPPUNIT_TEST(testOnErasingPeer);
   CPPUNIT_TEST(testReturnPeer);
@@ -39,12 +37,10 @@ public:
   void testCountPeer();
   void testDeleteUnusedPeer();
   void testAddPeer();
-  void testAddPeer_incomingPeer();
   void testGetPeer();
   void testIsPeerAvailable();
   void testActivatePeer();
   void testCalculateStat();
-  void testAddIncomingPeer();
   void testReturnPeer();
   void testOnErasingPeer();
 };
@@ -99,22 +95,6 @@ void DefaultPeerStorageTest::testDeleteUnusedPeer() {
   
 }
 
-void DefaultPeerStorageTest::testAddPeer_incomingPeer()
-{
-  DefaultPeerStorage ps(btContext, option);
-
-  PeerHandle peer1 = new Peer("192.168.0.1", 6889, btContext->getPieceLength(),
-			      btContext->getTotalLength());
-  PeerHandle peer2 = new Peer("192.168.0.1", 6889, btContext->getPieceLength(),
-			      btContext->getTotalLength());
-  
-  CPPUNIT_ASSERT(ps.addIncomingPeer(peer1));
-  CPPUNIT_ASSERT(ps.addPeer(peer1));// because same instance is stored in incomingPeers and peers.
-  CPPUNIT_ASSERT(!ps.addPeer(peer2));
-
-
-}
-
 void DefaultPeerStorageTest::testAddPeer() {
   DefaultPeerStorage ps(btContext, option);
 
@@ -145,22 +125,12 @@ void DefaultPeerStorageTest::testAddPeer() {
   // peer1 was deleted.
   CPPUNIT_ASSERT_EQUAL((int32_t)3, ps.countPeer());
   
-  peer4->startBadCondition();
-
-  PeerHandle peer5(new Peer("192.168.0.4", 6889, btContext->getPieceLength(),
+  PeerHandle peer5(new Peer("192.168.0.4", 0, btContext->getPieceLength(),
 			    btContext->getTotalLength()));
+  peer5->port = 6889;
 
-  // this returns false, because peer4 in the container has error.
+  // this returns false because the peer which has same ip and port has already added
   CPPUNIT_ASSERT_EQUAL(false, ps.addPeer(peer5));
-
-  peer3->cuid = 1;
-
-  PeerHandle peer6(new Peer("192.168.0.3", 6889, btContext->getPieceLength(),
-			    btContext->getTotalLength()));
-
-  // this is false, because peer3's cuid is not zero
-  CPPUNIT_ASSERT_EQUAL(false, ps.addPeer(peer6));
-  
 }
 
 void DefaultPeerStorageTest::testGetPeer() {
@@ -231,18 +201,6 @@ void DefaultPeerStorageTest::testActivatePeer() {
 void DefaultPeerStorageTest::testCalculateStat() {
 }
 
-
-void DefaultPeerStorageTest::testAddIncomingPeer()
-{
-  DefaultPeerStorage ps(btContext, option);
- 
-  CPPUNIT_ASSERT(ps.addIncomingPeer(new Peer("192.168.0.1", 6889,
-					     btContext->getPieceLength(),
-					     btContext->getTotalLength())));
-
-  CPPUNIT_ASSERT_EQUAL((size_t)1, ps.getIncomingPeers().size());
-}
-
 void DefaultPeerStorageTest::testReturnPeer()
 {
   DefaultPeerStorage ps(btContext, option);
@@ -254,20 +212,18 @@ void DefaultPeerStorageTest::testReturnPeer()
   ps.addPeer(peer1);
   ps.addPeer(peer2);
 
-  PeerHandle peer3(new Peer("192.168.0.3", 6889, btContext->getPieceLength(),
-			   btContext->getTotalLength()));
-  
-  ps.addIncomingPeer(peer3);
+  PeerHandle peer3(new Peer("192.168.0.3", 0, btContext->getPieceLength(),
+			   btContext->getTotalLength()));  
+  ps.addPeer(peer3);
 
-  CPPUNIT_ASSERT_EQUAL(string("192.168.0.2"),
-		       ps.getPeers().front()->ipaddr);
   ps.returnPeer(peer2);
-  CPPUNIT_ASSERT_EQUAL(string("192.168.0.1"),
-		       ps.getPeers().front()->ipaddr);
-
-  ps.returnPeer(peer3);
-  CPPUNIT_ASSERT_EQUAL((size_t)0, ps.getIncomingPeers().size());
+  // peer2 is moved to the end of container
+  CPPUNIT_ASSERT_EQUAL(string("192.168.0.2"),
+		       ps.getPeers().back()->ipaddr);
 
+  ps.returnPeer(peer3); // peer3 is removed from the container
+  CPPUNIT_ASSERT_EQUAL((size_t)2, ps.getPeers().size());
+  CPPUNIT_ASSERT(find(ps.getPeers().begin(), ps.getPeers().end(), peer3) == ps.getPeers().end());
 }
 
 void DefaultPeerStorageTest::testOnErasingPeer()
diff --git a/test/HandshakeExtensionMessageTest.cc b/test/HandshakeExtensionMessageTest.cc
index 6cecd611..37d43b39 100644
--- a/test/HandshakeExtensionMessageTest.cc
+++ b/test/HandshakeExtensionMessageTest.cc
@@ -1,8 +1,7 @@
 #include "HandshakeExtensionMessage.h"
 #include "Peer.h"
 #include "MockBtContext.h"
-#include "MockPeerStorage.h"
-#include "BtRegistry.h"
+#include "Exception.h"
 #include <cppunit/extensions/HelperMacros.h>
 
 class HandshakeExtensionMessageTest:public CppUnit::TestFixture {
@@ -21,23 +20,9 @@ private:
 public:
   HandshakeExtensionMessageTest():_btContext(0) {}
 
-  void setUp()
-  {
-    BtRegistry::unregisterAll();
-    MockBtContextHandle btContext = new MockBtContext();
-    unsigned char infohash[20];
-    memset(infohash, 0, sizeof(infohash));
-    btContext->setInfoHash(infohash);
-    _btContext = btContext;
-    MockPeerStorageHandle peerStorage = new MockPeerStorage();
-    BtRegistry::registerPeerStorage(_btContext->getInfoHashAsString(),
-				    peerStorage);
-  }
+  void setUp() {}
 
-  void tearDown()
-  {
-    BtRegistry::unregisterAll();
-  }
+  void tearDown() {}
 
   void testGetExtensionMessageID();
   void testGetExtensionName();
@@ -99,11 +84,6 @@ void HandshakeExtensionMessageTest::testDoReceivedAction()
   CPPUNIT_ASSERT_EQUAL((uint16_t)6889, peer->port);
   CPPUNIT_ASSERT_EQUAL((uint8_t)1, peer->getExtensionMessageID("ut_pex"));
   CPPUNIT_ASSERT_EQUAL((uint8_t)2, peer->getExtensionMessageID("a2_dht"));
-
-  CPPUNIT_ASSERT_EQUAL((size_t)1, PEER_STORAGE(_btContext)->getPeers().size());
-  PeerHandle p1 = PEER_STORAGE(_btContext)->getPeers().front();
-  CPPUNIT_ASSERT_EQUAL(string("192.168.0.1"), p1->ipaddr);
-  CPPUNIT_ASSERT_EQUAL((uint16_t)6889, p1->port);
 }
 
 void HandshakeExtensionMessageTest::testCreate()
diff --git a/test/MockPeerStorage.h b/test/MockPeerStorage.h
index 2ca331a1..aea23336 100644
--- a/test/MockPeerStorage.h
+++ b/test/MockPeerStorage.h
@@ -21,11 +21,6 @@ public:
     copy(peers.begin(), peers.end(), back_inserter(this->peers));
   }
 
-  virtual bool addIncomingPeer(const PeerHandle& peer)
-  {
-    return true;
-  }
-
   virtual const Peers& getPeers() {
     return peers;
   }