Expand the IP Address reporting capabilities #27731

thzero · 2019-04-06T15:36:43Z

Currently the IP_Address and the IP._get_local_addresses method only
report the ip addresses of the adapters available on the device. The
results do not give information on which adapter the ip address is
associated with, nor the type of address, e.g. TYPE_IPV4 or TYPE_IPV6.

This change adds a new method IP._get_local_addresses_full that reports
more information about each ip address on the adapters. The method
results an array of dictionary objects with the following information:
'adapter' name
'friendly' adapter name
ip 'address'
'type' of address, e.g. TYPE_IPV4 or TYPE_IPV6.

Additionally, the NetworkedMultiplayerENet has methods to return the
address of a peer, but does not provide the address of peer 1.

This chane also adds two ne methods to the NetworkedMultiplayerENet
objects to retrieve the bound ip address of the host or client, e.g.
the bind_ip.
get_bind_address retrieves the bind address
get_bind_port retrieves the bind port.

Currently the IP_Address and the IP._get_local_addresses method only report the ip addresses of the adapters available on the device. The results do not give information on which adapter the ip address is associated with, nor the type of address, e.g. TYPE_IPV4 or TYPE_IPV6. This change adds a new method IP._get_local_addresses_full that reports more information about each ip address on the adapters. The method results an array of dictionary objects with the following information: 'adapter' name 'friendly' adapter name ip 'address' 'type' of address, e.g. TYPE_IPV4 or TYPE_IPV6. Additionally, the NetworkedMultiplayerENet has methods to return the address of a peer, but does not provide the address of peer 1. This chane also adds two ne methods to the NetworkedMultiplayerENet objects to retrieve the bound ip address of the host or client, e.g. the bind_ip. get_bind_address retrieves the bind address get_bind_port retrieves the bind port.

Faless

Hi @thzero . First of all thank you for your interest in Godot Engine.

While the capability of extracting more detailed information for available network interfaces is a wanted capability (and it will be needed soon as we plan to support extracting multicast addresses for example), this PR has 2 main problems I can see.

The change is implemented for Windows and Unixes, but it seems not to take into account UWP (Universal Windows Platform), which has a separate function for collecting those info and was not modified.

godot/drivers/unix/ip_unix.cpp

Lines 132 to 135 in 5fc8602

    
           void IP_Unix::get_local_addresses(List<IP_Address> *r_addresses) const { 
        
           	using namespace Windows::Networking; 
        
           	using namespace Windows::Networking::Connectivity;

More importantly, those informations should not be stored in IP_Address. Probably, a dedicated struct should be defined in IP (e.g. struct InterfaceInfo) that contains the IPAddress, the adapter name, the friendly name (and in the future, adapter index, and list of multicast addreses).
A list of those struct should then be returned by a new virtual get_interfaces_info in IP (implemented for all platforms in IP_Unix) and reused by get_local_addresses (which can then be moved to IP from IP_Unix) flattening that result to a list of IP_Address.

Furthermore, can you please remove the enet changes from this PR and create a new one for those changes only?

Faless · 2019-04-09T13:16:54Z

core/io/ip_address.cpp

@@ -40,6 +40,9 @@ IP_Address::operator Variant() const {

 IP_Address::operator String() const {

+	if (wildcard)
+		return "wildcard";


Use "*" instead for compatibility.

thzero · 2019-04-16T01:28:58Z

@Faless so made changes to create a Interface_Info struct. Having issues with the UWP (not really wanting to install VS2015) so haven't had chance to compile let alone test it.

Faless · 2019-04-16T09:57:13Z

@thzero few things:

I think you forgot to remove adapter and adapter_friendly from IP_Address (and the relative setter/getters).
I think the struct should not have 2 IP_Address (ipv4 and ipv6) but a single List<IP_Address>. The same interface might have multiple IPs of the same type.

PS: We it's ready I can test it with UWP (and fix it if something is wrong). I'll wait till the PR is ready, and CI checks pass.

Faless · 2019-04-16T10:07:46Z

I'm also not sure why you chose to have a struct that way, with setters/getters and the like.
Couldn't it be just a struct inside IP with an initializer and public fields like I suggested above?
It seems unnecessary overhead to have setter and getters for something that is not even exposed to scripting, while it could be handled almost like a C struct.

thzero · 2019-04-20T15:42:24Z

I'm also not sure why you chose to have a struct that way, with setters/getters and the like.
Couldn't it be just a struct inside IP with an initializer and public fields like I suggested above?
It seems unnecessary overhead to have setter and getters for something that is not even exposed to scripting, while it could be handled almost like a C struct.

Ok, refactored based on your recommendations.

Faless · 2019-05-09T16:23:58Z

@thzero can you apply the following patch? (I known it's quite big, I'm trying to avoid duplicating code for interfaces and addresses):

diff --git a/core/io/ip.cpp b/core/io/ip.cpp
index 0bca4c2e0..75d7dab1c 100644
--- a/core/io/ip.cpp
+++ b/core/io/ip.cpp
@@ -237,9 +237,9 @@ Array IP::_get_local_addresses() const {
 Array IP::_get_local_interfaces() const {
 
 	Array results;
-	List<Interface_Info> interfaces;
+	Map<String, Interface_Info> interfaces;
 	get_local_interfaces(&interfaces);
-	for (List<Interface_Info>::Element *E = interfaces.front(); E; E = E->next()) {
+	for (Map<String, Interface_Info>::Element *E = interfaces.front(); E; E = E->next()) {
 		Interface_Info &c = E->get();
 		Dictionary rc;
 		rc["name"] = c.name;
@@ -251,18 +251,34 @@ Array IP::_get_local_interfaces() const {
 			IP_Address ip = F->get();
 			Dictionary rc_ip;
 			rc_ip["address"] = ip;
-			rc_ip["type"] = (ip.is_ipv4() ? IP::TYPE_IPV4 : ip.is_ipv6() ? IP::TYPE_IPV6 : ip.is_wildcard() ? IP::TYPE_ANY : IP::TYPE_NONE);
-			ips.push_back(rc_ip);
+			if (ip.is_wildcard())
+				rc_ip["type"] = IP::TYPE_ANY;
+			else if (!ip.is_valid())
+				rc_ip["type"] = IP::TYPE_NONE;
+			else
+				rc_ip["type"] = ip.is_ipv4() ? IP::TYPE_IPV4 : IP::TYPE_IPV6;
+			ips.push_front(rc_ip);
 		}
 
 		rc["addresses"] = ips;
 
-		results.push_back(rc);
+		results.push_front(rc);
 	}
 
 	return results;
 }
 
+void IP::get_local_addresses(List<IP_Address> *r_addresses) const {
+
+	Map<String, Interface_Info> interfaces;
+	get_local_interfaces(&interfaces);
+	for (Map<String, Interface_Info>::Element *E = interfaces.front(); E; E = E->next()) {
+		for (const List<IP_Address>::Element *F = E->get().ip_addresses.front(); F; F = F->next()) {
+			r_addresses->push_front(F->get());
+		}
+	}
+}
+
 void IP::_bind_methods() {
 
 	ClassDB::bind_method(D_METHOD("resolve_hostname", "host", "ip_type"), &IP::resolve_hostname, DEFVAL(IP::TYPE_ANY));
diff --git a/core/io/ip.h b/core/io/ip.h
index 00d1d5db3..2143b25e9 100644
--- a/core/io/ip.h
+++ b/core/io/ip.h
@@ -89,8 +89,8 @@ public:
 	ResolverID resolve_hostname_queue_item(const String &p_hostname, Type p_type = TYPE_ANY);
 	ResolverStatus get_resolve_item_status(ResolverID p_id) const;
 	IP_Address get_resolve_item_address(ResolverID p_id) const;
-	virtual void get_local_addresses(List<IP_Address> *r_addresses) const = 0;
-	virtual void get_local_interfaces(List<Interface_Info> *r_interfaces) const = 0;
+	virtual void get_local_addresses(List<IP_Address> *r_addresses) const;
+	virtual void get_local_interfaces(Map<String, Interface_Info> *r_interfaces) const = 0;
 	void erase_resolve_item(ResolverID p_id);
 
 	void clear_cache(const String &p_hostname = "");
diff --git a/core/io/ip_address.cpp b/core/io/ip_address.cpp
index ae929e29b..9305afac5 100644
--- a/core/io/ip_address.cpp
+++ b/core/io/ip_address.cpp
@@ -186,10 +186,6 @@ bool IP_Address::is_ipv4() const {
 	return (field32[0] == 0 && field32[1] == 0 && field16[4] == 0 && field16[5] == 0xffff);
 }
 
-bool IP_Address::is_ipv6() const {
-	return !is_ipv4();
-}
-
 const uint8_t *IP_Address::get_ipv4() const {
 	ERR_FAIL_COND_V(!is_ipv4(), &(field8[12])); // Not the correct IPv4 (it's an IPv6), but we don't want to return a null pointer risking an engine crash.
 	return &(field8[12]);
diff --git a/core/io/ip_address.h b/core/io/ip_address.h
index a3792d078..3a5f87b61 100644
--- a/core/io/ip_address.h
+++ b/core/io/ip_address.h
@@ -72,9 +72,9 @@ public:
 	bool is_wildcard() const { return wildcard; }
 	bool is_valid() const { return valid; }
 	bool is_ipv4() const;
-	bool is_ipv6() const;
 	const uint8_t *get_ipv4() const;
 	void set_ipv4(const uint8_t *p_ip);
+
 	const uint8_t *get_ipv6() const;
 	void set_ipv6(const uint8_t *p_buf);
 
diff --git a/drivers/unix/ip_unix.cpp b/drivers/unix/ip_unix.cpp
index b6d6bed9e..1fe50e879 100644
--- a/drivers/unix/ip_unix.cpp
+++ b/drivers/unix/ip_unix.cpp
@@ -129,59 +129,41 @@ IP_Address IP_Unix::_resolve_hostname(const String &p_hostname, Type p_type) {
 
 #if defined(UWP_ENABLED)
 
-void IP_Unix::get_local_addresses(List<IP_Address> *r_addresses) const {
+void IP_Unix::get_local_interfaces(Map<String, Interface_Info> *r_interfaces) const {
 
 	using namespace Windows::Networking;
 	using namespace Windows::Networking::Connectivity;
 
+	// Returns addresses, not interfaces.
 	auto hostnames = NetworkInformation::GetHostNames();
 
 	for (int i = 0; i < hostnames->Size; i++) {
 
-		if (hostnames->GetAt(i)->Type == HostNameType::Ipv4 || hostnames->GetAt(i)->Type == HostNameType::Ipv6 && hostnames->GetAt(i)->IPInformation != nullptr) {
+		auto hostname = hostnames->GetAt(i);
 
-			r_addresses->push_back(IP_Address(String(hostnames->GetAt(i)->CanonicalName->Data())));
-		}
-	}
-};
-
-void IP_Unix::get_local_interfaces(List<Interface_Info> *r_interfaces) const {
-
-	using namespace Windows::Networking;
-	using namespace Windows::Networking::Connectivity;
-
-	auto hostnames = NetworkInformation::GetHostNames();
-
-	Map<String, Interface_Info> interface_map;
-
-	for (int i = 0; i < hostnames->Size; i++) {
-
-		if (!((hostnames->GetAt(i)->Type == HostNameType::Ipv4 || hostnames->GetAt(i)->Type == HostNameType::Ipv6) && hostnames->GetAt(i)->IPInformation != nullptr))
+		if (hostname->Type != HostNameType::Ipv4 && hostname->Type != HostNameType::Ipv6)
 			continue;
 
-		String name = hostnames->GetAt(i)->DisplayName->Data();
-
-		Interface_Info info;
-
-		Map<String, Interface_Info>::Element *E = interface_map.find(name);
+		String name = hostname->RawName->Data();
+		Map<String, Interface_Info>::Element *E = r_interfaces->find(name);
 		if (!E) {
+			Interface_Info info;
 			info.name = name;
-			info.name_friendly = name;
-			interface_map[name] = info;
-		} else {
-			Interface_Info &c = E->get();
-			info = c;
+			info.name_friendly = hostname->DisplayName->Data();
+			E = r_interfaces->insert(name, info);
+			ERR_CONTINUE(!E);
 		}
 
-		IP_Address ip = _sockaddr2ip(hostnames->GetAt(i)->CanonicalName->Data()));
-		info.ip_addresses.push_front(ip);
+		Interface_Info &info = E->get();
 
-		r_interfaces->push_back(info);
+		IP_Address ip = IP_Address(hostname->CanonicalName->Data());
+		info.ip_addresses.push_front(ip);
 	}
 }
+
 #else
 
-void IP_Unix::get_local_addresses(List<IP_Address> *r_addresses) const {
+void IP_Unix::get_local_interfaces(Map<String, Interface_Info> *r_interfaces) const {
 
 	ULONG buf_size = 1024;
 	IP_ADAPTER_ADDRESSES *addrs;
@@ -206,65 +188,10 @@ void IP_Unix::get_local_addresses(List<IP_Address> *r_addresses) const {
 
 	IP_ADAPTER_ADDRESSES *adapter = addrs;
 
-	while (adapter != NULL) {
-
-		IP_ADAPTER_UNICAST_ADDRESS *address = adapter->FirstUnicastAddress;
-		while (address != NULL) {
-
-			IP_Address ip;
-
-			if (address->Address.lpSockaddr->sa_family == AF_INET) {
-
-				SOCKADDR_IN *ipv4 = reinterpret_cast<SOCKADDR_IN *>(address->Address.lpSockaddr);
-
-				ip.set_ipv4((uint8_t *)&(ipv4->sin_addr));
-
-			} else if (address->Address.lpSockaddr->sa_family == AF_INET6) { // ipv6
-
-				SOCKADDR_IN6 *ipv6 = reinterpret_cast<SOCKADDR_IN6 *>(address->Address.lpSockaddr);
-
-				ip.set_ipv6(ipv6->sin6_addr.s6_addr);
-			};
-
-			r_addresses->push_back(ip);
-
-			address = address->Next;
-		};
-		adapter = adapter->Next;
-	};
-
-	memfree(addrs);
-};
-
-void IP_Unix::get_local_interfaces(List<Interface_Info> *r_interfaces) const {
-
-	ULONG buf_size = 1024;
-	IP_ADAPTER_ADDRESSES *addrs;
-
-	while (true) {
-
-		addrs = (IP_ADAPTER_ADDRESSES *)memalloc(buf_size);
-		int err = GetAdaptersAddresses(AF_UNSPEC, GAA_FLAG_SKIP_ANYCAST | GAA_FLAG_SKIP_MULTICAST | GAA_FLAG_SKIP_DNS_SERVER | GAA_FLAG_SKIP_FRIENDLY_NAME,
-				NULL, addrs, &buf_size);
-		if (err == NO_ERROR) {
-			break;
-		};
-		memfree(addrs);
-		if (err == ERROR_BUFFER_OVERFLOW) {
-			continue; // will go back and alloc the right size
-		};
-
-		ERR_EXPLAIN("Call to GetLocalInterfaces failed with error " + itos(err));
-		ERR_FAIL();
-		return;
-	};
-
-	IP_ADAPTER_ADDRESSES *adapter = addrs;
-
 	while (adapter != NULL) {
 
 		Interface_Info info;
-		info.name = adapter->FriendlyName;
+		info.name = adapter->AdapterName;
 		info.name_friendly = adapter->FriendlyName;
 
 		IP_ADAPTER_UNICAST_ADDRESS *address = adapter->FirstUnicastAddress;
@@ -277,20 +204,20 @@ void IP_Unix::get_local_interfaces(List<Interface_Info> *r_interfaces) const {
 				SOCKADDR_IN *ipv4 = reinterpret_cast<SOCKADDR_IN *>(address->Address.lpSockaddr);
 
 				ip.set_ipv4((uint8_t *)&(ipv4->sin_addr));
+				info.ip_addresses.push_front(ip);
 
 			} else if (address->Address.lpSockaddr->sa_family == AF_INET6) { // ipv6
 
 				SOCKADDR_IN6 *ipv6 = reinterpret_cast<SOCKADDR_IN6 *>(address->Address.lpSockaddr);
 
 				ip.set_ipv6(ipv6->sin6_addr.s6_addr);
+				info.ip_addresses.push_front(ip);
 			};
 
-			info.ip_addresses.push_front(ip);
-
 			address = address->Next;
 		};
 
-		r_interfaces->push_back(info);
+		r_interfaces->insert(adapter->AdapterName, info);
 
 		adapter = adapter->Next;
 	};
@@ -302,31 +229,7 @@ void IP_Unix::get_local_interfaces(List<Interface_Info> *r_interfaces) const {
 
 #else // UNIX
 
-void IP_Unix::get_local_addresses(List<IP_Address> *r_addresses) const {
-
-	struct ifaddrs *ifAddrStruct = NULL;
-	struct ifaddrs *ifa = NULL;
-	int family;
-
-	getifaddrs(&ifAddrStruct);
-
-	for (ifa = ifAddrStruct; ifa != NULL; ifa = ifa->ifa_next) {
-		if (!ifa->ifa_addr)
-			continue;
-
-		family = ifa->ifa_addr->sa_family;
-
-		if (family != AF_INET && family != AF_INET6)
-			continue;
-
-		IP_Address ip = _sockaddr2ip(ifa->ifa_addr);
-		r_addresses->push_back(ip);
-	}
-
-	if (ifAddrStruct != NULL) freeifaddrs(ifAddrStruct);
-}
-
-void IP_Unix::get_local_interfaces(List<Interface_Info> *r_interfaces) const {
+void IP_Unix::get_local_interfaces(Map<String, Interface_Info> *r_interfaces) const {
 
 	struct ifaddrs *ifAddrStruct = NULL;
 	struct ifaddrs *ifa = NULL;
@@ -334,8 +237,6 @@ void IP_Unix::get_local_interfaces(List<Interface_Info> *r_interfaces) const {
 
 	getifaddrs(&ifAddrStruct);
 
-	Map<String, Interface_Info> interface_map;
-
 	for (ifa = ifAddrStruct; ifa != NULL; ifa = ifa->ifa_next) {
 		if (!ifa->ifa_addr)
 			continue;
@@ -345,26 +246,22 @@ void IP_Unix::get_local_interfaces(List<Interface_Info> *r_interfaces) const {
 		if (family != AF_INET && family != AF_INET6)
 			continue;
 
-		Interface_Info info;
-
-		Map<String, Interface_Info>::Element *E = interface_map.find(ifa->ifa_name);
+		Map<String, Interface_Info>::Element *E = r_interfaces->find(ifa->ifa_name);
 		if (!E) {
+			Interface_Info info;
 			info.name = ifa->ifa_name;
 			info.name_friendly = ifa->ifa_name;
-			interface_map[ifa->ifa_name] = info;
-		} else {
-			Interface_Info &c = E->get();
-			info = c;
+			E = r_interfaces->insert(ifa->ifa_name, info);
+			ERR_CONTINUE(!E);
 		}
 
+		Interface_Info &info = E->get();
+
 		IP_Address ip = _sockaddr2ip(ifa->ifa_addr);
 		info.ip_addresses.push_front(ip);
-
-		r_interfaces->push_back(info);
 	}
 
-	if (ifAddrStruct != NULL)
-		freeifaddrs(ifAddrStruct);
+	if (ifAddrStruct != NULL) freeifaddrs(ifAddrStruct);
 }
 #endif
 
diff --git a/drivers/unix/ip_unix.h b/drivers/unix/ip_unix.h
index 932713102..f9bb626cc 100644
--- a/drivers/unix/ip_unix.h
+++ b/drivers/unix/ip_unix.h
@@ -43,8 +43,7 @@ class IP_Unix : public IP {
 	static IP *_create_unix();
 
 public:
-	virtual void get_local_addresses(List<IP_Address> *r_addresses) const;
-	virtual void get_local_interfaces(List<Interface_Info> *r_interfaces) const;
+	virtual void get_local_interfaces(Map<String, Interface_Info> *r_interfaces) const;
 
 	static void make_default();
 	IP_Unix();

thzero · 2019-05-09T18:11:13Z

Sounds good... might be a day or two with Easter and all upcoming

akien-mga · 2019-05-28T10:27:21Z

Bump :)

akien-mga · 2019-05-28T10:27:48Z

The commits will need to be squashed together too.

Faless · 2019-06-21T08:35:21Z

Superseded by #29935.

thzero requested review from hpvb, reduz and a team as code owners April 6, 2019 15:36

Chaosus added enhancement topic:network labels Apr 6, 2019

Chaosus added this to the 3.2 milestone Apr 6, 2019

Faless requested changes Apr 9, 2019

View reviewed changes

thzero added 4 commits April 9, 2019 21:36

Use * instead for compatibility.

c948b9b

remove changes to enet to another PR

71d1dc4

Added Interfaces_Info to store network adapters

b885d23

Cleanup for Interface_Info addition

4b6a18d

thzero and others added 5 commits April 17, 2019 22:12

Cleanup for Interface_Info addition

ce746c5

moved interface info to ip class

24361fc

updates to get_local_interfaces for unix and uwp

e2e8d83

formatting cleanup

790cb48

update documentation for ip

12633e2

akien-mga requested review from Faless and removed request for reduz April 30, 2019 09:09

Faless closed this Jun 21, 2019

akien-mga added the archived label Jun 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand the IP Address reporting capabilities #27731

Expand the IP Address reporting capabilities #27731

thzero commented Apr 6, 2019

Faless left a comment

Faless Apr 9, 2019

thzero Apr 10, 2019

thzero commented Apr 16, 2019

Faless commented Apr 16, 2019 •

edited

Loading

Faless commented Apr 16, 2019

thzero commented Apr 20, 2019

Faless commented May 9, 2019

thzero commented May 9, 2019

akien-mga commented May 28, 2019

akien-mga commented May 28, 2019

Faless commented Jun 21, 2019

	void IP_Unix::get_local_addresses(List<IP_Address> *r_addresses) const {

	using namespace Windows::Networking;
	using namespace Windows::Networking::Connectivity;

Expand the IP Address reporting capabilities #27731

Expand the IP Address reporting capabilities #27731

Conversation

thzero commented Apr 6, 2019

Faless left a comment

Choose a reason for hiding this comment

Faless Apr 9, 2019

Choose a reason for hiding this comment

thzero Apr 10, 2019

Choose a reason for hiding this comment

thzero commented Apr 16, 2019

Faless commented Apr 16, 2019 • edited Loading

Faless commented Apr 16, 2019

thzero commented Apr 20, 2019

Faless commented May 9, 2019

thzero commented May 9, 2019

akien-mga commented May 28, 2019

akien-mga commented May 28, 2019

Faless commented Jun 21, 2019

Faless commented Apr 16, 2019 •

edited

Loading