widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

I have done a code review anyway. Since I don't know that much about networking, it's for code style only.

I have also given it a quick spin and found that IPv4 is preferred over IPv6, I think it should be the other way around if possible, since v6 is the more modern protocol.

Diff comments:

> 
> === modified file 'src/network/gameclient.cc'
> --- src/network/gameclient.cc	2017-05-11 10:45:44 +0000
> +++ src/network/gameclient.cc	2017-05-20 19:06:48 +0000
> @@ -89,17 +88,13 @@
>  	std::vector<ChatMessage> chatmessages;
>  };
>  
> -GameClient::GameClient(const std::string& host,
> -                       const uint16_t port,
> -                       const std::string& playername,
> -                       bool internet)
> +GameClient::GameClient(const NetAddress& host, const std::string& playername, bool internet)
>     : d(new GameClientImpl), internet_(internet) {
> -	d->net = NetClient::connect(host, port);
> +	d->net = NetClient::connect(host);
>  	if (!d->net || !d->net->is_connected()) {
>  		throw WLWarning(_("Could not establish connection to host"),
> -		                _("Widelands could not establish a connection to the given "
> -		                  "address.\n"
> -		                  "Either no Widelands server was running at the supposed port or\n"
> +		                _("Widelands could not establish a connection to the given address. "

Note: In general, do not change translatable strings without good reason, because it will break all the translations. I do agree with this particular change though.

> +		                  "Either no Widelands server was running at the supposed port or "
>  		                  "the server shut down as you tried to connect."));
>  	}
>  
> 
> === modified file 'src/network/nethost.cc'
> --- src/network/nethost.cc	2017-05-11 10:45:44 +0000
> +++ src/network/nethost.cc	2017-05-20 19:06:48 +0000
> @@ -47,68 +51,136 @@
>  		// Not connected anyway
>  		return;
>  	}
> -	SDLNet_TCP_DelSocket(sockset_, iter_client->second.socket);
> -	SDLNet_TCP_Close(iter_client->second.socket);
> +	boost::system::error_code ec;
> +	iter_client->second.socket.shutdown(boost::asio::ip::tcp::socket::shutdown_both, ec);
> +	iter_client->second.socket.close(ec);
>  	clients_.erase(iter_client);
>  }
>  
>  bool NetHost::try_accept(ConnectionId* new_id) {
>  	if (!is_listening())
>  		return false;
> +	boost::system::error_code ec;
> +	boost::asio::ip::tcp::socket socket(io_service_);
> +	if (acceptor_v4_.is_open()) {

I think that we should swap these and prefer IPv6 if we can get it. This will give us more testing exposure for IPv6. e.g. my network can handle both, so I'd always end up with IPv4 on a LAN connection.

> +		acceptor_v4_.accept(socket, ec);
> +		if (ec == boost::asio::error::would_block) {
> +			// No client wants to connect
> +			// New socket don't need to be closed since it isn't open yet
> +		} else if (ec) {
> +			// Some other error, close the acceptor
> +			acceptor_v4_.close(ec);
> +		} else {
> +			log("[NetHost]: Accepting IPv4 connection from %s\n",
> +				socket.remote_endpoint().address().to_string().c_str());
> +		}
> +	}
> +	if (acceptor_v6_.is_open() && !socket.is_open()) {
> +		// IPv4 did not get a connection
> +		acceptor_v6_.accept(socket, ec);
> +		if (ec == boost::asio::error::would_block) {
> +			;
> +		} else if (ec) {
> +			acceptor_v6_.close(ec);
> +		} else {
> +			log("[NetHost]: Accepting IPv6 connection from %s\n",
> +				socket.remote_endpoint().address().to_string().c_str());
> +		}
> +	}
>  
> -	TCPsocket sock = SDLNet_TCP_Accept(svrsock_);
> -	// No client wants to connect
> -	if (sock == nullptr)
> +	if (!socket.is_open()) {
> +		// No new connection
>  		return false;
> -	SDLNet_TCP_AddSocket(sockset_, sock);
> +	}
> +
> +	socket.non_blocking(true);
> +
>  	ConnectionId id = next_id_++;
>  	assert(id > 0);
>  	assert(clients_.count(id) == 0);
> -	clients_.insert(std::make_pair(id, Client(sock)));
> +	clients_.insert(std::make_pair(id, Client(std::move(socket))));
>  	assert(clients_.count(id) == 1);
>  	*new_id = id;
>  	return true;
>  }
>  
>  bool NetHost::try_receive(const ConnectionId id, RecvPacket* packet) {
> -
>  	// Always read all available data into buffers
>  	uint8_t buffer[512];
> -	while (SDLNet_CheckSockets(sockset_, 0) > 0) {
> -		for (auto& e : clients_) {
> -			if (SDLNet_SocketReady(e.second.socket)) {
> -				const int32_t bytes = SDLNet_TCP_Recv(e.second.socket, buffer, sizeof(buffer));
> -				if (bytes <= 0) {
> -					// Error while receiving
> -					close(e.first);
> -					// We have to run the for-loop again since we modified the map
> -					break;
> -				}
>  
> -				e.second.deserializer.read_data(buffer, bytes);
> -			}
> +	boost::system::error_code ec;
> +	for (auto it = clients_.begin(); it != clients_.end(); ) {
> +		size_t length = it->second.socket.read_some(boost::asio::buffer(buffer, 512), ec);

We have 512 multiple times in the code - make it a constexpr kNetworkBufferSize.

> +		if (ec == boost::asio::error::would_block) {
> +			// Nothing to read
> +			assert(length == 0);
> +			++it;
> +			continue;
> +		} else if (ec) {
> +			assert(length == 0);
> +			// Connection closed or some error, close the socket
> +			// close() will remove the client from the map so we have to increment the iterator first
> +			ConnectionId id_to_remove = it->first;
> +			++it;
> +			close(id_to_remove);
> +			continue;
>  		}
> +		assert(length > 0);
> +		assert(length <= 512);
> +		// Read something
> +		it->second.deserializer.read_data(buffer, length);
> +		++it;
>  	}
>  
>  	// Now check whether there is data for the requested client
>  	if (!is_connected(id))
>  		return false;
>  
> -	// Get one packet from the deserializer
> +	// Try to get one packet from the deserializer
>  	return clients_.at(id).deserializer.write_packet(packet);
>  }
>  
>  void NetHost::send(const ConnectionId id, const SendPacket& packet) {
> +	boost::system::error_code ec;
>  	if (is_connected(id)) {
> -		SDLNet_TCP_Send(clients_.at(id).socket, packet.get_data(), packet.get_size());
> -	}
> -}
> -
> -NetHost::NetHost(const uint16_t port) : svrsock_(nullptr), sockset_(nullptr), next_id_(1) {
> -
> -	IPaddress myaddr;
> -	SDLNet_ResolveHost(&myaddr, nullptr, port);
> -	svrsock_ = SDLNet_TCP_Open(&myaddr);
> -	// Maximal 16 sockets! This mean we can have at most 15 clients_ in our game (+ metaserver)
> -	sockset_ = SDLNet_AllocSocketSet(16);
> +		size_t written = boost::asio::write(clients_.at(id).socket,
> +											boost::asio::buffer(packet.get_data(), packet.get_size()), ec);
> +		// This one is an assertion of mine, I am not sure if it will hold
> +		// If it doesn't, set the socket to blocking before writing
> +		assert(ec != boost::asio::error::would_block);
> +		assert(written == packet.get_size() || ec);
> +		if (ec) {
> +			close(id);
> +		}
> +	}
> +}
> +
> +NetHost::NetHost(const uint16_t port)
> +	: clients_(), next_id_(1), io_service_(), acceptor_v4_(io_service_), acceptor_v6_(io_service_) {
> +
> +	if (open_acceptor(&acceptor_v4_, boost::asio::ip::tcp::endpoint(boost::asio::ip::tcp::v4(), port))) {
> +		log("[NetHost]: Opening a listening IPv4 socket on port %u\n", port);
> +	}
> +	if (open_acceptor(&acceptor_v6_, boost::asio::ip::tcp::endpoint(boost::asio::ip::tcp::v6(), port))) {
> +		log("[NetHost]: Opening a listening IPv6 socket on port %u\n", port);
> +	}
> +}
> +
> +bool NetHost::open_acceptor(boost::asio::ip::tcp::acceptor *acceptor,
> +						const boost::asio::ip::tcp::endpoint& endpoint) {
> +	try {
> +		acceptor->open(endpoint.protocol());
> +		acceptor->non_blocking(true);
> +		const boost::asio::socket_base::reuse_address option_reuse(true);
> +		acceptor->set_option(option_reuse);
> +		if (endpoint.protocol() == boost::asio::ip::tcp::v6()) {
> +			const boost::asio::ip::v6_only option_v6only(true);
> +			acceptor->set_option(option_v6only);
> +		}
> +		acceptor->bind(endpoint);
> +		acceptor->listen(boost::asio::socket_base::max_connections);
> +		return true;
> +	} catch (const boost::system::system_error&) {
> +		return false;
> +	}
>  }
> 
> === modified file 'src/network/network_lan_promotion.cc'
> --- src/network/network_lan_promotion.cc	2017-01-25 18:55:59 +0000
> +++ src/network/network_lan_promotion.cc	2017-05-20 19:06:48 +0000
> @@ -19,116 +19,293 @@
>  
>  #include "network/network_lan_promotion.h"
>  
> -#include <cstdio>
> -#include <cstring>
> +#ifndef _WIN32
> +#include <ifaddrs.h>
> +#endif
>  
> +#include "base/i18n.h"
>  #include "base/log.h"
> -#include "base/macros.h"
> +#include "base/warning.h"
>  #include "build_info.h"
>  #include "network/constants.h"
>  
> +namespace {
> +
> +	static const char *ip_versions[] = {"IPv4", "IPv6"};
> +
> +	/**
> +	 * Returns the matching string for the given IP address.
> +	 * \param addr The address object to get the IP version for.
> +	 * \return A pointer to a constant string naming the IP version.
> +	 */
> +	const char* get_ip_version_string(const boost::asio::ip::address& addr) {

This is used only in log output, so we could get rid of ip_versions[] and just return ints, then have "IPv%d" in the log output.

> +		assert(!addr.is_unspecified());
> +		if (addr.is_v4()) {
> +			return ip_versions[0];
> +		} else {
> +			assert(addr.is_v6());
> +			return ip_versions[1];
> +		}
> +	}
> +
> +	/**
> +	 * Returns the matching string for the given IP address.
> +	 * \param version A whatever object to get the IP version for.
> +	 * \return A pointer to a constant string naming the IP version.
> +	 */
> +	const char* get_ip_version_string(const boost::asio::ip::udp& version) {
> +		if (version == boost::asio::ip::udp::v4()) {
> +			return ip_versions[0];
> +		} else {
> +			assert(version == boost::asio::ip::udp::v6());
> +			return ip_versions[1];
> +		}
> +	}
> +}
> +
>  /*** class LanBase ***/
> -
> -LanBase::LanBase() {
> -
> -	sock = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP);  //  open the socket
> -
> -	int32_t opt = 1;
> -	//  the cast to char* is because microsoft wants it that way
> -	setsockopt(sock, SOL_SOCKET, SO_BROADCAST, reinterpret_cast<char*>(&opt), sizeof(opt));
> +LanBase::LanBase(uint16_t port)
> +	: io_service(), socket_v4(io_service), socket_v6(io_service) {
>  
>  #ifndef _WIN32
> -
> -	//  get a list of all local broadcast addresses
> -	struct if_nameindex* ifnames = if_nameindex();
> -	struct ifreq ifr;
> -
> -	for (int32_t i = 0; ifnames[i].if_index; ++i) {
> -		strncpy(ifr.ifr_name, ifnames[i].if_name, IFNAMSIZ);
> -
> -		DIAG_OFF("-Wold-style-cast")
> -		if (ioctl(sock, SIOCGIFFLAGS, &ifr) < 0)
> -			continue;
> -
> -		if (!(ifr.ifr_flags & IFF_BROADCAST))
> -			continue;
> -
> -		if (ioctl(sock, SIOCGIFBRDADDR, &ifr) < 0)
> -			continue;
> -		DIAG_ON("-Wold-style-cast")
> -
> -		broadcast_addresses.push_back(
> -		   reinterpret_cast<sockaddr_in*>(&ifr.ifr_broadaddr)->sin_addr.s_addr);
> -	}
> -
> -	if_freenameindex(ifnames);
> +	// Iterate over all interfaces. If they support IPv4, store the broadcast-address
> +	// of the interface and try to start the socket. If they support IPv6, just start
> +	// the socket. There is one fixed broadcast-address for IPv6 (well, actually multicast)
> +
> +	// Adapted example out of "man getifaddrs"
> +	// Admittedly: I don't like this part. But boost is not able to iterate over

Replace "Admittedly:" with "TODO(Notabilis):"

> +	// the local IPs at this time. If they ever add it, replace this code
> +	struct ifaddrs *ifaddr, *ifa;
> +	int family, s, n;
> +	char host[NI_MAXHOST];
> +	if (getifaddrs(&ifaddr) == -1) {
> +		perror("getifaddrs");
> +		exit(EXIT_FAILURE);
> +	}
> +	for (ifa = ifaddr, n = 0; ifa != nullptr; ifa = ifa->ifa_next, n++) {
> +		if (ifa->ifa_addr == nullptr)
> +			continue;
> +		family = ifa->ifa_addr->sa_family;
> +		if (family == AF_INET && (ifa->ifa_flags & IFF_BROADCAST)) {

How about:

if (ifa->ifa_flags & IFF_BROADCAST) {
    switch (family): {
        case AF_INET:
        ...
    }
}

> +			s = getnameinfo(ifa->ifa_ifu.ifu_broadaddr, sizeof(struct sockaddr_in),
> +						host, NI_MAXHOST, nullptr, 0, NI_NUMERICHOST);
> +			if (s == 0) {
> +				start_socket(&socket_v4, boost::asio::ip::udp::v4(), port);
> +				broadcast_addresses_v4.insert(host);
> +			}
> +		} else if (family == AF_INET6 && (ifa->ifa_flags & IFF_BROADCAST)) {
> +			start_socket(&socket_v6, boost::asio::ip::udp::v6(), port);
> +			// Nothing to insert here. There is only one "broadcast" address for IPv6 (I think)
> +		}
> +	}
> +	freeifaddrs(ifaddr);
>  #else
>  	//  As Microsoft does not seem to support if_nameindex, we just broadcast to
>  	//  INADDR_BROADCAST.
> -	broadcast_addresses.push_back(INADDR_BROADCAST);
> +	broadcast_addresses_v4.insert("255.255.255.255");
>  #endif
> +
> +	// Okay, needed this for development. But might be useful to debug network problems
> +	for (const std::string& ip : broadcast_addresses_v4)
> +		log("[LAN] Will broadcast to %s\n", ip.c_str());
> +
> +	if (!is_open()) {
> +		// Hm, not good. Just try to open them and hope for the best
> +		start_socket(&socket_v4, boost::asio::ip::udp::v4(), port);
> +		start_socket(&socket_v6, boost::asio::ip::udp::v6(), port);
> +	}
> +
> +	if (!is_open()) {
> +		// Still not open? Go back to main menu.
> +		report_network_error();
> +	}
>  }
>  
>  LanBase::~LanBase() {
> -	closesocket(sock);
> -}
> -
> -void LanBase::bind(uint16_t port) {
> -	sockaddr_in addr;
> -
> -	DIAG_OFF("-Wold-style-cast")
> -	addr.sin_family = AF_INET;
> -	addr.sin_addr.s_addr = INADDR_ANY;
> -	addr.sin_port = htons(port);
> -	DIAG_ON("-Wold-style-cast")
> -
> -	::bind(sock, reinterpret_cast<sockaddr*>(&addr), sizeof(addr));
> +	close_socket(&socket_v4);
> +	close_socket(&socket_v6);
>  }
>  
>  bool LanBase::avail() {

Rename to is_available?

> -	fd_set fds;
> -	timeval tv;
> -
> -	DIAG_OFF("-Wold-style-cast")
> -	FD_ZERO(&fds);
> -	FD_SET(sock, &fds);
> -	DIAG_ON("-Wold-style-cast")
> -
> -	tv.tv_sec = 0;
> -	tv.tv_usec = 0;
> -
> -	return select(sock + 1, &fds, nullptr, nullptr, &tv) == 1;
> -}
> -
> -ssize_t LanBase::receive(void* const buf, size_t const len, sockaddr_in* const addr) {
> -	socklen_t addrlen = sizeof(sockaddr_in);
> -	return recvfrom(
> -	   sock, static_cast<DATATYPE*>(buf), len, 0, reinterpret_cast<sockaddr*>(addr), &addrlen);
> -}
> -
> -void LanBase::send(void const* const buf, size_t const len, sockaddr_in const* const addr) {
> -	sendto(sock, static_cast<const DATATYPE*>(buf), len, 0, reinterpret_cast<const sockaddr*>(addr),
> -	       sizeof(sockaddr_in));
> -}
> -
> -void LanBase::broadcast(void const* const buf, size_t const len, uint16_t const port) {
> -	for (const in_addr_t& temp_address : broadcast_addresses) {
> -		sockaddr_in addr;
> -		addr.sin_family = AF_INET;
> -		addr.sin_addr.s_addr = temp_address;
> -		DIAG_OFF("-Wold-style-cast")
> -		addr.sin_port = htons(port);
> -		DIAG_ON("-Wold-style-cast")
> -
> -		sendto(sock, static_cast<const DATATYPE*>(buf), len, 0,
> -		       reinterpret_cast<const sockaddr*>(&addr), sizeof(addr));
> +	boost::system::error_code ec;
> +	bool available_v4 = (socket_v4.is_open() && socket_v4.available(ec) > 0);
> +	if (ec) {
> +		close_socket(&socket_v4);
> +		available_v4 = false;
> +	}
> +	bool available_v6 = (socket_v6.is_open() && socket_v6.available(ec) > 0);
> +	if (ec) {
> +		close_socket(&socket_v6);
> +		available_v4 = false;
> +	}
> +	return available_v4 || available_v6;
> +}
> +
> +bool LanBase::is_open() {
> +	return socket_v4.is_open() || socket_v6.is_open();
> +}
> +
> +ssize_t LanBase::receive(void* const buf, size_t const len, NetAddress *addr) {
> +	assert(buf != nullptr);
> +	assert(addr != nullptr);
> +	boost::asio::ip::udp::endpoint sender_endpoint;
> +	size_t recv_len;
> +	if (socket_v4.is_open()) {

Like above, is preference for IPV6 possible?

> +		try {
> +			if (socket_v4.available() > 0) {
> +				recv_len = socket_v4.receive_from(boost::asio::buffer(buf, len), sender_endpoint);
> +				*addr = NetAddress{sender_endpoint.address().to_string(), sender_endpoint.port()};
> +				assert(recv_len <= len);
> +				return recv_len;
> +			}
> +		} catch (const boost::system::system_error&) {
> +			// Some network error. Close the socket
> +			close_socket(&socket_v4);
> +		}
> +	}
> +	// We only reach this point if there was nothing to receive for IPv4
> +	if (socket_v6.is_open()) {
> +		try {
> +			if (socket_v6.available() > 0) {
> +				recv_len = socket_v6.receive_from(boost::asio::buffer(buf, len), sender_endpoint);
> +				*addr = NetAddress{sender_endpoint.address().to_string(), sender_endpoint.port()};
> +				assert(recv_len <= len);
> +				return recv_len;
> +			}
> +		} catch (const boost::system::system_error&) {
> +			close_socket(&socket_v6);
> +		}
> +	}
> +	// Nothing to receive at all. So lonely here...
> +	return 0;
> +}
> +
> +bool LanBase::send(void const* const buf, size_t const len, const NetAddress& addr) {
> +	boost::system::error_code ec;
> +	const boost::asio::ip::address address = boost::asio::ip::address::from_string(addr.ip, ec);
> +	// If this assert failed, then there is some bug in the code. NetAddress should only be filled
> +	// with valid IP addresses (e.g. no hostnames)
> +	assert(!ec);
> +	boost::asio::ip::udp::endpoint destination(address, addr.port);
> +	boost::asio::ip::udp::socket *socket = nullptr;
> +	if (destination.address().is_v4()) {
> +		socket = &socket_v4;
> +	} else if (destination.address().is_v6()) {
> +		socket = &socket_v6;
> +	} else {
> +		NEVER_HERE();
> +	}
> +	assert(socket != nullptr);
> +	if (!socket->is_open()) {
> +		// I think this shouldn't happen normally. It might happen, though, if we receive

Networks can be fickle things, so handling this is a good idea - leave it in :)

> +		// a broadcast and learn the IP, then our sockets goes down, then we try to send
> +		log("[LAN] Error: trying to send to an %s address but socket is not open",
> +			get_ip_version_string(address));
> +		return false;
> +	}
> +	socket->send_to(boost::asio::buffer(buf, len), destination, 0, ec);
> +	if (ec) {
> +		close_socket(socket);
> +		return false;
> +	}
> +	return true;
> +}
> +
> +bool LanBase::broadcast(void const* const buf, size_t const len, uint16_t const port) {
> +	boost::system::error_code ec;
> +	bool error = false;
> +	if (socket_v4.is_open()) {
> +		for (const std::string& address : broadcast_addresses_v4) {
> +			boost::asio::ip::udp::endpoint destination(boost::asio::ip::address::from_string(address), port);
> +			socket_v4.send_to(boost::asio::buffer(buf, len), destination, 0, ec);
> +			if (ec) {
> +				close_socket(&socket_v4);
> +				error = true;
> +				break;
> +			}
> +		}
> +	}
> +	if (socket_v6.is_open()) {
> +		boost::asio::ip::udp::endpoint destination(boost::asio::ip::address::from_string("ff02::1"), port);
> +		socket_v6.send_to(boost::asio::buffer(buf, len), destination, 0, ec);
> +		if (ec) {
> +			close_socket(&socket_v6);
> +				error = true;
> +		}
> +	}
> +	return !error;
> +}
> +
> +void LanBase::start_socket(boost::asio::ip::udp::socket *socket, boost::asio::ip::udp version, uint16_t port) {
> +
> +    if (socket->is_open())
> +		return;
> +
> +	boost::system::error_code ec;
> +	// Try to open the socket
> +	socket->open(version, ec);
> +	if (ec) {
> +		log("[LAN] Failed to start an %s socket: %s\n",
> +			get_ip_version_string(version), ec.message().c_str());
> +		return;
> +	}
> +
> +	const boost::asio::socket_base::broadcast option_broadcast(true);
> +	socket->set_option(option_broadcast, ec);
> +	if (ec) {
> +		log("[LAN] Error setting options for %s socket, closing socket: %s\n",
> +			get_ip_version_string(version), ec.message().c_str());
> +		// Retrieve the error code to avoid throwing but ignore it
> +		close_socket(socket);
> +		return;
> +	}
> +
> +	const boost::asio::socket_base::reuse_address option_reuse(true);
> +	socket->set_option(option_reuse, ec);
> +	// This one isn't really needed so ignore the error
> +
> +
> +	if (version == boost::asio::ip::udp::v6()) {
> +		const boost::asio::ip::v6_only option_v6only(true);
> +		socket->set_option(option_v6only, ec);
> +		// This one might not be needed, ignore the error and see whether we fail on bind()
> +	}
> +
> +	socket->bind(boost::asio::ip::udp::endpoint(version, port), ec);
> +	if (ec) {
> +		log("[LAN] Error binding %s socket to UDP port %d, closing socket: %s\n",
> +			get_ip_version_string(version), port, ec.message().c_str());
> +		close_socket(socket);
> +		return;
> +	}
> +
> +	log("[LAN] Started an %s socket on UDP port %d\n", get_ip_version_string(version), port);
> +}
> +
> +void LanBase::report_network_error() {
> +	// No socket open? Sorry, but we can't continue this way
> +	throw WLWarning(_("Failed to use the local network!"),
> +		_("Widelands was unable to use the local network. "
> +		  "Maybe some other process is already running a server on port %d, %d or %d "
> +		  "or your network setup is broken."),
> +		WIDELANDS_LAN_DISCOVERY_PORT, WIDELANDS_LAN_PROMOTION_PORT, WIDELANDS_PORT);
> +}
> +
> +void LanBase::close_socket(boost::asio::ip::udp::socket *socket) {
> +	boost::system::error_code ec;
> +	if (socket->is_open()) {
> +		const boost::asio::ip::udp::endpoint& endpoint = socket->local_endpoint(ec);
> +		if (!ec)
> +			log("[LAN] Closing an %s socket.\n", get_ip_version_string(endpoint.protocol()));
> +		socket->shutdown(boost::asio::ip::udp::socket::shutdown_both, ec);
> +		socket->close(ec);
>  	}
>  }
>  
>  /*** class LanGamePromoter ***/
>  
> -LanGamePromoter::LanGamePromoter() {
> -	bind(WIDELANDS_LAN_PROMOTION_PORT);
> +LanGamePromoter::LanGamePromoter()
> +	: LanBase(WIDELANDS_LAN_PROMOTION_PORT) {
>  
>  	needupdate = true;
>  
> @@ -153,20 +331,23 @@
>  	if (needupdate) {
>  		needupdate = false;
>  
> -		broadcast(&gameinfo, sizeof(gameinfo), WIDELANDS_LAN_DISCOVERY_PORT);
> +		if (!broadcast(&gameinfo, sizeof(gameinfo), WIDELANDS_LAN_DISCOVERY_PORT))

This is hard to read, please add {}

> +			report_network_error();
>  	}
>  
>  	while (avail()) {
>  		char magic[8];
> -		sockaddr_in addr;
> +		NetAddress addr;
>  
>  		if (receive(magic, 8, &addr) < 8)
>  			continue;
>  
> -		log("Received %s packet\n", magic);
> +		log("Received %s packet from %s\n", magic, addr.ip.c_str());
>  
> -		if (!strncmp(magic, "QUERY", 6) && magic[6] == LAN_PROMOTION_PROTOCOL_VERSION)
> -			send(&gameinfo, sizeof(gameinfo), &addr);
> +		if (!strncmp(magic, "QUERY", 6) && magic[6] == LAN_PROMOTION_PROTOCOL_VERSION) {
> +			if (!send(&gameinfo, sizeof(gameinfo), addr))

Add {}

> +				report_network_error();
> +		}
>  	}
>  }
>  
> 
> === modified file 'src/wlapplication.cc'
> --- src/wlapplication.cc	2017-05-13 13:14:29 +0000
> +++ src/wlapplication.cc	2017-05-20 19:06:48 +0000
> @@ -1176,18 +1167,16 @@
>  				break;
>  			}
>  			case FullscreenMenuBase::MenuTarget::kJoingame: {
> -				if (!host_address)
> -					throw WLWarning(
> -					   "Invalid Address", "%s", "The address of the game server is invalid");
> +				NetAddress addr;
> +				if (!ns.get_host_address(&addr)) {
> +					UI::WLMessageBox mmb(&ns, _("Invalid address"),
> +								_("The entered hostname or address is invalid and can't be connected to."),

We use typography wit translatable messages: can't => can’t

> +								UI::WLMessageBox::MBoxType::kOk);
> +					mmb.run<UI::Panel::Returncodes>();
> +					break;
> +				}
>  
> -				// TODO(Notabilis): Make this prettier. I am aware that this is quite ugly but it should
> -				// work
> -				// for now and will be removed shortly when we switch to boost.asio
> -				char ip_str[] = {"255.255.255.255"};
> -				sprintf(ip_str, "%d.%d.%d.%d", (addr & 0x000000ff), (addr & 0x0000ff00) >> 8,
> -				        (addr & 0x00ff0000) >> 16, (addr & 0xff000000) >> 24);
> -				port = (port >> 8) | ((port & 0xFF) << 8);
> -				GameClient netgame(ip_str, port, playername);
> +				GameClient netgame(addr, playername);
>  				netgame.run();
>  				break;
>  			}


-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/net-boost-asio into lp:widelands.