widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Approve

Only some nits. I reviewed the metaserver branch on GitHub.

Diff comments:

> 
> === modified file 'src/network/internet_gaming.cc'
> --- src/network/internet_gaming.cc	2017-06-04 16:01:13 +0000
> +++ src/network/internet_gaming.cc	2017-06-22 07:30:44 +0000
> @@ -122,6 +130,17 @@
>  	meta_ = meta;
>  	port_ = port;
>  
> +	// If we are not connecting to a registered account, create a random value
> +	// to send as password. Used so the metaserver can match our IPv4 and IPv6 connections
> +	if (!reg_) {
> +		// Admittedly this is a pretty stupid generator. But it should be fine for us
> +		static const char random_chars[] = "0123456789ABCDEF";
> +		pwd_ = "";
> +		while (pwd_.length() < 8) {

Please use c++11 random functionality: you probably want this: http://en.cppreference.com/w/cpp/numeric/random/uniform_int_distribution

> +			pwd_.push_back(random_chars[rand() % (sizeof(random_chars) - 1)]);
> +		}
> +	}
> +
>  	initialize_connection();
>  
>  	// If we are here, a connection was established and we can send our login package through the
> @@ -334,6 +356,40 @@
>  	}
>  }
>  
> +void InternetGaming::create_second_connection() {
> +	// TODO(Notabilis): This method could probably be executed by a separate thread. Do we want to do so?

no. not as long as it is not needed. We do not use multi threaded anywhere in Widelands and it seems like this is not the most important place to start.

> +	// Of course, we would have to be carefully that the main thread might call disconnect while doing so
> +	NetAddress addr;
> +	net->get_remote_address(&addr);
> +	if (!addr.is_ipv6()) {
> +		// Primary connection already is IPv4, abort
> +		return;
> +	}
> +
> +	if (!NetAddress::resolve_to_v4(&addr, meta_, port_)) {
> +		// Could not get the IPv4 address of the metaserver? Strange :-/
> +		return;
> +	}
> +
> +	std::unique_ptr<NetClient> tmpNet = NetClient::connect(addr);
> +

remove empty line?

> +	if (!tmpNet || !tmpNet->is_connected()) {
> +		// Connecting by IPv4 doesn't work? Well, nothing to do then
> +		return;
> +	}
> +
> +	// Okay, we have a connection. Send the login message and terminate the connection
> +	SendPacket s;
> +	s.string(IGPCMD_TELL_IP);
> +	s.string(boost::lexical_cast<std::string>(INTERNET_GAMING_PROTOCOL_VERSION));
> +	s.string(clientname_);
> +	s.string(pwd_);
> +	tmpNet->send(s);
> +
> +	// Close the connection
> +	tmpNet->close();
> +}
> +
>  /// Handle one packet received from the metaserver.
>  void InternetGaming::handle_packet(RecvPacket& packet) {
>  	std::string cmd = packet.string();
> 
> === modified file 'src/network/internet_gaming.h'
> --- src/network/internet_gaming.h	2017-06-03 05:27:36 +0000
> +++ src/network/internet_gaming.h	2017-06-22 07:30:44 +0000
> @@ -82,7 +82,7 @@
>  	void handle_metaserver_communication();
>  
>  	// Game specific functions
> -	const std::string& ip();
> +	const std::pair<NetAddress, NetAddress>& ips();

please add a comment what the entries of this pair are

>  	void join_game(const std::string& gamename);
>  	void open_game();
>  	void set_game_playing();
> @@ -173,7 +187,7 @@
>  
>  	/// information of the clients game
>  	std::string gamename_;
> -	std::string gameip_;
> +	std::pair<NetAddress, NetAddress> gameips_;

Also here: comment would be nice.

>  
>  	/// Metaserver information
>  	bool clientupdateonmetaserver_;
> 
> === modified file 'src/network/netclient.cc'
> --- src/network/netclient.cc	2017-06-10 16:36:29 +0000
> +++ src/network/netclient.cc	2017-06-22 07:30:44 +0000
> @@ -20,6 +20,15 @@
>  		close();
>  }
>  
> +bool NetClient::get_remote_address(NetAddress *addr) const {
> +	if (!is_connected())

Please add {}

> +		return false;
> +	boost::asio::ip::tcp::endpoint remote = socket_.remote_endpoint();
> +	addr->ip = remote.address();
> +	addr->port = remote.port();
> +	return true;
> +}
> +
>  bool NetClient::is_connected() const {
>  	return socket_.is_open();
>  }
> 
> === modified file 'src/network/netclient.h'
> --- src/network/netclient.h	2017-06-10 16:36:29 +0000
> +++ src/network/netclient.h	2017-06-22 07:30:44 +0000
> @@ -46,6 +46,14 @@
>  	~NetClient();
>  
>  	/**
> +	 * Returns the ip and port of the remote host we are connected to.
> +	 * \param addr A pointer to a NetAddress structure to write the address to.
> +	 * \return \c True when \c addr has successfully been written to. \c False otherwise,

Returns false when addr could not be filled in. This should only happen....

That the function returns true in the case of success is never needed, since it is implied by defining when it returns false

> +	 *   should only happen when the client is not connected.
> +	 */
> +	bool get_remote_address(NetAddress *addr) const;
> +
> +	/**
>  	 * Returns whether the client is connected.
>  	 * \return \c true if the connection is open, \c false otherwise.
>  	 */


-- 
https://code.launchpad.net/~widelands-dev/widelands/net-internetgaming-ipv6/+merge/326027
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-internetgaming-ipv6.