widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Looks mostly OK - I have added some nits.

We should test to make sure that 2 Widelands instances can be run from the same computer and join the same game. I need to figure out how to run a metaserver.

Diff comments:

> 
> === modified file 'src/network/internet_gaming.cc'
> --- src/network/internet_gaming.cc	2017-08-16 04:31:56 +0000
> +++ src/network/internet_gaming.cc	2017-10-14 06:58:24 +0000
> @@ -416,19 +405,24 @@
>  	if (state_ == CONNECTING) {
>  		if (cmd == IGPCMD_LOGIN) {
>  			// Clients request to login was granted
> -			clientname_ = packet.string();
> +			std::string assigned_name = packet.string();
> +			if (clientname_ != assigned_name) {
> +				format_and_add_chat("", "", true,
> +					(boost::format(
> +						_("You logged in as '%s' since your requested name is already in use or reserved."))

You have been logged in...

> +							% assigned_name).str());
> +			}
> +			clientname_ = assigned_name;
>  			clientrights_ = packet.string();
> +			if (clientrights_ == INTERNET_CLIENT_UNREGISTERED) {
> +				// Might happen that we log in with less rights than we wanted to.
> +				// Happens when we are already logged in with another client.
> +				reg_ = false;
> +			}
>  			state_ = LOBBY;
>  			log("InternetGaming: Client %s logged in.\n", clientname_.c_str());
>  			return;
>  
> -		} else if (cmd == IGPCMD_RELOGIN) {
> -			// Clients request to relogin was granted
> -			state_ = LOBBY;
> -			log("InternetGaming: Client %s relogged in.\n", clientname_.c_str());
> -			format_and_add_chat("", "", true, _("Successfully reconnected to the metaserver!"));
> -			return;
> -
>  		} else if (cmd == IGPCMD_ERROR) {
>  			std::string errortype = packet.string();
>  			if (errortype != "LOGIN" && errortype != "RELOGIN") {
> @@ -451,9 +445,8 @@
>  			   cmd.c_str());
>  		}
>  	}
> -
>  	try {
> -		if (cmd == IGPCMD_LOGIN || cmd == IGPCMD_RELOGIN) {
> +		if (cmd == IGPCMD_LOGIN) {// || cmd == IGPCMD_RELOGIN) {

Remove commented out code

>  			// Login specific commands but not in CONNECTING state...
>  			log("InternetGaming: Received %s cmd although client is not in CONNECTING state.\n",
>  			    cmd.c_str());
> @@ -766,6 +759,10 @@
>  
>  /// ChatProvider: sends a message via the metaserver.
>  void InternetGaming::send(const std::string& msg) {
> +	// TODO(Notabilis): Messages can get lost when we are temporarily disconnected from the metaserver,
> +	// even when we reconnect again. "Answered" messages like IGPCMD_GAME_CONNECT are resend but chat

resend -> resent

> +	// messages are not. Resend them after some time when we did not received the matching IGPCMD_CHAT

received -> receive

> +	// command from the server?
>  	if (!logged_in()) {
>  		format_and_add_chat(
>  		   "", "", true, _("Message could not be sent: You are not connected to the metaserver!"));
> @@ -777,6 +774,7 @@
>  
>  	if (msg.size() && *msg.begin() == '@') {
>  		// Format a personal message
> +		// TODO(Notabilis): msg should be trimmed before checking for the space character

You can do that right now with boost::trim. Include <boost/algorithm/string.hpp> for that.

>  		std::string::size_type const space = msg.find(' ');
>  		if (space >= msg.size() - 1) {
>  			format_and_add_chat(
> @@ -834,6 +832,7 @@
>  			SendPacket m;
>  			m.string(IGPCMD_ANNOUNCEMENT);
>  			m.string(arg);
> +			// NOCOM(Notabilis): This looks like a bug to me

Can you be a bit more specific? We shouldn't have NOCOM stuff in trunk.

>  			net->send(s);
>  			return;
>  		} else
> 
> === modified file 'src/network/internet_gaming.h'
> --- src/network/internet_gaming.h	2017-07-01 08:22:54 +0000
> +++ src/network/internet_gaming.h	2017-10-14 06:58:24 +0000
> @@ -57,10 +57,12 @@
>  	void reset();
>  
>  	// Login and logout
> +	// pwd should contain either the password for a registered account or the UUID otherwise
>  	void initialize_connection();
>  	bool login(const std::string& nick,
>  	           const std::string& pwd,
>  	           bool registered,
> +	           bool send_uuid,

sent_uuid or uuid_to_send?

>  	           const std::string& metaserver,
>  	           uint32_t port);
>  	bool relogin();
> @@ -189,6 +198,8 @@
>  	/// data saved for possible relogin
>  	std::string pwd_;
>  	bool reg_;
> +	bool send_uuid_;

sent_uuid_ or uuid_to_send_?

> +	std::string tmp_uuid_;
>  	std::string meta_;
>  	uint16_t port_;
>  
> 
> === modified file 'src/network/internet_gaming_protocol.h'
> --- src/network/internet_gaming_protocol.h	2017-07-01 08:22:54 +0000
> +++ src/network/internet_gaming_protocol.h	2017-10-14 06:58:24 +0000
> @@ -99,27 +103,38 @@
>   */
>  
>  /**
> - * The nonce:
> - *
> - * The nonce is used on the metaserver to link multiple connections by the same client. This
> - * normally
> - * happens when the client supports IPv4 and IPv6 and connects with both protocol versions. This
> - * way,
> - * the metaserver knows that the clients supports both versions and can show games / offer his game
> + * The UUID:
> + *
> + * The UUID is a semi-permanent ID stored in the configuration file of Widelands.
> + * It has to be stored in the file since it should survive crashes of the game or computer.
> + * If the game is not started for 24 hours, a new one is created to increase privacy.
> + * Basically it allows the metaserver to identify the user even when multiple users try to join with
> + * the same username. This is only used for unregistered users, registered users can use their
> + * password instead.
> + * Note that the send UUID differs from the stored one: The send UUID is hash(username | stored-id).

send -> sent

> + * Use-cases of the UUID in detail:
> + *
> + * 1) The UUID replaced the nonce introduced with IPv6.
> + * The UUID is used on the metaserver to link multiple connections by the same client. This
> + * normally happens when the client supports IPv4 and IPv6 and connects with both protocol versions. This
> + * way, the metaserver knows that the clients supports both versions and can show games / offer his game

clients support or client supports. his -> its

>   * of/for clients with both protocol versions.
>   *
> - * When a network client connects to the metaserver with (RE)LOGIN he also sends a nonce.
> - * When "another" netclient connects to the metaserver and sends TELL_IP containing the same nonce,
> + * When a network client connects to the metaserver with (RE)LOGIN he also sends the UUID.

he -> it

> + * When "another" netclient connects to the metaserver and sends TELL_IP containing the same UUID,
>   * it is considered the same game client connecting with another IP. This way, two connections by
> - * IPv4 and
> - * IPv6 can be matched so the server learns both addresses of the client.
> - *
> - * In the case of registered players, the password can be used instead of a random nonce. The
> - * username alone
> - * can not be used for this, especially not for unregistered users: The metaserver can not
> - * differentiate
> - * between a second connection by the user and an initial login of another user (with the same
> - * name).
> + * IPv4 and IPv6 can be matched so the server learns both addresses of the client.
> + *
> + * In the case of registered players, the password can be used instead of a UUID. The username
> + * alone can not be used for this, especially not for unregistered users: The metaserver can not
> + * differentiate between a second connection by the user and an initial login of another user
> + * (with the same name).
> + *
> + * 2) Reconnect after crash / network problems.
> + * When Widelands breaks the connection without logging out, the server still assumes that the old
> + * connection is active. So when the player reconnects, another name is chosen. Sending the UUID allows
> + * to reclaim the old name, since the server recognizes that there isn't a second player trying to use
> + * the same name.
>   */
>  
>  /**
> 
> === modified file 'src/ui_fsmenu/internet_lobby.cc'
> --- src/ui_fsmenu/internet_lobby.cc	2017-06-20 19:55:32 +0000
> +++ src/ui_fsmenu/internet_lobby.cc	2017-10-14 06:58:24 +0000
> @@ -103,7 +104,8 @@
>       // Login information
>       nickname_(nick),
>       password_(pwd),
> -     is_registered_(registered) {
> +     is_registered_(registered),
> +     send_uuid_(send_uuid) {

Please rename - same as above

>  	joingame_.sigclicked.connect(
>  	   boost::bind(&FullscreenMenuInternetLobby::clicked_joingame, boost::ref(*this)));
>  	hostgame_.sigclicked.connect(
> 
> === modified file 'src/ui_fsmenu/multiplayer.cc'
> --- src/ui_fsmenu/multiplayer.cc	2017-02-25 13:27:40 +0000
> +++ src/ui_fsmenu/multiplayer.cc	2017-10-14 06:58:24 +0000
> @@ -105,14 +105,17 @@
>  		nickname_ = s.get_string("nickname", _("nobody"));
>  		password_ = s.get_string("password", "nobody");
>  		register_ = s.get_bool("registered", false);
> +		send_uuid_ = s.get_bool("send_uuid", false);

Please rename - same as above

>  	} else {
>  		LoginBox lb(*this);
>  		if (lb.run<UI::Panel::Returncodes>() == UI::Panel::Returncodes::kOk) {
>  			nickname_ = lb.get_nickname();
>  			password_ = lb.get_password();
>  			register_ = lb.registered();
> +			send_uuid_ = lb.send_uuid();

Please rename - same as above

>  
>  			s.set_bool("registered", lb.registered());
> +			s.set_bool("send_uuid", lb.send_uuid());
>  			s.set_bool("auto_log", lb.set_automaticlog());
>  		} else {
>  			return;
> 
> === modified file 'src/wlapplication.cc'
> --- src/wlapplication.cc	2017-09-11 08:09:07 +0000
> +++ src/wlapplication.cc	2017-10-14 06:58:24 +0000
> @@ -763,6 +766,34 @@
>  	s.get_natural("metaserverport");
>  	// KLUDGE!
>  
> +	long int last_start = s.get_int("last_start", 0);
> +	if (last_start + 24*60*60 < time(nullptr)) {

We uses spaces between arithmetic operators: 24 * 60 * 60

> +		// First start of the game or not started for 24 hours. Create a (new) UUID.
> +		// Admittedly this is a pretty stupid generator. But it should be fine for us.
> +		// For the use of the UUID, see network/internet_gaming_protocol.h
> +		static const std::string random_chars = "0123456789ABCDEF";

Looks like you have the same code above - can you pull out a function?

> +		std::string uuid;
> +		std::random_device rd;
> +		std::mt19937 gen(rd());
> +		std::uniform_int_distribution<> dist(0, random_chars.length() - 1);
> +		while (uuid.length() < 16) {
> +			uuid.push_back(random_chars[dist(gen)]);
> +		}
> +		s.set_string("uuid", uuid);
> +	}
> +	s.set_int("last_start", time(nullptr));
> +
> +	// Save configuration now. Otherwise, the UUID is not saved
> +	// when the game crashes, loosing part of its advantage
> +	// NOCOM(Sebastian): This might be broken. Starting a second game re-enables the music

We should not have NOCOMs in the code - do you need help debugging?

> +	try {
> +		g_options.write("config", false);
> +	} catch (const std::exception& e) {
> +		log("WARNING: could not save configuration: %s\n", e.what());
> +	} catch (...) {
> +		log("WARNING: could not save configuration");
> +	}
> +
>  	return true;
>  }
>  


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