widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

I had another look at the code and added 9 small code style/English language nits. Fix as you see fit.

Diff comments:

> 
> === modified file 'src/network/netclient.cc'
> --- src/network/netclient.cc	2017-05-11 10:45:44 +0000
> +++ src/network/netclient.cc	2017-06-04 16:22:50 +0000
> @@ -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);

constexpr size_t kNetworkBufferSize = 512;

We have this in nethost too, maybe it can be moved to a header file to avoid duplication.

> +	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);
> +	// TODO(Notabilis): 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
> +	// If it does, remove this comment after build 20
> +	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_() {
> +
> +	assert(host.is_valid());
> +	const boost::asio::ip::tcp::endpoint destination(host.ip, host.port);
> +
> +	log("[NetClient]: Trying to connect to %s:%u ... ", host.ip.to_string().c_str(), host.port);
> +	boost::system::error_code ec;
> +	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/netclient.h'
> --- src/network/netclient.h	2017-05-11 10:45:44 +0000
> +++ src/network/netclient.h	2017-06-04 16:22:50 +0000
> @@ -22,23 +22,23 @@
>  
>  #include <memory>
>  
> -#include <SDL_net.h>
> -
>  #include "network/network.h"
>  
>  /**
>   * NetClient manages the network connection for a network game in which this computer
>   * participates as a client.
> + * This class only tries to create a single socket, either for IPv4 and IPv6.
> + * Which is used depends on what kind of address is given on call to connect().
>   */
>  class NetClient {
>  public:
> +
>  	/**
>  	 * Tries to establish a connection to the given host.
> -	 * @param ip_address A hostname or an IPv4 address as string.
> -	 * @param port The port to connect to.
> -	 * @return A pointer to a connected \c NetClient object or a nullptr if the connection failed.
> +	 * \param host The host to connect to.
> +	 * \return A pointer to a connected \c NetClient object or an invalid pointer if the connection failed.

Can we make this nullptr instead of an invalid pointer? There's a difference... nullptr is well-defined, an invalid pointer is not.

>  	 */
> -	static std::unique_ptr<NetClient> connect(const std::string& ip_address, const uint16_t port);
> +	static std::unique_ptr<NetClient> connect(const NetAddress& host);
>  
>  	/**
>  	 * Closes the connection.
> @@ -60,30 +60,35 @@
>  
>  	/**
>  	 * Tries to receive a packet.
> -	 * @param packet A packet that should be overwritten with the received data.
> -	 * @return \c true if a packet is available, \c false otherwise.
> +	 * \param packet A packet that should be overwritten with the received data.
> +	 * \return \c true if a packet is available, \c false otherwise.
>  	 *   The given packet is only modified when \c true is returned.
>  	 *   Calling this on a closed connection will return false.
>  	 */
> -	bool try_receive(RecvPacket* packet);
> +	bool try_receive(RecvPacket *packet);
>  
>  	/**
>  	 * Sends a packet.
>  	 * Calling this on a closed connection will silently fail.
> -	 * @param packet The packet to send.
> +	 * \param packet The packet to send.
>  	 */
> -	void send(const SendPacket& packet);
> +	 void send(const SendPacket& packet);
>  
>  private:
> -	NetClient(const std::string& ip_address, const uint16_t port);
> -
> -	/// The socket that connects us to the host
> -	TCPsocket sock_;
> -
> -	/// Socket set used for selection
> -	SDLNet_SocketSet sockset_;
> -
> -	/// Deserializer acts as a buffer for packets (reassembly/splitting up)
> +	/**
> +	 * Tries to establish a connection to the given host.
> +	 * If the connection attempt failed, is_connected() will return \c false.
> +	 * \param host The host to connect to.
> +	 */
> +	NetClient(const NetAddress& host);
> +
> +	/// An io_service needed by boost.asio. Primary needed for asynchronous operations.

Primary -> Primarily

> +	boost::asio::io_service io_service_;
> +
> +	/// The socket that connects us to the host.
> +	boost::asio::ip::tcp::socket socket_;
> +
> +	/// Deserializer acts as a buffer for packets (splitting stream to packets)
>  	Deserializer deserializer_;
>  };
>  
> 
> === modified file 'src/network/nethost.cc'
> --- src/network/nethost.cc	2017-05-11 10:45:44 +0000
> +++ src/network/nethost.cc	2017-06-04 16:22:50 +0000
> @@ -47,68 +72,144 @@
>  		// 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;
> -
> -	TCPsocket sock = SDLNet_TCP_Accept(svrsock_);
> -	// No client wants to connect
> -	if (sock == nullptr)
> +	boost::asio::ip::tcp::socket socket(io_service_);
> +
> +	const auto do_try_accept = [&socket](boost::asio::ip::tcp::acceptor& acceptor) {
> +		boost::system::error_code ec;
> +		if (acceptor.is_open()) {
> +			acceptor.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

don't -> doesn't

> +			} else if (ec) {
> +				// Some other error, close the acceptor
> +				log("[NetHost] No longer listening for IPv%d connections due to error: %s.\n",
> +					get_ip_version(acceptor), ec.message().c_str());
> +				acceptor.close(ec);
> +			} else {
> +				log("[NetHost]: Accepting IPv%d connection from %s.\n",
> +					get_ip_version(acceptor), socket.remote_endpoint().address().to_string().c_str());
> +			}
> +		}
> +	};
> +
> +	do_try_accept(acceptor_v4_);
> +	if (!socket.is_open())
> +		do_try_accept(acceptor_v6_);
> +
> +	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(); ) {

for (auto& client: clients_) {

Then you can get rid of the ++it statements too.

> +		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);

Why not use it->first directly? Then we can get rid of the variable.

> +			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);
> +		// TODO(Notabilis): 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
> +		// If it does, remove this comment after build 20
> +		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);
> +		}
> +		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.h'
> --- src/network/network.h	2017-05-23 21:07:47 +0000
> +++ src/network/network.h	2017-06-04 16:22:50 +0000
> @@ -36,6 +36,58 @@
>  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,

An NetAddress -> A NetAddress

For the functions below too ;)

> +	 *                  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);
> +
> +	/**
> +	 * Parses the given string to an IP address.
> +	 * \param[out] addr An NetAddress structure to write the result to,
> +	 *                  if parsing succeeds.
> +	 * \param ip An IP address as string.
> +	 * \param port The port on the host.
> +	 * \return \c True if the parsing succeeded, \c false otherwise.
> +	 */
> +	static bool parse_ip(NetAddress *addr, const std::string& ip, 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;
> +
> +	/**
> +	 * Returns whether valid IP address and port are stored.
> +	 * @return \c true if valid, \c false otherwise.
> +	 */
> +	bool is_valid() const;
> +
> +	boost::asio::ip::address ip;
> +	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-06-04 16:22:50 +0000
> @@ -19,116 +19,349 @@
>  
>  #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));
> +/**
> + * \internal
> + * In an ideal world, we would use the same code with boost asio for all three operating systems.
> + * Unfortunately, it isn't that easy and we need some platform specific code.
> + * For IPv4, windows needs a special case: For Linux and Apple we have to iterate over all assigned IPv4
> + * addresses (e.g. 192.168.1.68), transform them to broadcast addresses (e.g. 192.168.1.255) and send our
> + * packets to those addresses. For windows, we simply can sent to 255.255.255.255.

can sent -> can send

> + * For IPv6, Apple requires special handling. On the other two operating systems we can send to the multicast
> + * address ff02::1 (kind of a local broadcast) without specifying over which interface we want to send.
> + * On Apple we have to specify the interface, forcing us to send our message over all interfaces we can find.
> + */
> +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 and interfaces 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) && !(ifa->ifa_flags & IFF_MULTICAST))
> +			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:
> +#ifdef __APPLE__
> +				interface_indices_v6.insert(if_nametoindex(ifa->ifa_name));
> +#endif
> +				start_socket(&socket_v6, boost::asio::ip::udp::v6(), port);
> +				// No address to store here. 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() {
> +	const auto do_is_available = [this](boost::asio::ip::udp::socket& socket) -> bool {
> +		boost::system::error_code ec;
> +		bool available = (socket.is_open() && socket.available(ec) > 0);
> +		if (ec) {
> +			log("[LAN] Error when checking whether data is available on IPv%d socket, closing it: %s.\n",
> +				get_ip_version(socket.local_endpoint().protocol()), ec.message().c_str());
> +			close_socket(&socket);
> +			return false;
> +		}
> +		return available;
> +	};
> +
> +	return do_is_available(socket_v4) || do_is_available(socket_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);
> +	size_t recv_len = 0;
> +
> +	const auto do_receive
> +		= [this, &buf, &len, &recv_len, addr](boost::asio::ip::udp::socket& socket) -> bool {
> +		if (socket.is_open()) {
> +			try {
> +				if (socket.available() > 0) {
> +					boost::asio::ip::udp::endpoint sender_endpoint;
> +					recv_len = socket.receive_from(boost::asio::buffer(buf, len), sender_endpoint);
> +					*addr = NetAddress{sender_endpoint.address(), sender_endpoint.port()};
> +					assert(recv_len <= len);
> +					return true;
> +				}
> +			} catch (const boost::system::system_error& ec) {
> +				// Some network error. Close the socket
> +				log("[LAN] Error when receiving data on IPv%d socket, closing it: %s.\n",
> +						get_ip_version(socket.local_endpoint().protocol()), ec.what());
> +				close_socket(&socket);
> +			}
> +		}
> +		// Nothing received
> +		return false;
> +	};
> +
> +	// Try to receive something somewhere
> +	if (!do_receive(socket_v4))
> +		do_receive(socket_v6);
> +
> +	// Return how much has been received, might be 0
> +	return recv_len;
> +}
> +
> +bool LanBase::send(void const* const buf, size_t const len, const NetAddress& addr) {
> +	boost::system::error_code ec;
> +	assert(addr.is_valid());
> +	// 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(addr.ip, 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(addr.ip));
> +		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(addr.ip), 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) {
> +
> +	const auto do_broadcast
> +		= [this, buf, len, port](boost::asio::ip::udp::socket& socket, const std::string& address) -> bool {
> +		if (socket.is_open()) {
> +			boost::system::error_code ec;
> +			boost::asio::ip::udp::endpoint destination(boost::asio::ip::address::from_string(address), port);
> +			socket.send_to(boost::asio::buffer(buf, len), destination, 0, ec);
> +			if (!ec) {
> +				return true;
> +			}
> +#ifdef __APPLE__
> +			if (get_ip_version(destination.address()) == 4) {
> +#endif // __APPLE__
> +				log("[LAN] Error when broadcasting on IPv%d socket to %s, closing it: %s.\n",
> +					get_ip_version(destination.address()), address.c_str(), ec.message().c_str());
> +				close_socket(&socket);
> +#ifdef __APPLE__
> +			} else {
> +				log("[LAN] Error when broadcasting on IPv6 socket to %s: %s.\n",
> +					address.c_str(), ec.message().c_str());
> +			}
> +#endif // __APPLE__
> +		}
> +		return false;
> +	};
> +
> +	bool one_success = false;
> +
> +	// IPv4 broadcasting is the same for all
> +	for (const std::string& address : broadcast_addresses_v4) {
> +		one_success |= do_broadcast(socket_v4, address);
> +	}
> +#ifndef __APPLE__
> +	// For IPv6 on Linux and Windows just send on an undefined network interface
> +	one_success |= do_broadcast(socket_v6, "ff02::1");
> +#else // __APPLE__
> +
> +	// Apple forces us to define which interface we want to send through
> +	for (auto it = interface_indices_v6.begin(); it != interface_indices_v6.end(); ) {
> +		socket_v6.set_option(boost::asio::ip::multicast::outbound_interface(*it));
> +		bool success = do_broadcast(socket_v6, "ff02::1");
> +		one_success |= success;
> +		if (!success) {
> +			// Remove this interface id from the set
> +			it = interface_indices_v6.erase(it);
> +			if (interface_indices_v6.empty()) {
> +				log("[LAN] Warning: No more multicast capable IPv6 interfaces."
> +					"Other LAN players won't find your game.\n");
> +			}
> +		} else {
> +			++it;
> +		}
> +	}
> +#endif // __APPLE__
> +	return one_success;
> +}
> +
> +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;
>  
> 
> === modified file 'src/network/network_lan_promotion.h'
> --- src/network/network_lan_promotion.h	2017-05-07 20:27:21 +0000
> +++ src/network/network_lan_promotion.h	2017-06-04 16:22:50 +0000
> @@ -47,31 +41,115 @@
>  };
>  
>  struct NetOpenGame {
> -	in_addr_t address;
> -	in_port_t port;
> +	NetAddress address;
>  	NetGameInfo info;
>  };
>  
> +/**
> + * Base class for UDP networking.
> + * This class is used by derived classes to find open games on the
> + * local network and to announce a just opened game on the local network.
> + * This class tries to create sockets for IPv4 and IPv6.
> + */
>  struct LanBase {
>  protected:
> -	LanBase();
> +	/**
> +	 * Tries to start a socket on the given port.
> +	 * Sockets for IPv4 and IPv6 are started.
> +	 * When both fail, report_network_error() is called.
> +	 * \param port The port to listen on.
> +	 */
> +	LanBase(uint16_t port);
> +
> +	/**
> +	 * Destructor.

Does it do anything interesting? If so, comment it here. Otherwise, no comment needed.

> +	 */
>  	~LanBase();
>  
> -	void bind(uint16_t);
> -
> -	bool avail();
> -
> -	ssize_t receive(void*, size_t, sockaddr_in*);
> -
> -	void send(void const*, size_t, sockaddr_in const*);
> -	void broadcast(void const*, size_t, uint16_t);
> +	/**
> +	 * Returns whether data is available to be read.
> +	 * \return \c True when receive() will return data, \c false otherwise.
> +	 */
> +	bool is_available();
> +
> +	/**
> +	 * Returns whether at least one of the sockets is open.
> +	 * If this returns \c false, you probably have a problem.
> +	 * \return \c True when a socket is ready, \c false otherwise.
> +	 */
> +	bool is_open();
> +
> +	/**
> +	 * Tries to receive some data.
> +	 * \param[out] buf The buffer to read data into.
> +	 * \param len The length of the buffer.
> +	 * \param[out] addr The address we received data from. Since UDP is a connection-less
> +	 *                  protocol, each receive() might receive data from another address.
> +	 * \return How many bytes have been written to \c buf. If 0 is returned there either was no data
> +	 *         available (check before with avail()) or there was some error (check with is_open())
> +	 */
> +	ssize_t receive(void *buf, size_t len, NetAddress *addr);
> +
> +	/**
> +	 * Sends data to a specified address.
> +	 * \param buf The data to send.
> +	 * \param len The length of the buffer.
> +	 * \param addr The address to send to.
> +	 */
> +	bool send(void const *buf, size_t len, const NetAddress& addr);
> +
> +	/**
> +	 * Broadcast some data in the local network.
> +	 * \param buf The data to send.
> +	 * \param len The length of the buffer.
> +	 * \param port The port to send to. No address is required.
> +	 */
> +	bool broadcast(void const* buf, size_t len, uint16_t port);
> +
> +	/**
> +	 * Throws a WLWarning exception to jump back to the main menu.
> +	 * Calling this on network errors is in the responsibility of derived classes.
> +	 * (Most of the time, aborting makes sense when an error occurred. But e.g. in
> +	 * the destructor simply ignoring the error is okay.)
> +	 */
> +	void report_network_error();
>  
>  private:
> -	int32_t sock;
> -
> -	std::list<in_addr_t> broadcast_addresses;
> +
> +	/**
> +	 * Opens a listening UDP socket.
> +	 * \param[out] The socket to open. The object has to be created but the socket not opened before.
> +	 *             If it already has been opened before, nothing will be done.
> +	 * \param version Whether a IPv4 or IPv6 socket should be opened.
> +	 * \param port The port to listen on.
> +	 */
> +	void start_socket(boost::asio::ip::udp::socket *socket, boost::asio::ip::udp version, uint16_t port);
> +
> +	/**
> +	 * Closes the given socket.
> +	 * Does nothing if the socket already has been closed.
> +	 * \param socket The socket to close.
> +	 */
> +	void close_socket(boost::asio::ip::udp::socket *socket);
> +
> +	/// No idea what this does. I think it is only really used when asynchronous operations are done.
> +    boost::asio::io_service io_service;
> +    /// The socket for IPv4.
> +    boost::asio::ip::udp::socket socket_v4;
> +    /// The socket for IPv6.
> +    boost::asio::ip::udp::socket socket_v6;
> +    /// The found broadcast addresses for IPv4.
> +    /// No addresses for v6, there is only one fixed address.
> +	std::set<std::string> broadcast_addresses_v4;
> +#ifdef __APPLE__
> +	/// Apple forces us to define which interface to broadcast through.
> +	std::set<unsigned int> interface_indices_v6;
> +#endif // __APPLE__
>  };
>  
> +/**
> + * Used to promote opened games locally.
> + */
>  struct LanGamePromoter : public LanBase {
>  	LanGamePromoter();
>  	~LanGamePromoter();


-- 
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.