improve the async action and getStateVariable

git-svn-id: http://svn.code.sf.net/p/retroshare/code/trunk@1764 b45a01b8-16f6-495d-af2f-9b41ad6348cc

libretroshare/src/upnp/UPnPBase.cpp

@@ -729,37 +729,48 @@ bool CUPnPService::Execute(
 const std::string CUPnPService::GetStateVariable(
 	const std::string &stateVariableName)
 {
-	//this getvar is just to make an event happening
+	std::map<std::string, std::string>::iterator it;
-	DOMString StVarVal;
+	it = propertyMap.find(stateVariableName);
-	int ret = UpnpGetServiceVarStatus(
+	if  (it != propertyMap.end()) {
-		m_UPnPControlPoint.GetUPnPClientHandle(),
+	    std::cerr << "GetStateVariable(" << stateVariableName << ") = ";
-		GetAbsControlURL().c_str(),
+	    std::cerr << (*it).second << std::endl;
-		stateVariableName.c_str(),
+	    return (*it).second;
-		&StVarVal);
-	if (StVarVal != NULL) {
-	    std::string varValue = std::string(StVarVal);
-	    std::cerr << "GetStateVariable varValue returned by UpnpGetServiceVarStatus : " << varValue << std::endl;
-	    return varValue;
 	} else {
-	    //use event to get state variable
+	    //property map is not populated with the specified value.
-	    std::cerr << "GetStateVariable pausing in case of an UPnP event incomming.";
+	    //we will try to get it with an event
-	    time_t begin_time = time(NULL);
+
-	    while (true) {
+	    //this getvar is just to make the event happening
-		if (time(NULL) - begin_time > 7) {
+	    DOMString StVarVal;
-		    break;
+	    UpnpGetServiceVarStatus(
+		    m_UPnPControlPoint.GetUPnPClientHandle(),
+		    GetAbsControlURL().c_str(),
+		    stateVariableName.c_str(),
+		    &StVarVal);
+	    if (StVarVal != NULL) {
+		std::string varValue = std::string(StVarVal);
+		std::cerr << "GetStateVariable varValue returned by UpnpGetServiceVarStatus : " << varValue << std::endl;
+		return varValue;
+	    } else {
+		//use event to get state variable
+		std::cerr << "GetStateVariable pausing in case of an UPnP event incomming.";
+		time_t begin_time = time(NULL);
+		while (true) {
+		    if (time(NULL) - begin_time > 7) {
+			break;
+		    }
 		}
 	    }
-	    std::cerr << "GetStateVariable pause finished.";
 
-	    std::cerr << "GetStateVariable(" << stateVariableName << ") = ";
+	    //propertyMap should be populated by nom
-	    std::map<std::string, std::string>::iterator it;
 	    it = propertyMap.find(stateVariableName);
-	    if  (it == propertyMap.end()) {
+	    if  (it != propertyMap.end()) {
-		std::cerr << "Empty String" << std::endl;
+		std::cerr << "GetStateVariable(" << stateVariableName << ") = ";
-		return stdEmptyString;
-	    } else {
 		std::cerr << (*it).second << std::endl;
 		return (*it).second;
+	    } else {
+		std::cerr << "GetStateVariable(" << stateVariableName << ") = ";
+		std::cerr << "Empty String" << std::endl;
+		return stdEmptyString;
 	    }
 	}
 }
@@ -929,6 +940,11 @@ m_WanService(NULL)
 
 	}
 
+	//clean the PortMappingNumberOfEntries as it is erroneus on the first event with the french neufbox
+	if (WanServiceDetected()) {
+	    m_WanService->propertyMap.erase("PortMappingNumberOfEntries");
+	}
+
 	std::cerr << "CUPnPControlPoint Constructor finished" << std::endl;
 	return;
 
@@ -976,6 +992,14 @@ bool CUPnPControlPoint::AddPortMappings(
 	bool ok = false;
 
 	// Check the number of port mappings before
+	//have a little break in case we just modified the variable, so we have to wait for an event
+	std::cerr << "GetStateVariable pausing in case of an UPnP event incomming.";
+	time_t begin_time = time(NULL);
+	while (true) {
+	   if (time(NULL) - begin_time > 7) {
+	       break;
+	   }
+	}
 	std::istringstream OldPortMappingNumberOfEntries(
 		m_WanService->GetStateVariable(
 			"PortMappingNumberOfEntries"));
@@ -995,14 +1019,15 @@ bool CUPnPControlPoint::AddPortMappings(
 		}
 	}
 
-	// Debug only
+	// Not very good, must find a better test : check the new number of port entries
-#ifdef UPNP_DEBUG
+	//have a little break in case we just modified the variable, so we have to wait for an event
-	std::cerr << "CUPnPControlPoint::AddPortMappings: "
+	std::cerr << "GetStateVariable pausing in case of an UPnP event incomming.";
-		"m_ActivePortMappingsMap.size() == " <<
+	begin_time = time(NULL);
-		m_ActivePortMappingsMap.size() << std::endl;
+	while (true) {
-#endif
+	   if (time(NULL) - begin_time > 7) {
-
+	       break;
-	// Not very good, must find a better test
+	   }
+	}
 	std::istringstream NewPortMappingNumberOfEntries(
 		m_WanService->GetStateVariable(
 			"PortMappingNumberOfEntries"));
@@ -1024,10 +1049,16 @@ std::string CUPnPControlPoint::getExternalAddress()
 			"WAN Service not detected." << std::endl;
 		return false;
 	}
-	PrivateGetExternalIpAdress();
 	std::string result =  m_WanService->GetStateVariable("NewExternalIPAddress");
+	std::cerr <<  " m_WanService->GetStateVariable(NewExternalIPAddress) = " <<  result << std::endl;
 	if (result == "") {
-	    result = m_WanService->GetStateVariable("ExternalIPAddress");
+	    PrivateGetExternalIpAdress();
+	    result =  m_WanService->GetStateVariable("NewExternalIPAddress");
+	    std::cerr <<  " m_WanService->GetStateVariable(NewExternalIPAddress) = " <<  result << std::endl;
+	    if (result == "") {
+		result = m_WanService->GetStateVariable("ExternalIPAddress");
+		std::cerr <<  " m_WanService->GetStateVariable(ExternalIPAddress) = " <<  result << std::endl;
+	    }
 	}
 	return result;
 }

libretroshare/src/upnp/upnphandler.cc

@@ -100,8 +100,6 @@ bool upnphandler::background_setup_upnp(bool start, bool stop)
 
 bool upnphandler::start_upnp()
 {
-	RsStackMutex stack(dataMtx); /* LOCK STACK MUTEX */
-
 	if (!(upnpState >= RS_UPNP_S_READY))
 	{
 		std::cerr << "upnphandler::start_upnp() Not Ready" << std::endl;
@@ -111,94 +109,79 @@ bool upnphandler::start_upnp()
 	char eprot1[] = "TCP";
 	char eprot2[] = "UDP";
 
-	/* if we're to load -> load */
+	struct sockaddr_in localAddr;
-	/* select external ports */
-	eport_curr = eport;
-	if (!eport_curr)
 	{
-		/* use local port if eport is zero */
+	    RsStackMutex stack(dataMtx); /* LOCK STACK MUTEX */
-		eport_curr = iport;
+
-		std::cerr << "Using LocalPort for extPort!";
+	    /* if we're to load -> load */
-		std::cerr << std::endl;
+	    /* select external ports */
+	    eport_curr = eport;
+	    if (!eport_curr)
+	    {
+		    /* use local port if eport is zero */
+		    eport_curr = iport;
+		    std::cerr << "Using LocalPort for extPort!";
+		    std::cerr << std::endl;
+	    }
+
+	    if (!eport_curr)
+	    {
+		    std::cerr << "Invalid eport ... ";
+		    std::cerr << std::endl;
+		    return false;
+	    }
+
+
+	    /* our port */
+	    char in_addr[256];
+	    char in_port1[256];
+	    char eport1[256];
+
+	    upnp_iaddr.sin_port = htons(iport);
+	    localAddr = upnp_iaddr;
+	    uint32_t linaddr = ntohl(localAddr.sin_addr.s_addr);
+
+	    snprintf(in_port1, 256, "%d", ntohs(localAddr.sin_port));
+	    snprintf(in_addr, 256, "%d.%d.%d.%d",
+		     ((linaddr >> 24) & 0xff),
+		     ((linaddr >> 16) & 0xff),
+		     ((linaddr >> 8) & 0xff),
+		     ((linaddr >> 0) & 0xff));
+
+	    snprintf(eport1, 256, "%d", eport_curr);
+
+	    std::cerr << "Attempting Redirection: InAddr: " << in_addr;
+	    std::cerr << " InPort: " << in_port1;
+	    std::cerr << " ePort: " << eport1;
+	    std::cerr << " eProt: " << eprot1;
+	    std::cerr << std::endl;
 	}
 
-	if (!eport_curr)
-	{
-		std::cerr << "Invalid eport ... ";
-		std::cerr << std::endl;
-		return false;
-	}
-
-
-	/* our port */
-	char in_addr[256];
-	char in_port1[256];
-	char eport1[256];
-
-	upnp_iaddr.sin_port = htons(iport);
-	struct sockaddr_in localAddr = upnp_iaddr;
-	uint32_t linaddr = ntohl(localAddr.sin_addr.s_addr);
-
-	snprintf(in_port1, 256, "%d", ntohs(localAddr.sin_port));
-	snprintf(in_addr, 256, "%d.%d.%d.%d", 
-		 ((linaddr >> 24) & 0xff),
-		 ((linaddr >> 16) & 0xff),
-		 ((linaddr >> 8) & 0xff),
-		 ((linaddr >> 0) & 0xff));
-
-	snprintf(eport1, 256, "%d", eport_curr);
-
-	std::cerr << "Attempting Redirection: InAddr: " << in_addr;
-	std::cerr << " InPort: " << in_port1;
-	std::cerr << " ePort: " << eport1;
-	std::cerr << " eProt: " << eprot1;
-	std::cerr << std::endl;
-
 	//build port mapping config
 	std::vector<CUPnPPortMapping> upnpPortMapping1;
 	CUPnPPortMapping cUPnPPortMapping1 = CUPnPPortMapping(eport_curr, ntohs(localAddr.sin_port), "TCP", true, "tcp retroshare redirection");
 	upnpPortMapping1.push_back(cUPnPPortMapping1);
 	bool res = cUPnPControlPoint->AddPortMappings(upnpPortMapping1);
 
-	if (res) {
+	std::vector<CUPnPPortMapping> upnpPortMapping2;
-	    upnpState = RS_UPNP_S_ACTIVE;
+	CUPnPPortMapping cUPnPPortMapping2 = CUPnPPortMapping(eport_curr, ntohs(localAddr.sin_port), "UDP", true, "udp retroshare redirection");
-	} else {
+	upnpPortMapping2.push_back(cUPnPPortMapping2);
-	    upnpState = RS_UPNP_S_TCP_FAILED;
+	bool res2 = cUPnPControlPoint->AddPortMappings(upnpPortMapping2);
-	    std::vector<CUPnPPortMapping> upnpPortMapping2;
-	    CUPnPPortMapping cUPnPPortMapping2 = CUPnPPortMapping(eport_curr, ntohs(localAddr.sin_port), "UDP", true, "udp retroshare redirection");
-	    upnpPortMapping2.push_back(cUPnPPortMapping2);
-	    bool res2 = cUPnPControlPoint->AddPortMappings(upnpPortMapping2);
-	    if (res2) {
-		upnpState = RS_UPNP_S_ACTIVE;
-	    } else {
-		//upnpState = RS_UPNP_S_ACTIVE;
-		upnpState = RS_UPNP_S_UDP_FAILED;
-	    }
-	}
 
-	/* now store the external address */
+	struct sockaddr_in extAddr;
-	std::string externalAdress = cUPnPControlPoint->getExternalAddress();
+	bool extAddrResult = getExternalAddress(extAddr);
-	sockaddr_clear(&upnp_eaddr);
 
-	if(!externalAdress.empty())
 	{
-		const char* externalIPAddress = externalAdress.c_str();
+		RsStackMutex stack(dataMtx); /* LOCK STACK MUTEX */
 
-		std::cerr << "Stored External address: " << externalIPAddress;
+		if (extAddrResult && (res || res2)) {
-		std::cerr << ":" << eport_curr;
+		    upnpState = RS_UPNP_S_ACTIVE;
-		std::cerr << std::endl;
+		} else {
+		    upnpState = RS_UPNP_S_UDP_FAILED;
+		}
 
-		inet_aton(externalIPAddress, &(upnp_eaddr.sin_addr));
+		toStart = false;
-		upnp_eaddr.sin_family = AF_INET;
-		upnp_eaddr.sin_port = htons(eport_curr);
 	}
-	else
-	{
-		std::cerr << "FAILED To get external Address";
-		std::cerr << std::endl;
-	}
-
-	toStart = false;
 
 	return (upnpState == RS_UPNP_S_ACTIVE);
 
@@ -414,15 +397,28 @@ bool    upnphandler::getInternalAddress(struct sockaddr_in &addr)
 
 bool    upnphandler::getExternalAddress(struct sockaddr_in &addr)
 {
-//	std::cerr << "UPnPHandler::getExternalAddress() pre Lock!" << std::endl;
+	std::string externalAdress = cUPnPControlPoint->getExternalAddress();
-	dataMtx.lock();   /***  LOCK MUTEX  ***/
-//	std::cerr << "UPnPHandler::getExternalAddress() postLock!" << std::endl;
 
-	std::cerr << "UPnPHandler::getExternalAddress()" << std::endl;
+	if(!externalAdress.empty())
-	addr = upnp_eaddr;
+	{
-	bool valid = (upnpState == RS_UPNP_S_ACTIVE);
+		const char* externalIPAddress = externalAdress.c_str();
 
-	dataMtx.unlock(); /*** UNLOCK MUTEX ***/
+		std::cerr << "Stored External address: " << externalIPAddress;
+		std::cerr << ":" << eport_curr;
+		std::cerr << std::endl;
 
-	return valid;
+		dataMtx.lock();   /***  LOCK MUTEX  ***/
+		sockaddr_clear(&upnp_eaddr);
+		inet_aton(externalIPAddress, &(upnp_eaddr.sin_addr));
+		upnp_eaddr.sin_family = AF_INET;
+		upnp_eaddr.sin_port = htons(eport_curr);
+		dataMtx.unlock(); /*** UNLOCK MUTEX ***/
+
+		addr = upnp_eaddr;
+		return true;
+	}
+	else
+	{
+		return false;
+	}
 }