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.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
