widelands-dev team mailing list archive

[Klaus Halfmann <klaus.halfmann@xxxxxxxx>]

Review: Approve review, comoiel

Did a review now,
 * there is quite some duplicate code for 4/6 that should be avoided
 * Think about a common errro handling, that will save a lot of code, like
   if (hasError(ec)) { // implies logging

check the comments inline.

we still could get his in and improved this later.

Got some "visitors" from Libre Game Night, so my review is imcomplete sorry.

Can approve, and mabye fix the Issues later

Diff comments:

> 
> === modified file 'src/network/netclient.cc'
> --- src/network/netclient.cc	2017-05-11 10:45:44 +0000
> +++ src/network/netclient.cc	2017-05-26 20:56:15 +0000
> @@ -17,20 +18,25 @@
>  NetClient::~NetClient() {
>  	if (is_connected())
>  		close();
> -	if (sockset_ != nullptr)
> -		SDLNet_FreeSocketSet(sockset_);
>  }
>  
>  bool NetClient::is_connected() const {
> -	return sock_ != nullptr;
> +	return socket_.is_open();
>  }
>  
>  void NetClient::close() {
>  	if (!is_connected())
>  		return;
> -	SDLNet_TCP_DelSocket(sockset_, sock_);
> -	SDLNet_TCP_Close(sock_);
> -	sock_ = nullptr;
> +	boost::system::error_code ec;

Why resolve that remote_endpoint just for some logging?.
May result in extra errors only, we could just log the NetAddress?

> +	boost::asio::ip::tcp::endpoint remote = socket_.remote_endpoint(ec);
> +	if (!ec) {
> +		log("[NetClient] Closing network socket connected to %s:%i.\n",
> +			remote.address().to_string().c_str(), remote.port());
> +	} else {
> +		log("[NetClient] Closing network socket.\n");
> +	}
> +	socket_.shutdown(boost::asio::ip::tcp::socket::shutdown_both, ec);
> +	socket_.close(ec);
>  }
>  
>  bool NetClient::try_receive(RecvPacket* packet) {
> @@ -38,42 +44,56 @@
>  		return false;
>  
>  	uint8_t buffer[512];
> -	while (SDLNet_CheckSockets(sockset_, 0) > 0) {
> -
> -		const int32_t bytes = SDLNet_TCP_Recv(sock_, buffer, sizeof(buffer));
> -		if (bytes <= 0) {
> -			// Error while receiving
> -			close();
> -			return false;
> -		}
> -
> -		deserializer_.read_data(buffer, bytes);
> +	boost::system::error_code ec;
> +	size_t length = socket_.read_some(boost::asio::buffer(buffer, 512), ec);
> +	if (!ec) {
> +		assert(length > 0);
> +		assert(length <= 512);
> +		// Has read something
> +		deserializer_.read_data(buffer, length);
> +	}
> +
> +	if (ec && ec != boost::asio::error::would_block) {
> +		// Connection closed or some error, close the socket
> +		log("[NetClient] Error when trying to receive some data: %s.\n", ec.message().c_str());
> +		close();
> +		return false;
>  	}
>  	// Get one packet from the deserializer
>  	return deserializer_.write_packet(packet);
>  }
>  
>  void NetClient::send(const SendPacket& packet) {
> -	if (is_connected()) {
> -		SDLNet_TCP_Send(sock_, packet.get_data(), packet.get_size());
> -	}
> -}
> -
> -NetClient::NetClient(const std::string& ip_address, const uint16_t port)
> -   : sock_(nullptr), sockset_(nullptr), deserializer_() {
> -
> -	IPaddress addr;
> -	if (SDLNet_ResolveHost(&addr, ip_address.c_str(), port) != 0) {
> -		log("[Client]: Failed to resolve host address %s:%u.\n", ip_address.c_str(), port);
> +	if (!is_connected())
>  		return;
> +
> +	boost::system::error_code ec;
> +	size_t written = boost::asio::write(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

Make this a TODO with your name, please

> +	assert(ec != boost::asio::error::would_block);
> +	assert(written == packet.get_size() || ec);
> +	if (ec) {
> +		log("[NetClient] Error when trying to send some data: %s.\n", ec.message().c_str());
> +		close();
>  	}
> -	log("[Client]: Trying to connect to %s:%u ... ", ip_address.c_str(), port);
> -	sock_ = SDLNet_TCP_Open(&addr);
> -	if (is_connected()) {
> -		log("success\n");
> -		sockset_ = SDLNet_AllocSocketSet(1);
> -		SDLNet_TCP_AddSocket(sockset_, sock_);
> +}
> +
> +NetClient::NetClient(const NetAddress& host)
> +   : io_service_(), socket_(io_service_), deserializer_() {
> +
> +	boost::system::error_code ec;
> +	const boost::asio::ip::address address = boost::asio::ip::address::from_string(host.ip, ec);
> +	assert(!ec);
> +	const boost::asio::ip::tcp::endpoint destination(address, host.port);
> +
> +	log("[NetClient]: Trying to connect to %s:%u ... ", host.ip.c_str(), host.port);
> +	socket_.connect(destination, ec);
> +	if (!ec && is_connected()) {
> +		log("success.\n");
> +		socket_.non_blocking(true);
>  	} else {
> -		log("failed\n");
> +		log("failed.\n");
>  	}
>  }
> 
> === modified file 'src/network/nethost.cc'
> --- src/network/nethost.cc	2017-05-11 10:45:44 +0000
> +++ src/network/nethost.cc	2017-05-26 20:56:15 +0000
> @@ -47,68 +53,150 @@
>  		// Not connected anyway
>  		return;
>  	}
> -	SDLNet_TCP_DelSocket(sockset_, iter_client->second.socket);
> -	SDLNet_TCP_Close(iter_client->second.socket);
> +	boost::system::error_code ec;
> +	boost::asio::ip::tcp::endpoint remote = iter_client->second.socket.remote_endpoint(ec);
> +	if (!ec) {
> +		log("[NetHost] Closing network connection to %s:%i.\n",
> +			remote.address().to_string().c_str(), remote.port());
> +	} else {
> +		log("[NetHost] Closing network connection to some client.\n");
> +	}
> +	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_);

As we return this, would socket* not avoid the late std:move() ?

> +	if (acceptor_v4_.is_open()) {
> +		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
> +			log("[NetHost] No longer listening for IPv4 connections due to error: %s.\n",
> +				ec.message().c_str());
> +			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

Duplicate code, use common code for both acceptors

> +		acceptor_v6_.accept(socket, ec);
> +		if (ec == boost::asio::error::would_block) {
> +			// No client wants to connect
> +		} else if (ec) {
> +			log("[NetHost] No longer listening for IPv6 connections due to error: %s.\n",
> +				ec.message().c_str());
> +			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;
> -				}
> +	uint8_t buffer[kNetworkBufferSize];
>  
> -				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, kNetworkBufferSize), ec);
> +		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;
> +			log("[NetHost] Error when receiving from a client, closing connection: %s.\n",
> +				ec.message().c_str());
> +			close(id_to_remove);
> +			continue;
>  		}
> +		assert(length > 0);
> +		assert(length <= kNetworkBufferSize);
> +		// 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

Use TODO(yourname), please

> +		assert(ec != boost::asio::error::would_block);
> +		assert(written == packet.get_size() || ec);
> +		if (ec) {
> +			log("[NetHost] Error when sending to a client, closing connection: %s.\n", ec.message().c_str());
> +			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 TCP 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 TCP 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);

Why is this needed? can an IPV6 listening soket accept an IPv4 socket, strange.

> +		}
> +		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/nethost.h'
> --- src/network/nethost.h	2017-05-11 10:45:44 +0000
> +++ src/network/nethost.h	2017-05-26 20:56:15 +0000
> @@ -94,26 +96,49 @@
>  	/**
>  	 * Sends a packet.
>  	 * Calling this on a closed connection will silently fail.
> -	 * @param id The connection id of the client that should be sent to.
> -	 * @param packet The packet to send.
> +	 * \param id The connection id of the client that should be sent to.
> +	 * \param packet The packet to send.
>  	 */
>  	void send(ConnectionId id, const SendPacket& packet);
>  
>  private:
> +	/**
> +	 * Tries to listen on the given port.
> +	 * If it fails, is_listening() will return \c false.
> +	 * \param port The port to listen on.
> +	 */
>  	NetHost(const uint16_t port);
>  
> -	class Client {
> -	public:
> -		Client(TCPsocket sock);
> -
> -		TCPsocket socket;
> +	bool open_acceptor(boost::asio::ip::tcp::acceptor *acceptor,
> +						const boost::asio::ip::tcp::endpoint& endpoint);
> +
> +	/**
> +	 * Helper structure to store variables about a connected client.
> +	 */
> +	struct Client {
> +		/**
> +		 * Initializes the structure with the given socket.
> +		 * \param sock The socket to listen on. The socket is moved by this
> +		 *             constructor so the given socket is no longer valid.
> +		 */
> +		Client(boost::asio::ip::tcp::socket&& sock);
> +
> +		/// The socket to send/receive with.
> +		boost::asio::ip::tcp::socket socket;
> +		/// The deserializer to feed the received data to. It will transform it into data packets.
>  		Deserializer deserializer;
>  	};
>  
> -	TCPsocket svrsock_;
> -	SDLNet_SocketSet sockset_;
> +	/// A map of connected clients.

Please dsecribe what this map is ued for as the comment otherwise describes something obvious only.

>  	std::map<NetHost::ConnectionId, Client> clients_;
> +	/// The next client id that will be used
>  	NetHost::ConnectionId next_id_;
> +	/// An io_service needed by boost.asio. Primary needed for async operations.
> +	boost::asio::io_service io_service_;
> +	/// The acceptor we get IPv4 connection requests to.
> +	boost::asio::ip::tcp::acceptor acceptor_v4_;
> +	/// The acceptor we get IPv6 connection requests to.
> +	boost::asio::ip::tcp::acceptor acceptor_v6_;
>  };
>  
>  #endif  // end of include guard: WL_NETWORK_NETHOST_H
> 
> === modified file 'src/network/network.cc'
> --- src/network/network.cc	2017-05-14 14:40:24 +0000
> +++ src/network/network.cc	2017-05-26 20:56:15 +0000
> @@ -19,8 +19,51 @@
>  
>  #include "network/network.h"
>  
> +#include <SDL.h>
> +#include <boost/asio.hpp>
> +
>  #include "base/log.h"
>  
> +
> +namespace {
> +
> +bool do_resolve(const boost::asio::ip::tcp& protocol, NetAddress *addr, const std::string& hostname, uint16_t port) {
> +	assert(addr != nullptr);
> +	try {
> +		boost::asio::io_service io_service;
> +		boost::asio::ip::tcp::resolver resolver(io_service);
> +		boost::asio::ip::tcp::resolver::query query(protocol, hostname, boost::lexical_cast<std::string>(port));
> +		boost::asio::ip::tcp::resolver::iterator iter = resolver.resolve(query);
> +		if (iter == boost::asio::ip::tcp::resolver::iterator()) {
> +			// Resolution failed
> +			return false;
> +		}
> +		addr->ip = iter->endpoint().address().to_string();
> +		addr->port = port;
> +		return true;
> +	} catch (const boost::system::system_error&) {
> +		// Resolution failed

We should have some logging here

> +		return false;
> +	}
> +}
> +
> +}
> +
> +bool NetAddress::resolve_to_v4(NetAddress *addr, const std::string& hostname, uint16_t port) {
> +	return do_resolve(boost::asio::ip::tcp::v4(), addr, hostname, port);
> +}
> +
> +bool NetAddress::resolve_to_v6(NetAddress *addr, const std::string& hostname, uint16_t port) {
> +	return do_resolve(boost::asio::ip::tcp::v6(), addr, hostname, port);
> +}
> +
> +bool NetAddress::is_ipv6() const {
> +	// Don't catch errors. If the stored address is no valid IP address,
> +	// we have a coding error somewhere
> +	boost::asio::ip::address addr = boost::asio::ip::address::from_string(ip);
> +	return addr.is_v6();
> +}
> +
>  CmdNetCheckSync::CmdNetCheckSync(uint32_t const dt, SyncCallback* const cb)
>     : Command(dt), callback_(cb) {
>  }
> 
> === modified file 'src/network/network.h'
> --- src/network/network.h	2017-05-23 21:07:47 +0000
> +++ src/network/network.h	2017-05-26 20:56:15 +0000
> @@ -36,6 +35,42 @@
>  class Deserializer;
>  class FileRead;
>  
> +/**
> + * Simple structure to hold the IP address and port of a server.
> + * This structure must not contain a hostname but only IP addresses.
> + */
> +struct NetAddress {
> +	/**
> +	 * Tries to resolve the given hostname to an IPv4 address.
> +	 * \param[out] addr An NetAddress structure to write the result to,
> +	 *                  if resolution succeeds.
> +	 * \param hostname The name of the host.
> +	 * \param port The port on the host.
> +	 * \return \c True if the resolution succeeded, \c false otherwise.
> +	 */
> +	static bool resolve_to_v4(NetAddress *addr, const std::string& hostname, uint16_t port);
> +
> +	/**
> +	 * Tries to resolve the given hostname to an IPv6 address.
> +	 * \param[out] addr An NetAddress structure to write the result to,
> +	 *                  if resolution succeeds.
> +	 * \param hostname The name of the host.
> +	 * \param port The port on the host.
> +	 * \return \c True if the resolution succeeded, \c false otherwise.
> +	 */
> +	static bool resolve_to_v6(NetAddress *addr, const std::string& hostname, uint16_t port);
> +
> +	/**
> +	 * Returns whether the stored IP is in IPv6 format.
> +	 * @return \c true if the stored IP is in IPv6 format, \c false otherwise.
> +	 *   If it isn't an IPv6 address, it is an IPv4 address.
> +	 */
> +	bool is_ipv6() const;

This should be cached as this class may not polymorph between IP versions.
Setting this in resolve_to_ should be the way

> +
> +	std::string ip;

Could this not be just ip::address? this would avoid back and forth conversions?

> +	uint16_t port;
> +};
> +
>  struct SyncCallback {
>  	virtual ~SyncCallback() {
>  	}
> 
> === 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-26 20:56:15 +0000
> @@ -19,116 +19,316 @@
>  
>  #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 {
> +
> +	/**
> +	 * Returns the IP version.
> +	 * \param addr The address object to get the IP version for.
> +	 * \return Either 4 or 6, depending on the version of the given address.
> +	 */
> +	int get_ip_version(const boost::asio::ip::address& addr) {
> +		assert(!addr.is_unspecified());
> +		if (addr.is_v4()) {
> +			return 4;
> +		} else {
> +			assert(addr.is_v6());
> +			return 6;
> +		}
> +	}
> +
> +	/**
> +	 * Returns the IP version.
> +	 * \param version A whatever object to get the IP version for.
> +	 * \return Either 4 or 6, depending on the version of the given address.
> +	 */
> +	int get_ip_version(const boost::asio::ip::udp& version) {
> +		if (version == boost::asio::ip::udp::v4()) {
> +			return 4;
> +		} else {
> +			assert(version == boost::asio::ip::udp::v6());
> +			return 6;
> +		}
> +	}
> +}
> +
>  /*** 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"
> +	// TODO(Notabilis): I don't like this part. But boost is not able to iterate over
> +	// the local IPs at this time. If they ever add it, replace this code
> +	struct ifaddrs *ifaddr, *ifa;
> +	int 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;
> +		if (!(ifa->ifa_flags & IFF_BROADCAST))
> +			continue;
> +		switch (ifa->ifa_addr->sa_family) {
> +			case AF_INET:
> +				s = getnameinfo(ifa->ifa_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);
> +				}
> +				break;
> +			case AF_INET6:
> +				start_socket(&socket_v6, boost::asio::ip::udp::v6(), port);
> +				// Nothing to insert here. As far as I know, there is only one "broadcast" address for IPv6
> +				break;
> +		}
> +	}
> +	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
> +
> +	if (!is_open()) {
> +		// Hm, not good. Just try to open them and hope for the best
> +		log("[LAN] Trying to open both sockets.\n");
> +		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.
> +		log("[LAN] Error: No sockets could be opened.\n");
> +		report_network_error();
> +	}
> +
> +	for (const std::string& ip : broadcast_addresses_v4)
> +		log("[LAN] Will broadcast to %s.\n", ip.c_str());
> +	if (socket_v6.is_open())
> +		log("[LAN] Will broadcast for IPv6.\n");
>  }
>  
>  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));
> -}
> -
> -bool LanBase::avail() {
> -	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));
> +	close_socket(&socket_v4);
> +	close_socket(&socket_v6);
> +}
> +
> +bool LanBase::is_available() {
> +	boost::system::error_code ec;
> +	bool available_v4 = (socket_v4.is_open() && socket_v4.available(ec) > 0);
> +	if (ec) {
> +		log("[LAN] Error when checking whether data is available on IPv4 socket, closing it: %s.\n",
> +			ec.message().c_str());
> +		close_socket(&socket_v4);
> +		available_v4 = false;
> +	}
> +	bool available_v6 = (socket_v6.is_open() && socket_v6.available(ec) > 0);

Duplicate code, please avoid

> +	if (ec) {
> +		log("[LAN] Error when checking whether data is available on IPv6 socket, closing it: %s.\n",
> +			ec.message().c_str());
> +		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()) {
> +		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& ec) {
> +			// Some network error. Close the socket
> +			log("[LAN] Error when receiving data on IPv4 socket, closing it: %s.\n", ec.what());
> +			close_socket(&socket_v4);
> +		}
> +	}
> +	// We only reach this point if there was nothing to receive for IPv4
> +	if (socket_v6.is_open()) {

Duplciate code, please avoid

> +		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& ec) {
> +			log("[LAN] Error when receiving data on IPv6 socket, closing it: %s.\n", ec.what());
> +			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
> +		// a broadcast and learn the IP, then our sockets goes down, then we try to send
> +		log("[LAN] Error: trying to send to an IPv%d address but socket is not open.\n",
> +			get_ip_version(address));
> +		return false;
> +	}
> +	socket->send_to(boost::asio::buffer(buf, len), destination, 0, ec);
> +	if (ec) {
> +		log("[LAN] Error when trying to send something over IPv%d, closing socket: %s.\n",
> +			get_ip_version(address), ec.message().c_str());
> +		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_v4 = 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) {
> +				log("[LAN] Error when broadcasting on IPv4 socket to %s, closing it: %s.\n",
> +					address.c_str(), ec.message().c_str());
> +				close_socket(&socket_v4);
> +				error_v4 = true;
> +				break;
> +			}
> +		}
> +	}
> +	if (socket_v6.is_open()) {
> +		boost::asio::ip::address addr(boost::asio::ip::address::from_string("ff02::1", ec));
> +		assert(!ec);
> +		boost::asio::ip::udp::endpoint destination(addr, port);
> +		socket_v6.send_to(boost::asio::buffer(buf, len), destination, 0, ec);
> +		if (ec) {
> +			log("[LAN] Error when broadcasting on IPv6 socket, closing it: %s.\n", ec.message().c_str());
> +			close_socket(&socket_v6);
> +			if (error_v4) {
> +				return false;
> +			}
> +		}
> +		// At least one of them succeeded
> +		return true;
> +	}
> +	// IPv6 did not run, return whether IPv4 succeeded
> +	return !error_v4;
> +}
> +
> +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 IPv%d socket: %s.\n",
> +			get_ip_version(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 IPv%d socket, closing socket: %s.\n",
> +			get_ip_version(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 IPv%d socket to UDP port %d, closing socket: %s.\n",
> +			get_ip_version(version), port, ec.message().c_str());
> +		close_socket(socket);
> +		return;
> +	}
> +
> +	log("[LAN] Started an IPv%d socket on UDP port %d.\n", get_ip_version(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 IPv%d socket.\n", get_ip_version(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;
>  


-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-boost-asio.