widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Added some ideas for small improvements in the comments.

Diff comments:

> 
> === renamed file 'src/network/nethost.h' => 'src/network/gamehost.h'
> --- src/network/nethost.h	2017-02-10 14:12:36 +0000
> +++ src/network/gamehost.h	2017-05-10 05:43:49 +0000
> @@ -125,7 +125,7 @@
>  
>  	void handle_packet(uint32_t i, RecvPacket&);
>  	void handle_network();
> -	void send_file_part(TCPsocket, uint32_t);
> +	void send_file_part(uint32_t, uint32_t);

void send_file_part(NetHost::ConId client_sock_id, uint32_t part)

>  
>  	void check_hung_clients();
>  	void broadcast_real_speed(uint32_t speed);
> 
> === modified file 'src/network/internet_gaming.cc'
> --- src/network/internet_gaming.cc	2017-02-10 15:37:44 +0000
> +++ src/network/internet_gaming.cc	2017-05-10 05:43:49 +0000
> @@ -34,8 +34,7 @@
>  /// will ensure
>  /// that only one instance is running at time.
>  InternetGaming::InternetGaming()
> -   : sock_(nullptr),
> -     sockset_(nullptr),
> +   : net(),

An explicit nullptr would make things more clear.

>       state_(OFFLINE),
>       reg_(false),
>       port_(INTERNET_GAMING_PORT),
> 
> === added file 'src/network/netclient.cc'
> --- src/network/netclient.cc	1970-01-01 00:00:00 +0000
> +++ src/network/netclient.cc	2017-05-10 05:43:49 +0000
> @@ -0,0 +1,102 @@
> +#include "network/netclient.h"
> +
> +#include <memory>
> +
> +#include <SDL_net.h>
> +
> +#include "base/log.h"
> +
> +class NetClientImpl {
> +	public:
> +		NetClientImpl()
> +			: sock(nullptr), sockset(nullptr), deserializer() {
> +		}
> +
> +		/// 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)
> +		Deserializer deserializer;
> +};
> +
> +std::unique_ptr<NetClient> NetClient::connect(const std::string& ip_address, const uint16_t port) {
> +	std::unique_ptr<NetClient> ptr(new NetClient(ip_address, port));
> +	if (ptr->is_connected()) {
> +		return ptr;
> +	} else {
> +		ptr.reset();
> +		return ptr;
> +	}
> +}
> +
> +NetClient::~NetClient() {
> +	if (is_connected())
> +		close();
> +	if (d->sockset != nullptr)
> +		SDLNet_FreeSocketSet(d->sockset);
> +	delete d;

Can we turn d into a unique_ptr as well? Not necessary in this branch though.

> +}
> +
> +bool NetClient::is_connected() const {
> +	return d->sock != nullptr;
> +}
> +
> +void NetClient::close() {
> +	if (!is_connected())
> +		return;
> +	SDLNet_TCP_DelSocket(d->sockset, d->sock);
> +	SDLNet_TCP_Close(d->sock);
> +	d->sock = nullptr;
> +}
> +
> +bool NetClient::try_receive(RecvPacket& packet) {
> +	if (!is_connected())
> +		return false;
> +
> +	uint8_t buffer[512];
> +	while (SDLNet_CheckSockets(d->sockset, 0) > 0) {
> +
> +		const int32_t bytes = SDLNet_TCP_Recv(d->sock, buffer, sizeof(buffer));
> +		if (bytes <= 0) {
> +			// Error while receiving
> +			close();
> +			return false;
> +		}
> +
> +		d->deserializer.read_data(buffer, bytes);
> +	}
> +	// Get one packet from the deserializer
> +	if (d->deserializer.write_packet(packet)) {

Shorten to return d->deserializer.write_packet(packet) ?

> +		return true;
> +	} else {
> +		return false;
> +	}
> +}
> +
> +void NetClient::send(const SendPacket& packet) {
> +	if (is_connected()) {
> +		SDLNet_TCP_Send(d->sock, packet.get_data(), packet.get_size());
> +	}
> +}
> +
> +NetClient::NetClient(const std::string& ip_address, const uint16_t port)
> +	: d(new NetClientImpl) {
> +
> +	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);
> +		return;
> +	}
> +	log("[Client]: Trying to connect to %s:%u ... ", ip_address.c_str(), port);
> +	d->sock = SDLNet_TCP_Open(&addr);
> +	if (is_connected()) {
> +		log("success\n");
> +		d->sockset = SDLNet_AllocSocketSet(1);
> +		SDLNet_TCP_AddSocket(d->sockset, d->sock);
> +	} else {
> +		log("failed\n");
> +	}
> +}
> 
> === added file 'src/network/netclient.h'
> --- src/network/netclient.h	1970-01-01 00:00:00 +0000
> +++ src/network/netclient.h	2017-05-10 05:43:49 +0000
> @@ -0,0 +1,87 @@
> +/*
> + * Copyright (C) 2008-2017 by the Widelands Development Team
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> + *
> + */
> +
> +#ifndef WL_NETWORK_NETCLIENT_H
> +#define WL_NETWORK_NETCLIENT_H
> +
> +#include <memory>
> +
> +#include "network/network.h"
> +
> +class NetClientImpl;
> +
> +/**
> + * GameClient manages the lifetime of a network game in which this computer

This is still the old description, needs updating.

> + * participates as a client.
> + *
> + * This includes running the game setup screen and the actual game after
> + * launch, as well as dealing with the actual network protocol.
> + */
> +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 an invalid pointer if the connection failed.

invalid pointer => nullptr?

> +		 */
> +		static std::unique_ptr<NetClient> connect(const std::string& ip_address,
> +												const uint16_t port);
> +
> +		/**
> +		 * Closes the connection.
> +		 * If you want to send a goodbye-message to the host, do so before freeing the object.
> +		 */
> +		~NetClient();
> +
> +		/**
> +		 * Returns whether the client is connected.
> +		 * @return \c true if the connection is open, \c false otherwise.
> +		 */
> +		bool is_connected() const;
> +
> +		/**
> +		 * Closes the connection.
> +		 * If you want to send a goodbye-message to the host, do so before calling this.
> +		 */
> +		void close();
> +
> +		/**
> +		 * 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.
> +		 *   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);
> +
> +		/**
> +		 * Sends a packet.
> +		 * Calling this on a closed connection will silently fail.
> +		 * @param packet The packet to send.
> +		 */
> +		 void send(const SendPacket& packet);
> +
> +	private:
> +		NetClient(const std::string& ip_address, const uint16_t port);
> +
> +		NetClientImpl *d;
> +};
> +
> +#endif  // end of include guard: WL_NETWORK_NETCLIENT_H
> 
> === added file 'src/network/nethost.cc'
> --- src/network/nethost.cc	1970-01-01 00:00:00 +0000
> +++ src/network/nethost.cc	2017-05-10 05:43:49 +0000
> @@ -0,0 +1,143 @@
> +#include "network/nethost.h"
> +
> +#include <map>
> +#include <memory>
> +
> +#include <SDL_net.h>
> +
> +#include "base/log.h"
> +
> +class NetHostImpl {
> +	public:
> +
> +		class Client {
> +			public:
> +				Client(TCPsocket sock)
> +					: socket(sock), deserializer() {
> +				}
> +				TCPsocket socket;
> +				Deserializer deserializer;
> +		};
> +
> +		NetHostImpl()
> +			: svsock(nullptr), sockset(nullptr), next_id(1) {
> +		}
> +
> +		TCPsocket svsock;

Rename to svrsock?

> +		SDLNet_SocketSet sockset;
> +		std::map<NetHost::ConId, NetHostImpl::Client> clients;
> +		NetHost::ConId next_id;
> +};
> +
> +std::unique_ptr<NetHost> NetHost::listen(const uint16_t port) {
> +	std::unique_ptr<NetHost> ptr(new NetHost(port));
> +	if (ptr->is_listening()) {
> +		return ptr;
> +	} else {
> +		ptr.reset();
> +		return ptr;
> +	}
> +}
> +
> +NetHost::~NetHost() {
> +	stop_listening();
> +	while (!d->clients.empty()) {
> +		close(d->clients.begin()->first);
> +	}
> +	SDLNet_FreeSocketSet(d->sockset);
> +	delete d;

Make d a unique_ptr?

> +}
> +
> +
> +bool NetHost::is_listening() const {
> +	return d->svsock != nullptr;
> +}
> +
> +bool NetHost::is_connected(const ConId id) const {
> +	return d->clients.count(id) > 0;
> +}
> +
> +void NetHost::stop_listening() {
> +	if (!is_listening())
> +		return;
> +	SDLNet_TCP_DelSocket(d->sockset, d->svsock);
> +	SDLNet_TCP_Close(d->svsock);
> +	d->svsock = nullptr;
> +}
> +
> +void NetHost::close(const ConId id) {
> +	auto iter_client = d->clients.find(id);
> +	if (iter_client == d->clients.end()) {
> +		// Not connected anyway
> +		return;
> +	}
> +	SDLNet_TCP_DelSocket(d->sockset, iter_client->second.socket);
> +	SDLNet_TCP_Close(iter_client->second.socket);
> +	d->clients.erase(iter_client);
> +}
> +
> +bool NetHost::try_accept(ConId& new_id) {
> +	if (!is_listening())
> +		return false;
> +
> +	TCPsocket sock = SDLNet_TCP_Accept(d->svsock);
> +	// No client wants to connect
> +	if (sock == nullptr)
> +		return false;
> +	SDLNet_TCP_AddSocket(d->sockset, sock);
> +	ConId id = d->next_id++;
> +	assert(id > 0);
> +	assert(d->clients.count(id) == 0);
> +	d->clients.insert(std::make_pair(id, NetHostImpl::Client(sock)));
> +	assert(d->clients.count(id) == 1);
> +	new_id = id;
> +	return true;
> +}
> +
> +bool NetHost::try_receive(const ConId id, RecvPacket& packet) {
> +
> +	// Always read all available data into buffers
> +	uint8_t buffer[512];
> +	while (SDLNet_CheckSockets(d->sockset, 0) > 0) {
> +		for (auto& e : d->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);
> +			}
> +		}
> +	}
> +
> +	// Now check whether there is data for the requested client
> +	if (!is_connected(id))
> +		return false;
> +
> +	// Get one packet from the deserializer
> +	if (d->clients.at(id).deserializer.write_packet(packet)) {

return d->clients.at(id).deserializer.write_packet(packet);

> +		return true;
> +	} else {
> +		return false;
> +	}
> +}
> +
> +void NetHost::send(const ConId id, const SendPacket& packet) {
> +	if (is_connected(id)) {
> +		SDLNet_TCP_Send(d->clients.at(id).socket, packet.get_data(), packet.get_size());
> +	}
> +}
> +
> +NetHost::NetHost(const uint16_t port)
> +	: d(new NetHostImpl) {
> +
> +	IPaddress myaddr;
> +	SDLNet_ResolveHost(&myaddr, nullptr, port);
> +	d->svsock = SDLNet_TCP_Open(&myaddr);
> +	// Maximal 16 sockets! This mean we can have at most 15 clients in our game (+ metaserver)
> +	d->sockset = SDLNet_AllocSocketSet(16);
> +}
> 
> === added file 'src/network/nethost.h'
> --- src/network/nethost.h	1970-01-01 00:00:00 +0000
> +++ src/network/nethost.h	2017-05-10 05:43:49 +0000
> @@ -0,0 +1,111 @@
> +/*
> + * Copyright (C) 2008-2017 by the Widelands Development Team
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> + *
> + */
> +
> +#ifndef WL_NETWORK_NETHOST_H
> +#define WL_NETWORK_NETHOST_H
> +
> +#include <memory>
> +
> +#include "network/network.h"
> +
> +class NetHostImpl;
> +
> +/**
> + * GameClient manages the lifetime of a network game in which this computer

Update the description

> + * participates as a client.
> + *
> + * This includes running the game setup screen and the actual game after
> + * launch, as well as dealing with the actual network protocol.
> + */
> +class NetHost {
> +	public:
> +
> +		using ConId = uint32_t;

ConnectionId?

> +
> +		/**
> +		 * Tries to listen on the given port.
> +		 * @param port The port to listen on.
> +		 * @return A pointer to a listening \c NetHost object or an invalid pointer if the connection failed.

invalid pointer => nullptr

> +		 */
> +		static std::unique_ptr<NetHost> listen(const uint16_t port);
> +
> +		/**
> +		 * Closes the server.
> +		 */
> +		~NetHost();
> +
> +		/**
> +		 * Returns whether the server is started and is listening.
> +		 * @return \c true if the server is listening, \c false otherwise.
> +		 */
> +		bool is_listening() const;
> +
> +		/**
> +		 * Returns whether the given client is connected.
> +		 * @param The id of the client to check.
> +		 * @return \c true if the connection is open, \c false otherwise.
> +		 */
> +		bool is_connected(ConId id) const;
> +
> +		/**
> +		 * Stops listening for connections.
> +		 */
> +		void stop_listening();
> +
> +		/**
> +		 * Closes the connection to the given client.
> +		 * @param id The id of the client to close the connection to.
> +		 */
> +		void close(ConId id);
> +
> +		/**
> +		 * Tries to accept a new client.
> +		 * @param new_id The connection id of the new client will be stored here.
> +		 * @return \c true if a client has connected, \c false otherwise.
> +		 *   The given id is only modified when \c true is returned.
> +		 *   Calling this on a closed server will return false.
> +		 *   The returned id is always greater 0.

greater than 0

> +		 */
> +		bool try_accept(ConId& new_id);
> +
> +		/**
> +		 * Tries to receive a packet.
> +		 * @param id The connection id of the client that should be received.
> +		 * @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(ConId id, RecvPacket& packet);

If the packet's contents are changed, it should be a pointer rather than a reference -> makes the code easier to read. Same for similar functions.

> +
> +		/**
> +		 * Sends a packet.
> +		 * Calling this on a closed connection will silently fail.
> +		 * @param id The connection id of the client that should be send to.

send -> sent

> +		 * @param packet The packet to send.
> +		 */
> +		 void send(ConId id, const SendPacket& packet);
> +
> +	private:
> +		NetHost(const uint16_t port);
> +
> +		NetHostImpl *d;
> +};
> +
> +#endif  // end of include guard: WL_NETWORK_NETHOST_H
> 
> === modified file 'src/network/network.h'
> --- src/network/network.h	2017-01-25 18:55:59 +0000
> +++ src/network/network.h	2017-05-10 05:43:49 +0000
> @@ -43,7 +43,7 @@
>  };
>  
>  /**
> - * This non-gamelogic command is used by \ref NetHost and \ref NetClient
> + * This non-gamelogic command is used by \ref GameHost and \ref GameClient

Game or Net?

>   * to schedule taking a synchronization hash.
>   */
>  struct CmdNetCheckSync : public Widelands::Command {


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